Multi-Tenancy from a Live CRM, and the Two Holes the Second Review Found

Last time I shipped v0.2.x of LaraFoundry: authentication and users, built on top of Fortify. This post is v0.3.0, and it's the one a SaaS actually needs before anything else makes sense: multi-tenancy. Companies, teams, who-can-see-what.

If you've read the series, you know the pattern by now. I'm not writing this from scratch. I'm extracting it out of a CRM that's been running in production for years, re-reading the old code on the way out, and modernizing it as it lands in the package. Multi-tenancy is the module where that re-reading paid off the most, because the original had a data-isolation bug that had been quietly sitting there the whole time.

And then, because I got cocky, I ran my code review, saw "8 of 12 findings handled," felt good, and almost shipped. Then I ran it a second time. The second pass found two security holes the first one missed. Both real. Both the kind that let one tenant touch another tenant's stuff. More on those at the end, they're the reason I'm writing this one.

What multi-tenancy actually means here

One database, many tenants, row-level isolation. No database-per-tenant, no schema-per-tenant. Every tenant-owned row carries a company_id, and a global query scope makes sure you only ever see rows for the company you're currently acting as.

The core ships both modes behind a config switch:

• teams - the tenant is a Company. Users belong to one or more companies, switch between them, invite each other. This is what my CRM uses.
• personal - the tenant is the User. No companies at all, every user is their own island. Filtered by user_id instead of company_id, no fake one-person company in the way.

The host picks the mode in config and the rest of the package adapts. Same scope, same trait, different column.

The decision that mattered: fail-closed

Here is the old code, roughly, from the live CRM:

// legacy global scope, simplified
public function apply(Builder $builder, Model $model)
{
    $companyId = session('active_company_id');

    if ($companyId) {
        $builder->where('company_id', $companyId);
    }
    // else: no filter. all rows.
}

Read that else again. If there is no active company in the session, the scope adds no filter, and the query returns every row in the table for every tenant.

In normal use you always have an active company, so this never fired. But "normal use" is not a security boundary. A background job, a console command, a request that hits the model before the active company is resolved, a session that expired mid-request: any of those gives you $companyId = null, and now your tenant scope is wide open. That's a textbook IDOR, fail-open by construction.

The package version inverts it. No resolvable tenant means no rows, not all rows:

public function apply(Builder $builder, Model $model): void
{
    $tenantKey = static::currentTenantKey();

    if ($tenantKey === null) {
        // Fail closed. No tenant resolved -> show nothing, never everything.
        $builder->whereRaw('0 = 1');

        return;
    }

    $builder->where(static::tenantColumn(), $tenantKey);
}

If something goes wrong, you see an empty list. That's annoying. The old behavior was a data leak. I'll take annoying.

The same principle runs through the write path. When you create a tenant-owned model, the company_id is filled automatically from the resolver, and it is not mass-assignable, so a crafted form payload can't set it to someone else's company. And if there's no tenant resolved at create time, it throws instead of writing an orphan row:

static::creating(function (Model $model) {
    $tenantKey = static::currentTenantKey();

    if ($tenantKey === null) {
        throw new RuntimeException('Cannot create a tenant-owned model without an active tenant.');
    }

    $model->setAttribute(static::tenantColumn(), $tenantKey);
});

The resolver, and why it's behind an interface

"Which company am I acting as right now" is a question with more than one answer depending on where you are. In a browser session it's stored per-device. Under an API token (which is coming in a later phase for the mobile app) it'll be derived from the token. I didn't want the tenant scope to know or care which.

So the active tenant comes from a TenantResolver contract:

interface TenantResolver
{
    public function current(Authenticatable $user): ?Tenant;
    public function setCurrent(Authenticatable $user, Tenant|int|null $tenant): void;
    public function forget(Authenticatable $user): void;
}

In teams mode the binding is a SessionTenantResolver that reads and writes user_sessions.active_company_id (the active company lives per-device, on the session row I already track from the auth phase). In personal mode it's a resolver that just hands back the user. Later, the API phase adds a token resolver here and nothing else changes. The scope, the trait, the controllers all keep talking to the interface.

That one seam, by the way, is also where the single nastiest bug of this phase lived. I'll get to it.

The user side is a trait, not a base class

The host's User model already uses the auth trait from the last phase. Tenancy is a second trait that sits next to it:

class User extends Authenticatable
{
    use IsLaraFoundryUser;   // phase 1.1: identity, sessions, 2FA
    use BelongsToTenancy;    // phase 1.2: companies, active company, owner checks
}

BelongsToTenancy gives the user companies(), getActiveCompany(), setActiveCompany(), isOwnerOf(), and the bits the company switcher needs. RBAC (roles and permissions inside a company) is its own trait in the next phase. The point of the trait-slot idiom is that each phase adds a slice without rewriting the model or colliding with the host's own code.

The Company model itself is deliberately thin. The core owns only the columns every multi-tenant app needs (name, slug, uuid, country, the founder). The host extends it and points one config value at the subclass to add business columns. Billing is a phase-3 seam: hasAccess() returns true in the free core today, and the paid billing add-on overrides it with real subscription logic later, without any call site changing.

Invitations, and the email-spoof guard I kept

You invite someone by email. The package stores a 64-char random token and a hard expiry, and emails a link. So far, normal.

The guard that matters is on acceptance. A token alone is not enough to join a company. The logged-in user's email has to match the invited email:

public function isFor(string $email): bool
{
    return mb_strtolower($this->email) === mb_strtolower($email);
}

The legacy app had this, and I kept it, because without it a leaked invite link is a free pass into the company for whoever holds it. With it, the link is only useful to the person it was addressed to.

That felt airtight. It was not. Which brings me to the part I actually wanted to write about.

