Skip to content

Auto-join SSO users to first shared search space on login#18

Merged
UsamaSadiq merged 4 commits into
foss-mainfrom
jawad/default-workspace-feat
May 14, 2026
Merged

Auto-join SSO users to first shared search space on login#18
UsamaSadiq merged 4 commits into
foss-mainfrom
jawad/default-workspace-feat

Conversation

@jawad-khan
Copy link
Copy Markdown

Description

ProxyAuthMiddleware now checks whether the authenticated user has any non-owner search space membership. If not, they are added to the first shared search space (not owned by them) with the default role.

Motivation and Context

FIX #

Screenshots

API Changes

  • This PR includes API changes

Change Type

  • Bug fix
  • New feature
  • Performance improvement
  • Refactoring
  • Documentation
  • Dependency/Build system
  • Breaking change
  • Other (specify):

Testing Performed

  • Tested locally
  • Manual/QA verification

Checklist

  • Follows project coding standards and conventions
  • Documentation updated as needed
  • Dependencies updated as needed
  • No lint/build errors or new warnings
  • All relevant tests are passing

email,
)

# SMB shared-space join runs in users.current_active_user so Bearer JWT
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Let's remove the comments where another app is being referenced as the solution. This comment is not needed anymore.

Comment thread surfsense_backend/app/services/smb_auto_join.py Outdated
Comment thread surfsense_backend/app/users.py Outdated
Comment thread surfsense_backend/app/middleware/proxy_auth.py Outdated
Comment thread surfsense_backend/app/middleware/proxy_auth.py Outdated
Comment thread surfsense_backend/app/middleware/proxy_auth.py Outdated
Co-authored-by: Usama Sadiq <usama7274@gmail.com>
@UsamaSadiq UsamaSadiq merged commit 905ac6c into foss-main May 14, 2026
4 of 7 checks passed
@awais786
Copy link
Copy Markdown

Security follow-up — per-request placement created an un-revokable membership path

Heads-up that the per-request placement here (current_active_user invoking auto_join_smb_search_space on every authenticated request) interacts with the hard-delete DELETE /searchspaces/{id}/members/{membership_id} endpoint (rbac_routes.py:705) to make admin removal silently a no-op:

  1. Owner Alice calls DELETE /api/v1/searchspaces/<id>/members/<bob-membership-id> to evict Editor Bob → row deleted, returns 200.
  2. Bob's frontend polls any authenticated endpoint with his Bearer JWT.
  3. current_active_user runs → auto_join_smb_search_space(bob.id) → no membership row → INSERT (Editor again).
  4. Bob retains documents:create/read/update, chats:create/read/update, members:invite, connectors:create/update. He cannot be removed at all while his SSO sign-in still works.

SearchSpaceMembership has no tombstone column (no removed_at, is_active, is_blocked on the model — verified at db.py:1811-1857), so the auto-join function has no signal that the user was previously evicted.

The per-request placement was deliberate per the function's docstring ("Safe when auth uses Bearer JWT only: runs from current_active_user as well."), to handle the JWT-only path where the proxy middleware doesn't fire. The trade-off (un-revokability) wasn't called out in the PR description.

Fix

#21 moves the call to UserManager.on_after_register (one-shot at user creation). Both registration paths still trigger it:

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

So new users are still auto-joined exactly once. Removed users stay removed.

Trade-off acknowledged

The original design intentionally caught the case "user's first auth is JWT, no on_after_register ran on the proxy path" via per-request enforcement. The fix loses that case — users who registered BEFORE the SMB workspace existed aren't retroactively auto-joined and need a one-time INSERT INTO search_space_memberships ... from an operator. Worth knowing if your deployment relied on the catch-up behavior.

Refs

cc @jawad-khan @UsamaSadiq for context — not a critique of the merge, the per-request choice was reasonable for the JWT-only-path concern, just an interaction with the DELETE endpoint that's worth documenting in the followup.

🤖 Generated with Claude Code

aznszn pushed a commit that referenced this pull request May 19, 2026
* feat: Add user to default workspace on signin
* fix: deleted unwanted file
* fix: fixed sso workspace
* fix: apply suggestions from code review
Co-authored-by: Usama Sadiq <usama7274@gmail.com>

---------

Co-authored-by: Usama Sadiq <usama.sadiq@arbisoft.com>
aznszn pushed a commit that referenced this pull request May 19, 2026
Regression-guard for SurfSense's architectural immunity to the
cross-app stale-session-on-user-switch class of bug. SurfSense doesn't
need an explicit Rule-2-style "compare upstream identity vs session
identity → flush on mismatch" middleware (like Plane MODSetter#29, Outline #19,
Penpot #18, Twenty #8 do) because `current_active_user` in app.users
already resolves to the upstream identity (proxy_user) over the
persisted session (jwt_user) whenever both are present.

That precedence IS the contract. If a future refactor flips it (or
removes the proxy_user check during a "simplify auth" pass) the
stale-session bug class is silently re-introduced — and type-checks
pass, so it would ship.

These five tests pin the contract:

  1. proxy_user wins when both proxy_user and jwt_user are present
     with different identities (the user-switch scenario)
  2. Falls back to jwt_user when proxy_user is absent (header-absent
     is NOT a logout signal — internal calls, OPTIONS preflight,
     direct backend hits at 127.0.0.1 legitimately arrive without
     a proxy header)
  3. Raises 401 when neither is present (sanity)
  4. Same precedence for current_optional_user
  5. current_optional_user returns None (does not raise) when neither
     is present

Cross-app contract:
  awais786/sso-rules-moneta:openspec/specs/proxy-auth-middleware/spec.md
SurfSense's architectural-immunity reasoning:
  awais786/sso-rules-moneta:surfsense-security.md

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aznszn pushed a commit that referenced this pull request May 19, 2026
* feat: Add user to default workspace on signin
* fix: deleted unwanted file
* fix: fixed sso workspace
* fix: apply suggestions from code review
Co-authored-by: Usama Sadiq <usama7274@gmail.com>

---------

Co-authored-by: Usama Sadiq <usama.sadiq@arbisoft.com>
aznszn pushed a commit that referenced this pull request May 19, 2026
Regression-guard for SurfSense's architectural immunity to the
cross-app stale-session-on-user-switch class of bug. SurfSense doesn't
need an explicit Rule-2-style "compare upstream identity vs session
identity → flush on mismatch" middleware (like Plane MODSetter#29, Outline #19,
Penpot #18, Twenty #8 do) because `current_active_user` in app.users
already resolves to the upstream identity (proxy_user) over the
persisted session (jwt_user) whenever both are present.

That precedence IS the contract. If a future refactor flips it (or
removes the proxy_user check during a "simplify auth" pass) the
stale-session bug class is silently re-introduced — and type-checks
pass, so it would ship.

These five tests pin the contract:

  1. proxy_user wins when both proxy_user and jwt_user are present
     with different identities (the user-switch scenario)
  2. Falls back to jwt_user when proxy_user is absent (header-absent
     is NOT a logout signal — internal calls, OPTIONS preflight,
     direct backend hits at 127.0.0.1 legitimately arrive without
     a proxy header)
  3. Raises 401 when neither is present (sanity)
  4. Same precedence for current_optional_user
  5. current_optional_user returns None (does not raise) when neither
     is present

Cross-app contract:
  awais786/sso-rules-moneta:openspec/specs/proxy-auth-middleware/spec.md
SurfSense's architectural-immunity reasoning:
  awais786/sso-rules-moneta:surfsense-security.md

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