Open
Conversation
📝 WalkthroughWalkthroughPromotes OIDC SSO into core, adds core SSO module (routes, service, models), splits SAML into an enterprise module with SAML service/routes, updates SPI registrations, threads Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CoreRoutes as "SsoRoutes (Core)"
participant CoreService as "SsoService"
participant IdP as "OIDC IdP"
participant DB as "Database"
participant Auth as "AuthService"
Client->>CoreRoutes: POST /auth/sso/init (email/orgSlug)
CoreRoutes->>CoreService: initSso(email, orgSlug)
CoreService->>DB: load SSO config, org
CoreService->>CoreService: generateSecureState(orgId)
CoreService-->>CoreRoutes: SsoInitResponse (redirect URL, state)
CoreRoutes-->>Client: Redirect to IdP
Client->>IdP: Authenticate
IdP-->>Client: Redirect with code+state
Client->>CoreRoutes: GET /auth/sso/oidc/callback?code=&state=
CoreRoutes->>CoreService: handleOidcCallback(code, state)
CoreService->>IdP: Exchange code for tokens
IdP-->>CoreService: ID token / user claims
CoreService->>DB: findOrCreateSsoUser / ensure membership
CoreService->>Auth: generateToken(user)
Auth-->>CoreService: JWT
CoreService-->>CoreRoutes: SsoCallbackData
CoreRoutes-->>Client: Set auth cookie, redirect to frontend
sequenceDiagram
participant Client
participant SamlRoutes as "SamlRoutes (EE)"
participant SamlService
participant IdP as "SAML IdP"
participant DB as "Database"
Client->>SamlRoutes: POST /auth/sso/init/saml (email/orgSlug)
SamlRoutes->>SamlService: initSaml(email, orgSlug)
SamlService->>DB: load SAML config
SamlService->>SamlService: generateSecureState(orgId) & generateSamlRequest()
SamlService-->>SamlRoutes: SsoInitResponse (redirect URL, state)
SamlRoutes-->>Client: Redirect to SAML IdP
Client->>IdP: Authenticate
IdP-->>Client: POST SAML Response to ACS
Client->>SamlRoutes: POST /auth/sso/acs
SamlRoutes->>SamlService: handleSamlResponse(response, relayState)
SamlService->>DB: validate request nonce, load config
SamlService->>DB: findOrCreateSsoUser / ensure membership
SamlService-->>SamlRoutes: SsoCallbackData
SamlRoutes-->>Client: Set auth cookie, redirect to frontend
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/kotlin/com/moneat/auth/services/AuthService.kt (1)
538-548:⚠️ Potential issue | 🟠 MajorPopulate org context in every
AuthResponse.user.This path now sets
organizationSlug/orgRole, butsignup(),login(), andrefreshToken()in this same file still buildUserResponsewithout them. The dashboard now gates Team/SSO offuser.orgRole, so the same user can get different UI capabilities depending on which auth response last hydrated state. Please return the same org context from allAuthServiceUserResponsebuilders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/com/moneat/auth/services/AuthService.kt` around lines 538 - 548, AuthService builds UserResponse without org context in signup(), login(), and refreshToken() causing inconsistent UI; update those UserResponse constructors to include the same organizationSlug and orgRole values used elsewhere (i.e. compute the final organization slug (finalSlug or equivalent from membership.organization?.slug) and use membership.role for orgRole) and pass them into the UserResponse instead of the current null, so every auth path returns a UserResponse populated with organizationSlug and orgRole.
🧹 Nitpick comments (7)
ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt (3)
157-204: SimplifygetSamlMetadatawith early return andrequireNotNull.The method structure can be simplified. Also, the null check at line 183 is redundant after the query - if
orgSlugis null, the exception is thrown at line 178-180, sossoConfigbeing null is the actual check needed.♻️ Proposed fix
fun getSamlMetadata(orgSlug: String?): String = transaction { - val ssoConfig = - if (orgSlug != null) { - val org = - Organizations - .selectAll() - .where { Organizations.slug eq orgSlug } - .firstOrNull() - ?: throw IllegalArgumentException( - "Organization not found" - ) - - SsoConfigurations - .selectAll() - .where { - (SsoConfigurations.organizationId eq org[Organizations.id]) and - (SsoConfigurations.providerType eq "saml") and - (SsoConfigurations.isEnabled eq true) - }.firstOrNull() - } else { - throw IllegalArgumentException( - "Organization slug is required" - ) - } - - if (ssoConfig == null) { - throw IllegalArgumentException( - "SAML SSO is not configured for this organization" - ) - } + requireNotNull(orgSlug) { "Organization slug is required" } + + val org = Organizations + .selectAll() + .where { Organizations.slug eq orgSlug } + .firstOrNull() + ?: throw IllegalArgumentException("Organization not found") + + val ssoConfig = requireNotNull( + SsoConfigurations + .selectAll() + .where { + (SsoConfigurations.organizationId eq org[Organizations.id]) and + (SsoConfigurations.providerType eq "saml") and + (SsoConfigurations.isEnabled eq true) + }.firstOrNull() + ) { "SAML SSO is not configured for this organization" } val settings = buildSamlSettings(ssoConfig)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt` around lines 157 - 204, Refactor getSamlMetadata to simplify control flow: validate orgSlug early with requireNotNull (or throw) and load the organization via Organizations.selectAll().where { Organizations.slug eq orgSlug }.firstOrNull(), then fetch the SSO row from SsoConfigurations (filtering organizationId, providerType "saml", isEnabled) and use requireNotNull on that result (instead of the later if (ssoConfig == null) block); keep calling buildSamlSettings(ssoConfig), validate metadata with Saml2Settings.validateMetadata, log errors with logger.error and throw if invalid, and return metadata—this removes nested if/else and the redundant null check while preserving existing validations.
74-78: UserequireNotNull()for null checks with exceptions.This pattern can be simplified using the idiomatic
requireNotNull().♻️ Proposed fix
- if (ssoConfig == null) { - throw IllegalArgumentException( - "SAML SSO is not configured for this organization" - ) - } + requireNotNull(ssoConfig) { + "SAML SSO is not configured for this organization" + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt` around lines 74 - 78, Replace the manual null check for ssoConfig in SamlService with the idiomatic Kotlin requireNotNull usage: use requireNotNull(ssoConfig) with the same message ("SAML SSO is not configured for this organization") and assign the result to a non-null val (or rebind) so subsequent code uses the non-null value; this keeps the IllegalArgumentException semantics but simplifies the check and makes the intent clearer.
38-42: Userequire()for argument preconditions.Per coding guidelines,
require()should be used for argument preconditions in helper/library code. This service method validates input arguments and should use the idiomatic Kotlin approach.♻️ Proposed fix
- if (email == null && orgSlug == null) { - throw IllegalArgumentException( - "Either email or orgSlug must be provided" - ) - } + require(!(email == null && orgSlug == null)) { + "Either email or orgSlug must be provided" + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt` around lines 38 - 42, Replace the manual null-check and throw with Kotlin's idiomatic require() precondition: in SamlService.kt where the code currently checks if (email == null && orgSlug == null) and throws IllegalArgumentException("Either email or orgSlug must be provided"), change it to use require(email != null || orgSlug != null) { "Either email or orgSlug must be provided" } so the argument validation uses the standard Kotlin helper instead of an explicit throw; update the surrounding method in SamlService accordingly.ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.kt (1)
27-96: Consider extracting error handling to reduce cognitive complexity.Static analysis reports cognitive complexity of 18 (threshold: 15). The repetitive try-catch patterns across routes contribute to this. While the current code is readable and consistent, you could optionally extract a helper for error handling.
This is a minor concern for a route file with standard error handling patterns. The current implementation is clear and maintainable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.kt` around lines 27 - 96, The three route handlers in samlRoutes() (post("/init/saml"), get("/saml/metadata"), post("/saml/acs")) repeat identical try/catch logic causing high cognitive complexity; extract a reusable helper (e.g., withErrorHandling or handleSamlRoute) that accepts a suspend lambda and centralizes the IllegalArgumentException and generic Exception handling/logging/response behavior so you can replace the inline try/catch in each handler while still invoking SamlService.initSaml, SamlService.getSamlMetadata, and SamlService.handleSamlResponse and preserving AuthCookieUtils.setAuthCookie and the same redirects/responses.backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt (2)
112-114: Consider usingrequire()for preconditions in service code.SonarCloud suggests replacing this
if-throwwithrequire(). Since this is service/library code (not a route handler), usingrequire()is idiomatic Kotlin for argument validation. As per coding guidelines,require()is appropriate "for internal argument preconditions in helper/library code."♻️ Proposed refactor
-if (email == null && orgSlug == null) { - throw IllegalArgumentException("Either email or orgSlug must be provided") -} +require(email != null || orgSlug != null) { + "Either email or orgSlug must be provided" +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt` around lines 112 - 114, Replace the manual if-throw precondition in SsoService (where the method checks email and orgSlug) with Kotlin's idiomatic require(): instead of the current if (email == null && orgSlug == null) throw IllegalArgumentException(...), call require(email != null || orgSlug != null) { "Either email or orgSlug must be provided" } so the argument validation uses require() for this service/library-level precondition.
266-398: High cognitive complexity (24 vs. 15 allowed) — consider decomposition.SonarCloud flags this method for exceeding cognitive complexity limits. The function handles authorization, validation for multiple provider types, and persistence (both insert and update paths). Consider extracting:
validateSsoConfigRequest()for provider-specific validationpersistSsoConfig()for the insert/update logicThis would improve readability and testability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt` around lines 266 - 398, The configureSso method in SsoService is too complex; extract provider-specific validation and DB persistence into helper functions to reduce cognitive complexity: create validateSsoConfigRequest(providerType, request) (called from configureSso before transactions) that encapsulates SAML/OIDC field checks and UrlValidator usage, and create persistSsoConfig(organizationId, request, providerType, baseUrl) that performs the encryptSecret logic, computes effectiveRequireSso, and executes the insert-or-update against SsoConfigurations (preserving current field assignments and updatedAt behavior), then have configureSso call these helpers and return getSsoConfig(organizationId) as before. Ensure new helpers reference the same symbols used now (SsoProviderType, UrlValidator, encryptSecret, FeatureRegistry.hasModule("SAML"), SsoConfigurations, getSsoConfig) and keep all transactional DB access inside persistSsoConfig to maintain existing behavior.dashboard/src/hooks/useAuth.ts (1)
47-51: UseglobalThis.sessionStoragefor SSR compatibility.The hook accesses
sessionStoragedirectly. Per coding guidelines, preferglobalThis.sessionStorageover bare globals for SSR compatibility.♻️ Proposed fix
// Keep session flag in sync - sessionStorage.setItem('authenticated', 'true') + globalThis.sessionStorage.setItem('authenticated', 'true') }) .catch(() => { setUser(null) - sessionStorage.removeItem('authenticated') + globalThis.sessionStorage.removeItem('authenticated') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dashboard/src/hooks/useAuth.ts` around lines 47 - 51, The hook uses the global sessionStorage directly which breaks SSR; update references in useAuth (the promise handlers that call sessionStorage.setItem and sessionStorage.removeItem) to use globalThis.sessionStorage instead, i.e., replace direct sessionStorage access in the success and .catch blocks (where setUser is called) with globalThis.sessionStorage to ensure safe access during server-side rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/kotlin/com/moneat/sso/routes/SsoRoutes.kt`:
- Around line 161-207: The current catch-all for IllegalArgumentException in the
put("/config") handler misclassifies authorization/licensing failures as 400;
change the error handling so only validation errors are returned as 400 and let
domain/authorization/licensing errors propagate to the central StatusPages
mapper (or be thrown as domain-specific exceptions). Concretely: in SsoRoutes'
put("/config") block stop catching IllegalArgumentException broadly—catch only
BadRequestException (or equivalent validation errors) and respond
HttpStatusCode.BadRequest with ErrorResponse; remove the
IllegalArgumentException catch so configureSso(orgId, userId, request) can throw
domain-specific exceptions (e.g., an AuthorizationException or
LicensingException) or IllegalArgumentException/IllegalStateException to be
mapped by StatusPages to 403/appropriate codes. Ensure ssoService.configureSso
emits domain-specific exceptions for owner/licensing checks rather than
IllegalArgumentException if not already.
In `@backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt`:
- Around line 789-806: The consumeNonce function currently swallows
non-IllegalArgumentExceptions when Redis is unavailable, allowing nonce replay;
change this to "fail closed" by replacing the generic exception handler so that
any Redis-access failure (exceptions from RedisConfig.sync().get/del) results in
throwing a clear runtime exception (e.g., IllegalStateException or a custom
SsoVerificationException) with context including the nonceB64 and
SSO_NONCE_PREFIX rather than only logger.warn; keep the existing behavior of
rethrowing IllegalArgumentException for explicit nonce reuse, but for all other
exceptions from RedisConfig ensure you propagate an error to abort the SSO flow
instead of silently continuing.
- Around line 620-622: The current use of kotlinx.coroutines.runBlocking in
discoverOidcEndpoints blocks the DB transaction thread (called from transaction
contexts like handleOidcCallback) and risks exhausting the connection pool;
change discoverOidcEndpoints to be a suspending function that performs the
httpClient.get call without runBlocking and either (a) invoke it before entering
the transaction in handleOidcCallback, or (b) if it must run inside
transactional logic, call it from within a coroutine-aware transaction using
newSuspendedTransaction so the HTTP call does not block the DB thread.
- Around line 642-649: The code force-unwraps issuerUrl when calling
discoverOidcEndpoints(issuerUrl!!) even though earlier validation only runs if
issuerUrl is non-blank; make issuerUrl non-null before use by either enforcing
an early check (e.g., requireNotNull or throw a descriptive exception) or by
moving UrlValidator.validateExternalUrl to run unconditionally after a non-null
assertion, and then call discoverOidcEndpoints with the guaranteed-non-null
issuerUrl; update references in this block: issuerUrl,
SsoConfigurations.oidcIssuerUrl, UrlValidator.validateExternalUrl, and
discoverOidcEndpoints to reflect the non-null contract.
- Around line 103-106: The current lazy property encryptionKey in SsoService
uses jwtSecret.toByteArray().copyOf(AES_KEY_LENGTH) which pads/truncates
insecurely; replace this with a proper key derivation step (e.g., HKDF or
PBKDF2) that deterministically derives a fixed-length AES key from jwtSecret and
any necessary salt/info, then return the derived key bytes (AES_KEY_LENGTH) as
the encryptionKey; locate the encryptionKey property and update it to call a KDF
helper (implement e.g., deriveAesKeyFromJwtSecret(jwtSecret): ByteArray using
javax.crypto.SecretKeyFactory/PBKDF2 or an HKDF implementation, and wrap result
in a SecretKeySpec or copyOfExact to AES_KEY_LENGTH), ensuring errors are
handled and no silent truncation/padding occurs.
In `@dashboard/src/components/SsoSettings.tsx`:
- Around line 308-325: The switch is disabled whenever hasSamlModule is false,
which prevents downgraded orgs from turning off an already-true
formData.requireSso; update the Switch so it's only disabled when the module is
missing AND the current value is false (e.g. disabled={!hasSamlModule &&
!formData.requireSso}), keeping the existing onCheckedChange (setFormData) logic
so users can flip requireSso off even after a downgrade; ensure you update the
Switch component usage in SsoSettings (checked={formData.requireSso},
onCheckedChange, disabled) accordingly.
In `@dashboard/src/routes/settings.tsx`:
- Around line 172-174: The SSO access logic is too permissive: derive a view
gate and an owner-only configure gate so the UI matches backend entitlements.
Replace the single canUseSso with two booleans, e.g. canViewSsoTab (true if
isSelfHosted && hasSamlModule OR (not isSelfHosted && (tier === 'TEAM' || tier
=== 'BUSINESS'))) and canConfigureSso (true only when user?.orgRole === 'owner'
and likewise scoped to self-hosted/module or SaaS tiers), and update usages of
configureSso() and the tab rendering to use canConfigureSso for editable actions
and canViewSsoTab for visibility; keep hasSamlModule considered only for
self-hosted paths and do not grant SaaS FREE-tier access based on the global
features list.
In `@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.kt`:
- Line 29: The code in SamlRoutes.kt uses EnvConfig.get("FRONTEND_URL")!! which
can NPE; replace this with a safe retrieval that either fails fast with a
descriptive exception or provides a sensible dev default. Locate the frontendUrl
assignment in SamlRoutes and change it to use a null-coalescing check on
EnvConfig.get("FRONTEND_URL") (e.g., EnvConfig.get(...) ?: throw
IllegalStateException("FRONTEND_URL must be set for SAML routes")) or supply a
clearly documented default (e.g., "http://localhost:3000") so the app won't
crash with a raw NPE. Ensure the error message references FRONTEND_URL and SAML
startup so logs are actionable.
In
`@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt`:
- Around line 114-123: The SAML response validation is using relayState
incorrectly; change the flow so that when you create the AuthnRequest
(authnRequest.getId()) you capture and store that requestId alongside the relay
state, and then call samlResponseObj.isValid(requestId) instead of
samlResponseObj.isValid(relayState); update SamlService (the code around
SamlResponse, SamlResponse.loadXmlFromBase64 and the isValid call) to retrieve
the stored requestId and pass it into isValid(requestId), and remove relayState
from the isValid invocation so InResponseTo is validated correctly.
---
Outside diff comments:
In `@backend/src/main/kotlin/com/moneat/auth/services/AuthService.kt`:
- Around line 538-548: AuthService builds UserResponse without org context in
signup(), login(), and refreshToken() causing inconsistent UI; update those
UserResponse constructors to include the same organizationSlug and orgRole
values used elsewhere (i.e. compute the final organization slug (finalSlug or
equivalent from membership.organization?.slug) and use membership.role for
orgRole) and pass them into the UserResponse instead of the current null, so
every auth path returns a UserResponse populated with organizationSlug and
orgRole.
---
Nitpick comments:
In `@backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt`:
- Around line 112-114: Replace the manual if-throw precondition in SsoService
(where the method checks email and orgSlug) with Kotlin's idiomatic require():
instead of the current if (email == null && orgSlug == null) throw
IllegalArgumentException(...), call require(email != null || orgSlug != null) {
"Either email or orgSlug must be provided" } so the argument validation uses
require() for this service/library-level precondition.
- Around line 266-398: The configureSso method in SsoService is too complex;
extract provider-specific validation and DB persistence into helper functions to
reduce cognitive complexity: create validateSsoConfigRequest(providerType,
request) (called from configureSso before transactions) that encapsulates
SAML/OIDC field checks and UrlValidator usage, and create
persistSsoConfig(organizationId, request, providerType, baseUrl) that performs
the encryptSecret logic, computes effectiveRequireSso, and executes the
insert-or-update against SsoConfigurations (preserving current field assignments
and updatedAt behavior), then have configureSso call these helpers and return
getSsoConfig(organizationId) as before. Ensure new helpers reference the same
symbols used now (SsoProviderType, UrlValidator, encryptSecret,
FeatureRegistry.hasModule("SAML"), SsoConfigurations, getSsoConfig) and keep all
transactional DB access inside persistSsoConfig to maintain existing behavior.
In `@dashboard/src/hooks/useAuth.ts`:
- Around line 47-51: The hook uses the global sessionStorage directly which
breaks SSR; update references in useAuth (the promise handlers that call
sessionStorage.setItem and sessionStorage.removeItem) to use
globalThis.sessionStorage instead, i.e., replace direct sessionStorage access in
the success and .catch blocks (where setUser is called) with
globalThis.sessionStorage to ensure safe access during server-side rendering.
In `@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.kt`:
- Around line 27-96: The three route handlers in samlRoutes()
(post("/init/saml"), get("/saml/metadata"), post("/saml/acs")) repeat identical
try/catch logic causing high cognitive complexity; extract a reusable helper
(e.g., withErrorHandling or handleSamlRoute) that accepts a suspend lambda and
centralizes the IllegalArgumentException and generic Exception
handling/logging/response behavior so you can replace the inline try/catch in
each handler while still invoking SamlService.initSaml,
SamlService.getSamlMetadata, and SamlService.handleSamlResponse and preserving
AuthCookieUtils.setAuthCookie and the same redirects/responses.
In
`@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt`:
- Around line 157-204: Refactor getSamlMetadata to simplify control flow:
validate orgSlug early with requireNotNull (or throw) and load the organization
via Organizations.selectAll().where { Organizations.slug eq orgSlug
}.firstOrNull(), then fetch the SSO row from SsoConfigurations (filtering
organizationId, providerType "saml", isEnabled) and use requireNotNull on that
result (instead of the later if (ssoConfig == null) block); keep calling
buildSamlSettings(ssoConfig), validate metadata with
Saml2Settings.validateMetadata, log errors with logger.error and throw if
invalid, and return metadata—this removes nested if/else and the redundant null
check while preserving existing validations.
- Around line 74-78: Replace the manual null check for ssoConfig in SamlService
with the idiomatic Kotlin requireNotNull usage: use requireNotNull(ssoConfig)
with the same message ("SAML SSO is not configured for this organization") and
assign the result to a non-null val (or rebind) so subsequent code uses the
non-null value; this keeps the IllegalArgumentException semantics but simplifies
the check and makes the intent clearer.
- Around line 38-42: Replace the manual null-check and throw with Kotlin's
idiomatic require() precondition: in SamlService.kt where the code currently
checks if (email == null && orgSlug == null) and throws
IllegalArgumentException("Either email or orgSlug must be provided"), change it
to use require(email != null || orgSlug != null) { "Either email or orgSlug must
be provided" } so the argument validation uses the standard Kotlin helper
instead of an explicit throw; update the surrounding method in SamlService
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 565bd550-8405-415c-8bda-50d57079dd73
📒 Files selected for processing (24)
.env.exampleCONTRIBUTING.mdREADME.mdbackend/src/main/kotlin/com/moneat/auth/services/AuthService.ktbackend/src/main/kotlin/com/moneat/events/models/ApiModels.ktbackend/src/main/kotlin/com/moneat/events/routes/ApiRoutes.ktbackend/src/main/kotlin/com/moneat/sso/SsoModule.ktbackend/src/main/kotlin/com/moneat/sso/models/SsoModels.ktbackend/src/main/kotlin/com/moneat/sso/routes/SsoRoutes.ktbackend/src/main/kotlin/com/moneat/sso/services/SsoService.ktbackend/src/main/resources/META-INF/services/com.moneat.enterprise.EnterpriseModuledashboard/src/components/SsoSettings.tsxdashboard/src/docs/pages/billing.mdxdashboard/src/docs/pages/sso-authentication.mdxdashboard/src/hooks/useAuth.tsdashboard/src/lib/api/modules/user.tsdashboard/src/routes/settings.tsxee/README.mdee/backend/src/main/kotlin/com/moneat/enterprise/sso/SamlModule.ktee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.ktee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SsoRoutes.ktee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.ktee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SsoService.ktee/backend/src/main/resources/META-INF/services/com.moneat.enterprise.EnterpriseModule
💤 Files with no reviewable changes (2)
- ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SsoRoutes.kt
- ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SsoService.kt
ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.kt
Outdated
Show resolved
Hide resolved
ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt
Show resolved
Hide resolved
|
AdrianLeeElder
force-pushed
the
develop
branch
5 times, most recently
from
3ce339b to
1ad7ed6
Compare
AdrianLeeElder
force-pushed
the
migrte-sso-to-open-core
branch
from
cbdaf21 to
93c7926
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt (1)
567-577:⚠️ Potential issue | 🟠 MajorMake state consumption fail-closed and atomic.
generateSecureState()still hands out a usable state after Redis write failures, andconsumeNonce()returns early when Redis isn't initialized, so the single-use guarantee disappears whenever the nonce store is unhealthy. Even when Redis is up, separateget/delcalls leave a race where two concurrent callbacks can consume the same nonce.Also applies to: 815-829
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt` around lines 567 - 577, The current flow hands out valid states when Redis is unavailable and uses separate get/del operations, breaking single-use guarantees; update generateSecureState() to fail-closed by refusing to return a state if Redis is not initialized or the SETEX write throws (log and propagate/throw the error instead of swallowing it), and update consumeNonce() to perform an atomic read-and-delete against Redis (use GETDEL if available or a short Lua EVAL script that GETs the key and DELs it) against the "$SSO_NONCE_PREFIX$nonceB64" key with SSO_NONCE_TTL_SECONDS behavior, returning an error when Redis is unhealthy so nonces are never considered consumed unless the atomic operation succeeds; apply the same changes to the duplicate block around lines 815-829 so both code paths use the atomic GET+DEL and fail-closed semantics.
🧹 Nitpick comments (2)
backend/src/main/kotlin/com/moneat/auth/services/AuthService.kt (1)
551-562: Use named arguments for consistency and maintainability.This method uses positional arguments for
UserResponsewhilesignup,login, andrefreshTokenuse named arguments. Positional construction is fragile—ifUserResponsefields are reordered or new fields are inserted, this code will silently break or fail to compile.♻️ Refactor to named arguments
return UserResponse( - user.id, - user.email, - user.name, - user.emailVerified, - true, - user.isAdmin, - finalSlug, - membership.role, - null, - hiddenItems + id = user.id, + email = user.email, + name = user.name, + emailVerified = user.emailVerified, + onboardingCompleted = true, + isAdmin = user.isAdmin, + organizationSlug = finalSlug, + orgRole = membership.role, + sidebarHiddenItems = hiddenItems )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/com/moneat/auth/services/AuthService.kt` around lines 551 - 562, The construction of UserResponse using positional arguments is fragile; update this return to use named arguments like the other call sites (signup, login, refreshToken). Replace the positional call to UserResponse(user.id, user.email, user.name, user.emailVerified, true, user.isAdmin, finalSlug, membership.role, null, hiddenItems) with a named-argument instantiation that explicitly assigns each parameter (e.g., id = user.id, email = user.email, name = user.name, emailVerified = user.emailVerified, verified = true, isAdmin = user.isAdmin, slug = finalSlug, role = membership.role, plan = null, hiddenItems = hiddenItems) so field order changes won’t break UserResponse construction.backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt (1)
113-116: Extract the PBKDF2 iteration count into a constant.This policy value is still hard-coded inline, which makes the crypto settings harder to audit and change safely.
As per coding guidelines: Use named constants instead of magic numbers in Kotlin code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt` around lines 113 - 116, The PBKDF2 iteration count (100_000) is a magic number; extract it into a named constant (e.g., PBKDF2_ITERATIONS) and use that constant when constructing the PBEKeySpec in SsoService (the code around spec = PBEKeySpec(secret.toCharArray(), salt, 100_000, AES_KEY_LENGTH * 8)); update any related references (keep AES_KEY_LENGTH as-is) and ensure the constant is defined as a kotlin val with an appropriate visibility and documentation comment for auditability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/kotlin/com/moneat/sso/routes/SsoRoutes.kt`:
- Around line 88-107: The current post("/init") handler in SsoRoutes.kt catches
all Exceptions (including Ktor's BadRequestException thrown by
call.receive<SsoInitRequest>()), causing 400-level parse errors to be turned
into 500s; update the handler in the init route (and the analogous route around
lines 197-217) so that you either remove the try/catch around call.receive(...)
to allow BadRequestException to propagate to the global StatusPages, or
explicitly catch BadRequestException before the generic Exception catch and
rethrow or respond with HttpStatusCode.BadRequest; adjust the catch blocks in
the ssoService.initSso call (and the corresponding ssoService methods) to only
handle expected domain exceptions (e.g., IllegalArgumentException) and let
Ktor's BadRequestException be handled globally.
In `@backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt`:
- Around line 203-208: The lookup in the transaction block that fetches SSO
config via SsoConfigurations.selectAll().where {
SsoConfigurations.organizationId eq orgId }.firstOrNull() should also filter to
only return active OIDC configs; update the where clause (or replace selectAll
with a filtered query) to include SsoConfigurations.isEnabled eq true and
SsoConfigurations.providerType eq "oidc" so that disabled or non-OIDC providers
are ignored and OIDC callbacks cannot complete for disabled configs.
- Around line 653-665: The OIDC discovery endpoints are cached without
validation; modify the SsoService discovery logic so that after extracting
authorization_endpoint and token_endpoint (used to build OidcDiscoveryCache and
stored in discoveryCache) you call UrlValidator.validateExternalUrl(...) on both
URLs, and if either validation fails throw an IllegalArgumentException (or
propagate an error) instead of caching; ensure you validate the values before
constructing OidcDiscoveryCache or assigning discoveryCache[issuerUrl].
- Around line 417-426: The update block in SsoService.update is preserving
previous oidcClientSecret because encryptedSecret?.let { ... } only writes when
a new secret exists; to avoid stale OIDC secrets, explicitly clear
SsoConfigurations.oidcClientSecret when the incoming provider is not OIDC: set
it to null (or empty) inside the same SsoConfigurations.update call when
request.providerType.lowercase() != "oidc", while keeping the existing
encryptedSecret assignment for OIDC cases; reference the SsoService update logic
and the SsoConfigurations.oidcClientSecret field to locate where to add the
nulling assignment.
- Around line 263-286: The code uses OIDCTokenResponseParser.parse(...) and then
directly reads oidcTokens.idToken.jwtClaimsSet without validating the ID token;
update SsoService to fully validate the ID token (verify signature using the
provider's JWKs, check issuer and audience, check exp/nbf timestamps, ensure
email_verified is true, and validate the subject) before provisioning or linking
accounts (references: OIDCTokenResponseParser.parse,
successResponse.oidcTokens.idToken, idToken.jwtClaimsSet). Also implement proper
nonce handling: generate and store a cryptographically random nonce at
authorization request time, include it in the outbound authorization request,
and verify the nonce claim in the validated ID token to prevent replay
(reference the authorization-request builder/place where the nonce should be
set). Use the provider's JWKS + an OIDC token verifier utility to perform these
checks and throw/return errors on any validation failure rather than proceeding
to create or link accounts.
In `@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.kt`:
- Around line 28-39: The generic Exception catch in handleSamlInit is swallowing
deserialization errors from call.receive<SsoInitRequest>() and returning 500;
remove the generic "catch (e: Exception)" block so
deserialization/BadRequestException can propagate and be handled globally, or
replace it by catching the specific deserialization exception (e.g.,
BadRequestException or ContentTransformationException depending on your setup)
and rethrowing/throwing a BadRequestException; update the method around
call.receive<SsoInitRequest>() and the existing IllegalArgumentException catch
to implement this change.
In
`@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt`:
- Around line 115-122: The lookup that assigns ssoConfig in SamlService.kt
should also require the SSO row to be enabled and of the SAML provider type so
ACS responses cannot succeed for disabled or non‑SAML configs; update the query
that currently filters only by SsoConfigurations.organizationId to additionally
include conditions on SsoConfigurations.enabled (true) and
SsoConfigurations.providerType (the SAML value/enum your codebase uses) before
firstOrNull(), keeping the variable name ssoConfig and the surrounding logic
unchanged.
- Around line 100-105: generateSamlRequest() currently skips storing requestId
when Redis isn't initialized and uses a racy get+del; make the AuthnRequest
binding mandatory by failing service initialization if
RedisConfig.isInitialized() is false, always persist the requestId using the
SsoService.SSO_SAML_AUTHN_REQUEST_KEY_PREFIX, and consume it atomically (use
Redis GETDEL or a single atomic command via RedisConfig.sync()) instead of
separate get() + del(). Apply the same mandatory+atomic changes to the second
occurrence (around lines 213-220) so both code paths consistently store and
atomically consume the binding.
---
Duplicate comments:
In `@backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt`:
- Around line 567-577: The current flow hands out valid states when Redis is
unavailable and uses separate get/del operations, breaking single-use
guarantees; update generateSecureState() to fail-closed by refusing to return a
state if Redis is not initialized or the SETEX write throws (log and
propagate/throw the error instead of swallowing it), and update consumeNonce()
to perform an atomic read-and-delete against Redis (use GETDEL if available or a
short Lua EVAL script that GETs the key and DELs it) against the
"$SSO_NONCE_PREFIX$nonceB64" key with SSO_NONCE_TTL_SECONDS behavior, returning
an error when Redis is unhealthy so nonces are never considered consumed unless
the atomic operation succeeds; apply the same changes to the duplicate block
around lines 815-829 so both code paths use the atomic GET+DEL and fail-closed
semantics.
---
Nitpick comments:
In `@backend/src/main/kotlin/com/moneat/auth/services/AuthService.kt`:
- Around line 551-562: The construction of UserResponse using positional
arguments is fragile; update this return to use named arguments like the other
call sites (signup, login, refreshToken). Replace the positional call to
UserResponse(user.id, user.email, user.name, user.emailVerified, true,
user.isAdmin, finalSlug, membership.role, null, hiddenItems) with a
named-argument instantiation that explicitly assigns each parameter (e.g., id =
user.id, email = user.email, name = user.name, emailVerified =
user.emailVerified, verified = true, isAdmin = user.isAdmin, slug = finalSlug,
role = membership.role, plan = null, hiddenItems = hiddenItems) so field order
changes won’t break UserResponse construction.
In `@backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt`:
- Around line 113-116: The PBKDF2 iteration count (100_000) is a magic number;
extract it into a named constant (e.g., PBKDF2_ITERATIONS) and use that constant
when constructing the PBEKeySpec in SsoService (the code around spec =
PBEKeySpec(secret.toCharArray(), salt, 100_000, AES_KEY_LENGTH * 8)); update any
related references (keep AES_KEY_LENGTH as-is) and ensure the constant is
defined as a kotlin val with an appropriate visibility and documentation comment
for auditability.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b2fd11b-014f-46e6-ab34-d1bd9bde506d
📒 Files selected for processing (10)
backend/src/main/kotlin/com/moneat/auth/services/AuthService.ktbackend/src/main/kotlin/com/moneat/plugins/Monitoring.ktbackend/src/main/kotlin/com/moneat/sso/SsoForbiddenException.ktbackend/src/main/kotlin/com/moneat/sso/routes/SsoRoutes.ktbackend/src/main/kotlin/com/moneat/sso/services/SsoService.ktdashboard/src/components/SsoSettings.tsxdashboard/src/hooks/useAuth.tsdashboard/src/routes/settings.tsxee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.ktee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt
✅ Files skipped from review due to trivial changes (1)
- backend/src/main/kotlin/com/moneat/sso/SsoForbiddenException.kt
ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.kt
Show resolved
Hide resolved
ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt
Outdated
Show resolved
Hide resolved
ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/kotlin/com/moneat/events/routes/ApiRoutes.kt (1)
126-150:⚠️ Potential issue | 🟠 MajorResolve
/usermembership from the token's org context before returningorgRole.
firstOrNull()over all memberships is arbitrary once a user belongs to multiple orgs. With this change,/usercan now return anorgRole/organizationSlugpair for a different org than the one in the JWT, so the dashboard can render permissions for the wrong workspace.🔧 Suggested fix
get("/user") { val principal = call.principal<JWTPrincipal>() val userId = principal!!.payload.getClaim("userId").asInt() + val orgId = principal.payload.getClaim("orgId").asInt() val demoEpochMs = call.getDemoEpochMs() val (user, orgSlug, orgRole, sidebarHiddenItems) = transaction { val userRow = Users.selectAll().where { Users.id eq userId }.firstOrNull() ?: return@transaction Quadruple(null, null, null, emptyList()) val membership = Memberships .selectAll() - .where { Memberships.user_id eq userId } + .where { + (Memberships.user_id eq userId) and + (Memberships.organization_id eq orgId) + } .firstOrNull()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/com/moneat/events/routes/ApiRoutes.kt` around lines 126 - 150, The current code picks an arbitrary membership with Memberships.selectAll().where { Memberships.user_id eq userId }.firstOrNull(), which can return a membership for a different org than the one in the JWT; update the membership lookup to filter by the organization id from the token (e.g. tokenOrgId or orgIdFromJwt) so you fetch Memberships.where { (Memberships.user_id eq userId) and (Memberships.organization_id eq tokenOrgId) }. Use that filtered membership to derive role (orgRole) and sidebar_hidden_items and derive orgSlug from Organizations using that membership's organization_id; keep the existing fallback to empty/null when tokenOrgId is absent or no matching membership exists.
♻️ Duplicate comments (4)
ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt (2)
100-113:⚠️ Potential issue | 🔴 CriticalMake the AuthnRequest binding mandatory and consume it atomically.
Lines 214-220 still skip persisting
requestIdwhen Redis is unavailable, soinitSaml()can return a redirect the ACS path is guaranteed to reject later. Lines 101-106 also useget+del, which allows concurrent ACS posts to observe the same binding. Fail init when the binding store isn't ready, and consume the key with a single atomic operation.Also applies to: 214-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt` around lines 100 - 113, The AuthnRequest binding retrieval must be mandatory and consumed atomically: in the block that reads requestId (and the similar code in initSaml()), fail fast if Redis is not initialized instead of falling back to null, and replace the separate get + del sequence with an atomic consume (e.g., Redis GETDEL or an EVAL that returns-and-deletes) using the same key constructed from SsoService.SSO_SAML_AUTHN_REQUEST_KEY_PREFIX + stateData.nonce; if the atomic consume returns no value, throw the IllegalStateException as currently written so initSaml()/ACS cannot proceed with a missing binding.
115-122:⚠️ Potential issue | 🟠 MajorMirror the enabled/provider filter on the ACS lookup.
Line 118 currently filters only by
organizationId. That can pick up a disabled row or a non-SAML provider, even though the init and metadata lookups already requireisEnabled == trueandproviderType == "saml".🔧 Suggested change
val ssoConfig = SsoConfigurations .selectAll() - .where { SsoConfigurations.organizationId eq orgId } + .where { + (SsoConfigurations.organizationId eq orgId) and + (SsoConfigurations.isEnabled eq true) and + (SsoConfigurations.providerType eq "saml") + } .firstOrNull()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt` around lines 115 - 122, The ACS lookup currently fetching ssoConfig via SsoConfigurations.selectAll() only filters by organizationId and may return disabled or non‑SAML rows; update the query that assigns ssoConfig in SamlService (the variable ssoConfig from SsoConfigurations.selectAll()...) to also filter for SsoConfigurations.isEnabled eq true and SsoConfigurations.providerType eq "saml" so it mirrors the checks used by the init/metadata paths and only returns an enabled SAML provider for the given orgId.backend/src/main/kotlin/com/moneat/sso/routes/SsoRoutes.kt (1)
88-108:⚠️ Potential issue | 🟠 MajorDon't turn malformed request bodies into 500s.
Lines 90 and 199 call
receive(...), but Lines 102 and 209 catchExceptionbeforebackend/src/main/kotlin/com/moneat/plugins/Monitoring.ktcan mapBadRequestExceptionto 400. LetBadRequestExceptionpropagate, or catch/rethrow it explicitly, so bad JSON stays a client error.Also applies to: 197-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/com/moneat/sso/routes/SsoRoutes.kt` around lines 88 - 108, The catch-all Exception handler in the post("/init") route is turning malformed request bodies into 500s by swallowing BadRequestException from call.receive<SsoInitRequest>(); update the error handling in the init route (and the similar block at 197-217) so BadRequestException is not converted to 500: either remove the generic catch(Exception) to let BadRequestException propagate to Monitoring.kt, or change the catch to detect and rethrow BadRequestException (e.g. if (e is BadRequestException) throw e else handle/log and respond with 500). Ensure this change references the call.receive<SsoInitRequest>() call and the catch(Exception) block around ssoService.initSso so client JSON errors remain 400s.ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.kt (1)
28-39:⚠️ Potential issue | 🟠 MajorLet malformed init payloads reach the global 400 mapper.
Line 30 can fail before any service code runs, but Line 36 catches
Exceptionand responds 500. That bypasses the globalBadRequestExceptionhandling inbackend/src/main/kotlin/com/moneat/plugins/Monitoring.ktand misclassifies client input errors as server failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.kt` around lines 28 - 39, The handler handleSamlInit currently catches all Exception after calling call.receive<SsoInitRequest>() which causes malformed request parsing errors to be turned into 500s; change the error handling so that parsing/validation errors propagate to the global BadRequestException mapper — either remove the broad catch (the catch(Exception) block) or rethrow if the exception originates from call.receive (e.g., let exceptions other than IllegalArgumentException/BAD_REQUEST types bubble up); keep only service-related exception handling around samlService.initSaml (or specifically catch service-specific exceptions) and use call.respond only for service errors so that global BadRequestException handling in Monitoring.kt can run.
🧹 Nitpick comments (2)
dashboard/src/routes/settings.tsx (1)
170-176: SSO gating logic correctly addresses previous review concern.The refactored logic properly separates visibility (
canViewSsoTab) from configuration permissions (canConfigureSso), and ensures SaaS FREE-tier users cannot access the SSO tab. Self-hosted users can always view the tab (since OIDC is now open-source), with SAML gated viahasSamlModule.Minor simplification available — the redundant
!isSelfHosted &&check can be removed:♻️ Optional simplification
const canViewSsoTab = - isSelfHosted || (!isSelfHosted && (tier === 'TEAM' || tier === 'BUSINESS')) + isSelfHosted || tier === 'TEAM' || tier === 'BUSINESS'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dashboard/src/routes/settings.tsx` around lines 170 - 176, The canViewSsoTab expression includes a redundant "!isSelfHosted &&" check; simplify the logic by removing the unnecessary negation so canViewSsoTab only uses isSelfHosted and the tier check (i.e., set canViewSsoTab to true if isSelfHosted or tier is 'TEAM' or 'BUSINESS'), leaving canConfigureSso and hasSamlModule unchanged.backend/src/main/kotlin/com/moneat/sso/routes/SsoRoutes.kt (1)
58-79: Make the org scope explicit instead of relying on an implicit parameter.Line 64 requires
organizationId, but Lines 140-195 register/v1/sso/configwithout declaring{organizationId}anywhere in the route tree. If callers are meant to pass?organizationId=..., readrequest.queryParametersexplicitly; otherwise add the path parameter so the contract is unambiguous.Also applies to: 140-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/com/moneat/sso/routes/SsoRoutes.kt` around lines 58 - 79, The requireSsoAuth function currently pulls organizationId from parameters["organizationId"] which is ambiguous because the /v1/sso/config routes are registered without a path param; make the org scope explicit by either (A) changing requireSsoAuth to read the org id from request.queryParameters["organizationId"] (and adjust the error message constant ERROR_MISSING_ORG_ID if needed) if callers should pass ?organizationId=..., or (B) update the route registrations that use requireSsoAuth to include an explicit path parameter like "{organizationId}" so parameters["organizationId"] is valid; ensure all places that call requireSsoAuth and the route definitions (the /v1/sso/config registration) are updated consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/kotlin/com/moneat/auth/services/AuthService.kt`:
- Around line 587-598: The UserResponse is built using
membershipRepository.getFirstMembershipForUser(userId), which can pick the wrong
org for multi-org users after a token refresh; instead derive the organization
context from the rotated token you just issued (tokenPair.accessToken) and use
that to fetch membership and organization data. Parse the organization
identifier (e.g., organizationId or organizationSlug) from the rotated access
token's claims, then call the membership lookup that accepts both userId and
that org id (instead of getFirstMembershipForUser) and use
organizationRepository.findById/findBySlug based on that org id to populate
organizationSlug and orgRole so the refresh response matches the token's org
context.
In `@backend/src/main/kotlin/com/moneat/events/routes/ApiRoutes.kt`:
- Around line 104-109: The /subscription handler currently calls
pricingTierService.getPrimaryOrganizationIdForUser(userId) which ignores the
organization present in the access token; change it to read the organization id
from the JWT/claims (the org selected in the token) and use that orgId when
calling pricingTierService.getEffectiveTierForOrganization(orgId), returning
HttpStatusCode.NotFound only if the token has no org claim or the org is
invalid; update any references in ApiRoutes.kt to stop deriving orgId from
getPrimaryOrganizationIdForUser and instead pull it from the request
principal/claims before calling getEffectiveTierForOrganization.
In `@backend/src/main/kotlin/com/moneat/sso/routes/SsoRoutes.kt`:
- Around line 96-103: Replace usages of logger.error(e) that pass raw Throwables
with a capped error string: call e.stackTraceToString().take(500) (or
e.toString().take(500) if you only want the summary) and log it via the
logger.error { "SSO init failed: ${capped}" } style so the message contains the
trimmed stack/message rather than the raw throwable. Update the catch blocks
shown (the IllegalArgumentException and generic Exception handlers in
SsoRoutes.kt and the other catch blocks at the referenced ranges) to build a
capped string from the throwable and pass that string to logger.error instead of
the throwable itself.
In `@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.kt`:
- Around line 33-38: The catch blocks currently pass the full throwable to
logger.error (e.g., the IllegalArgumentException catch and the generic Exception
catch in SamlRoutes), which emits full stack traces; change those
logger.error(e) calls to log a bounded stack/message string instead — for
example use logger.error { "SAML init failed:
${e.stackTraceToString().take(500)}" } (and similarly for the other catch
blocks) so each logged error message is truncated to 500 chars while keeping the
existing response behavior (call.respond(..., ErrorResponse(...))).
---
Outside diff comments:
In `@backend/src/main/kotlin/com/moneat/events/routes/ApiRoutes.kt`:
- Around line 126-150: The current code picks an arbitrary membership with
Memberships.selectAll().where { Memberships.user_id eq userId }.firstOrNull(),
which can return a membership for a different org than the one in the JWT;
update the membership lookup to filter by the organization id from the token
(e.g. tokenOrgId or orgIdFromJwt) so you fetch Memberships.where {
(Memberships.user_id eq userId) and (Memberships.organization_id eq tokenOrgId)
}. Use that filtered membership to derive role (orgRole) and
sidebar_hidden_items and derive orgSlug from Organizations using that
membership's organization_id; keep the existing fallback to empty/null when
tokenOrgId is absent or no matching membership exists.
---
Duplicate comments:
In `@backend/src/main/kotlin/com/moneat/sso/routes/SsoRoutes.kt`:
- Around line 88-108: The catch-all Exception handler in the post("/init") route
is turning malformed request bodies into 500s by swallowing BadRequestException
from call.receive<SsoInitRequest>(); update the error handling in the init route
(and the similar block at 197-217) so BadRequestException is not converted to
500: either remove the generic catch(Exception) to let BadRequestException
propagate to Monitoring.kt, or change the catch to detect and rethrow
BadRequestException (e.g. if (e is BadRequestException) throw e else handle/log
and respond with 500). Ensure this change references the
call.receive<SsoInitRequest>() call and the catch(Exception) block around
ssoService.initSso so client JSON errors remain 400s.
In `@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.kt`:
- Around line 28-39: The handler handleSamlInit currently catches all Exception
after calling call.receive<SsoInitRequest>() which causes malformed request
parsing errors to be turned into 500s; change the error handling so that
parsing/validation errors propagate to the global BadRequestException mapper —
either remove the broad catch (the catch(Exception) block) or rethrow if the
exception originates from call.receive (e.g., let exceptions other than
IllegalArgumentException/BAD_REQUEST types bubble up); keep only service-related
exception handling around samlService.initSaml (or specifically catch
service-specific exceptions) and use call.respond only for service errors so
that global BadRequestException handling in Monitoring.kt can run.
In
`@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt`:
- Around line 100-113: The AuthnRequest binding retrieval must be mandatory and
consumed atomically: in the block that reads requestId (and the similar code in
initSaml()), fail fast if Redis is not initialized instead of falling back to
null, and replace the separate get + del sequence with an atomic consume (e.g.,
Redis GETDEL or an EVAL that returns-and-deletes) using the same key constructed
from SsoService.SSO_SAML_AUTHN_REQUEST_KEY_PREFIX + stateData.nonce; if the
atomic consume returns no value, throw the IllegalStateException as currently
written so initSaml()/ACS cannot proceed with a missing binding.
- Around line 115-122: The ACS lookup currently fetching ssoConfig via
SsoConfigurations.selectAll() only filters by organizationId and may return
disabled or non‑SAML rows; update the query that assigns ssoConfig in
SamlService (the variable ssoConfig from SsoConfigurations.selectAll()...) to
also filter for SsoConfigurations.isEnabled eq true and
SsoConfigurations.providerType eq "saml" so it mirrors the checks used by the
init/metadata paths and only returns an enabled SAML provider for the given
orgId.
---
Nitpick comments:
In `@backend/src/main/kotlin/com/moneat/sso/routes/SsoRoutes.kt`:
- Around line 58-79: The requireSsoAuth function currently pulls organizationId
from parameters["organizationId"] which is ambiguous because the /v1/sso/config
routes are registered without a path param; make the org scope explicit by
either (A) changing requireSsoAuth to read the org id from
request.queryParameters["organizationId"] (and adjust the error message constant
ERROR_MISSING_ORG_ID if needed) if callers should pass ?organizationId=..., or
(B) update the route registrations that use requireSsoAuth to include an
explicit path parameter like "{organizationId}" so parameters["organizationId"]
is valid; ensure all places that call requireSsoAuth and the route definitions
(the /v1/sso/config registration) are updated consistently.
In `@dashboard/src/routes/settings.tsx`:
- Around line 170-176: The canViewSsoTab expression includes a redundant
"!isSelfHosted &&" check; simplify the logic by removing the unnecessary
negation so canViewSsoTab only uses isSelfHosted and the tier check (i.e., set
canViewSsoTab to true if isSelfHosted or tier is 'TEAM' or 'BUSINESS'), leaving
canConfigureSso and hasSamlModule unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 068fcabe-d4b4-41aa-a044-ac8cdfda5f12
📒 Files selected for processing (26)
.env.exampleCONTRIBUTING.mdREADME.mdbackend/src/main/kotlin/com/moneat/auth/services/AuthService.ktbackend/src/main/kotlin/com/moneat/events/models/ApiModels.ktbackend/src/main/kotlin/com/moneat/events/routes/ApiRoutes.ktbackend/src/main/kotlin/com/moneat/plugins/Monitoring.ktbackend/src/main/kotlin/com/moneat/sso/SsoForbiddenException.ktbackend/src/main/kotlin/com/moneat/sso/SsoModule.ktbackend/src/main/kotlin/com/moneat/sso/models/SsoModels.ktbackend/src/main/kotlin/com/moneat/sso/routes/SsoRoutes.ktbackend/src/main/kotlin/com/moneat/sso/services/SsoService.ktbackend/src/main/resources/META-INF/services/com.moneat.enterprise.EnterpriseModuledashboard/src/components/SsoSettings.tsxdashboard/src/docs/pages/billing.mdxdashboard/src/docs/pages/sso-authentication.mdxdashboard/src/hooks/useAuth.tsdashboard/src/lib/api/modules/user.tsdashboard/src/routes/settings.tsxee/README.mdee/backend/src/main/kotlin/com/moneat/enterprise/sso/SamlModule.ktee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.ktee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SsoRoutes.ktee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.ktee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SsoService.ktee/backend/src/main/resources/META-INF/services/com.moneat.enterprise.EnterpriseModule
💤 Files with no reviewable changes (2)
- ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SsoService.kt
- ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SsoRoutes.kt
✅ Files skipped from review due to trivial changes (9)
- backend/src/main/kotlin/com/moneat/sso/SsoForbiddenException.kt
- .env.example
- dashboard/src/docs/pages/billing.mdx
- backend/src/main/resources/META-INF/services/com.moneat.enterprise.EnterpriseModule
- backend/src/main/kotlin/com/moneat/sso/models/SsoModels.kt
- README.md
- ee/README.md
- ee/backend/src/main/resources/META-INF/services/com.moneat.enterprise.EnterpriseModule
- dashboard/src/lib/api/modules/user.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- dashboard/src/hooks/useAuth.ts
- backend/src/main/kotlin/com/moneat/events/models/ApiModels.kt
- CONTRIBUTING.md
- dashboard/src/docs/pages/sso-authentication.mdx
- backend/src/main/kotlin/com/moneat/sso/SsoModule.kt
- backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt
Comment on lines
+587
to
+598
| val membership = membershipRepository.getFirstMembershipForUser(userId) | ||
| val organizationSlug = | ||
| membership?.let { organizationRepository.findById(it.organizationId)?.slug } | ||
| UserResponse( | ||
| userId, | ||
| email, | ||
| userRow.name, | ||
| userRow.emailVerified, | ||
| userRow.onboardingCompleted, | ||
| userRow.isAdmin | ||
| id = userId, | ||
| email = email, | ||
| name = userRow.name, | ||
| emailVerified = userRow.emailVerified, | ||
| onboardingCompleted = userRow.onboardingCompleted, | ||
| isAdmin = userRow.isAdmin, | ||
| organizationSlug = organizationSlug, | ||
| orgRole = membership?.role, |
There was a problem hiding this comment.
Derive the refresh response from the rotated token's org context.
This now uses getFirstMembershipForUser(userId), so a multi-org user can receive organizationSlug / orgRole for a different org than the tokenPair.accessToken you just issued. After refresh, the client can end up rendering the wrong org context or role until the next full reload.
🔧 Suggested fix
val decodedJWT = jwtVerifier.verify(tokenPair.accessToken)
val userId = decodedJWT.getClaim("userId").asInt()
val email = decodedJWT.getClaim("email").asString()
+ val orgId = decodedJWT.getClaim("orgId").asInt()
+ val orgRole = decodedJWT.getClaim("orgRole").asString()
val user = run {
val userRow = userRepository.findById(userId) ?: return null
- val membership = membershipRepository.getFirstMembershipForUser(userId)
- val organizationSlug =
- membership?.let { organizationRepository.findById(it.organizationId)?.slug }
+ val organizationSlug = organizationRepository.findById(orgId)?.slug
UserResponse(
id = userId,
email = email,
name = userRow.name,
emailVerified = userRow.emailVerified,
onboardingCompleted = userRow.onboardingCompleted,
isAdmin = userRow.isAdmin,
organizationSlug = organizationSlug,
- orgRole = membership?.role,
+ orgRole = orgRole,
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/main/kotlin/com/moneat/auth/services/AuthService.kt` around lines
587 - 598, The UserResponse is built using
membershipRepository.getFirstMembershipForUser(userId), which can pick the wrong
org for multi-org users after a token refresh; instead derive the organization
context from the rotated token you just issued (tokenPair.accessToken) and use
that to fetch membership and organization data. Parse the organization
identifier (e.g., organizationId or organizationSlug) from the rotated access
token's claims, then call the membership lookup that accepts both userId and
that org id (instead of getFirstMembershipForUser) and use
organizationRepository.findById/findBySlug based on that org id to populate
organizationSlug and orgRole so the refresh response matches the token's org
context.
Comment on lines
+104
to
+109
| val orgId = | ||
| pricingTierService.getPrimaryOrganizationIdForUser(userId) ?: run { | ||
| call.respond(HttpStatusCode.NotFound, ErrorResponse("No organization access")) | ||
| return@get | ||
| } | ||
| val context = pricingTierService.getEffectiveTierForOrganization(orgId) |
There was a problem hiding this comment.
Scope /subscription to the org in the JWT, not the user's primary org.
This endpoint is used for tier-gated SSO UI, but getPrimaryOrganizationIdForUser(userId) ignores the org currently selected in the access token. For multi-org users, that can return the wrong tier and expose or hide SSO settings for the wrong workspace.
🔧 Suggested fix
get("/subscription") {
val principal = call.principal<JWTPrincipal>()
val userId = principal!!.payload.getClaim("userId").asInt()
+ val orgIdClaim = principal.payload.getClaim("orgId").asInt()
val pricingTierService = koin.get<PricingTierService>()
val orgId =
- pricingTierService.getPrimaryOrganizationIdForUser(userId) ?: run {
+ orgIdClaim ?: pricingTierService.getPrimaryOrganizationIdForUser(userId) ?: run {
call.respond(HttpStatusCode.NotFound, ErrorResponse("No organization access"))
return@get
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/main/kotlin/com/moneat/events/routes/ApiRoutes.kt` around lines
104 - 109, The /subscription handler currently calls
pricingTierService.getPrimaryOrganizationIdForUser(userId) which ignores the
organization present in the access token; change it to read the organization id
from the JWT/claims (the org selected in the token) and use that orgId when
calling pricingTierService.getEffectiveTierForOrganization(orgId), returning
HttpStatusCode.NotFound only if the token has no org claim or the org is
invalid; update any references in ApiRoutes.kt to stop deriving orgId from
getPrimaryOrganizationIdForUser and instead pull it from the request
principal/claims before calling getEffectiveTierForOrganization.
Comment on lines
+96
to
+103
| } catch (e: IllegalArgumentException) { | ||
| logger.error(e) { "SSO init failed: ${e.message}" } | ||
| call.respond( | ||
| HttpStatusCode.BadRequest, | ||
| ErrorResponse(e.message), | ||
| ) | ||
| } catch (e: Exception) { | ||
| logger.error(e) { "SSO init error" } |
There was a problem hiding this comment.
Cap these error logs instead of passing raw throwables.
Every logger.error(e) here will emit an unbounded stack trace. Log a trimmed message/stack string instead of the raw Throwable.
As per coding guidelines, "Keep stack traces under 500 characters when logging errors in Kotlin (use .take(500) for error messages)".
Also applies to: 127-132, 151-156, 209-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/main/kotlin/com/moneat/sso/routes/SsoRoutes.kt` around lines 96 -
103, Replace usages of logger.error(e) that pass raw Throwables with a capped
error string: call e.stackTraceToString().take(500) (or e.toString().take(500)
if you only want the summary) and log it via the logger.error { "SSO init
failed: ${capped}" } style so the message contains the trimmed stack/message
rather than the raw throwable. Update the catch blocks shown (the
IllegalArgumentException and generic Exception handlers in SsoRoutes.kt and the
other catch blocks at the referenced ranges) to build a capped string from the
throwable and pass that string to logger.error instead of the throwable itself.
Comment on lines
+33
to
+38
| } catch (e: IllegalArgumentException) { | ||
| logger.error(e) { "SAML init failed: ${e.message}" } | ||
| call.respond(HttpStatusCode.BadRequest, ErrorResponse(e.message)) | ||
| } catch (e: Exception) { | ||
| logger.error(e) { "SAML init error" } | ||
| call.respond(HttpStatusCode.InternalServerError, ErrorResponse("SAML initialization failed")) |
There was a problem hiding this comment.
Trim the throwable logging in these handlers.
These logger.error(e) calls will emit full stack traces. Please log a bounded message/stack string instead of the raw throwable so the output stays within the repo limit.
As per coding guidelines, "Keep stack traces under 500 characters when logging errors in Kotlin (use .take(500) for error messages)".
Also applies to: 47-52, 64-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.kt`
around lines 33 - 38, The catch blocks currently pass the full throwable to
logger.error (e.g., the IllegalArgumentException catch and the generic Exception
catch in SamlRoutes), which emits full stack traces; change those
logger.error(e) calls to log a bounded stack/message string instead — for
example use logger.error { "SAML init failed:
${e.stackTraceToString().take(500)}" } (and similarly for the other catch
blocks) so each logged error message is truncated to 500 chars while keeping the
existing response behavior (call.respond(..., ErrorResponse(...))).
OIDC routes and SsoService are integration-heavy (HTTP, token exchange) similar to Ktor plugins; counting them dropped backend coverage below 60%. Keeps jacocoTestCoverageVerification and PR coverage-check aligned with existing exclusions for wiring-heavy code. Made-with: Cursor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 6 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
Fixed 3 file(s) based on 6 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
|
Thank you for your contribution! Before we can merge this PR, you need to sign our Contributor License Agreement (CLA). To sign, please reply to this comment with:
This is a one-time requirement. You won't be asked again for future contributions. I have read the CLA Document and I hereby sign the CLA 1 out of 2 committers have signed the CLA. |
|
Tip For best results, initiate chat on the files or code changes.
Could the repository maintainers please configure the CLA Assistant to exclude bot accounts (e.g., accounts ending in |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt (1)
113-120:⚠️ Potential issue | 🟠 MajorFilter ACS lookups to enabled SAML configs.
This query only keys on
organizationId, so an in-flight response can still succeed after SAML is disabled or the org is switched to OIDC. Mirror the init path by requiringisEnabled == trueandproviderType == "saml".🛠️ Suggested fix
val ssoConfig = SsoConfigurations .selectAll() - .where { SsoConfigurations.organizationId eq orgId } + .where { + (SsoConfigurations.organizationId eq orgId) and + (SsoConfigurations.isEnabled eq true) and + (SsoConfigurations.providerType eq "saml") + } .firstOrNull() ?: throw IllegalArgumentException( "SSO configuration not found" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt` around lines 113 - 120, The SSO lookup selects by organizationId only, so update the query that sets ssoConfig in SamlService (the block assigning ssoConfig) to also require SsoConfigurations.isEnabled eq true and SsoConfigurations.providerType eq "saml" (mirror the init path) so only enabled SAML configurations are returned; keep throwing IllegalArgumentException when none found.ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.kt (1)
33-35:⚠️ Potential issue | 🟡 MinorTrim throwable logging in these handlers.
These catches still pass the raw
Throwabletologger.error(...), so a single SAML failure can dump an unbounded stack trace. Log a truncated message/stack string instead.As per coding guidelines, "Keep stack traces under 500 characters when logging errors in Kotlin (use
.take(500)for error messages)".Also applies to: 44-49, 61-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.kt` around lines 33 - 35, Replace raw throwable logging in the SAML route catch blocks so they do not log full stack traces: in SamlRoutes.kt update the catch handlers (the IllegalArgumentException catch around SAML init and the other catches at lines handling validation and logout) to build a short error string (e.g., e.stackTraceToString().take(500) or (e.message ?: e::class.simpleName).take(500)) and pass that string to logger.error instead of the throwable; continue to respond with ErrorResponse(e.message) but ensure logger.error is called with the truncated string to keep logs under 500 characters.backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt (1)
704-721:⚠️ Potential issue | 🔴 CriticalValidate
jwks_uribefore handing it to Nimbus.
discoverOidcEndpoints()still trustsjwks_urifrom the discovery document, andRemoteJWKSetwill fetch it during ID token validation. A malicious or compromised issuer can point that URL at an internal service.🛠️ Suggested fix
val jwksUri = json["jwks_uri"]?.jsonPrimitive?.content ?: throw IllegalArgumentException( "OIDC discovery missing jwks_uri" ) + try { + UrlValidator.validateExternalUrl(jwksUri) + } catch (e: UrlValidator.SsrfException) { + throw IllegalArgumentException("Invalid jwks_uri URL: ${e.message}") + } + val entry = OidcDiscoveryCache(authEndpoint, tokenEndpoint, jwksUri, now) discoveryCache[issuerUrl] = entry return entryAlso applies to: 917-920
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt` around lines 704 - 721, discoverOidcEndpoints() currently trusts jwks_uri from the discovery document which can lead RemoteJWKSet to fetch unsafe internal URLs; validate jwksUri the same way authEndpoint and tokenEndpoint are validated by calling UrlValidator.validateExternalUrl(jwksUri) and catch UrlValidator.SsrfException, rethrowing an IllegalArgumentException with a clear message (e.g., "Invalid jwks_uri URL: ${e.message}"); apply the same validation to the other occurrence mentioned (lines ~917-920) so jwksUri is never passed to RemoteJWKSet unvalidated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt`:
- Around line 143-149: The lookup of SSO rows is case/whitespace sensitive:
normalize the domain on both persist and lookup by applying trim().lowercase()
to values used with SsoConfigurations.emailDomain; update the domain derivation
in SsoService (where domain = email.substringAfter("@")) and any other places
that compute/compare domains (e.g., checkSsoRequired()) to use
domain.trim().lowercase(), and ensure the write path that inserts/updates
SsoConfigurations.emailDomain also stores trim().lowercase(); factor this
normalization into a shared helper and call it from SsoService and the
enterprise SamlService init path so both reuse the same function.
- Line 1006: Add a final newline at the end of the file containing the
SsoService class so the file ends with a trailing newline character (fix the
FinalNewline/NewLineAtEndOfFile issue); simply ensure the file ends after the
closing brace '}' for SsoService with a single newline.
- Around line 211-215: The OIDC nonce lifecycle is broken: generateSecureState()
can return a state when the Redis write fails and decodeState() currently
consumes/discards the stored nonce so handleOidcCallback()'s
retrieveOidcNonceFromState(stateData.nonce) fails; fix by making
generateSecureState() only return a state after a successful Redis write (or
surface the write error), change decodeState() to not consume the nonce from
storage, and ensure retrieveOidcNonceFromState(nonce) is the single place that
both verifies-and-consumes the nonce at callback time (or alternatively make
generateSecureState() embed an HMACed nonce so callback validation doesn't
require a prior consumed read). Update the flow in generateSecureState(),
decodeState(), handleOidcCallback(), and retrieveOidcNonceFromState() so the
write/read/consume semantics are consistent and Redis failures surface as
internal errors rather than bad-request nonce misses.
- Around line 674-676: In SsoService replace logger.error(e) { "Failed to
decrypt SSO secret" } with a single-message log that does NOT pass the Throwable
directly: build a truncated stack/message (e.g. val err =
e.stackTraceToString().take(500) or e.message?.take(500)) and call logger.error
{ "Failed to decrypt SSO secret: $err" }; apply the same change to the other
catch block referenced (the second occurrence in SsoService) so no raw
exceptions are passed into logger.error and stack traces are truncated to 500
chars.
---
Duplicate comments:
In `@backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt`:
- Around line 704-721: discoverOidcEndpoints() currently trusts jwks_uri from
the discovery document which can lead RemoteJWKSet to fetch unsafe internal
URLs; validate jwksUri the same way authEndpoint and tokenEndpoint are validated
by calling UrlValidator.validateExternalUrl(jwksUri) and catch
UrlValidator.SsrfException, rethrowing an IllegalArgumentException with a clear
message (e.g., "Invalid jwks_uri URL: ${e.message}"); apply the same validation
to the other occurrence mentioned (lines ~917-920) so jwksUri is never passed to
RemoteJWKSet unvalidated.
In `@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.kt`:
- Around line 33-35: Replace raw throwable logging in the SAML route catch
blocks so they do not log full stack traces: in SamlRoutes.kt update the catch
handlers (the IllegalArgumentException catch around SAML init and the other
catches at lines handling validation and logout) to build a short error string
(e.g., e.stackTraceToString().take(500) or (e.message ?:
e::class.simpleName).take(500)) and pass that string to logger.error instead of
the throwable; continue to respond with ErrorResponse(e.message) but ensure
logger.error is called with the truncated string to keep logs under 500
characters.
In
`@ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt`:
- Around line 113-120: The SSO lookup selects by organizationId only, so update
the query that sets ssoConfig in SamlService (the block assigning ssoConfig) to
also require SsoConfigurations.isEnabled eq true and
SsoConfigurations.providerType eq "saml" (mirror the init path) so only enabled
SAML configurations are returned; keep throwing IllegalArgumentException when
none found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7fb4ded3-4e53-4778-bac4-b4b32033f142
📒 Files selected for processing (4)
backend/build.gradle.ktsbackend/src/main/kotlin/com/moneat/sso/services/SsoService.ktee/backend/src/main/kotlin/com/moneat/enterprise/sso/routes/SamlRoutes.ktee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt
✅ Files skipped from review due to trivial changes (1)
- backend/build.gradle.kts
Comment on lines
+143
to
+149
| val domain = email.substringAfter("@") | ||
| SsoConfigurations | ||
| .selectAll() | ||
| .where { | ||
| (SsoConfigurations.emailDomain eq domain) and | ||
| (SsoConfigurations.isEnabled eq true) | ||
| }.firstOrNull() |
There was a problem hiding this comment.
Normalize emailDomain on both write and lookup.
These comparisons are case/whitespace sensitive today. User@EXAMPLE.com can miss the configured row, and checkSsoRequired() can be bypassed with case changes. Normalize with trim().lowercase() when persisting and when deriving the lookup domain. The enterprise ee/backend/src/main/kotlin/com/moneat/enterprise/sso/services/SamlService.kt init path should reuse the same helper.
🛠️ Suggested fix
- val domain = email.substringAfter("@")
+ val domain = email.substringAfter("@").trim().lowercase()- it[SsoConfigurations.emailDomain] = request.emailDomain
+ it[SsoConfigurations.emailDomain] = request.emailDomain?.trim()?.lowercase()- it[SsoConfigurations.emailDomain] = request.emailDomain
+ it[SsoConfigurations.emailDomain] = request.emailDomain?.trim()?.lowercase()- val domain = email.substringAfter("@")
+ val domain = email.substringAfter("@").trim().lowercase()Also applies to: 463-479, 531-540
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt` around lines
143 - 149, The lookup of SSO rows is case/whitespace sensitive: normalize the
domain on both persist and lookup by applying trim().lowercase() to values used
with SsoConfigurations.emailDomain; update the domain derivation in SsoService
(where domain = email.substringAfter("@")) and any other places that
compute/compare domains (e.g., checkSsoRequired()) to use
domain.trim().lowercase(), and ensure the write path that inserts/updates
SsoConfigurations.emailDomain also stores trim().lowercase(); factor this
normalization into a shared helper and call it from SsoService and the
enterprise SamlService init path so both reuse the same function.
Comment on lines
+211
to
+215
| val stateData = decodeState(state) | ||
| val orgId = stateData.orgId | ||
|
|
||
| val expectedOidcNonce = retrieveOidcNonceFromState(stateData.nonce) | ||
| ?: throw IllegalArgumentException("OIDC nonce not found or expired") |
There was a problem hiding this comment.
The OIDC nonce lifecycle is broken across init and callback.
generateSecureState() can mint a state even if the Redis write failed, decodeState() consumes and discards the stored value, and handleOidcCallback() then does a second lookup for that same nonce. In practice this makes callbacks fail with "OIDC nonce not found or expired" and turns Redis failures into bad-request errors.
🛠️ Suggested fix
- val expectedOidcNonce = retrieveOidcNonceFromState(stateData.nonce)
+ val expectedOidcNonce = stateData.oidcNonce
?: throw IllegalArgumentException("OIDC nonce not found or expired")- try {
- if (RedisConfig.isInitialized()) {
- val redisValue = if (oidcNonce != null) {
- "$orgId:$oidcNonce"
- } else {
- orgId.toString()
- }
- RedisConfig.sync().setex(
- "$SSO_NONCE_PREFIX$nonceB64",
- SSO_NONCE_TTL_SECONDS,
- redisValue
- )
- }
- } catch (e: Exception) {
- logger.warn(e) { "Failed to store SSO nonce in Redis" }
- }
+ check(RedisConfig.isInitialized()) { "Redis must be initialized for SSO state storage" }
+ val redisValue = if (oidcNonce != null) {
+ "$orgId:$oidcNonce"
+ } else {
+ orgId.toString()
+ }
+ RedisConfig.sync().setex(
+ "$SSO_NONCE_PREFIX$nonceB64",
+ SSO_NONCE_TTL_SECONDS,
+ redisValue
+ )- consumeNonce(nonceB64)
-
- return SsoStateData(nonceB64, orgId, timestamp)
+ val stored = consumeNonce(nonceB64)
+ val oidcNonce = stored.substringAfter(':', "").ifBlank { null }
+
+ return SsoStateData(nonceB64, orgId, timestamp, oidcNonce)
} catch (e: IllegalArgumentException) {
throw e
+ } catch (e: IllegalStateException) {
+ throw e
} catch (e: Exception) {
throw IllegalArgumentException("Invalid state parameter")
}- private fun consumeNonce(nonceB64: String) {
+ private fun consumeNonce(nonceB64: String): String {
try {
- if (!RedisConfig.isInitialized()) {
- return
- }
+ check(RedisConfig.isInitialized()) { "Redis must be initialized for SSO state validation" }
val redisKey = "$SSO_NONCE_PREFIX$nonceB64"
- val stored = RedisConfig.sync().get(redisKey)
- requireNotNull(stored) { "State already used or expired" }
- RedisConfig.sync().del(redisKey)
+ return RedisConfig.sync().getdel(redisKey)
+ ?: throw IllegalArgumentException("State already used or expired")
} catch (e: IllegalArgumentException) {
throw e
} catch (e: Exception) {
throw IllegalStateException(
"Failed to verify SSO nonce ($SSO_NONCE_PREFIX$nonceB64)",
e
)
}
}Also applies to: 603-643, 873-907
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt` around lines
211 - 215, The OIDC nonce lifecycle is broken: generateSecureState() can return
a state when the Redis write fails and decodeState() currently consumes/discards
the stored nonce so handleOidcCallback()'s
retrieveOidcNonceFromState(stateData.nonce) fails; fix by making
generateSecureState() only return a state after a successful Redis write (or
surface the write error), change decodeState() to not consume the nonce from
storage, and ensure retrieveOidcNonceFromState(nonce) is the single place that
both verifies-and-consumes the nonce at callback time (or alternatively make
generateSecureState() embed an HMACed nonce so callback validation doesn't
require a prior consumed read). Update the flow in generateSecureState(),
decodeState(), handleOidcCallback(), and retrieveOidcNonceFromState() so the
write/read/consume semantics are consistent and Redis failures surface as
internal errors rather than bad-request nonce misses.
Comment on lines
+674
to
+676
| } catch (e: Exception) { | ||
| logger.error(e) { "Failed to decrypt SSO secret" } | ||
| return null |
There was a problem hiding this comment.
Bound throwable logging in SsoService.
These calls still pass raw exceptions to the logger, so the new OIDC flows can emit full stack traces. Log a truncated message/stack string instead.
As per coding guidelines, "Keep stack traces under 500 characters when logging errors in Kotlin (use .take(500) for error messages)".
Also applies to: 967-969
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt` around lines
674 - 676, In SsoService replace logger.error(e) { "Failed to decrypt SSO
secret" } with a single-message log that does NOT pass the Throwable directly:
build a truncated stack/message (e.g. val err = e.stackTraceToString().take(500)
or e.message?.take(500)) and call logger.error { "Failed to decrypt SSO secret:
$err" }; apply the same change to the other catch block referenced (the second
occurrence in SsoService) so no raw exceptions are passed into logger.error and
stack traces are truncated to 500 chars.
| private const val TOKEN_TTL_MS = 3_600_000L | ||
| private const val MILLIS_PER_SECOND = 1000L | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Add the trailing newline.
Sonar/detekt is already failing on this file for FinalNewline / NewLineAtEndOfFile.
🧰 Tools
🪛 GitHub Actions: SonarQube Analysis
[error] 1006-1006: detekt: File is not ending with a new line. [NewLineAtEndOfFile]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/main/kotlin/com/moneat/sso/services/SsoService.kt` at line 1006,
Add a final newline at the end of the file containing the SsoService class so
the file ends with a trailing newline character (fix the
FinalNewline/NewLineAtEndOfFile issue); simply ensure the file ends after the
closing brace '}' for SsoService with a single newline.
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
1 participant
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.
Description
Per #186, I've decided to allow OIDC without requiring a license, and focus on allowing enterprise-grade features like enforce SSO for the enterprise license.
Summary by CodeRabbit
New Features
Bug Fixes / UX
Documentation