Repository navigation
Conversation
Closes the offboarding gap in the OIDC/SAML SSO feature: SSO login no longer silently reactivates a deactivated user, deactivation now revokes live sessions immediately, and orgs can wire up SCIM 2.0 (Okta/Azure AD/Google Workspace) for real-time automated deprovisioning. Adds an OIDC back-channel logout endpoint and an optional reconciliation poll as a fallback for OIDC orgs that skip SCIM. SAML Single Logout was not implemented (documented gap in docs/sso.md) since the SAML library has no safe primitive for verifying an inbound signed LogoutRequest. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
…nd env-gated Razorpay tests
go test ./... had five failing packages unrelated to any feature work, left
unfixed since before this branch. Root-caused and fixed each:
- internal/license/payment: Razorpay sandbox integration tests failed
outright without RAZORPAY_TEST_KEY/SECRET instead of skipping; they now
skip cleanly when credentials are absent.
- internal/provider_input/bitbucket: a capture fixture's path was never
updated after a repo-wide docs/raw/ reorg moved it.
- internal/provider_input/azuredevops: test asserted a raw trailing
newline that stripMentionTokens intentionally trims.
- internal/provider_output/github: test predated the intentional
blockquote-reply feature for issue comments.
- internal/mcpagent: classify_test.go still used the shape names
("chat", "count_query") from before they were renamed to
"product_guidance"/"analytics", so the classify call never actually
parsed and every downstream assertion was exercising degraded-fallback
behavior instead of the real branch. Fixing the names surfaced two real
bugs the stale tests had been masking: assembleAnalyticsResponse
discarded per-report notes (no-data/failure sentences) and always
returned a generic fallback, and the analytics-shape dispatch branch
never swapped history[0]'s system prompt like every other branch does,
leaking the previous turn's prompt into analytics turns. Also added one
corrective retry to the multi-interpret call on unparseable JSON,
matching the retry budget the SQL repair phases already have.
LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Settings SSO tab showing the new reconciliation-check toggle (OIDC) and SCIM Provisioning section with a freshly generated, reveal-once token, captured against a running local instance. LiveReview Pre-Commit Check: skipped (iter:1, coverage:0%)
Implement SCIM and SSO User Lifecycle ManagementOverviewImplement SCIM 2.0 for automated user provisioning and deprovisioning. Enhance existing SSO to manage user lifecycles more robustly. Add OIDC back-channel logout and a fallback reconciliation mechanism. Technical Highlights
Impact
|
| CREATE TABLE IF NOT EXISTS scim_tokens ( | ||
| id BIGSERIAL PRIMARY KEY, | ||
| org_id BIGINT NOT NULL REFERENCES orgs(id) ON DELETE CASCADE, | ||
| token_hash VARCHAR(64) NOT NULL UNIQUE, |
There was a problem hiding this comment.
Severity: critical
Ensure token_hash uses a strong, salted hashing algorithm. This is critical for security.
Suggestions:
- Confirm the application-level hashing implementation uses bcrypt, Argon2, or a similar KDF.
- Document the hashing algorithm used for SCIM tokens.
| id BIGSERIAL PRIMARY KEY, | ||
| org_id BIGINT NOT NULL REFERENCES orgs(id) ON DELETE CASCADE, | ||
| token_hash VARCHAR(64) NOT NULL UNIQUE, | ||
| token_prefix VARCHAR(16) NOT NULL, |
There was a problem hiding this comment.
Severity: warning
Verify token_prefix length is sufficient for display but short enough to not reveal too much of the token.
Suggestions:
- Confirm the application generates a prefix that is not guessable and does not expose sensitive information.
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||
| expires_at TIMESTAMPTZ, | ||
| revoked_at TIMESTAMPTZ, | ||
| created_by_user_id BIGINT REFERENCES users(id) ON DELETE SET NULL |
There was a problem hiding this comment.
Severity: warning
Consider adding a revoked_by_user_id column for full auditability of token revocation events.
Suggestions:
- Add
revoked_by_user_id BIGINT REFERENCES users(id) ON DELETE SET NULL.
| org_id BIGINT NOT NULL REFERENCES orgs(id) ON DELETE CASCADE, | ||
| user_id BIGINT NOT NULL REFERENCES users(id) ON DELETE CASCADE, | ||
| provider_type VARCHAR(16) NOT NULL CHECK (provider_type IN ('oidc', 'saml')), | ||
| subject VARCHAR(512) NOT NULL, |
There was a problem hiding this comment.
Severity: warning
A VARCHAR(512) for subject is generally sufficient, but confirm IdP sub claims do not exceed this length.
Suggestions:
- Verify maximum
subclaim length from common IdPs (Okta, Azure AD, Google Workspace) to prevent truncation.
| user_id BIGINT NOT NULL REFERENCES users(id) ON DELETE CASCADE, | ||
| provider_type VARCHAR(16) NOT NULL CHECK (provider_type IN ('oidc', 'saml')), | ||
| subject VARCHAR(512) NOT NULL, | ||
| oidc_refresh_token_enc TEXT, |
There was a problem hiding this comment.
Severity: critical
Ensure strong encryption is used for oidc_refresh_token_enc at the application layer.
Suggestions:
- Confirm the encryption method uses authenticated encryption (e.g., AES-256-GCM) with a robust key management strategy.
- Avoid storing encryption keys directly in code or environment variables without proper rotation.
| user_id BIGINT NOT NULL REFERENCES users(id) ON DELETE CASCADE, | ||
| provider_type VARCHAR(16) NOT NULL CHECK (provider_type IN ('oidc', 'saml')), | ||
| subject VARCHAR(512) NOT NULL, | ||
| oidc_refresh_token_enc TEXT, |
There was a problem hiding this comment.
Severity: warning
Consider if TEXT is the most efficient storage for encrypted tokens, or if BYTEA would be more appropriate.
Suggestions:
- If the encrypted token is binary data,
BYTEAmight offer slight storage or performance benefits overTEXT.
| last_checked_at TIMESTAMPTZ, | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||
| updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||
| UNIQUE (org_id, provider_type, subject) |
There was a problem hiding this comment.
Severity: info
The unique constraint on (org_id, provider_type, subject) is crucial for preventing identity collisions and ensuring correct mapping.
| UNIQUE (org_id, provider_type, subject) | ||
| ); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_sso_identities_user_id ON sso_identities(user_id); |
There was a problem hiding this comment.
Severity: info
An index on user_id is beneficial for quickly finding all SSO identities associated with a specific user.
| ALTER TABLE org_sso_configs | ||
| ADD COLUMN IF NOT EXISTS reconciliation_enabled BOOLEAN NOT NULL DEFAULT FALSE, | ||
| ADD COLUMN IF NOT EXISTS reconciliation_interval TEXT NOT NULL DEFAULT 'daily' | ||
| CHECK (reconciliation_interval IN ('hourly', 'daily', 'weekly')), |
There was a problem hiding this comment.
Severity: info
The CHECK constraint on reconciliation_interval is a good practice for data validation.
| | `auth.logout` | `auth` | session | info | Signed out | A user ended a session. | | ||
| | `auth.password.change` | `auth` | user | warning | Password changed | A user changed their password. | | ||
| | `auth.password.reset.complete` | `auth` | user | warning | Password reset completed | A user completed a required password reset. | | ||
| | `auth.session.revoke` | `auth` | session | warning | Sessions revoked | A user's active sessions and refresh tokens were revoked, typically following deactivation. | |
There was a problem hiding this comment.
Severity: critical
Ensure auth.session.revoke is triggered whenever a user's sessions are invalidated, especially during deactivation or password resets.
Suggestions:
- Verify all code paths that revoke user sessions explicitly log this event.
| | `ruleset.create` | `repository` | ruleset | warning | Ruleset created | A CI ruleset was created. | | ||
| | `ruleset.delete` | `repository` | ruleset | warning | Ruleset deleted | A CI ruleset was deleted. | | ||
| | `ruleset.update` | `repository` | ruleset | warning | Ruleset updated | A CI ruleset was changed. | | ||
| | `scim.token.create` | `auth` | scim_token | warning | SCIM token created | An org's SCIM bearer token was generated or rotated. | |
There was a problem hiding this comment.
Severity: info
The new SCIM audit events are crucial for tracking identity lifecycle management and detecting suspicious activity.
| } | ||
| h.recordAuthEvent(c, auditlog.Event{ | ||
| OrgID: orgID, | ||
| ActorUserID: &userID, |
There was a problem hiding this comment.
Severity: warning
The ActorKindMember might be misleading here. The actor is the IdP system, not a LiveReview member. Consider auditlog.ActorKindWebhook or auditlog.ActorKindSystem for clarity.
Suggestions:
- Change
ActorKindtoauditlog.ActorKindWebhookorauditlog.ActorKindSystemto accurately reflect the IdP as the initiator of the logout event.
| if method == "sso_saml" { | ||
| provider = sso.ProviderSAML | ||
| } | ||
| if err := h.ssoIdentities.Upsert(orgID, user.ID, provider, identity.Subject, identity.RefreshToken); err != nil { |
There was a problem hiding this comment.
Severity: warning
Logging this as non-fatal is appropriate, but consider adding a metric or alert for persistent failures to record identity mappings. This could indicate a deeper issue affecting deprovisioning capabilities.
Suggestions:
- Add a Prometheus metric or similar observability signal to track
ssoIdentities.Upsertfailures. - Consider an alert if these failures exceed a certain threshold to prevent silent degradation of deprovisioning.
| } else if err != nil { | ||
| return err | ||
| } else { | ||
| if !isActive { |
There was a problem hiding this comment.
Severity: critical
This is a critical correctness fix. Preventing SSO from reactivating deactivated accounts closes a significant security and compliance gap.
Suggestions:
- Ensure this behavior is thoroughly covered by unit and integration tests, especially for various user states (new, active, deactivated, reactivated).
| created = true | ||
| } | ||
| if _, err := tx.Exec(`UPDATE users SET default_org_id = COALESCE(default_org_id, $1), is_active = TRUE WHERE id = $2`, orgID, userID); err != nil { | ||
| if _, err := tx.Exec(`UPDATE users SET default_org_id = COALESCE(default_org_id, $1) WHERE id = $2`, orgID, userID); err != nil { |
There was a problem hiding this comment.
Severity: critical
Removing is_active = TRUE from the update statement is essential. This ensures that SSO login no longer implicitly reactivates users, reinforcing the explicit deactivation policy.
Suggestions:
- Verify that all other paths that should reactivate a user (e.g., admin UI) explicitly set
is_active = TRUE.
| err = ts.db.QueryRow(` | ||
| SELECT id, email, password_hash, default_org_id, created_at, updated_at | ||
| FROM users WHERE id = $1 | ||
| FROM users WHERE id = $1 AND is_active = true |
There was a problem hiding this comment.
Severity: warning
Adding AND is_active = true to the user lookup is an excellent defense-in-depth measure. It ensures that even if a token was not revoked, it cannot be used by a deactivated user.
Suggestions:
- Ensure this behavior is documented as part of the overall deactivation and session management strategy.
| // adding "scim" to a cloud plan's Features later needs no code change here. | ||
| func (s *Server) orgHasSCIMFeature(orgID int64) bool { | ||
| var planCode string | ||
| _ = s.db.QueryRow(`SELECT current_plan_code FROM org_billing_state WHERE org_id = $1`, orgID).Scan(&planCode) |
There was a problem hiding this comment.
Severity: warning
Ignoring the error from QueryRow can lead to planCode being an empty string if the query fails, potentially causing HasFeature to return an incorrect result.
Suggestions:
- Handle the error returned by
s.db.QueryRow. If an error occurs, log it and returnfalseor an appropriate default. - Consider if a missing
org_billing_stateentry should imply no SCIM feature or an error state.
| return c.JSON(http.StatusUnauthorized, scim.NewError(http.StatusUnauthorized, "invalid, expired, or revoked token")) | ||
| } | ||
| if !s.orgHasSCIMFeature(orgID) { | ||
| return c.JSON(http.StatusForbidden, scim.NewError(http.StatusForbidden, "SCIM requires an enterprise self-hosted license")) |
There was a problem hiding this comment.
Severity: warning
The error message 'SCIM requires an enterprise self-hosted license' contradicts the orgHasSCIMFeature logic, which checks the plan code. This message is too specific.
Suggestions:
- Align the error message with the
orgHasSCIMFeaturelogic. A more generic message like 'SCIM is not enabled for your organization's plan' would be more accurate. - If SCIM is strictly self-hosted, remove the
orgHasSCIMFeaturecheck and rely solely onapimiddleware.IsCloudMode().
| return strings.ToLower(strings.TrimSpace(e.Value)) | ||
| } | ||
| } | ||
| if len(r.Emails) > 0 && r.Emails[0].Value != "" { |
There was a problem hiding this comment.
Severity: warning
Falling back to the first email if no primary is explicitly set might not be robust. Some IdPs might not guarantee the first email is the most relevant.
Suggestions:
- Consult SCIM RFC 7643 for guidance on handling multi-valued attributes without a primary flag. If no standard fallback exists, consider a more explicit error or configuration.
- Document this assumption about the first email for future maintainers.
| // exist" before creating - userName eq "x@y.com" or emails.value eq | ||
| // "x@y.com". Full SCIM filter grammar (and/or/not, other attributes) is not | ||
| // needed for this use case and isn't parsed. | ||
| func parseEmailFilter(filter string) string { |
There was a problem hiding this comment.
Severity: warning
The parseEmailFilter function is very strict and might break if IdPs send slightly different filter formats (e.g., extra whitespace, different casing for eq).
Suggestions:
- Consider using a regular expression or a more flexible parsing library if more robust filter parsing is needed.
- Add more test cases for
parseEmailFilterto cover variations in whitespace and casing aroundeqand attribute names. - Explicitly document the exact filter formats supported and any known limitations for IdP integrations.
| sqlRows, err := s.db.Query(` | ||
| SELECT u.id, u.email, u.is_active, u.first_name, u.last_name, u.created_at, u.updated_at | ||
| FROM users u JOIN user_roles ur ON ur.user_id = u.id | ||
| WHERE ur.org_id = $1 ORDER BY u.id LIMIT 200 |
There was a problem hiding this comment.
Severity: warning
The hardcoded LIMIT 200 without proper pagination (startIndex, count) is not fully SCIM 2.0 compliant. IdPs might expect to retrieve all users or specify pagination parameters.
Suggestions:
- Implement SCIM pagination by honoring
startIndexandcountquery parameters, and updateListResponseaccordingly. - If full pagination is out of scope for now, explicitly document this limitation for IdP integrations and consider returning a
400 Bad RequestifstartIndexorcountare provided but not supported.
| return c.JSON(http.StatusInternalServerError, scim.NewError(http.StatusInternalServerError, "failed to provision user")) | ||
| } | ||
| if created && s.auditRecorder != nil { | ||
| _ = s.auditRecorder.Webhook(c.Request().Context(), orgID, auditlog.ActionSCIMUserCreate, auditlog.Details("email", email)) |
There was a problem hiding this comment.
Severity: warning
Ignoring the error from s.auditRecorder.Webhook means audit log failures are silent.
Suggestions:
- Log the error from
s.auditRecorder.Webhookto ensure audit failures are visible, even if they don't block the main operation. - Consider a retry mechanism for audit logging if its reliability is critical.
| if err != nil { | ||
| return c.JSON(http.StatusBadRequest, scim.NewError(http.StatusBadRequest, "invalid id")) | ||
| } | ||
| if _, err := s.userService.GetUserInOrg(orgID, userID); err != nil { |
There was a problem hiding this comment.
Severity: info
This GetUserInOrg call is redundant. The user will be fetched again at line 282 after the patch operations. Fetch once and pass the user object.
Suggestions:
- Fetch the user once at the beginning of the handler. Store the result and reuse it for the
if err != nilcheck and the final response. - Pass the fetched user object to the
DeactivateUserInOrgBySCIMandReactivateUserInOrgBySCIMmethods if they need it for internal logic.
| continue | ||
| } | ||
| if active { | ||
| if err := s.userService.ReactivateUserInOrgBySCIM(userID); err != nil { |
There was a problem hiding this comment.
Severity: warning
ReactivateUserInOrgBySCIM only takes userID. It should ideally take orgID as well to ensure the operation is strictly scoped to the authenticated organization.
Suggestions:
- Modify
ReactivateUserInOrgBySCIMto acceptorgIDand include it in its internal query to enforce organizational boundaries. - Alternatively, ensure
userServicemethods always perform anorgIDcheck internally when operating on auserID.
| if err := s.userService.ReactivateUserInOrgBySCIM(userID); err != nil { | ||
| return c.JSON(http.StatusInternalServerError, scim.NewError(http.StatusInternalServerError, "failed to reactivate user")) | ||
| } | ||
| if s.auditRecorder != nil { |
There was a problem hiding this comment.
Severity: warning
Ignoring the error from s.auditRecorder.Webhook means audit log failures are silent.
Suggestions:
- Log the error from
s.auditRecorder.Webhookto ensure audit failures are visible, even if they don't block the main operation. - Consider a retry mechanism for audit logging if its reliability is critical.
| _ = s.auditRecorder.Webhook(c.Request().Context(), orgID, auditlog.ActionSCIMUserReactivate, auditlog.Details("user_id", userID)) | ||
| } | ||
| } else { | ||
| if err := s.userService.DeactivateUserInOrgBySCIM(userID); err != nil { |
There was a problem hiding this comment.
Severity: warning
DeactivateUserInOrgBySCIM only takes userID. It should ideally take orgID as well to ensure the operation is strictly scoped to the authenticated organization.
Suggestions:
- Modify
DeactivateUserInOrgBySCIMto acceptorgIDand include it in its internal query to enforce organizational boundaries. - Alternatively, ensure
userServicemethods always perform anorgIDcheck internally when operating on auserID.
| if err := s.userService.DeactivateUserInOrgBySCIM(userID); err != nil { | ||
| return c.JSON(http.StatusInternalServerError, scim.NewError(http.StatusInternalServerError, "failed to deactivate user")) | ||
| } | ||
| if s.auditRecorder != nil { |
There was a problem hiding this comment.
Severity: warning
Ignoring the error from s.auditRecorder.Webhook means audit log failures are silent.
Suggestions:
- Log the error from
s.auditRecorder.Webhookto ensure audit failures are visible, even if they don't block the main operation. - Consider a retry mechanism for audit logging if its reliability is critical.
| if err := pc.RequireOrgOwner(); err != nil { | ||
| return c.JSON(http.StatusForbidden, map[string]string{"error": "only org owners can manage SCIM provisioning"}) | ||
| } | ||
| if apimiddleware.IsCloudMode() { |
There was a problem hiding this comment.
Severity: warning
This requireSCIMConfigAccess function duplicates significant logic from requireSSOAccess in sso_config.go. Consolidate common access checks.
Suggestions:
- Extract the common permission and feature checks into a reusable middleware or helper function.
- Consider a generic
requireOrgAdminFeatureAccess(c echo.Context, orgID int64, feature string)function.
| return c.JSON(http.StatusForbidden, map[string]string{"error": "only org owners can manage SCIM provisioning"}) | ||
| } | ||
| if apimiddleware.IsCloudMode() { | ||
| return c.JSON(http.StatusForbidden, map[string]string{"error": "SCIM is available on self-hosted enterprise deployments only"}) |
There was a problem hiding this comment.
Severity: warning
The apimiddleware.IsCloudMode() check and error message here ('SCIM is available on self-hosted enterprise deployments only') is redundant and potentially inconsistent with orgHasSCIMFeature.
Suggestions:
- Rely solely on
s.orgHasSCIMFeature(orgID)for feature gating, as it's designed to be flexible with plan codes. - If SCIM is truly self-hosted only, ensure
orgHasSCIMFeaturealways returns false in cloud mode, or remove theorgHasSCIMFeaturecheck for this specific access control.
| SAMLMetadataSet bool `json:"saml_metadata_set"` | ||
|
|
||
| ReconciliationEnabled bool `json:"reconciliation_enabled,omitempty"` | ||
| ReconciliationInterval string `json:"reconciliation_interval,omitempty"` |
There was a problem hiding this comment.
Severity: warning
Consider using an enum or a custom type for ReconciliationInterval instead of string to enforce valid values at compile time.
Suggestions:
- Define a
type ReconciliationInterval stringwith constants forHourly,Daily,Weekly. - Implement
UnmarshalJSONandMarshalJSONfor the custom type to handle serialization.
| ReconciliationInterval: cfg.ReconciliationInterval, | ||
| } | ||
| if cfg.LastReconciledAt != nil { | ||
| resp.LastReconciledAt = cfg.LastReconciledAt.Format(time.RFC3339) |
There was a problem hiding this comment.
Severity: warning
Ensure time.RFC3339 is the expected and consistent date format across all API responses.
Suggestions:
- Add a comment or internal documentation confirming
time.RFC3339as the standard API date format. - Consider defining a global constant for the preferred API date format to ensure consistency.
| if err := c.Bind(&req); err != nil { | ||
| return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid request body"}) | ||
| } | ||
| if req.ReconciliationInterval != "" && req.ReconciliationInterval != "hourly" && req.ReconciliationInterval != "daily" && req.ReconciliationInterval != "weekly" { |
There was a problem hiding this comment.
Severity: warning
This validation duplicates the logic in sso_config.go:63 if ReconciliationInterval were a custom type.
Suggestions:
- If
ReconciliationIntervalis made a custom type, move this validation into itsUnmarshalJSONmethod. - Alternatively, extract the validation logic into a shared helper function.
| SAMLIDPMetadataURL: req.SAMLIDPMetadataURL, | ||
| SAMLIDPMetadataXML: req.SAMLIDPMetadataXML, | ||
| SAMLIDPEntityID: req.SAMLIDPEntityID, | ||
| ReconciliationEnabled: provider == sso.ProviderOIDC && req.ReconciliationEnabled, |
There was a problem hiding this comment.
Severity: info
This logic correctly restricts reconciliation to OIDC providers, as SAML does not support this mechanism.
| func (uh *UserHandlers) ReactivateUser(c echo.Context) error { | ||
| permCtx := auth.MustGetPermissionContext(c) | ||
|
|
||
| if !permCtx.HasPermission(auth.PermissionDeleteUsers) { |
There was a problem hiding this comment.
Severity: warning
Using PermissionDeleteUsers for reactivation is a strong permission. Confirm this is the intended authorization level.
Suggestions:
- If reactivation is a distinct administrative action, consider creating a more specific permission like
PermissionReactivateUsers. - Document the rationale for mapping reactivation to
PermissionDeleteUsers.
| } | ||
|
|
||
| // DeactivateUserInOrg deactivates a user in a specific organization | ||
| // DeactivateUserInOrg deactivates a user in a specific organization and |
There was a problem hiding this comment.
Severity: info
Revoking active sessions is a critical security enhancement for user deactivation.
| var isActive bool | ||
| err := us.store.TxQueryRow(tx, `SELECT id, is_active FROM users WHERE email = $1`, email).Scan(&existingUserID, &isActive) | ||
| if err == sql.ErrNoRows { | ||
| randBytes := make([]byte, 32) |
There was a problem hiding this comment.
Severity: info
Generating a random, unusable password for SCIM-provisioned users is correct, as they should only authenticate via IdP.
| } | ||
|
|
||
| userID = existingUserID | ||
| if !isActive { |
There was a problem hiding this comment.
Severity: info
Returning ErrSCIMAccountDeactivated prevents silent reactivation of manually deactivated accounts, which is a critical security and data integrity safeguard.
|
|
||
| // ReactivateUserInOrgBySCIM mirrors DeactivateUserInOrgBySCIM for PATCH active:true. | ||
| func (us *UserService) ReactivateUserInOrgBySCIM(userID int64) error { | ||
| _, err := us.db.Exec(` |
There was a problem hiding this comment.
Severity: critical
This function uses us.db.Exec directly, not us.store.WithTx. This is inconsistent with DeactivateUserInOrgBySCIM and ReactivateUserInOrg and can lead to atomicity issues if future changes add more database operations.
Suggestions:
- Wrap this operation in
us.store.WithTxto ensure consistency and atomicity, mirroringDeactivateUserInOrgBySCIM. - Ensure the caller handles potential partial updates if this is intended to be a standalone operation.
| meta(ActionIdentityAdd, CategoryAuth, "identity", "Identity linked", "A login identity was linked to a user.", SeverityWarning), | ||
| meta(ActionIdentityRemove, CategoryAuth, "identity", "Identity unlinked", "A login identity was removed from a user.", SeverityWarning), | ||
| meta(ActionSSOProvision, CategoryAuth, "user", "SSO account provisioned", "An account was provisioned through SSO.", SeverityInfo), | ||
| sensitive(meta(ActionSessionRevoke, CategoryAuth, "session", "Sessions revoked", "A user's active sessions and refresh tokens were revoked, typically following deactivation.", SeverityWarning)), |
There was a problem hiding this comment.
Severity: info
Marking session revocation as SeverityWarning and sensitive is appropriate given its impact on user access and security.
LiveReview Pre-Commit Check: ran (iter:1, coverage:0%)
livereviewbot
left a comment
livereviewbot
left a comment
There was a problem hiding this comment.
❌ LiveReview policy: Not approved (blocked at Risk score) · Review #13484
| Stage | Result | Detail |
|---|---|---|
| Eligibility | ✅ | author level 1 >= required 1 |
| Static rules | ✅ | no blocking rulesets configured |
| Risk score | ❌ | Risk score 1.00 (P100) is above the cutoff of 1.00 (P20) |
| LLM classification | ✅ | class "logic-change", confidence 10 |
| Final decision | ❌ | risk: Risk score 1.00 (P100) is above the cutoff of 1.00 (P20) |
🔍 LiveReview
⚙️ Customize risk-based approval policy for this repository or your organization.
The problem in one sentence
PR #146 gave self-hosted enterprise customers SSO login through their identity provider (Okta, Azure AD, Google Workspace, Keycloak...), but it only covered getting in — there was no way for LiveReview to find out, automatically, when someone should be locked out.
Who hits this, and how
The IT admin at an enterprise customer, during offboarding: they disable or delete someone in Okta. Before this PR, that action had zero effect on LiveReview — the person's browser session kept working until its JWT happened to expire on its own, and if they tried logging in again via SSO, LiveReview would silently re-activate their account even if an admin had deactivated them by hand.
The org owner inside LiveReview's Settings UI, who previously had exactly one offboarding tool: manually click "Deactivate" on a user. That flag was cosmetic — it never touched their live session tokens, so it didn't actually cut them off.
The identity provider itself, which, for admins who wire up SCIM (the standard Okta/Azure AD/Google Workspace push-based user-lifecycle protocol), can now tell LiveReview directly and immediately: "this person is gone."
This PR gives all three a real answer, layered so the floor is solid even when an org skips the fancier options:
How it works, mechanically
Four independent layers, each one a fallback for when the layer above isn't configured or doesn't fire in time:
Deactivation now means something.
DeactivateUserInOrgand the new SSO login path both revoke every row inauth_tokensfor that user in the same DB transaction as the deactivation — no gap between "admin clicked the button" and "the token stops working." Token validation also independently re-checksusers.is_activeon every request, as a second line of defense in case any future code path forgets to revoke explicitly.SCIM 2.0 (
internal/scim/,internal/api/scim_handlers.go) — the real, push-based answer. An org generates one static bearer token (Settings → SSO → SCIM Provisioning → "Generate Token", reveal-once like an API key) and pastes it plus a base URL into their IdP's SCIM app config. The IdP then callsGET/POST /scim/v2/UsersandPATCH /scim/v2/Users/:idwhenever it creates, updates, or disables someone —{"active": false}deprovisions them in LiveReview in whatever time the IdP's own sync interval is (typically under a minute).Reconciliation poll (
internal/jobqueue/sso_reconciliation_worker.go) — a fallback for OIDC orgs that never set up SCIM. If enabled, LiveReview requests anoffline_accessrefresh token at login and periodically (hourly/daily/weekly, your choice) attempts to silently refresh it. The IdP rejects a refresh for a disabled/deleted account (invalid_grant), which LiveReview treats exactly like a SCIM deactivation. This is only available for OIDC — SAML has no equivalent standing credential, so SAML-only orgs have to rely on SCIM or manual deactivation.OIDC back-channel logout — if the IdP supports immediately pushing a signed logout event (Keycloak does; so do many enterprise IdPs),
POST /api/v1/auth/sso/oidc/:org_id/backchannel-logoutverifies it and revokes the session in real time, no polling involved.What's deliberately not done
SAML Single Logout (SLO) is not implemented. The SAML library this project uses (
crewjam/samlv0.5.1) only exposes signature verification for the full response/assertion flow — it has no safe, public primitive for verifying a signed, IdP-initiatedLogoutRequeston its own. Hand-rolling XML-dsig verification for one message type seemed like the wrong tradeoff to ship under time pressure: a broken verifier here means anyone could force-log-out an arbitrary user by forging an "IdP" logout request. SAML orgs fall back to manual deactivation or SCIM for this gap — documented plainly indocs/sso.mdso it isn't a silent limitation.Data model
Three new tables, additive only (no changes to the existing
org_sso_configscolumns from PR #146, only new ones appended):scim_tokens—org_id, hashed bearer token + prefix (same pattern as the existing per-user API keys),revoked_at. One live token per org; rotating issues a new row and revokes the old one.sso_identities—(org_id, provider_type, subject) → user_id, plus an encrypted OIDC refresh token when reconciliation is enabled. This exists because an inbound logout/reconciliation event only ever carries the IdP's own subject identifier, never a LiveReview user id — this table is the join between the two. Populated on every successful SSO login.org_sso_configsgainsreconciliation_enabled,reconciliation_interval(hourly/daily/weekly),last_reconciled_at.Every new mutating action (SCIM user create/update/deactivate, SCIM token create/revoke, session revoke, reactivate) got its own audit catalog entry per this repo's audit-logging rules — SCIM-triggered events are attributed to the system (
auditRecorder.Webhook) since there's no LiveReview user session behind them, same category as a provider webhook.Screenshots
Settings → SSO — new SCIM Provisioning section (baseline, nothing configured yet)
Settings → SSO — reconciliation check enabled + a freshly generated SCIM token (reveal-once)
Also in this PR: fixed the full pre-existing test suite
Unrelated to the SSO work, but fixed in the same branch per a direct request:
go test ./...had five packages failing before this branch, left unfixed from earlier sessions. Root-caused and fixed each (Razorpay sandbox tests now skip cleanly without credentials instead of failing; a stale test fixture path; two tests asserting on behavior that production deliberately changed out from under them; and — the real find —internal/mcpagent's classify tests still used shape names from before a rename, which had been silently masking two actual production bugs: droppednotestext in analytics responses, and a missing system-prompt swap on the analytics dispatch branch). Full detail in that commit's message.Test plan
go build ./...— cleango test ./...— fully clean, including the previously-failing packagesgo test ./internal/auditlog/...— catalog/docs contract cleannpx tsc --noEmit— cleanenterprise-selfhostedplan, enabled the OIDC reconciliation toggle, generated/viewed a SCIM tokenLiveReview auto-approval decisions