Now the two holes

I ran my review process, a high-effort multi-angle code review over the diff. First pass: about a dozen findings, I handled the actionable ones, and I had that "this is basically clean" feeling. Then I ran it a second time on the same diff. The second pass surfaced two security findings the first walked right past. Here's both.

Hole 1: company takeover through the invite flow

The email-match guard above checks that the account's email equals the invited email. Fine. But three things lined up:

1. The accept route was behind auth, but not behind verified.
2. Local registration sets email_verified_at = null. You can register an account on any email address without proving you own it.
3. The public invitation landing page was rendering the invited email into its props.

Put those together. An attacker gets a leaked invite link (they leak, links get forwarded, they sit in logs). The public landing page hands them the exact email it was sent to. They register a fresh account on that email, no verification needed. Their email now equals the invited email, so isFor() passes. They accept. They're in a company that was never theirs.

The string match was never the weak part. The weak part was that "has this email" and "owns this email" were treated as the same thing, and the landing page handed out the email for free.

Two fixes. The accept path now requires a verified email, so having the address isn't enough, you have to prove the mailbox is yours:

if (! $invitation->isFor($user->email) || ! $this->hasVerifiedEmail($user)) {
    return redirect('/')->with('error', __('larafoundry::tenancy.invitation.email_mismatch'));
}

And the email came out of the public page's props entirely. The landing page now says "you've been invited to join this company" and nothing about who. If you weren't sent the link, the page tells you nothing useful.

Hole 2: privilege resurrection back to owner

This one is subtler and I like it more.

Removing an employee is a soft delete. The pivot row stays for audit, with is_deleted = true. Re-adding someone restores that same row instead of making a new one (there's a unique constraint on company + user). And re-adding is owner-sticky: if you were already an owner, being re-added as a plain member must not silently demote you. So the new owner flag was OR-ed with whatever the row already had:

'is_owner' => $isOwner || ($existingRow && $existingRow->is_owner),

Looks reasonable. Now walk it through.

Someone is an owner. You remove them (soft delete, row keeps is_owner = true, because removal only flipped is_deleted). Months later you invite them back as a regular member. $isOwner is false. But $existingRow->is_owner is still true from before. The OR brings ownership back. A plain-member re-invite silently hands them owner rights again.

The fix is one line in the removal path: clearing ownership when you soft-remove, so a dormant row never carries a stale owner flag for the OR to pick up.

public function removeEmployee(Authenticatable $user): void
{
    $this->users()->updateExistingPivot($user->getAuthIdentifier(), [
        'is_deleted' => true,
        'is_owner' => false, // don't let a removed owner come back as one
    ]);
}

Owner-stickiness is still there, but now it's scoped to people who are currently members, which is the only place it was ever meant to apply.

Why the second pass caught what the first didn't

I think it's because the first pass and I were both reviewing the same way: read the diff, is each change correct in isolation. Both of these holes are correct in isolation. The accept guard correctly matches emails. The owner-OR correctly preserves ownership. Neither is wrong on its own line. They're only dangerous when you trace state across the whole flow: register -> see email -> accept, or remove -> wait -> re-invite.

"8 of 12 handled" gave me a number that felt like progress and wasn't really telling me about the two that mattered. The second run wasn't smarter, it just looked again with the question "what crosses a tenant boundary," and that question found things "is this line correct" didn't.

The tests

Every piece has Pest coverage, and the regression tests for both holes above are now in the suite, so they can't quietly come back. A couple that I care about:

it('SECURITY: refuses acceptance from an UNVERIFIED account even with the matching email', function () {
    [$company, $invitation] = companyWithInvite('invitee@x.test');
    $unverified = User::create([
        'email' => 'invitee@x.test',
        'email_verified_at' => null,
    ]);

    $this->actingAs($unverified)
        ->post("/invitations/{$invitation->token}/accept")
        ->assertRedirect('/');

    expect($unverified->fresh()->companies()->count())->toBe(0);
});

it('a soft-removed ex-owner re-invited as a member does NOT return as owner', function () {
    // remove the owner, then re-add as a plain member, assert they are not an owner.
});

it('the public landing page does not reveal the invited email', function () {
    $this->withHeader('X-Inertia', 'true')
        ->get("/invitations/{$invitation->token}")
        ->assertOk()
        ->assertJsonMissing(['email' => 'secret-invitee@x.test']);
});

The fail-closed scope has its own tests too: no active tenant returns zero rows, a foreign company's data is invisible, a crafted company_id in a create payload gets overwritten by the resolver and not honored.

The package suite is green (Pest plus the Vue component tests), and then I did the thing that has earned its place in my process: I wired v0.3.0 into the real host app and clicked through it. Create a company, invite someone, switch active company, land on the right page under the verified plus active-tenant middleware. That integration smoke is now seven host-side tests of its own, and it caught a config gotcha (a stale published config file shadowing the new tenancy keys, because Laravel's config merge doesn't add missing sub-keys to a section that already exists). Not a security bug this time, but exactly the kind of thing that only shows up when the package meets a real app.

Where this leaves the core

After three releases the package now has primitives, auth, and multi-tenancy. That's the spine. The next phase is RBAC, roles and permissions inside a company, and the hook for it is already in: creating a company fires an event the roles phase will listen on to seed default roles. Billing comes after that, sitting on the hasAccess() seam that returns true for now.

If you're building a multi-tenant Laravel app, the one thing I'd take from this regardless of my package: make your tenant scope fail closed, and write the test that proves an empty tenant shows nothing. The default-deny version costs you one whereRaw('0 = 1') and saves you the bug I'd been shipping for years.

Code is on GitHub. It's source-available, free for non-commercial use. If you want to follow the build, the repo and the series are the place.

Next: RBAC.