mirror of
https://github.com/tiennm99/DocsGPT.git
synced 2026-10-20 02:16:39 +00:00
fix(oidc): harden session security from PR review (CSRF, revocation, refresh)
Address the high/medium correctness findings on the OIDC/SCIM PR: - Login CSRF / session fixation: bind `state` to a Secure/HttpOnly/SameSite=Lax cookie at login and require the callback to echo it, so a code+state captured from another browser can't silently sign a victim into the attacker's account. - Require `exp` on session JWTs under AUTH_TYPE=oidc (require_exp), so an exp-less HS256 token signed with JWT_SECRET_KEY can't authenticate forever or outlive the denylist. - Denylist now keys revocation on an `iat` watermark instead of a deletable flag: a fresh login (newer iat) self-supersedes a revocation without clearing it, so sessions revoked on other devices stay revoked. Drops the login/SCIM-reactivation denylist-clearing paths (allow_user/allow_idp_sub). - Refresh: gate the disabled-account check on the post-grant identity (not just the old sub); attempt the IdP grant before consuming the refresh token and return a retryable 503 (restoring the token) on transient IdP errors instead of force-logging-out a live session. - Gate the oidc blueprint at request time on AUTH_TYPE=oidc, so non-oidc deployments cleanly 404 these routes instead of 500-ing on an unset OIDC_ISSUER (mirrors SCIM_ENABLED). - Surface revocation write failures: back-channel logout returns 502, and SCIM deactivation rolls back and returns 503, when the denylist write fails — so the IdP retries instead of recording a logout/deprovision that didn't revoke. - Back-channel logout: require `jti`, run the replay check unconditionally, and reject stale `iat` beyond the replay-cache window. - Make migration 0017 idempotent (IF NOT EXISTS) so re-apply can't wedge startup. - SCIM userName matching is case-insensitive (caseExact=false) for the list filter and create-dedup. Tests added/updated across test_oidc.py, test_scim.py, test_auth.py, test_app_routes.py and the SCIM integration test.
This commit is contained in:
1 parent
054f0f1b8b
commit
a3d09e8890
12 files changed
+429
-107
No files matched your search
+20
-8
@@ -63,17 +63,19 @@ def scim_mocks():
|
||||
"application.api.scim.routes.AuthEventsRepository"
|
||||
) as audit_cls, patch(
|
||||
"application.api.scim.routes.deny_user"
|
||||
) as deny_user_mock, patch(
|
||||
"application.api.scim.routes.allow_user"
|
||||
) as allow_user_mock:
|
||||
) as deny_user_mock:
|
||||
conn = MagicMock(name="conn")
|
||||
db_session_mock.return_value.__enter__.return_value = conn
|
||||
db_readonly_mock.return_value.__enter__.return_value = conn
|
||||
# Don't let the mocked context manager swallow exceptions (a real
|
||||
# ``engine.begin()`` __exit__ returns None) — _RevocationUnavailable
|
||||
# must propagate to the blueprint error handler.
|
||||
db_session_mock.return_value.__exit__.return_value = False
|
||||
db_readonly_mock.return_value.__exit__.return_value = False
|
||||
yield SimpleNamespace(
|
||||
users=users_cls.return_value,
|
||||
audit=audit_cls.return_value,
|
||||
deny_user=deny_user_mock,
|
||||
allow_user=allow_user_mock,
|
||||
conn=conn,
|
||||
)
|
||||
|
||||
@@ -355,7 +357,7 @@ class TestReplaceUser:
|
||||
assert response.get_json()["scimType"] == "mutability"
|
||||
scim_mocks.users.set_active.assert_not_called()
|
||||
|
||||
def test_put_active_true_triggers_allow(self, client, scim_settings, scim_mocks):
|
||||
def test_put_active_true_reactivates(self, client, scim_settings, scim_mocks):
|
||||
row = _user_row(active=False)
|
||||
scim_mocks.users.get_by_pk.return_value = row
|
||||
scim_mocks.users.set_active.return_value = {**row, "active": True}
|
||||
@@ -367,7 +369,8 @@ class TestReplaceUser:
|
||||
assert response.status_code == 200
|
||||
assert response.get_json()["active"] is True
|
||||
scim_mocks.users.set_active.assert_called_once_with(USER_PK, True)
|
||||
scim_mocks.allow_user.assert_called_once_with("alice@example.com")
|
||||
# Reactivation does not touch the denylist (the deactivation watermark
|
||||
# already lets a fresh login through on a newer iat).
|
||||
scim_mocks.deny_user.assert_not_called()
|
||||
scim_mocks.audit.insert.assert_called_once_with(
|
||||
"alice@example.com", "scim_reactivated", metadata={"via": "scim"}
|
||||
@@ -400,7 +403,6 @@ class TestReplaceUser:
|
||||
assert response.status_code == 200
|
||||
scim_mocks.users.set_active.assert_not_called()
|
||||
scim_mocks.deny_user.assert_not_called()
|
||||
scim_mocks.allow_user.assert_not_called()
|
||||
scim_mocks.audit.insert.assert_not_called()
|
||||
|
||||
def test_put_missing_user_returns_404(self, client, scim_settings, scim_mocks):
|
||||
@@ -459,7 +461,7 @@ class TestPatchUser:
|
||||
assert response.status_code == 200
|
||||
assert response.get_json()["active"] is True
|
||||
scim_mocks.users.set_active.assert_called_once_with(USER_PK, True)
|
||||
scim_mocks.allow_user.assert_called_once_with("alice@example.com")
|
||||
scim_mocks.deny_user.assert_not_called()
|
||||
scim_mocks.audit.insert.assert_called_once_with(
|
||||
"alice@example.com", "scim_reactivated", metadata={"via": "scim"}
|
||||
)
|
||||
@@ -537,6 +539,16 @@ class TestDeleteUser:
|
||||
response = client.delete(f"/scim/v2/Users/{USER_PK}", headers=AUTH)
|
||||
assert response.status_code == 404
|
||||
|
||||
def test_deactivation_revocation_failure_returns_503(self, client, scim_settings, scim_mocks):
|
||||
# Redis down: deny_user returns False, so the deactivation rolls back and
|
||||
# the IdP gets a 503 to retry instead of a false deprovision success.
|
||||
row = _user_row(active=True)
|
||||
scim_mocks.users.get_by_pk.return_value = row
|
||||
scim_mocks.users.set_active.return_value = {**row, "active": False}
|
||||
scim_mocks.deny_user.return_value = False
|
||||
response = client.delete(f"/scim/v2/Users/{USER_PK}", headers=AUTH)
|
||||
assert response.status_code == 503
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestGroups:
|
||||
|
||||
Reference in new issue
Block a user