Skip to content

fix(auth): move SMB auto-join from current_active_user to on_after_register#21

Merged
UsamaSadiq merged 2 commits into
foss-mainfrom
fix/auto-join-on-register-only
May 18, 2026
Merged

fix(auth): move SMB auto-join from current_active_user to on_after_register#21
UsamaSadiq merged 2 commits into
foss-mainfrom
fix/auto-join-on-register-only

Conversation

@awais786

Copy link
Copy Markdown

Summary

auto_join_smb_search_space was being called from current_active_user, FastAPI's per-request auth dependency. Combined with the hard-delete DELETE /searchspaces/{id}/members/{membership_id} endpoint (no tombstone column on SearchSpaceMembership), this made admin removal a no-op: the very next API request from a removed user re-INSERTed their membership row.

This PR moves the call to `on_after_register` so it fires exactly once per user, at creation time. Removed users stay removed.

Reproduction (against current main)

  1. Owner Alice has an Editor Bob on the SMB workspace.
  2. Alice: `DELETE /api/v1/searchspaces//members/` → 200.
  3. `SELECT * FROM search_space_memberships WHERE user_id = ` → 0 rows.
  4. Bob's frontend polls `GET /api/v1/searchspaces//documents` (with valid Bearer JWT).
  5. `SELECT * FROM search_space_memberships WHERE user_id = ` → 1 row, role_id = Editor.
  6. Bob has full Editor permissions back: `documents:create/read/update`, `chats:create/read/update`, `members:invite`, `connectors:create/update`.

Fix

`users.py`:

  • Remove the two `auto_join_smb_search_space` calls from `current_active_user` (one in the proxy_user branch, one in the jwt_user branch).
  • Add a single call inside `on_after_register`, after the personal default-SearchSpace setup commits.
  • Update the docstrings on both functions to reflect the new placement and the reason.

Both registration paths land in `on_after_register`:

  • Standard FastAPI Users register flow
  • `ProxyAuthMiddleware._resolve_user` (already invokes `on_after_register` after creating a user via header-trust — see `proxy_auth.py:179-209`)

So new users from either flow are still auto-joined exactly once. Existing users at fix time who don't have an SMB membership row aren't backfilled — operators can do a one-time SQL INSERT against `search_space_memberships` for those.

Why not a tombstone column

A tombstone column on `SearchSpaceMembership` (`removed_at` or `is_blocked`) was the alternative — preserves the original "auto-join existing users when SMB workspace appears" UX while making removals durable. Larger change (schema migration + DELETE refactor + auto-join skip-tombstoned-users logic). Saved for a follow-up if the one-shot trade-off proves limiting.

Test plan

  • Sign up a new user (proxy-auth path) → confirm SMB membership exists after registration.
  • Sign up a new user (JWT/email-password path) → same.
  • As Owner, DELETE an Editor's membership → confirm 0 rows in `search_space_memberships` for that user/space.
  • As the removed Editor, hit any authenticated endpoint repeatedly → confirm membership row stays at 0 (does NOT re-INSERT).
  • Sign out + back in as the removed Editor → confirm still no membership re-INSERT.
  • Existing users who already have SMB memberships unaffected.

Refs

External writeup: awais786/sso-rules `surfsense-security.md` §Finding 1 (PR pending)

🤖 Generated with Claude Code

…gister

`auto_join_smb_search_space` was called from `current_active_user`,
FastAPI's per-request auth dependency. That dependency runs on every
authenticated API request — both proxy-auth and JWT paths.

Combined with `DELETE /searchspaces/{id}/members/{membership_id}`
(`rbac_routes.py:705`) which hard-deletes the membership row with no
tombstone (the SearchSpaceMembership model has no `removed_at` /
`is_blocked` / `deleted_at` column), this made admin removal a no-op:

1. Owner Alice calls `DELETE /api/v1/searchspaces/42/members/77` to
   evict Editor Bob → row deleted, API returns 200.
2. Bob's next request (`GET /api/v1/searchspaces/42/documents`) hits
   `current_active_user` → triggers `auto_join_smb_search_space(bob.id)`
   → no membership row found → INSERT (Editor again).
3. Bob is back inside the workspace, can re-read every document, mint
   invite links, etc. He cannot be removed at all while his SSO sign-in
   still works at the IdP.

Editor permissions include documents:create/read/update,
chats:create/read/update, members:invite, connectors:create/update —
significant write surface to silently restore.

Fix: move the call to `on_after_register` so it fires exactly once per
user, at user creation. Both registration paths land here:
- Standard FastAPI Users register flow
- ProxyAuthMiddleware (already invokes `on_after_register` after
  creating a user via header-trust — see proxy_auth.py:179-209)

Trade-off documented in the comment: users who registered BEFORE the
SMB workspace existed are not retroactively auto-joined. Operators
can backfill with a one-time SQL INSERT against
`search_space_memberships`. The previous design intentionally caught
that case via per-request enforcement; the un-revokability cost was
not worth the convenience.

Refs: awais786/sso-rules surfsense-security.md §"Finding 1"

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Moves the auto_join_smb_search_space call out of the per-request current_active_user dependency and into on_after_register, so it executes exactly once at user creation. Previously, every authenticated request from a removed user re-INSERTed their SMB membership row, because the DELETE .../members/{membership_id} endpoint hard-deletes without a tombstone — making admin removals effectively a no-op.

