fix: prevent stale signup_flow from bypassing tenant access lookup#1173
Open
fix: prevent stale signup_flow from bypassing tenant access lookup#1173
Conversation
Move get_user_tenant_access() before the signup_flow check in google_callback() so returning users with a stale session flag are not incorrectly redirected to onboarding when they already have tenant access. The flag is now cleared with an info log when the user has existing tenants, and only redirects to onboarding when total_access is genuinely zero. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ions - Add mock_access.assert_called_once_with() to verify tenant access lookup is actually invoked (proves the ordering invariant) - Convert test_stale_signup_flow test from 6 mocks to real integration: creates Tenant + User via factories, lets get_user_tenant_access hit the real DB instead of returning predetermined mock values - Add UserFactory to tests/factories/core.py (was missing) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
KonstantinMirin
left a comment
KonstantinMirin
left a comment
There was a problem hiding this comment.
Great work on this, Chris! The fix is clean and the root cause analysis is spot-on — the ordering issue with signup_flow bypassing tenant access was a subtle bug.
A few suggestions that might help:
Existing test infrastructure you could leverage
There's a ui_client fixture in tests/admin/conftest.py (from #1146) that handles Flask app creation, test mode auth, and cleanup:
@pytest.fixture
def ui_client(ui_test_mode):
"""Provide Flask client configured for UI testing."""
...And integration_db handles the DB lifecycle — factory session binding and cleanup via transaction rollback, so you wouldn't need the manual get_db_session() + session.delete() try/finally blocks.
There's also AdminAccountEnv in tests/harness/admin_accounts.py (from #1170) which wraps Flask test_client for admin scenarios with automatic tenant/session management. Might be worth looking at for patterns.
Not a blocker at all — the manual approach works and the tests are solid. Just pointing these out in case they save some boilerplate.
Minor notes
- The mock target
"src.admin.blueprints.auth.get_user_tenant_access"relies on the import being inline ingoogle_callback()— maybe worth a small comment so future refactors don't accidentally break the mock by hoisting the import to module level. UserFactoryis clean and follows all the established patterns. Nice addition.
…-flow-bypasses-tenant-lookup
Address reviewer feedback on PR #1173: - Extract ALL_FACTORIES session binding into a reusable factory_session fixture using SASession(bind=engine) matching the harness pattern - Remove manual session.delete() cleanup (integration_db drops the entire database on teardown) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…aplog - Remove unused `client` fixture from both OAuth callback tests - Add caplog assertion verifying stale-flag code path was executed - Add guard assertion to factory_session (mirrors IntegrationEnv) - Expand factory_session docstring with rationale Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The caplog.at_level() assertion failed in CI despite the production fix working correctly (all 6 behavioral assertions pass). The session state assertions already prove the stale flag was cleared — verifying the log message is an implementation detail, not behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pytest-playwright pinned pytest<9.0.0, blocking the upgrade. Bumped pytest-playwright from 0.6.1 to >=0.7.0 (and playwright from ==1.48.0 to >=1.48.0) to unblock the resolution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Feedback addressed |
…-flow-bypasses-tenant-lookup # Conflicts: # pyproject.toml # uv.lock
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix for #1057
Move get_user_tenant_access() before the signup_flow check in google_callback() so returning users with a stale session flag are not incorrectly redirected to onboarding when they already have tenant access. The flag is now cleared with an info log when the user has existing tenants, and only redirects to onboarding when total_access is genuinely zero.