Changes:

  • Add a single auto_join_smb_search_space(user.id) call at the end of UserManager.on_after_register, wrapped in try/except with logger.exception.
  • Remove the two per-request auto_join_smb_search_space invocations (proxy_user and jwt_user branches) from current_active_user.
  • Update docstrings to document the new placement and rationale.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

CI's ruff-check moved `from app.services.smb_auto_join import
auto_join_smb_search_space` from between `app.config` and `app.db` to
between `app.prompts.system_defaults` and `app.utils.refresh_tokens` —
correct alphabetical position within the `app.*` block.

Auto-fix from `ruff check --fix surfsense_backend/app/users.py`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@UsamaSadiq UsamaSadiq merged commit 7f21e5b into foss-main May 18, 2026
6 of 8 checks passed
aznszn pushed a commit that referenced this pull request May 19, 2026
…gister (#21)

* fix(auth): move SMB auto-join from current_active_user to on_after_register

`auto_join_smb_search_space` was called from `current_active_user`,
FastAPI's per-request auth dependency. That dependency runs on every
authenticated API request — both proxy-auth and JWT paths.

Combined with `DELETE /searchspaces/{id}/members/{membership_id}`
(`rbac_routes.py:705`) which hard-deletes the membership row with no
tombstone (the SearchSpaceMembership model has no `removed_at` /
`is_blocked` / `deleted_at` column), this made admin removal a no-op:

1. Owner Alice calls `DELETE /api/v1/searchspaces/42/members/77` to
   evict Editor Bob → row deleted, API returns 200.
2. Bob's next request (`GET /api/v1/searchspaces/42/documents`) hits
   `current_active_user` → triggers `auto_join_smb_search_space(bob.id)`
   → no membership row found → INSERT (Editor again).
3. Bob is back inside the workspace, can re-read every document, mint
   invite links, etc. He cannot be removed at all while his SSO sign-in
   still works at the IdP.

Editor permissions include documents:create/read/update,
chats:create/read/update, members:invite, connectors:create/update —
significant write surface to silently restore.

Fix: move the call to `on_after_register` so it fires exactly once per
user, at user creation. Both registration paths land here:
- Standard FastAPI Users register flow
- ProxyAuthMiddleware (already invokes `on_after_register` after
  creating a user via header-trust — see proxy_auth.py:179-209)

Trade-off documented in the comment: users who registered BEFORE the
SMB workspace existed are not retroactively auto-joined. Operators
can backfill with a one-time SQL INSERT against
`search_space_memberships`. The previous design intentionally caught
that case via per-request enforcement; the un-revokability cost was
not worth the convenience.

Refs: awais786/sso-rules surfsense-security.md §"Finding 1"

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore: fix ruff import ordering

CI's ruff-check moved `from app.services.smb_auto_join import
auto_join_smb_search_space` from between `app.config` and `app.db` to
between `app.prompts.system_defaults` and `app.utils.refresh_tokens` —
correct alphabetical position within the `app.*` block.

Auto-fix from `ruff check --fix surfsense_backend/app/users.py`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aznszn pushed a commit that referenced this pull request May 19, 2026
…gister (#21)

* fix(auth): move SMB auto-join from current_active_user to on_after_register

`auto_join_smb_search_space` was called from `current_active_user`,
FastAPI's per-request auth dependency. That dependency runs on every
authenticated API request — both proxy-auth and JWT paths.

Combined with `DELETE /searchspaces/{id}/members/{membership_id}`
(`rbac_routes.py:705`) which hard-deletes the membership row with no
tombstone (the SearchSpaceMembership model has no `removed_at` /
`is_blocked` / `deleted_at` column), this made admin removal a no-op:

1. Owner Alice calls `DELETE /api/v1/searchspaces/42/members/77` to
   evict Editor Bob → row deleted, API returns 200.
2. Bob's next request (`GET /api/v1/searchspaces/42/documents`) hits
   `current_active_user` → triggers `auto_join_smb_search_space(bob.id)`
   → no membership row found → INSERT (Editor again).
3. Bob is back inside the workspace, can re-read every document, mint
   invite links, etc. He cannot be removed at all while his SSO sign-in
   still works at the IdP.

Editor permissions include documents:create/read/update,
chats:create/read/update, members:invite, connectors:create/update —
significant write surface to silently restore.

Fix: move the call to `on_after_register` so it fires exactly once per
user, at user creation. Both registration paths land here:
- Standard FastAPI Users register flow
- ProxyAuthMiddleware (already invokes `on_after_register` after
  creating a user via header-trust — see proxy_auth.py:179-209)

Trade-off documented in the comment: users who registered BEFORE the
SMB workspace existed are not retroactively auto-joined. Operators
can backfill with a one-time SQL INSERT against
`search_space_memberships`. The previous design intentionally caught
that case via per-request enforcement; the un-revokability cost was
not worth the convenience.

Refs: awais786/sso-rules surfsense-security.md §"Finding 1"

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore: fix ruff import ordering

CI's ruff-check moved `from app.services.smb_auto_join import
auto_join_smb_search_space` from between `app.config` and `app.db` to
between `app.prompts.system_defaults` and `app.utils.refresh_tokens` —
correct alphabetical position within the `app.*` block.

Auto-fix from `ruff check --fix surfsense_backend/app/users.py`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants