feat: implement OIDC provider management in GraphQL API #1563
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds end-to-end OIDC SSO: file-backed provider persistence and migration, discovery/validation, auth URL and callback handling, ephemeral one-time session tokens, GraphQL schema/resolvers/queries, REST/CLI integration, frontend SSO components and JSONForms enhancements, docs, and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Web as Web (Login UI)
participant API as API REST
participant OIDC as OIDC Provider
participant Auth as OidcAuthService
participant Sess as OidcSessionService
User->>Web: Click provider button
Web->>API: GET /graphql/api/auth/oidc/authorize/:providerId?state=...
API->>Auth: getAuthorizationUrl(providerId, state, host)
Auth-->>API: authorization URL
API-->>Web: 302 redirect to OIDC
Web->>OIDC: Authorization request
OIDC-->>API: Redirect /graphql/api/auth/oidc/callback?code&state
API->>Auth: handleCallback(providerId, code, state, host, fullUrl)
Auth->>Sess: createSession(providerId, subject)
Sess-->>Auth: one-time token
API-->>Web: 302 redirect to /login#token=<token>
Web->>Web: Submit login form with token (useSsoAuth)
sequenceDiagram
autonumber
participant CLI as CLI (validate-token)
participant API as API GraphQL
participant Res as SsoResolver
participant Sess as OidcSessionService
CLI->>API: validateOidcSession(token)
API->>Res: resolve validateOidcSession
Res->>Sess: validateSession(token)
Sess-->>Res: { valid, username? }
Res-->>API: OidcSessionValidation
API-->>CLI: { valid, username }
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🔭 Outside diff range comments (1)
packages/unraid-shared/package.json (1)
29-38:class-transformermust be a runtime / peer dependency, not merely a dev dependency
class-transformeris required at runtime whenever decorated DTOs/entities from@unraid/sharedare instantiated, but it is declared here only underdevDependencies.
Down-stream packages that install@unraid/sharedin production will therefore miss the module and crash withERR_MODULE_NOT_FOUND.Action:
- "devDependencies": { - … - "class-transformer": "0.5.1", - … - }, + "dependencies": { + "class-transformer": "0.5.1" + }, + "peerDependencies": { + …, + "class-transformer": "0.5.1", + … + },Move it to
dependencies(or at minimumpeerDependencies) and bump the lock-file accordingly.
🧹 Nitpick comments (17)
api/src/unraid-api/graph/resolvers/resolvers.module.ts (1)
30-52: ImportingSsoModulelooks good, check circular-dependency risk
SsoModuleis injected into the main resolver tree. As it depends onAuthModuleinternally, make sure there is no circular import between the two (Nest will warn at runtime). If none, this addition is fine.api/.gitignore (1)
86-88: Redundant ignore rule – already covered bydev/configs/*
dev/configs/*(Line 90) already excludes every file in that folder, so explicitly listingdev/configs/oidc.local.jsonis unnecessary noise.Apply:
-# local OIDC config for testing - contains secrets -dev/configs/oidc.local.jsonapi/.env.development (1)
20-20: Keep.envkeys sorted to silence dotenv-linter
PATHS_OIDC_JSONis out of alphabetical order, triggering the linter warning. Move it abovePATHS_PARITY_CHECKSto stay consistent with the rest of the file.web/components/queries/oidc-providers.query.ts (1)
3-22: Consider adding__typenamefor better Apollo cache normalizationIncluding
__typenamein the selection set helps Apollo identify the object type and avoids cache misses when the same query is executed elsewhere with__typenamepresent.buttonIcon + __typenameapi/src/unraid-api/graph/resolvers/sso/oidc-session-validation.model.ts (1)
3-10: Optional: add class-level description for schema clarityA short description via
@ObjectType({ description: '...' })makes the generated GraphQL schema self-documenting and improves SDL readability.-@ObjectType() +@ObjectType({ description: 'Result of validating an OIDC session token' })api/src/unraid-api/cli/queries/validate-oidc-session.query.ts (1)
3-10: Add__typenameto avoid Apollo cache collisionsFor consistency with other queries and safer cache behaviour, include
__typename:username + __typenameweb/server/api/debug/validate-token.post.ts (1)
31-41: Improve JSON parsing robustness.The current JSON parsing logic could be more robust and easier to maintain.
- // Look for JSON output from the end - for (let i = lines.length - 1; i >= 0; i--) { - const line = lines[i].trim(); - if (line.startsWith('{') && line.endsWith('}')) { - try { - parsedOutput = JSON.parse(line); - break; - } catch { - // Continue looking - } - } - } + // Look for JSON output from the end + parsedOutput = lines + .reverse() + .find(line => { + const trimmed = line.trim(); + if (trimmed.startsWith('{') && trimmed.endsWith('}')) { + try { + return JSON.parse(trimmed); + } catch { + return false; + } + } + return false; + });web/components/sso/SsoProviderButton.vue (2)
34-41: Improve accessibility for provider icons.The alt text should be more descriptive for screen readers.
<img v-if="provider.buttonIcon" :src="provider.buttonIcon" class="w-5 h-5 mr-2" - :alt="provider.name" + :alt="`${provider.name} logo`" >
44-57: Consider reducing !important declarations in CSS.Excessive use of
!importantcan indicate styling conflicts and make the component harder to customize.Consider using more specific CSS selectors or CSS modules to avoid the need for
!importantdeclarations. This would make the component more maintainable and easier to customize.web/components/sso/useSsoProviders.ts (2)
54-62: Consider implementing exponential backoff for polling.Polling every 2 seconds indefinitely could be resource-intensive, especially if the API takes longer to become available.
+ const pollDelay = ref(2000); + const maxPollDelay = 30000; // 30 seconds max + onMounted(() => { checkApiAvailability(); - // Poll every 2 seconds if API is not available + // Poll with exponential backoff if API is not available pollInterval.value = setInterval(() => { if (!apiAvailable.value) { checkApiAvailability(); + // Increase delay up to maximum + pollDelay.value = Math.min(pollDelay.value * 1.5, maxPollDelay); } - }, 2000); + }, pollDelay.value); });
20-27: Consider using cache-first fetch policy.The
network-onlyfetch policy might be too aggressive and could impact performance.const { result: providersResult, refetch: refetchProviders } = useQuery( PUBLIC_OIDC_PROVIDERS, null, { - fetchPolicy: 'network-only', + fetchPolicy: 'cache-first', errorPolicy: 'all', } );This would allow the component to use cached data when available while still allowing manual refetches during polling.
api/src/unraid-api/graph/resolvers/settings/settings.service.ts (1)
81-83: Clean up commented codeThe commented
extraOriginsSlicecode should either be removed or properly documented with a TODO/FIXME comment explaining the retention reason.- // Because CORS is effectively disabled, this setting is no longer necessary - // keeping it here for in case it needs to be re-enabled - // slices.push(this.extraOriginsSlice()); + // TODO: CORS settings temporarily disabled - may need to re-enable if CORS restrictions are reinstated + // slices.push(this.extraOriginsSlice());Or remove entirely if the feature is permanently deprecated.
web/components/sso/SsoButtons.vue (1)
43-45: Remove empty style sectionThe empty style section can be removed if no component-specific styles are needed.
Remove lines 43-45 entirely if no styles are planned.
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.ts (1)
83-100: Consider more flexible UUID extractionThe regex pattern
^OIDC-SESSION-([a-f0-9-]+)-only matches lowercase hex characters, but UUIDs can contain uppercase letters when using certain UUID versions or formats.Consider using a more flexible pattern:
- const match = signature.match(/^OIDC-SESSION-([a-f0-9-]+)-/); + const match = signature.match(/^OIDC-SESSION-([a-fA-F0-9-]+)-/);web/pages/login.vue (1)
130-172: Consider moving debug tools to a separate development-only pageWhile the debug modal is useful for development, having it on the production login page could confuse users or expose implementation details.
Consider:
- Adding an environment check to only show the debug functionality in development
- Moving this to a separate
/debug/ssoroute- Adding a feature flag to control visibility
Would you like me to help implement a development-only flag for this feature?
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts (1)
169-177: Add comment clarifying JWT parsing is for claims extraction onlyThe manual JWT parsing without verification could be misunderstood as a security issue. Add a comment to clarify that signature verification is handled by openid-client.
Add clarifying comment:
// Parse ID token to get user info let claims: { sub?: string } | null = null; if (tokens.id_token) { try { - // Decode the JWT manually + // Decode the JWT manually for claims extraction only + // Note: Signature verification is already done by openid-client const payload = tokens.id_token.split('.')[1]; claims = JSON.parse(Buffer.from(payload, 'base64').toString()); this.logger.debug(`ID token claims: ${JSON.stringify(claims)}`);api/generated-schema.graphql (1)
1403-1409: Consider whether SsoSettings really needs to be a top-level Node.SsoSettings is only ever accessed through the Settings root-object. Exposing it as a first-class Node (with its own globally unique ID) increases API surface and resolver work for no practical gain. If there is no use-case for fetching an
SsoSettingsinstance by ID, droppingimplements Nodehere will keep the schema smaller and simpler.
elibosley
force-pushed
the
api-feat-oidc-sso
branch
from
7b32d5b to
67b4a0c
Compare
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (12)
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts (1)
101-110:cleanProviderstill omits several optional fields
jwksUri,buttonText, andbuttonVariantare not normalised, so empty-string values survive persistence and re-appear in the UI. Please trim/undefine them exactly like the other optional strings.- buttonIcon: provider.buttonIcon?.trim() || undefined, - buttonStyle: provider.buttonStyle?.trim() || undefined, + jwksUri: provider.jwksUri?.trim() || undefined, + buttonIcon: provider.buttonIcon?.trim() || undefined, + buttonText: provider.buttonText?.trim() || undefined, + buttonVariant: provider.buttonVariant?.trim() || undefined, + buttonStyle: provider.buttonStyle?.trim() || undefined,api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts (1)
339-348: Refactor hard-coded redirect URI logicPrevious feedback about configurability remains unaddressed – hard-coded localhost detection and default
tower.localwill break behind reverse proxies or non-standard ports. Delegate to config values (OIDC_CALLBACK_URL, etc.) for flexibility.web/pages/login.vue (2)
38-42: Do not echo plaintext passwords in debug modalShowing the submitted password jeopardises users’ credentials. Mask or omit it as previously suggested.
50-70: Fix TS2339 by typing$fetchresponseThe pipeline error persists. Define
ValidateTokenResponse(with optionalstdout,error, etc.) and use$fetch<ValidateTokenResponse>()or assert the type before accessingstdout.api/src/unraid-api/rest/rest.controller.ts (2)
69-88: Validate providerId input (defense-in-depth).Add a simple allowlist check for providerId before passing to the service to reduce attack surface.
) { try { if (!state) { return res.status(400).send('State parameter is required'); } + + // Validate providerId format (alphanumeric, dot, underscore, dash, max 64) + if (!/^[A-Za-z0-9._-]{1,64}$/.test(providerId)) { + return res.status(400).send('Invalid provider ID format'); + }
133-159: Simplify and sanitize error handling to avoid leaking details.The deep inspection/logging of UnauthorizedException response increases exposure risk. Keep user-facing message minimal and log only safe info.
- if (error instanceof UnauthorizedException) { - // NestJS exceptions store the message in the response property - const response = error.getResponse(); - this.logger.debug(`UnauthorizedException response: ${JSON.stringify(response)}`); - this.logger.debug(`UnauthorizedException message: ${error.message}`); - - // Try to get the message from various places - if (typeof response === 'string') { - errorMessage = response; - } else if (typeof response === 'object' && response !== null) { - // Check for message in response object - if ('message' in response && response.message) { - errorMessage = Array.isArray(response.message) - ? response.message[0] - : String(response.message); - } else if ('error' in response && response.error) { - errorMessage = String(response.error); - } - } - - // If we still don't have a message, use the error's message property - if (errorMessage === 'Authentication failed' && error.message) { - errorMessage = error.message; - } - } else if (error instanceof Error) { - errorMessage = error.message; - } + if (error instanceof UnauthorizedException) { + // Map to safe, user-friendly messages only + const resp = error.getResponse() as any; + const msg = typeof resp === 'string' ? resp : resp?.message; + if (typeof msg === 'string') { + if (msg.includes('authorization requirements')) { + errorMessage = 'Access denied: Authorization requirements not met'; + } else if (msg.includes('Provider') && msg.includes('not found')) { + errorMessage = 'Invalid authentication provider'; + } + } + } else if (error instanceof Error && error.message) { + // Safe generic error text + errorMessage = 'Authentication failed'; + }api/docs/public/oidc-provider-examples.md (1)
17-20: Emphasize HTTPS requirement for production redirect URIs.Reinforce secure redirects immediately after examples.
- Local development: `http://localhost:3001/graphql/api/auth/oidc/callback` - Production: `http://192.168.1.100:3001/graphql/api/auth/oidc/callback` - With custom domain: `https://unraid.example.com/graphql/api/auth/oidc/callback` + +⚠️ Security Warning: Always use HTTPS for production redirect URIs to prevent token interception. HTTP should only be used for local development.api/generated-schema.graphql (4)
1463-1522: Do not expose clientSecret and implement Node on OidcProvider.Prevent accidental secret leakage and align with Node-based resources.
-type OidcProvider { +type OidcProvider implements Node { """The unique identifier for the OIDC provider""" id: PrefixedID! @@ - """OAuth2 client secret (if required by provider)""" - clientSecret: String + # clientSecret intentionally omitted from API output
1942-1953: Add auth directives to admin/internal queries and fix id type.Secure provider/config/session endpoints and use consistent ID typing.
"""Get public OIDC provider information for login buttons""" publicOidcProviders: [PublicOidcProvider!]! """Get all configured OIDC providers (admin only)""" - oidcProviders: [OidcProvider!]! + oidcProviders: [OidcProvider!]! @usePermissions(action: READ, resource: "SSO", possession: ANY) """Get a specific OIDC provider by ID""" - oidcProvider(id: String!): OidcProvider + oidcProvider(id: PrefixedID!): OidcProvider @usePermissions(action: READ, resource: "SSO", possession: ANY) """Validate an OIDC session token (internal use for CLI validation)""" - validateOidcSession(token: String!): OidcSessionValidation! + validateOidcSession(token: String!): OidcSessionValidation! @usePermissions(action: READ, resource: "SSO", possession: OWN)
1999-2004: Enforce permissions on mutations and use PrefixedID for delete.Match API conventions and protect update/delete.
"""Create or update an OIDC provider""" - upsertOidcProvider(provider: OidcProviderInput!): OidcProvider! + upsertOidcProvider(provider: OidcProviderInput!): OidcProvider! @usePermissions(action: UPDATE, resource: "SSO", possession: ANY) """Delete an OIDC provider""" - deleteOidcProvider(id: String!): Boolean! + deleteOidcProvider(id: PrefixedID!): Boolean! @usePermissions(action: DELETE, resource: "SSO", possession: ANY)
2046-2105: Make id optional in OidcProviderInput to support create vs. update.Standard upsert pattern: absent id → create; present id → update.
input OidcProviderInput { """The unique identifier for the OIDC provider""" - id: String! + id: Stringweb/composables/gql/graphql.ts (1)
1145-1175: Avoid querying clientSecret and sanitize buttonStyle downstream.The generated type includes clientSecret, but your queries wisely don’t select it. Keep it that way; after server-side removal/masking, this type will update. Also, ensure any use of buttonStyle in the UI strips dangerous tokens like url(, @import, expression( to mitigate CSS injection.
🧹 Nitpick comments (7)
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts (1)
149-160: Normalise before persisting
cleanedProvideris built manually and may still carry stray empty strings. Re-usecleanProvider(or extract a shared helper) before writing to disk to guarantee consistency between read & write paths.api/.env.development (1)
20-20: Keep .env keys ordered to satisfy dotenv-linterMove
PATHS_OIDC_JSONabovePATHS_PARITY_CHECKS(Line 12) to maintain alphabetical grouping and silence the linter warning.api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts (1)
90-91: Replace hard-coded JWT with obvious dummy tokenThe long JWT-looking string triggers secret-scanners. Shorten it (e.g.
DUMMY.JWT.TOKEN) – functionality of the test doesn’t depend on token contents.api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts (1)
170-176: Avoid logging raw ID-token claims
this.logger.debugprints the full JWT claims, potentially leaking PII in logs. Mask or omit sensitive fields before logging.api/docs/public/oidc-provider-examples.md (1)
297-306: Align documented operators with schema support.Docs list regex/in operators that are not in the current schema. Clarify supported operators or add a note.
- - `regex`: Regular expression match - - `in`: Claim value must be in the provided list + (Currently supported: `equals`, `contains`, `endsWith`, `startsWith`)If
regex/inare intended, extend the schema’s AuthorizationOperator accordingly.api/generated-schema.graphql (1)
1456-1461: Optionally extend AuthorizationOperator if docs require it.If you want regex/in semantics, add them here; otherwise keep docs limited.
enum AuthorizationOperator { EQUALS CONTAINS ENDS_WITH STARTS_WITH + # REGEX + # IN }web/composables/gql/graphql.ts (1)
1013-1016: Re-run GraphQL codegen after installing dependenciesThe
deleteOidcProviderinput type should switch toPrefixedIDonce the server schema is updated. Currently inweb/composables/gql/graphql.ts(lines 1013–1016):export type MutationDeleteOidcProviderArgs = { id: Scalars['String']['input']; };Please verify the following after your schema fixes:
- Ensure dependencies are installed (so
graphql-codegenis available):pnpm install- Regenerate types/docs in the web package:
pnpm --filter web codegen- Confirm that
MutationDeleteOidcProviderArgs.idnow usesPrefixedIDinstead ofString.- Repeat verification for lines 1063–1066 and 1415–1417 in the same file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
api/src/unraid-api/cli/generated/gql.tsis excluded by!**/generated/**api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (48)
api/.env.development(1 hunks)api/.gitignore(1 hunks)api/dev/configs/README.md(1 hunks)api/dev/configs/api.json(1 hunks)api/dev/configs/connect.json(1 hunks)api/dev/configs/oidc.json(1 hunks)api/docs/public/index.md(1 hunks)api/docs/public/oidc-provider-examples.md(1 hunks)api/generated-schema.graphql(5 hunks)api/package.json(1 hunks)api/src/unraid-api/app/app.module.ts(1 hunks)api/src/unraid-api/cli/queries/validate-oidc-session.query.ts(1 hunks)api/src/unraid-api/cli/sso/validate-token.command.ts(2 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(2 hunks)api/src/unraid-api/graph/resolvers/settings/settings.model.ts(2 hunks)api/src/unraid-api/graph/resolvers/settings/settings.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/settings/settings.resolver.ts(5 hunks)api/src/unraid-api/graph/resolvers/settings/settings.service.ts(4 hunks)api/src/unraid-api/graph/resolvers/settings/sso-settings.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-session-validation.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-session.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/public-oidc-provider.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/sso-settings.types.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/sso.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts(1 hunks)api/src/unraid-api/graph/utils/form-utils.ts(1 hunks)api/src/unraid-api/rest/rest.controller.ts(2 hunks)api/src/unraid-api/rest/rest.module.ts(1 hunks)packages/unraid-shared/package.json(1 hunks)unraid-ui/src/forms/ObjectArrayField.vue(1 hunks)unraid-ui/src/forms/renderers.ts(4 hunks)web/components/SsoButton.ce.vue(1 hunks)web/components/queries/oidc-providers.query.ts(1 hunks)web/components/queries/public-oidc-providers.query.ts(1 hunks)web/components/sso/SsoButtons.vue(1 hunks)web/components/sso/SsoProviderButton.vue(1 hunks)web/components/sso/useSsoAuth.ts(1 hunks)web/components/sso/useSsoProviders.ts(1 hunks)web/composables/gql/gql.ts(3 hunks)web/composables/gql/graphql.ts(16 hunks)web/nuxt.config.ts(1 hunks)web/pages/login.vue(9 hunks)web/server/api/debug/validate-token.post.ts(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- api/package.json
- api/src/unraid-api/rest/rest.module.ts
- web/components/SsoButton.ce.vue
- api/dev/configs/connect.json
- api/.gitignore
- api/src/unraid-api/graph/resolvers/sso/oidc-session-validation.model.ts
- api/src/unraid-api/graph/resolvers/sso/public-oidc-provider.model.ts
🚧 Files skipped from review as they are similar to previous changes (32)
- packages/unraid-shared/package.json
- api/docs/public/index.md
- api/src/unraid-api/app/app.module.ts
- api/dev/configs/api.json
- api/src/unraid-api/graph/resolvers/settings/settings.module.ts
- web/nuxt.config.ts
- api/src/unraid-api/cli/queries/validate-oidc-session.query.ts
- web/components/sso/useSsoProviders.ts
- api/src/unraid-api/graph/resolvers/settings/sso-settings.model.ts
- web/components/queries/oidc-providers.query.ts
- api/src/unraid-api/graph/utils/form-utils.ts
- api/src/unraid-api/graph/resolvers/resolvers.module.ts
- web/components/sso/SsoProviderButton.vue
- web/server/api/debug/validate-token.post.ts
- api/src/unraid-api/cli/sso/validate-token.command.ts
- api/dev/configs/oidc.json
- api/src/unraid-api/graph/resolvers/sso/oidc-session.service.ts
- api/src/unraid-api/graph/resolvers/settings/settings.model.ts
- api/src/unraid-api/graph/resolvers/sso/sso-settings.types.ts
- web/components/queries/public-oidc-providers.query.ts
- unraid-ui/src/forms/ObjectArrayField.vue
- api/src/unraid-api/graph/resolvers/sso/sso.module.ts
- api/dev/configs/README.md
- api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts
- web/composables/gql/gql.ts
- api/src/unraid-api/graph/resolvers/settings/settings.resolver.ts
- unraid-ui/src/forms/renderers.ts
- web/components/sso/useSsoAuth.ts
- web/components/sso/SsoButtons.vue
- api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts
- api/src/unraid-api/graph/resolvers/settings/settings.service.ts
- api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts
🧰 Additional context used
📓 Path-based instructions (4)
api/**/*
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*: Use pnpm as the only package manager
Always run scripts from api/package.json unless specifically requested otherwise
Files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsapi/docs/public/oidc-provider-examples.mdapi/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.tsapi/generated-schema.graphqlapi/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
api/src/unraid-api/**/*
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the NestJS repository located at api/src/unraid-api/ instead of the legacy code
Prefer adding new files to the NestJS repo located at api/src/unraid-api/ instead of the legacy code
Files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsapi/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.tsapi/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
api/**/*.{test,spec}.{js,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,ts,tsx}: Use Vitest as the test suite; do not use Jest
Prefer not to mock simple dependencies in tests
Files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
api/**/*.{test,spec}.{ts,js}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Test suite is VITEST, do not use jest in the API package
Files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
🧠 Learnings (74)
📓 Common learnings
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:111-113
Timestamp: 2024-11-06T20:59:25.809Z
Learning: In the Unraid API project, error handling for mutations is handled at the service level rather than in the GraphQL resolvers. Specifically, in `api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts`, methods like `removeRoleFromApiKey` rely on service-level error handling.
📚 Learning: 2024-11-05T14:49:07.308Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In `api/src/unraid-api/auth/api-key.service.ts`, when handling read errors in the `findById` method, throw a `GraphQLError` instead of an `InternalServerErrorException`.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsweb/pages/login.vueapi/src/unraid-api/rest/rest.controller.ts
📚 Learning: 2024-11-06T20:59:25.809Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:111-113
Timestamp: 2024-11-06T20:59:25.809Z
Learning: In the Unraid API project, error handling for mutations is handled at the service level rather than in the GraphQL resolvers. Specifically, in `api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts`, methods like `removeRoleFromApiKey` rely on service-level error handling.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsweb/pages/login.vueapi/src/unraid-api/rest/rest.controller.tsapi/generated-schema.graphql
📚 Learning: 2024-11-04T22:00:41.946Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.service.ts:42-105
Timestamp: 2024-11-04T22:00:41.946Z
Learning: In the 'AuthService' within `api/src/unraid-api/auth/auth.service.ts`, rate limiting is not required because the API is not public facing.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsapi/src/unraid-api/rest/rest.controller.ts
📚 Learning: 2025-04-23T20:19:42.542Z
Learnt from: pujitm
PR: unraid/api#1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts
📚 Learning: 2024-11-15T16:22:12.460Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.module.ts:94-96
Timestamp: 2024-11-15T16:22:12.460Z
Learning: In `CookieService.defaultOpts()` in `api/src/unraid-api/auth/cookie.service.ts`, use `sameSite: 'lax'` for session cookie configuration.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts
📚 Learning: 2025-03-04T14:55:00.903Z
Learnt from: elibosley
PR: unraid/api#1151
File: plugin/builder/utils/consts.ts:6-6
Timestamp: 2025-03-04T14:55:00.903Z
Learning: The startingDir constant in plugin/builder/utils/consts.ts is defined using process.cwd(), which can cause issues if directory changes occur after importing this constant. Using __dirname.split('/builder')[0] would make it more reliable by making it relative to the file location rather than the current working directory.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts
📚 Learning: 2025-07-24T18:48:44.035Z
Learnt from: mdatelle
PR: unraid/api#1533
File: web/components/Docker/Edit.vue:16-32
Timestamp: 2025-07-24T18:48:44.035Z
Learning: In web/components/Docker/Edit.vue, the hardcoded configuration values in the config ref are intentional temporary/dummy data used during initial UI development phase while building out the real Docker components, as clarified by mdatelle in PR #1533.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsweb/pages/login.vue
📚 Learning: 2025-05-08T19:28:37.034Z
Learnt from: elibosley
PR: unraid/api#1381
File: plugin/builder/cli/common-environment.ts:18-22
Timestamp: 2025-05-08T19:28:37.034Z
Learning: In plugin/builder/cli/common-environment.ts, the getDefaultBaseUrl() function intentionally returns an invalid URL when running in CI environments to force explicit specification of the base URL parameter rather than relying on defaults.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts
📚 Learning: 2025-01-27T14:57:46.617Z
Learnt from: elibosley
PR: unraid/api#1063
File: web/_data/serverState.ts:137-147
Timestamp: 2025-01-27T14:57:46.617Z
Learning: The values in `web/_data/serverState.ts` are used for testing purposes and should remain as hardcoded mock data to facilitate testing different scenarios.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts
📚 Learning: 2025-03-10T18:43:13.016Z
Learnt from: mdatelle
PR: unraid/api#1219
File: api/src/unraid-api/types/fastify.ts:25-30
Timestamp: 2025-03-10T18:43:13.016Z
Learning: In the Unraid API, cookies should be marked as optional (with `?`) in the FastifyRequest type definition since the cookies may not always be present, which is consistent with the authentication flow throughout the codebase.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsapi/docs/public/oidc-provider-examples.mdapi/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.tsapi/generated-schema.graphql
📚 Learning: 2024-11-18T20:52:47.800Z
Learnt from: elibosley
PR: unraid/api#963
File: web/helpers/markdown.ts:1-13
Timestamp: 2024-11-18T20:52:47.800Z
Learning: All configurations and direct usage of `marked` should be centralized in the `safeParseMarkdown` function in `web/helpers/markdown.ts` to maintain security and consistency.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts
📚 Learning: 2025-04-25T13:29:07.868Z
Learnt from: elibosley
PR: unraid/api#1369
File: web/components/Activation/WelcomeModal.ce.vue:40-40
Timestamp: 2025-04-25T13:29:07.868Z
Learning: The `#confirmPassword` selector in the WelcomeModal.ce.vue component is intentionally looking for an element that exists in the Unraid OS login page (.login.php), not within the component itself. This is used as part of a font-size adjustment workaround to handle different CSS between login and authenticated pages.
Applied to files:
web/pages/login.vue
📚 Learning: 2025-02-18T17:36:22.670Z
Learnt from: pujitm
PR: unraid/api#1143
File: web/components/DummyServerSwitcher.vue:16-19
Timestamp: 2025-02-18T17:36:22.670Z
Learning: The DummyServerSwitcher component in web/components/DummyServerSwitcher.vue is a development tool for testing different server states, and includes intentional debug elements to aid development.
Applied to files:
web/pages/login.vue
📚 Learning: 2025-07-24T18:48:43.590Z
Learnt from: mdatelle
PR: unraid/api#1533
File: web/components/Docker/Console.vue:22-29
Timestamp: 2025-07-24T18:48:43.590Z
Learning: The Docker Console component in web/components/Docker/Console.vue is temporary/dummy code with placeholder terminal responses, used for building out the UI layout while developing the real components in PR #1533.
Applied to files:
web/pages/login.vue
📚 Learning: 2025-01-27T14:31:42.305Z
Learnt from: elibosley
PR: unraid/api#1063
File: web/components/SsoButton.ce.vue:5-8
Timestamp: 2025-01-27T14:31:42.305Z
Learning: In the Unraid API web components, SSO-related props are intentionally provided in both camelCase (`ssoEnabled`) and lowercase (`ssoenabled`) variants to support interchangeable usage across different contexts (e.g., HTML attributes vs Vue props).
Applied to files:
web/pages/login.vueapi/docs/public/oidc-provider-examples.mdweb/composables/gql/graphql.ts
📚 Learning: 2025-07-21T14:01:04.139Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/__test__/components/**/*.ts : Use `mount` from Vue Test Utils for component testing
Applied to files:
web/pages/login.vue
📚 Learning: 2025-02-06T17:24:58.784Z
Learnt from: mdatelle
PR: unraid/api#1122
File: web/components/UserProfile/DropdownLaunchpad.vue:38-42
Timestamp: 2025-02-06T17:24:58.784Z
Learning: In the DropdownLaunchpad.vue component, the current implementation of conditional icon rendering using `h(BrandLoading, { variant: 'white' })` is a temporary fix to maintain build stability, with a proper refactoring being handled in a separate branch.
Applied to files:
web/pages/login.vue
📚 Learning: 2025-05-23T20:11:08.857Z
Learnt from: mdatelle
PR: unraid/api#1362
File: api/src/unraid-api/graph/resolvers/rclone/rclone-api.service.ts:421-424
Timestamp: 2025-05-23T20:11:08.857Z
Learning: When logging parameters or request data in error messages, always sanitize sensitive information by checking for common sensitive parameter names (pass, secret, token, key) and replacing their values with '***REDACTED***' or similar placeholders to prevent credential exposure in logs.
Applied to files:
web/pages/login.vueapi/docs/public/oidc-provider-examples.mdapi/src/unraid-api/rest/rest.controller.ts
📚 Learning: 2025-01-15T21:34:00.006Z
Learnt from: pujitm
PR: unraid/api#1047
File: api/src/unraid-api/graph/sandbox-plugin.ts:57-57
Timestamp: 2025-01-15T21:34:00.006Z
Learning: In the GraphQL sandbox (api/src/unraid-api/graph/sandbox-plugin.ts), CSRF token validation should fail silently with a fallback value to maintain sandbox accessibility, as it's a development tool where strict security measures aren't required.
Applied to files:
web/pages/login.vueapi/docs/public/oidc-provider-examples.mdweb/composables/gql/graphql.tsapi/generated-schema.graphql
📚 Learning: 2025-03-07T19:29:12.540Z
Learnt from: pujitm
PR: unraid/api#1219
File: api/src/unraid-api/main.ts:18-25
Timestamp: 2025-03-07T19:29:12.540Z
Learning: When handling type mismatches between dependencies in TypeScript (such as between NestJS and Fastify), prefer using `ts-expect-error` with a detailed explanation comment instead of `as any` type assertions. This better documents the reason for the type bypass and makes the issue more visible for future maintenance.
Applied to files:
web/pages/login.vueapi/src/unraid-api/rest/rest.controller.ts
📚 Learning: 2025-02-06T17:25:45.397Z
Learnt from: mdatelle
PR: unraid/api#1122
File: web/package.json:16-16
Timestamp: 2025-02-06T17:25:45.397Z
Learning: The build script in web/package.json should include type-check command (`npm run type-check`) even when NODE_ENV is production, as it provides value for local builds by catching type errors before pushing to CI/CD.
Applied to files:
web/pages/login.vue
📚 Learning: 2025-07-21T14:01:04.139Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/__test__/store/**/*.ts : Verify proper error handling in store tests
Applied to files:
web/pages/login.vueapi/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
📚 Learning: 2025-01-28T16:39:50.278Z
Learnt from: elibosley
PR: unraid/api#1078
File: api/src/unraid-api/unraid-file-modifier/modifications/sso.modification.ts:35-36
Timestamp: 2025-01-28T16:39:50.278Z
Learning: When reviewing TypeScript files containing PHP code snippets intended for file modifications (like in `SSOFileModification`), PHP functions that appear undefined should not be flagged as issues since they will be available in the target PHP environment where the code is injected.
Applied to files:
web/pages/login.vueapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts
📚 Learning: 2025-07-21T14:01:04.139Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/__test__/components/**/*.ts : Use `flushPromises()` for more complex promise chains in async tests
Applied to files:
web/pages/login.vue
📚 Learning: 2025-07-21T14:01:04.139Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/__test__/components/**/*.ts : Check for expected prop handling and event emissions in component tests
Applied to files:
web/pages/login.vue
📚 Learning: 2024-12-11T17:20:09.604Z
Learnt from: pujitm
PR: unraid/api#978
File: web/helpers/apollo-cache/index.ts:133-157
Timestamp: 2024-12-11T17:20:09.604Z
Learning: In `web/helpers/apollo-cache/index.ts`, when updating the cache in the `cache.updateQuery` function, it's sufficient to check if `data` is present, as type checking inside `data` is handled at the GraphQL layer.
Applied to files:
web/pages/login.vue
📚 Learning: 2024-12-09T15:45:46.492Z
Learnt from: pujitm
PR: unraid/api#975
File: web/components/Notifications/TabList.vue:1-4
Timestamp: 2024-12-09T15:45:46.492Z
Learning: In our Nuxt.js setup for the `web` project, it's not necessary to explicitly import `computed` from `vue` in Vue components, as it's globally available.
Applied to files:
web/pages/login.vue
📚 Learning: 2024-12-09T15:47:29.325Z
Learnt from: pujitm
PR: unraid/api#975
File: web/components/Notifications/TabList.vue:1-24
Timestamp: 2024-12-09T15:47:29.325Z
Learning: In our Nuxt setup using Vue.js, components defined within the codebase are autoloaded and do not require explicit import statements in the script section. For example, components like `TabsList` and `TabsTrigger` used in `web/components/Notifications/TabList.vue` are automatically available without imports.
Applied to files:
web/pages/login.vue
📚 Learning: 2024-12-06T17:34:16.133Z
Learnt from: pujitm
PR: unraid/api#974
File: web/components/Loading/Error.vue:50-50
Timestamp: 2024-12-06T17:34:16.133Z
Learning: In this project, the `Button` component from `~/components/shadcn/Button.vue` is autoloaded and does not need to be imported manually in components like `web/components/Loading/Error.vue`.
Applied to files:
web/pages/login.vue
📚 Learning: 2024-12-17T13:55:42.068Z
Learnt from: elibosley
PR: unraid/api#972
File: web/components/ColorSwitcher.ce.vue:1-2
Timestamp: 2024-12-17T13:55:42.068Z
Learning: In this Nuxt.js project, components used in templates are automatically imported by Nuxt, so explicit import statements for components are unnecessary.
Applied to files:
web/pages/login.vue
📚 Learning: 2025-07-28T15:34:24.715Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/**/store/**/*.{ts,js} : Ensure Vue reactivity imports are added to store files (computed, ref, watchEffect)
Applied to files:
web/pages/login.vue
📚 Learning: 2025-03-27T23:52:57.888Z
Learnt from: zackspear
PR: unraid/api#0
File: :0-0
Timestamp: 2025-03-27T23:52:57.888Z
Learning: In the unraid/api project, Vue components are compiled into web components. Using `setActivePinia(createPinia())` in store files ensures that all web components share a single Pinia store instance, which is the desired behavior. Without this initialization, each web component would have its own isolated store, breaking the intended architecture.
Applied to files:
web/pages/login.vue
📚 Learning: 2025-03-27T23:52:57.888Z
Learnt from: zackspear
PR: unraid/api#0
File: :0-0
Timestamp: 2025-03-27T23:52:57.888Z
Learning: In the unraid/api project, Vue components are compiled into web components. The `setActivePinia(createPinia())` call at the module level in store files is intentional and ensures all web components share a single Pinia store instance, which is the desired behavior. This shared state approach is critical for the application's architecture to function correctly.
Applied to files:
web/pages/login.vue
📚 Learning: 2024-12-06T17:38:40.999Z
Learnt from: elibosley
PR: unraid/api#974
File: web/components/Loading/Error.vue:1-3
Timestamp: 2024-12-06T17:38:40.999Z
Learning: In Nuxt.js projects, components are automatically imported, so explicit import statements for components like `LoadingSpinner` in `web/components/Loading/Error.vue` are not necessary.
Applied to files:
web/pages/login.vue
📚 Learning: 2024-11-01T17:52:55.311Z
Learnt from: pujitm
PR: unraid/api#941
File: web/components/Notifications/Item.vue:0-0
Timestamp: 2024-11-01T17:52:55.311Z
Learning: In the `web/components/Notifications/Item.vue` component, ensure that when delete or archive mutations fail, the notification remains displayed, and an error message is shown below the action buttons to inform the user.
Applied to files:
web/pages/login.vueweb/composables/gql/graphql.ts
📚 Learning: 2024-12-17T14:59:32.458Z
Learnt from: elibosley
PR: unraid/api#972
File: web/store/theme.ts:46-49
Timestamp: 2024-12-17T14:59:32.458Z
Learning: In the `web/store/theme.ts` file of the Unraid web application, the header is intentionally designed to have a light background with dark text in dark mode, and a dark background with light text in light mode.
Applied to files:
web/pages/login.vue
📚 Learning: 2025-03-07T17:35:50.406Z
Learnt from: mdatelle
PR: unraid/api#1219
File: api/src/unraid-api/main.ts:32-55
Timestamp: 2025-03-07T17:35:50.406Z
Learning: Helmet security configuration in the Unraid API is intentionally relaxed (with disabled CSP, CORS policies, and HSTS) to maintain compatibility with existing Unraid plugins. Stricter security settings might break current plugin functionality.
Applied to files:
api/docs/public/oidc-provider-examples.md
📚 Learning: 2025-04-28T20:35:32.980Z
Learnt from: elibosley
PR: unraid/api#1368
File: api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch:0-0
Timestamp: 2025-04-28T20:35:32.980Z
Learning: For Unraid GUI mode, automatic session creation with root privileges is an acceptable pattern since these pages are already protected and only visible to authenticated users.
Applied to files:
api/docs/public/oidc-provider-examples.md
📚 Learning: 2024-11-06T20:59:06.096Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:87-89
Timestamp: 2024-11-06T20:59:06.096Z
Learning: Error handling in mutations like `addRoleForUser` in `auth.resolver.ts` is performed at the service level, so additional error handling in the resolver is unnecessary.
Applied to files:
api/src/unraid-api/rest/rest.controller.ts
📚 Learning: 2025-01-23T21:26:17.766Z
Learnt from: elibosley
PR: unraid/api#1063
File: api/src/unraid-api/cli/sso/validate-token.command.ts:76-81
Timestamp: 2025-01-23T21:26:17.766Z
Learning: In the unraid/api codebase, store dispatches using Redux Toolkit's `unwrap()` method already handle errors appropriately and don't require additional try-catch blocks.
Applied to files:
api/src/unraid-api/rest/rest.controller.ts
📚 Learning: 2025-03-13T16:17:55.820Z
Learnt from: elibosley
PR: unraid/api#1211
File: api/src/unraid-api/graph/connect/connect-settings.service.ts:48-53
Timestamp: 2025-03-13T16:17:55.820Z
Learning: In NestJS services, dynamic imports should be wrapped in try/catch blocks with proper error logging and fallback handling to prevent unhandled rejections from crashing the application.
Applied to files:
api/src/unraid-api/rest/rest.controller.ts
📚 Learning: 2024-12-20T14:56:29.464Z
Learnt from: elibosley
PR: unraid/api#992
File: api/src/mothership/utils/create-local-connect-api-key.ts:23-29
Timestamp: 2024-12-20T14:56:29.464Z
Learning: The user prefers not to add error handling for the Redux dispatch in create-local-connect-api-key.ts.
Applied to files:
api/src/unraid-api/rest/rest.controller.ts
📚 Learning: 2025-01-31T14:42:06.752Z
Learnt from: pujitm
PR: unraid/api#1075
File: web/components/Notifications/Sidebar.vue:41-58
Timestamp: 2025-01-31T14:42:06.752Z
Learning: When handling errors in code that uses toast notifications, avoid using toast in the error handler itself since toast functionality might be the source of the error. Use console.error or other reliable logging mechanisms instead.
Applied to files:
api/src/unraid-api/rest/rest.controller.ts
📚 Learning: 2025-07-21T14:00:06.258Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/src/unraid-api/**/* : Prefer adding new files to the NestJS repository located at api/src/unraid-api/ instead of the legacy code
Applied to files:
api/src/unraid-api/rest/rest.controller.ts
📚 Learning: 2025-07-28T15:34:24.715Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to api/src/unraid-api/**/* : Prefer adding new files to the NestJS repo located at api/src/unraid-api/ instead of the legacy code
Applied to files:
api/src/unraid-api/rest/rest.controller.ts
📚 Learning: 2025-04-23T20:19:42.542Z
Learnt from: pujitm
PR: unraid/api#1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom ConfigService implementation that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts
📚 Learning: 2025-03-12T13:35:43.900Z
Learnt from: pujitm
PR: unraid/api#1211
File: web/codegen.ts:14-14
Timestamp: 2025-03-12T13:35:43.900Z
Learning: The JSON scalar type in web/codegen.ts was temporarily changed from 'string' to 'any' for compatibility with JsonForms integration. This change facilitates the implementation of the Connect settings web component.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.tsweb/composables/gql/graphql.ts
📚 Learning: 2024-11-05T14:48:56.949Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:41-47
Timestamp: 2024-11-05T14:48:56.949Z
Learning: Input types for GraphQL mutations can be pulled from the GraphQL codegen types, as they are generated into the API files. Therefore, explicit typing in the resolver files may be unnecessary.
Applied to files:
web/composables/gql/graphql.tsapi/generated-schema.graphql
📚 Learning: 2025-03-12T13:48:14.850Z
Learnt from: pujitm
PR: unraid/api#1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
Applied to files:
web/composables/gql/graphql.tsapi/generated-schema.graphql
📚 Learning: 2025-03-14T16:10:38.562Z
Learnt from: pujitm
PR: unraid/api#1211
File: api/src/graphql/schema/types/connect/connect.graphql:142-146
Timestamp: 2025-03-14T16:10:38.562Z
Learning: The updateApiSettings mutation in api/src/unraid-api/graph/connect/connect.resolver.ts is protected with the UsePermissions decorator that requires UPDATE permission on the CONFIG resource.
Applied to files:
web/composables/gql/graphql.tsapi/generated-schema.graphql
📚 Learning: 2025-07-21T14:00:25.140Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Applies to web/**/*.{query,mutation}.ts : All GraphQL under `web/` must follow this naming convention
Applied to files:
web/composables/gql/graphql.ts
📚 Learning: 2025-07-28T15:34:24.715Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/**/*.{query,mutation}.ts : All GraphQL under web/ must follow this naming convention (.query.ts for queries, .mutation.ts for mutations)
Applied to files:
web/composables/gql/graphql.ts
📚 Learning: 2025-07-21T14:00:25.140Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Applies to web/**/*.mutation.ts : GraphQL mutations must be placed in `.mutation.ts` files
Applied to files:
web/composables/gql/graphql.ts
📚 Learning: 2025-01-30T19:56:42.055Z
Learnt from: pujitm
PR: unraid/api#1075
File: web/components/Notifications/Sidebar.vue:52-57
Timestamp: 2025-01-30T19:56:42.055Z
Learning: Notification links in the Unraid API are validated at creation time on the server side, making additional client-side validation unnecessary when receiving notifications through GraphQL subscriptions.
Applied to files:
web/composables/gql/graphql.ts
📚 Learning: 2025-02-24T14:51:21.328Z
Learnt from: elibosley
PR: unraid/api#1181
File: web/store/theme.ts:0-0
Timestamp: 2025-02-24T14:51:21.328Z
Learning: In the Unraid API project's theme system, exact TypeScript type definitions are preferred over index signatures for theme variables to ensure better type safety.
Applied to files:
web/composables/gql/graphql.ts
📚 Learning: 2025-07-28T15:34:24.715Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Always run pnpm codegen for GraphQL code generation in the web directory
Applied to files:
api/.env.development
📚 Learning: 2025-07-21T14:00:25.140Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-graphql.mdc:0-0
Timestamp: 2025-07-21T14:00:25.140Z
Learning: Always run `pnpm codegen` for GraphQL code generation in the web directory
Applied to files:
api/.env.development
📚 Learning: 2025-06-20T20:50:14.050Z
Learnt from: elibosley
PR: unraid/api#1425
File: api/dev/configs/connect.json:5-13
Timestamp: 2025-06-20T20:50:14.050Z
Learning: Connect configuration files like api/dev/configs/connect.json use their own naming conventions (e.g., apikey, accesstoken, idtoken, refreshtoken) as implementation details, and these should not be changed to follow general camelCase patterns.
Applied to files:
api/.env.development
📚 Learning: 2025-07-28T15:34:24.715Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Always run scripts from api/package.json unless requested
Applied to files:
api/.env.development
📚 Learning: 2025-07-21T14:01:04.139Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/__test__/store/**/*.ts : Test getter dependencies are properly mocked in store tests
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
📚 Learning: 2025-07-21T14:01:04.139Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/__test__/store/**/*.ts : Verify state changes after actions in store tests
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
📚 Learning: 2025-07-21T14:01:04.139Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/__test__/store/**/*.ts : Verify state changes by updating the store in store tests
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
📚 Learning: 2025-07-21T14:01:04.139Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/__test__/store/**/*.ts : Mock external dependencies appropriately in store tests
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
📚 Learning: 2025-07-28T15:34:24.715Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to api/**/*.{test,spec}.{ts,js} : Test suite is VITEST, do not use jest in the API package
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
📚 Learning: 2025-07-21T14:01:04.139Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/__test__/store/**/*.ts : Test action side effects and state changes in store tests
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
📚 Learning: 2025-07-21T14:01:04.139Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/__test__/{components,store,mocks}/**/*.ts : Clear mocks between tests to ensure isolation
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
📚 Learning: 2025-07-21T14:01:04.139Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/__test__/store/**/*.ts : Test async operations completely in store tests
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
📚 Learning: 2025-07-21T14:01:04.139Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/__test__/{components,store,mocks}/**/*.ts : Mock external dependencies and services in tests
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
📚 Learning: 2025-07-28T15:34:24.715Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/__test__/**/*store*.{test,spec}.{ts,js} : Clear mocks between tests to ensure isolation in store tests
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
📚 Learning: 2025-08-09T01:03:29.676Z
Learnt from: elibosley
PR: unraid/api#1575
File: packages/unraid-shared/src/services/socket-config.service.spec.ts:10-13
Timestamp: 2025-08-09T01:03:29.676Z
Learning: Vitest is used for all testing across all repositories in the unraid organization, not Jest. Always use `vi` for mocking utilities, not `jest`.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
📚 Learning: 2025-07-28T15:34:24.715Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js} : Mock external dependencies and services in web/__test__
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
📚 Learning: 2025-07-21T14:01:04.139Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/__test__/{components,store,mocks}/**/*.ts : Remember that `vi.mock()` calls are hoisted
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
📚 Learning: 2025-07-21T14:01:04.139Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/__test__/{components,store,mocks}/**/*.ts : Reset mocks between tests with `vi.clearAllMocks()`
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
🧬 Code Graph Analysis (1)
web/composables/gql/graphql.ts (2)
api/src/unraid-api/cli/generated/graphql.ts (17)
Scalars(11-75)OidcProvider(1188-1220)MutationConfigureUpsArgs(1003-1005)UpsConfigInput(1861-1882)MutationDeleteOidcProviderArgs(1031-1033)MutationUpsertOidcProviderArgs(1087-1089)OidcProviderInput(1222-1253)Maybe(3-3)InputMaybe(4-4)PublicOidcProvider(1390-1398)UpsDevice(1916-1930)QueryOidcProviderArgs(1486-1488)QueryUpsDeviceByIdArgs(1491-1493)SsoSettings(1773-1778)Node(1097-1099)UpsBattery(1842-1850)UpsPower(1938-1946)web/components/sso/useSsoProviders.ts (1)
OidcProvider(5-12)
🪛 GitHub Actions: CI - Main (API)
web/pages/login.vue
[error] 64-64: TypeScript error TS2339: Property 'stdout' does not exist on the specified type.
🪛 dotenv-linter (3.3.0)
api/.env.development
[warning] 20-20: [UnorderedKey] The PATHS_OIDC_JSON key should go before the PATHS_PARITY_CHECKS key
🪛 Gitleaks (8.27.2)
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
91-91: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🔇 Additional comments (4)
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts (1)
70-76: Possible misuse of ConfigService default value
this.configService.get<string[]>('api.ssoSubIds', [])relies on a non-standard second-parameter default. Ensure the customConfigServicein this repo supports it, otherwise handleundefinedexplicitly:const ssoSubIds = this.configService.get<string[]>('api.ssoSubIds') ?? [];api/src/unraid-api/rest/rest.controller.ts (1)
121-128: 302 redirect implementation looks good.Manual Location header with 302 is appropriate for proxy compatibility here.
web/composables/gql/graphql.ts (2)
2441-2450: Public OIDC providers query looks correct.Only public-safe fields are requested; no secrets. Good.
2524-2525: Admin OIDC providers query selection is appropriate.No secret fields are requested; fields align with admin UI needs.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
1 similar comment
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
unraid-ui/src/forms/SteppedLayout.vue (1)
84-92: Gracefully handle elements that omitoptions.stepCurrently, any element without a
stepvalue is silently dropped. If the intention is “step 0 by default”, treat the absence explicitly:- const filtered = elements.filter((element) => { - return ( - typeof element.options === 'object' && - element.options !== null && - element.options.step === currentStep.value - ); - }); + const filtered = elements.filter((el) => { + const step = typeof el.options === 'object' && el.options + ? el.options.step ?? 0 + : 0; + return step === currentStep.value; + });This avoids accidental loss of untargeted elements and removes the cast when you switch to the helper type suggested above.
♻️ Duplicate comments (10)
api/src/unraid-api/graph/resolvers/settings/settings.model.ts (1)
51-53: Missing@Typedecorator breaks nested validation
@ValidateNested()alone won’t instantiateSsoSettings; add@Type(() => SsoSettings)(fromclass-transformer) to ensure proper validation & transformation.api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts (2)
127-129: Avoid unsafe cast from input to entityCasting
provider as OidcProviderbypasses type safety and may hide missing required fields – map explicitly or change the service signature.
151-156:validateOidcSessionlacks access controlThe endpoint is marked “internal use” but is publicly accessible. Add appropriate guards or an explicit
@Public()with justification.api/src/unraid-api/rest/rest.controller.ts (1)
64-76: ValidateproviderIdinput to mitigate injection / path-traversal risksReject IDs not matching
[a-zA-Z0-9._-]+before using them.api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts (2)
433-503: Authorization rule evaluation rejects arrays and treats them as objectsArray-valued claims (e.g., groups) should be supported. Also, the object-check returns early before the array-check. Handle arrays explicitly and evaluate against each element.
- // Log detailed claim analysis - if (typeof claimValue === 'object' && claimValue !== null) { - this.logger.warn( - `unexpected JWT claim value encountered - claim ${rule.claim} is object type: ${JSON.stringify(claimValue)}` - ); - return false; - } - - if (Array.isArray(claimValue)) { - this.logger.warn( - `unexpected JWT claim value encountered - claim ${rule.claim} is array type: ${JSON.stringify(claimValue)}` - ); - return false; - } - - const value = String(claimValue); - this.logger.debug(`Processing claim ${rule.claim} with string value: "${value}"`); + // Normalize to list of string values; support array claims + let values: string[]; + if (Array.isArray(claimValue)) { + values = (claimValue as unknown[]).map((v) => String(v)); + } else if (typeof claimValue === 'object' && claimValue !== null) { + this.logger.warn( + `unexpected JWT claim value encountered - claim ${rule.claim} is complex object: ${JSON.stringify(claimValue)}` + ); + return false; + } else { + values = [String(claimValue)]; + } + this.logger.debug(`Processing claim ${rule.claim} with values: ${JSON.stringify(values)}`); @@ - case AuthorizationOperator.EQUALS: - result = rule.value.some((v) => value === v); + case AuthorizationOperator.EQUALS: + result = rule.value.some((v) => values.includes(v)); @@ - case AuthorizationOperator.CONTAINS: - result = rule.value.some((v) => value.includes(v)); + case AuthorizationOperator.CONTAINS: + result = rule.value.some((v) => values.some((val) => val.includes(v))); @@ - case AuthorizationOperator.STARTS_WITH: - result = rule.value.some((v) => value.startsWith(v)); + case AuthorizationOperator.STARTS_WITH: + result = rule.value.some((v) => values.some((val) => val.startsWith(v))); @@ - case AuthorizationOperator.ENDS_WITH: - result = rule.value.some((v) => value.endsWith(v)); + case AuthorizationOperator.ENDS_WITH: + result = rule.value.some((v) => values.some((val) => val.endsWith(v)));
518-528: Refactor redirect URI configuration (avoid hardcoded URLs)Make redirect URIs fully configurable and environment-aware to avoid brittle localhost checks.
- private getRedirectUri(requestHost?: string): string { - // Always use the proxied path through /graphql to match production - if (requestHost && requestHost.includes('localhost')) { - // In development, use the Nuxt proxy at port 3000 - return `http://localhost:3000/graphql/api/auth/oidc/callback`; - } - - // In production, use the configured base URL with /graphql prefix - const baseUrl = this.configService.get('BASE_URL', 'http://tower.local'); - return `${baseUrl}/graphql/api/auth/oidc/callback`; - } + private getRedirectUri(requestHost?: string): string { + const isDevelopment = this.configService.get('NODE_ENV') === 'development'; + if (isDevelopment && requestHost) { + const protocol = requestHost.includes('localhost') ? 'http' : 'https'; + return `${protocol}://${requestHost}/graphql/api/auth/oidc/callback`; + } + const callbackUrl = this.configService.get('OIDC_CALLBACK_URL'); + if (callbackUrl) return callbackUrl; + const baseUrl = this.configService.get('BASE_URL', 'http://tower.local'); + return `${baseUrl}/graphql/api/auth/oidc/callback`; + }api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts (1)
110-120: cleanProvider misses optional string fields (jwksUri, buttonText, buttonVariant)Extend cleaning to all optional string fields for consistency.
return { ...provider, clientSecret: provider.clientSecret?.trim() || undefined, authorizationEndpoint: provider.authorizationEndpoint?.trim() || undefined, tokenEndpoint: provider.tokenEndpoint?.trim() || undefined, + jwksUri: provider.jwksUri?.trim() || undefined, + buttonText: provider.buttonText?.trim() || undefined, buttonIcon: provider.buttonIcon?.trim() || undefined, + buttonVariant: provider.buttonVariant?.trim() || undefined, buttonStyle: provider.buttonStyle?.trim() || undefined, };api/generated-schema.graphql (3)
1886-1945: Make id optional in OidcProviderInput for create vs updateFor upsert ergonomics, let the server generate ids on create; require on update.
Apply in source schema:
- id: String! + id: StringDocument behavior: absent → create, present → update.
1466-1525: Do not expose clientSecret in public GraphQL type; implement NodeLeaking clientSecret is a high-severity risk. Remove it from OidcProvider output and align with the API by implementing Node for consistency.
Note: apply these changes to the source schema/resolvers so this file regenerates accordingly.
-type OidcProvider { +type OidcProvider implements Node { """The unique identifier for the OIDC provider""" id: PrefixedID! @@ - """OAuth2 client secret (if required by provider)""" - clientSecret: String
1791-1801: Add authorization directives to admin/internal queries and fix id typeProtect sensitive queries with @usePermissions and use PrefixedID for id consistency if OidcProvider implements Node.
Apply in source schema:
- publicOidcProviders: [PublicOidcProvider!]! - oidcProviders: [OidcProvider!]! - oidcProvider(id: String!): OidcProvider - validateOidcSession(token: String!): OidcSessionValidation! + publicOidcProviders: [PublicOidcProvider!]! + oidcProviders: [OidcProvider!]! @usePermissions(action: READ, resource: "SSO", possession: ANY) + oidcProvider(id: PrefixedID!): OidcProvider @usePermissions(action: READ, resource: "SSO", possession: ANY) + validateOidcSession(token: String!): OidcSessionValidation! @usePermissions(action: READ, resource: "SSO", possession: OWN)
🧹 Nitpick comments (7)
unraid-ui/src/forms/SteppedLayout.vue (1)
21-27: Avoid narrowingoptionson a subtype – keep the base typing and add a helper type instead
UISchemaElementalready exposes anoptions?: anyfield. By redefining it here with a narrower shape you:
• create an unnecessary new interface,
• risk incompatibilities whenoptionsis not an object.A lighter-weight, type-safe pattern:
type UISchemaWithStep = UISchemaElement & { options?: { step?: number } & Record<string, unknown>; };…and use
UISchemaWithSteplocally without extending the base.
This keeps the original contract intact while giving you the extra IntelliSense you want.unraid-ui/src/forms/ObjectArrayField.vue (3)
126-149: Shadowed variableschemainsideforEachhampers readabilityUsing the same identifier for the outer
schemaand the inner loop variable is confusing:- Object.entries(properties).forEach(([key, propSchema]) => { - const schema = propSchema as JsonSchema; - if (schema.default !== undefined) { + Object.entries(properties).forEach(([key, propSchema]) => { + const prop = propSchema as JsonSchema; + if (prop.default !== undefined) {Renaming prevents accidental misuse and improves clarity.
170-180: Index-based keys onTabsTriggermay cause state leaks on reorder
indexworks until items are removed from the middle, after which Vue reuses DOM nodes and bound state. Prefer a stable identifier:- <TabsTrigger - v-for="(item, index) in items" - :key="index" + <TabsTrigger + v-for="(item, index) in items" + :key="item.id ?? index" <!-- requires each item to expose an `id` -->If your schema doesn’t define a natural key, generate one (
crypto.randomUUID()) when adding items.
241-245: Minor interpolation issue – call.toLowerCase()on the unwrapped value
itemTypeNameis a computed ref. In templates it auto-unwraps, but chaining a method is brittle in strict templates. Use a helper:<p class="text-muted-foreground mb-4"> No {{ itemTypeNameLower }}s configured </p>with
const itemTypeNameLower = computed(() => itemTypeName.value.toLowerCase());Safer and avoids surprises when switching to script-only render functions.
api/src/unraid-api/graph/resolvers/sso/oidc-validation.service.ts (1)
12-12: Unused injectedConfigService
configServiceis injected but never referenced. Drop the dependency or use it (e.g., to read whitelist/timeout configs) to avoid dead code.api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts (2)
77-87: Unused allowInsecureRequests handling when building authorization URLYou compute clientOptions for HTTP issuers but don’t use it (and it isn’t needed for building an auth URL). Either remove this block or add a clarifying comment.
- // For HTTP endpoints, we need to pass the allowInsecureRequests option - const serverUrl = new URL(provider.issuer || ''); - let clientOptions: any = undefined; - if (serverUrl.protocol === 'http:') { - this.logger.debug( - `Building authorization URL with allowInsecureRequests for ${provider.id}` - ); - clientOptions = { - execute: [client.allowInsecureRequests], - }; - } - - const authUrl = client.buildAuthorizationUrl(config, parameters); + // Build the authorization URL; allowInsecureRequests is only needed for network requests, + // not for URL construction. + const authUrl = client.buildAuthorizationUrl(config, parameters);Also applies to: 68-76
78-81: Avoid new URL(provider.issuer || '') — rely on discovered issuerGuard against empty issuer and keep consistency by using the discovered issuer.
- const serverUrl = new URL(provider.issuer || ''); + const serverUrl = new URL(config.serverMetadata().issuer);Also applies to: 186-189
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
api/generated-schema.graphql(6 hunks)api/package.json(2 hunks)api/src/unraid-api/graph/resolvers/settings/settings.model.ts(3 hunks)api/src/unraid-api/graph/resolvers/settings/settings.resolver.ts(6 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-validation.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/sso.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts(1 hunks)api/src/unraid-api/graph/utils/form-utils.ts(1 hunks)api/src/unraid-api/rest/rest.controller.ts(2 hunks)packages/unraid-shared/src/services/user-settings.ts(3 hunks)unraid-ui/src/components/common/index.ts(1 hunks)unraid-ui/src/components/form/index.ts(1 hunks)unraid-ui/src/forms/HorizontalLayout.vue(2 hunks)unraid-ui/src/forms/ObjectArrayField.vue(1 hunks)unraid-ui/src/forms/SteppedLayout.vue(2 hunks)unraid-ui/src/forms/UnraidSettingsLayout.vue(2 hunks)unraid-ui/src/forms/VerticalLayout.vue(2 hunks)
✅ Files skipped from review due to trivial changes (5)
- unraid-ui/src/forms/UnraidSettingsLayout.vue
- unraid-ui/src/forms/VerticalLayout.vue
- unraid-ui/src/components/form/index.ts
- unraid-ui/src/forms/HorizontalLayout.vue
- unraid-ui/src/components/common/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- api/package.json
- api/src/unraid-api/graph/resolvers/sso/sso.module.ts
- api/src/unraid-api/graph/utils/form-utils.ts
- api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts
- api/src/unraid-api/graph/resolvers/settings/settings.resolver.ts
🧰 Additional context used
📓 Path-based instructions (2)
api/**/*
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*: Use pnpm as the only package manager
Always run scripts from api/package.json unless specifically requested otherwise
Files:
api/src/unraid-api/graph/resolvers/settings/settings.model.tsapi/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsapi/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/sso.resolver.tsapi/src/unraid-api/graph/resolvers/sso/oidc-validation.service.tsapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.tsapi/generated-schema.graphql
api/src/unraid-api/**/*
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the NestJS repository located at api/src/unraid-api/ instead of the legacy code
Prefer adding new files to the NestJS repo located at api/src/unraid-api/ instead of the legacy code
Files:
api/src/unraid-api/graph/resolvers/settings/settings.model.tsapi/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsapi/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/sso.resolver.tsapi/src/unraid-api/graph/resolvers/sso/oidc-validation.service.tsapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts
🧠 Learnings (49)
📓 Common learnings
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:111-113
Timestamp: 2024-11-06T20:59:25.809Z
Learning: In the Unraid API project, error handling for mutations is handled at the service level rather than in the GraphQL resolvers. Specifically, in `api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts`, methods like `removeRoleFromApiKey` rely on service-level error handling.
📚 Learning: 2025-03-14T16:10:38.562Z
Learnt from: pujitm
PR: unraid/api#1211
File: api/src/graphql/schema/types/connect/connect.graphql:142-146
Timestamp: 2025-03-14T16:10:38.562Z
Learning: The updateApiSettings mutation in api/src/unraid-api/graph/connect/connect.resolver.ts is protected with the UsePermissions decorator that requires UPDATE permission on the CONFIG resource.
Applied to files:
api/src/unraid-api/graph/resolvers/settings/settings.model.tspackages/unraid-shared/src/services/user-settings.tsapi/src/unraid-api/graph/resolvers/sso/sso.resolver.tsapi/generated-schema.graphql
📚 Learning: 2025-01-27T14:31:42.305Z
Learnt from: elibosley
PR: unraid/api#1063
File: web/components/SsoButton.ce.vue:5-8
Timestamp: 2025-01-27T14:31:42.305Z
Learning: In the Unraid API web components, SSO-related props are intentionally provided in both camelCase (`ssoEnabled`) and lowercase (`ssoenabled`) variants to support interchangeable usage across different contexts (e.g., HTML attributes vs Vue props).
Applied to files:
api/src/unraid-api/graph/resolvers/settings/settings.model.tsapi/src/unraid-api/graph/resolvers/sso/sso.resolver.tsapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts
📚 Learning: 2025-03-12T13:35:43.900Z
Learnt from: pujitm
PR: unraid/api#1211
File: web/codegen.ts:14-14
Timestamp: 2025-03-12T13:35:43.900Z
Learning: The JSON scalar type in web/codegen.ts was temporarily changed from 'string' to 'any' for compatibility with JsonForms integration. This change facilitates the implementation of the Connect settings web component.
Applied to files:
api/src/unraid-api/graph/resolvers/settings/settings.model.tsunraid-ui/src/forms/SteppedLayout.vueapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts
📚 Learning: 2024-11-06T20:59:25.809Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:111-113
Timestamp: 2024-11-06T20:59:25.809Z
Learning: In the Unraid API project, error handling for mutations is handled at the service level rather than in the GraphQL resolvers. Specifically, in `api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts`, methods like `removeRoleFromApiKey` rely on service-level error handling.
Applied to files:
api/src/unraid-api/graph/resolvers/settings/settings.model.tsapi/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsapi/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/sso.resolver.tsapi/src/unraid-api/graph/resolvers/sso/oidc-validation.service.tsapi/generated-schema.graphql
📚 Learning: 2025-03-12T13:48:14.850Z
Learnt from: pujitm
PR: unraid/api#1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
Applied to files:
api/src/unraid-api/graph/resolvers/settings/settings.model.tsapi/src/unraid-api/graph/resolvers/sso/sso.resolver.tsapi/generated-schema.graphql
📚 Learning: 2025-03-07T19:29:12.540Z
Learnt from: pujitm
PR: unraid/api#1219
File: api/src/unraid-api/main.ts:18-25
Timestamp: 2025-03-07T19:29:12.540Z
Learning: When handling type mismatches between dependencies in TypeScript (such as between NestJS and Fastify), prefer using `ts-expect-error` with a detailed explanation comment instead of `as any` type assertions. This better documents the reason for the type bypass and makes the issue more visible for future maintenance.
Applied to files:
api/src/unraid-api/graph/resolvers/settings/settings.model.tsapi/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/sso.resolver.ts
📚 Learning: 2025-04-21T18:44:15.414Z
Learnt from: elibosley
PR: unraid/api#1352
File: packages/unraid-api-plugin-connect/src/config.entity.ts:16-26
Timestamp: 2025-04-21T18:44:15.414Z
Learning: For the Unraid API project, class-validator should be used for validation to avoid mismatches between different validation schemas (like Zod).
Applied to files:
api/src/unraid-api/graph/resolvers/settings/settings.model.tsapi/src/unraid-api/graph/resolvers/sso/oidc-validation.service.ts
📚 Learning: 2025-04-21T18:44:24.383Z
Learnt from: elibosley
PR: unraid/api#1352
File: packages/unraid-api-plugin-connect/src/config.entity.ts:37-39
Timestamp: 2025-04-21T18:44:24.383Z
Learning: The project prefers using class-validator decorators directly on entity classes rather than separate Zod schemas to maintain type consistency between GraphQL entities and validation rules.
Applied to files:
api/src/unraid-api/graph/resolvers/settings/settings.model.ts
📚 Learning: 2025-01-28T16:39:50.278Z
Learnt from: elibosley
PR: unraid/api#1078
File: api/src/unraid-api/unraid-file-modifier/modifications/sso.modification.ts:35-36
Timestamp: 2025-01-28T16:39:50.278Z
Learning: When reviewing TypeScript files containing PHP code snippets intended for file modifications (like in `SSOFileModification`), PHP functions that appear undefined should not be flagged as issues since they will be available in the target PHP environment where the code is injected.
Applied to files:
api/src/unraid-api/graph/resolvers/settings/settings.model.ts
📚 Learning: 2025-07-21T14:01:04.139Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/__test__/{components,store,mocks}/**/*.ts : Don't mix mock declarations and module mocks incorrectly
Applied to files:
api/src/unraid-api/graph/resolvers/settings/settings.model.ts
📚 Learning: 2024-11-05T14:48:56.949Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:41-47
Timestamp: 2024-11-05T14:48:56.949Z
Learning: Input types for GraphQL mutations can be pulled from the GraphQL codegen types, as they are generated into the API files. Therefore, explicit typing in the resolver files may be unnecessary.
Applied to files:
api/src/unraid-api/graph/resolvers/settings/settings.model.tsapi/src/unraid-api/graph/resolvers/sso/sso.resolver.tsapi/generated-schema.graphql
📚 Learning: 2025-01-15T21:34:00.006Z
Learnt from: pujitm
PR: unraid/api#1047
File: api/src/unraid-api/graph/sandbox-plugin.ts:57-57
Timestamp: 2025-01-15T21:34:00.006Z
Learning: In the GraphQL sandbox (api/src/unraid-api/graph/sandbox-plugin.ts), CSRF token validation should fail silently with a fallback value to maintain sandbox accessibility, as it's a development tool where strict security measures aren't required.
Applied to files:
api/src/unraid-api/graph/resolvers/settings/settings.model.tsapi/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/sso.resolver.tsapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.tsapi/generated-schema.graphql
📚 Learning: 2025-02-24T14:51:21.328Z
Learnt from: elibosley
PR: unraid/api#1181
File: web/store/theme.ts:0-0
Timestamp: 2025-02-24T14:51:21.328Z
Learning: In the Unraid API project's theme system, exact TypeScript type definitions are preferred over index signatures for theme variables to ensure better type safety.
Applied to files:
unraid-ui/src/forms/SteppedLayout.vue
📚 Learning: 2025-02-04T17:21:39.710Z
Learnt from: mdatelle
PR: unraid/api#1106
File: unraid-ui/src/components/index.ts:2-2
Timestamp: 2025-02-04T17:21:39.710Z
Learning: The unraid-ui package is undergoing a major refactoring process, and breaking changes are expected during this transition period.
Applied to files:
unraid-ui/src/forms/SteppedLayout.vueunraid-ui/src/forms/ObjectArrayField.vuepackages/unraid-shared/src/services/user-settings.ts
📚 Learning: 2025-03-14T16:56:20.520Z
Learnt from: pujitm
PR: unraid/api#1211
File: api/src/unraid-api/graph/connect/connect-settings.service.ts:199-199
Timestamp: 2025-03-14T16:56:20.520Z
Learning: The JSONForms library has narrow type definitions for interfaces like SchemaBasedCondition that make augmentation difficult. Type assertions (using 'as') are sometimes necessary despite not being ideal from a typing perspective.
Applied to files:
unraid-ui/src/forms/SteppedLayout.vue
📚 Learning: 2025-04-02T21:21:29.168Z
Learnt from: elibosley
PR: unraid/api#1308
File: unraid-ui/src/components/common/loading/Error.vue:2-2
Timestamp: 2025-04-02T21:21:29.168Z
Learning: Components in the unraid-ui folder require explicit imports and are not autoloaded, unlike other parts of the project that may use Nuxt.js autoloading features.
Applied to files:
unraid-ui/src/forms/ObjectArrayField.vue
📚 Learning: 2024-12-09T15:47:29.325Z
Learnt from: pujitm
PR: unraid/api#975
File: web/components/Notifications/TabList.vue:1-24
Timestamp: 2024-12-09T15:47:29.325Z
Learning: In our Nuxt setup using Vue.js, components defined within the codebase are autoloaded and do not require explicit import statements in the script section. For example, components like `TabsList` and `TabsTrigger` used in `web/components/Notifications/TabList.vue` are automatically available without imports.
Applied to files:
unraid-ui/src/forms/ObjectArrayField.vue
📚 Learning: 2025-03-14T19:14:10.408Z
Learnt from: pujitm
PR: unraid/api#1211
File: plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/Connect.page:0-0
Timestamp: 2025-03-14T19:14:10.408Z
Learning: *.ce.vue components in `web/components` get built as custom elements and prefixed with `unraid-`, making them available as HTML custom elements at runtime.
Applied to files:
unraid-ui/src/forms/ObjectArrayField.vue
📚 Learning: 2025-07-28T15:34:24.715Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to web/**/store/**/*.{ts,js} : Ensure Vue reactivity imports are added to store files (computed, ref, watchEffect)
Applied to files:
unraid-ui/src/forms/ObjectArrayField.vue
📚 Learning: 2025-03-13T16:17:21.897Z
Learnt from: elibosley
PR: unraid/api#1211
File: unraid-ui/src/components/form/number/NumberFieldInput.vue:1-21
Timestamp: 2025-03-13T16:17:21.897Z
Learning: The unraid-ui is a UI library being published externally, where wrapping third-party components (like those from reka-ui) is an intentional design choice to maintain a consistent interface, standardize styling, and control the exposed API.
Applied to files:
unraid-ui/src/forms/ObjectArrayField.vue
📚 Learning: 2025-03-27T23:52:57.888Z
Learnt from: zackspear
PR: unraid/api#0
File: :0-0
Timestamp: 2025-03-27T23:52:57.888Z
Learning: In the unraid/api project, Vue components are compiled into web components. The `setActivePinia(createPinia())` call at the module level in store files is intentional and ensures all web components share a single Pinia store instance, which is the desired behavior. This shared state approach is critical for the application's architecture to function correctly.
Applied to files:
unraid-ui/src/forms/ObjectArrayField.vue
📚 Learning: 2025-03-27T23:33:13.215Z
Learnt from: zackspear
PR: unraid/api#0
File: :0-0
Timestamp: 2025-03-27T23:33:13.215Z
Learning: In the unraid/api project, Vue components are compiled into web components. Using `setActivePinia(createPinia())` in store files would break the build by causing all web components to share a singular Pinia store instance. Each web component needs its own Pinia store instance to maintain proper isolation and encapsulation.
Applied to files:
unraid-ui/src/forms/ObjectArrayField.vue
📚 Learning: 2025-02-18T18:38:24.981Z
Learnt from: zackspear
PR: unraid/api#1143
File: web/pages/webComponents.vue:28-29
Timestamp: 2025-02-18T18:38:24.981Z
Learning: In Vue components, when there's a repeating pattern of heading + content + divider, prefer using a wrapper component that takes the heading as a prop and content via slots, rather than duplicating the structure. This improves maintainability and reduces code duplication.
Applied to files:
unraid-ui/src/forms/ObjectArrayField.vue
📚 Learning: 2025-01-30T19:38:02.478Z
Learnt from: pujitm
PR: unraid/api#1075
File: unraid-ui/src/register.ts:15-34
Timestamp: 2025-01-30T19:38:02.478Z
Learning: In the web components registration process for unraid-ui, use a soft-fail approach (logging warnings/errors) instead of throwing errors, to ensure other components can still register successfully even if one component fails.
Applied to files:
packages/unraid-shared/src/services/user-settings.ts
📚 Learning: 2025-07-21T14:01:04.139Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-07-21T14:01:04.139Z
Learning: Applies to web/__test__/components/**/*.ts : Set input values with `await input.setValue('value')`
Applied to files:
packages/unraid-shared/src/services/user-settings.ts
📚 Learning: 2024-11-05T14:49:07.308Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In `api/src/unraid-api/auth/api-key.service.ts`, when handling read errors in the `findById` method, throw a `GraphQLError` instead of an `InternalServerErrorException`.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsapi/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/sso.resolver.tsapi/src/unraid-api/graph/resolvers/sso/oidc-validation.service.ts
📚 Learning: 2024-11-04T22:00:41.946Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.service.ts:42-105
Timestamp: 2024-11-04T22:00:41.946Z
Learning: In the 'AuthService' within `api/src/unraid-api/auth/auth.service.ts`, rate limiting is not required because the API is not public facing.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsapi/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/sso.resolver.ts
📚 Learning: 2024-11-15T16:22:12.460Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.module.ts:94-96
Timestamp: 2024-11-15T16:22:12.460Z
Learning: In `CookieService.defaultOpts()` in `api/src/unraid-api/auth/cookie.service.ts`, use `sameSite: 'lax'` for session cookie configuration.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsapi/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/sso.resolver.ts
📚 Learning: 2025-03-04T14:55:00.903Z
Learnt from: elibosley
PR: unraid/api#1151
File: plugin/builder/utils/consts.ts:6-6
Timestamp: 2025-03-04T14:55:00.903Z
Learning: The startingDir constant in plugin/builder/utils/consts.ts is defined using process.cwd(), which can cause issues if directory changes occur after importing this constant. Using __dirname.split('/builder')[0] would make it more reliable by making it relative to the file location rather than the current working directory.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts
📚 Learning: 2025-07-24T18:48:44.035Z
Learnt from: mdatelle
PR: unraid/api#1533
File: web/components/Docker/Edit.vue:16-32
Timestamp: 2025-07-24T18:48:44.035Z
Learning: In web/components/Docker/Edit.vue, the hardcoded configuration values in the config ref are intentional temporary/dummy data used during initial UI development phase while building out the real Docker components, as clarified by mdatelle in PR #1533.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts
📚 Learning: 2025-05-08T19:28:37.034Z
Learnt from: elibosley
PR: unraid/api#1381
File: plugin/builder/cli/common-environment.ts:18-22
Timestamp: 2025-05-08T19:28:37.034Z
Learning: In plugin/builder/cli/common-environment.ts, the getDefaultBaseUrl() function intentionally returns an invalid URL when running in CI environments to force explicit specification of the base URL parameter rather than relying on defaults.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts
📚 Learning: 2025-01-27T14:57:46.617Z
Learnt from: elibosley
PR: unraid/api#1063
File: web/_data/serverState.ts:137-147
Timestamp: 2025-01-27T14:57:46.617Z
Learning: The values in `web/_data/serverState.ts` are used for testing purposes and should remain as hardcoded mock data to facilitate testing different scenarios.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts
📚 Learning: 2025-03-10T18:43:13.016Z
Learnt from: mdatelle
PR: unraid/api#1219
File: api/src/unraid-api/types/fastify.ts:25-30
Timestamp: 2025-03-10T18:43:13.016Z
Learning: In the Unraid API, cookies should be marked as optional (with `?`) in the FastifyRequest type definition since the cookies may not always be present, which is consistent with the authentication flow throughout the codebase.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsapi/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/sso.resolver.tsapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.tsapi/generated-schema.graphql
📚 Learning: 2024-11-18T20:52:47.800Z
Learnt from: elibosley
PR: unraid/api#963
File: web/helpers/markdown.ts:1-13
Timestamp: 2024-11-18T20:52:47.800Z
Learning: All configurations and direct usage of `marked` should be centralized in the `safeParseMarkdown` function in `web/helpers/markdown.ts` to maintain security and consistency.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts
📚 Learning: 2024-11-06T20:59:06.096Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:87-89
Timestamp: 2024-11-06T20:59:06.096Z
Learning: Error handling in mutations like `addRoleForUser` in `auth.resolver.ts` is performed at the service level, so additional error handling in the resolver is unnecessary.
Applied to files:
api/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/sso.resolver.ts
📚 Learning: 2025-01-23T21:26:17.766Z
Learnt from: elibosley
PR: unraid/api#1063
File: api/src/unraid-api/cli/sso/validate-token.command.ts:76-81
Timestamp: 2025-01-23T21:26:17.766Z
Learning: In the unraid/api codebase, store dispatches using Redux Toolkit's `unwrap()` method already handle errors appropriately and don't require additional try-catch blocks.
Applied to files:
api/src/unraid-api/rest/rest.controller.ts
📚 Learning: 2025-05-23T20:11:08.857Z
Learnt from: mdatelle
PR: unraid/api#1362
File: api/src/unraid-api/graph/resolvers/rclone/rclone-api.service.ts:421-424
Timestamp: 2025-05-23T20:11:08.857Z
Learning: When logging parameters or request data in error messages, always sanitize sensitive information by checking for common sensitive parameter names (pass, secret, token, key) and replacing their values with '***REDACTED***' or similar placeholders to prevent credential exposure in logs.
Applied to files:
api/src/unraid-api/rest/rest.controller.ts
📚 Learning: 2025-07-21T14:00:06.258Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-07-21T14:00:06.258Z
Learning: Applies to api/src/unraid-api/**/* : Prefer adding new files to the NestJS repository located at api/src/unraid-api/ instead of the legacy code
Applied to files:
api/src/unraid-api/rest/rest.controller.ts
📚 Learning: 2025-07-28T15:34:24.715Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T15:34:24.715Z
Learning: Applies to api/src/unraid-api/**/* : Prefer adding new files to the NestJS repo located at api/src/unraid-api/ instead of the legacy code
Applied to files:
api/src/unraid-api/rest/rest.controller.ts
📚 Learning: 2024-12-11T17:20:09.604Z
Learnt from: pujitm
PR: unraid/api#978
File: web/helpers/apollo-cache/index.ts:133-157
Timestamp: 2024-12-11T17:20:09.604Z
Learning: In `web/helpers/apollo-cache/index.ts`, when updating the cache in the `cache.updateQuery` function, it's sufficient to check if `data` is present, as type checking inside `data` is handled at the GraphQL layer.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts
📚 Learning: 2024-11-15T16:22:03.485Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/api-key.service.ts:176-188
Timestamp: 2024-11-15T16:22:03.485Z
Learning: Atomic writes are not required for the `saveApiKey` method in `ApiKeyService` (`api/src/unraid-api/auth/api-key.service.ts`) unless specifically needed.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts
📚 Learning: 2024-11-04T20:44:46.432Z
Learnt from: mdatelle
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.service.ts:0-0
Timestamp: 2024-11-04T20:44:46.432Z
Learning: When modifying `apiKey.roles` in `removeRoleFromApiKey` and `addRoleToApiKey` within `api/src/unraid-api/auth/auth.service.ts`, concurrency issues are not a concern because the keys are stored in the file system.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts
📚 Learning: 2025-03-10T17:24:06.914Z
Learnt from: mdatelle
PR: unraid/api#1219
File: api/src/unraid-api/auth/cookie.strategy.ts:19-20
Timestamp: 2025-03-10T17:24:06.914Z
Learning: In the auth system, CSRF token validation and cookie validation have been unified in the `validateCookiesCasbin()` method in the AuthService class, which takes the entire FastifyRequest object and performs both validations sequentially.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts
📚 Learning: 2025-04-23T20:19:42.542Z
Learnt from: pujitm
PR: unraid/api#1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts
📚 Learning: 2025-04-23T20:19:42.542Z
Learnt from: pujitm
PR: unraid/api#1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom ConfigService implementation that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts
📚 Learning: 2025-03-07T17:35:50.406Z
Learnt from: mdatelle
PR: unraid/api#1219
File: api/src/unraid-api/main.ts:32-55
Timestamp: 2025-03-07T17:35:50.406Z
Learning: Helmet security configuration in the Unraid API is intentionally relaxed (with disabled CSP, CORS policies, and HSTS) to maintain compatibility with existing Unraid plugins. Stricter security settings might break current plugin functionality.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts
📚 Learning: 2025-04-23T20:22:36.001Z
Learnt from: pujitm
PR: unraid/api#1367
File: api/src/unraid-api/app/pubsub.module.ts:24-28
Timestamp: 2025-04-23T20:22:36.001Z
Learning: In the Unraid API project, string literals are intentionally used for dependency injection tokens instead of Symbols to enable "collision" behavior across modules that don't directly import from each other. This is a temporary approach during refactoring until a shared library for constants is implemented.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts
📚 Learning: 2025-03-07T17:36:52.790Z
Learnt from: mdatelle
PR: unraid/api#1219
File: api/src/unraid-api/main.ts:57-63
Timestamp: 2025-03-07T17:36:52.790Z
Learning: The CORS configuration in the unraid API is intentionally set to allow all origins (`origin: true`). This was a deliberate architectural decision when removing the previous custom CORS implementation and implementing helmet security headers instead.
Applied to files:
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts
🧬 Code Graph Analysis (1)
api/src/unraid-api/graph/resolvers/settings/settings.model.ts (1)
web/composables/gql/graphql.ts (1)
SsoSettings(1673-1678)
🪛 GitHub Check: CodeQL
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts
[failure] 292-292: Incomplete URL substring sanitization
'unraid.net' can be anywhere in the URL, and arbitrary hosts may come before or after it.
🔇 Additional comments (4)
api/src/unraid-api/graph/resolvers/sso/oidc-validation.service.ts (1)
25-30: Deprecation / incompatibility ofallowInsecureRequests
allowInsecureRequestswas removed in openid-client ≥5. Instead set
custom.setHttpOptionsDefaults({ agent: new http.Agent({ rejectUnauthorized:false }) })or run discovery over http withIssuer.discover("http://…", { … }). Verify against the library version used.packages/unraid-shared/src/services/user-settings.ts (1)
151-159: Return shape now always includesrestartRequired– verify callers
updateNamespacedValuespreviously forwarded fragment responses verbatim; it now forcesrestartRequiredto always be present. Ensure all callers expect a non-optional boolean or keep it optional for backward compatibility.api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts (1)
52-64: Custom auth params path only sets callbackUrl + stateVerify whether the custom endpoint requires client_id/scope/response_type; if so, include them to avoid provider errors.
Would you like me to adjust this branch to optionally include standard params when customAuthParams is true?
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts (1)
36-45: PATHS_OIDC_JSON handling: ignoring directory segmentfileName() strips directories from PATHS_OIDC_JSON and returns only the filename. Confirm this is intentional; otherwise the override may not point to the requested path.
If the intent is to allow full path override, consider returning envPath as-is and updating ConfigFilePersister to honor absolute paths.
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
1 similar comment
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
api/generated-schema.graphql (4)
1466-1469: Remove clientSecret from API output and implement Node on OidcProviderExposing clientSecret risks accidental disclosure. Also, other resources implement Node; keep schema consistent.
-type OidcProvider { +type OidcProvider implements Node { """The unique identifier for the OIDC provider""" id: PrefixedID! @@ - """OAuth2 client secret (if required by provider)""" - clientSecret: String + # clientSecret intentionally omitted from API outputAlso applies to: 1476-1478
1945-1956: Add authorization directives and fix ID type for provider lookupProtect admin/owner-scoped data and keep ID type consistent across API.
"""Get public OIDC provider information for login buttons""" publicOidcProviders: [PublicOidcProvider!]! """Get all configured OIDC providers (admin only)""" - oidcProviders: [OidcProvider!]! + oidcProviders: [OidcProvider!]! @usePermissions(action: READ, resource: "SSO", possession: ANY) """Get a specific OIDC provider by ID""" - oidcProvider(id: String!): OidcProvider + oidcProvider(id: PrefixedID!): OidcProvider @usePermissions(action: READ, resource: "SSO", possession: ANY) """Validate an OIDC session token (internal use for CLI validation)""" - validateOidcSession(token: String!): OidcSessionValidation! + validateOidcSession(token: String!): OidcSessionValidation! @usePermissions(action: READ, resource: "SSO", possession: OWN)
2002-2006: Protect mutations with permissions and use PrefixedID for deleteEnforce admin-only changes and maintain ID consistency.
- upsertOidcProvider(provider: OidcProviderInput!): OidcProvider! - deleteOidcProvider(id: String!): Boolean! + upsertOidcProvider(provider: OidcProviderInput!): OidcProvider! @usePermissions(action: UPDATE, resource: "SSO", possession: ANY) + deleteOidcProvider(id: PrefixedID!): Boolean! @usePermissions(action: DELETE, resource: "SSO", possession: ANY)
2049-2052: Make id optional in OidcProviderInput for create vs updateAvoid forcing clients to pre-generate IDs. Absent → create, present → update. Optionally use PrefixedID for consistency.
input OidcProviderInput { """The unique identifier for the OIDC provider""" - id: String! + id: String }
🧹 Nitpick comments (4)
api/generated-schema.graphql (4)
1429-1431: Prefer non-null list for warningsUsing a non-null list avoids null handling in clients; return an empty array when no warnings.
- warnings: [String!] + warnings: [String!]!
1447-1456: Document evaluation semantics for authorization rulesClarify behavior on multiple values (any vs all), claim case-sensitivity, and missing-claim handling. This avoids mismatched expectations between UI and backend.
1458-1464: Operator set: consider future-proofing (optional)If needed later, consider NOT_EQUALS or regex-like operator. Fine to defer until a real use-case emerges.
1527-1530: Optional: include reason for invalid sessionsAdding a short code or message field can help clients present actionable errors (e.g., expired, invalid_signature).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/generated-schema.graphql(6 hunks)api/src/unraid-api/graph/resolvers/sso/sso.module.ts(1 hunks)web/pages/login.vue(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/unraid-api/graph/resolvers/sso/sso.module.ts
- web/pages/login.vue
🔇 Additional comments (3)
api/generated-schema.graphql (3)
1403-1408: Good addition: SSO settings shape looks consistent and implements NodeType and field naming align with existing conventions. No issues.
1440-1442: Settings.sso field addition LGTMClear namespacing under Settings; sensible default for future SSO providers.
1532-1539: Public provider type looks safe and minimalContains only UI-safe fields. No issues.
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/__test__/components/SsoButton.test.ts (1)
2-2: Update the header comment to match the component nameThe header comment references "SsoButton" but the actual component being tested is "SsoButtons".
- * SsoButton Component Test Coverage + * SsoButtons Component Test Coverage
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/__test__/components/SsoButton.test.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
web/__test__/components/SsoButton.test.ts
web/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All web GraphQL code must follow the .query.ts/.mutation.ts filename conventions (do not place operations in other file types)
Files:
web/__test__/components/SsoButton.test.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
web/__test__/components/SsoButton.test.ts
web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx}: Web (Vue) component tests should use mount from Vue Test Utils
Web (Vue) component tests should stub complex child components not under test
Web (Vue) component tests should mock external dependencies and services
Web (Vue) component tests should query elements semantically (e.g., find('button')) rather than using data-test IDs
Web (Vue) component tests should use createTestingPinia() for mocking stores in components
Web (Vue) component tests should use await nextTick() for DOM updates and await all async operations before assertions
Files:
web/__test__/components/SsoButton.test.ts
web/**/*.{test,spec}.{ts,js,tsx,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
web/**/*.{test,spec}.{ts,js,tsx,jsx}: Store testing with Pinia: use createPinia() and setActivePinia; only use createTestingPinia when you need its features
Store testing with Pinia: let stores initialize with natural defaults; do not mock the store under test
Store testing with Pinia: place all mock declarations at top level; use factory functions for module mocks; clear mocks between tests
Files:
web/__test__/components/SsoButton.test.ts
web/__test__/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests for the web app are located under web/test
Files:
web/__test__/components/SsoButton.test.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
web/__test__/components/SsoButton.test.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
web/__test__/components/SsoButton.test.ts
**/__test__/components/**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/web-testing-rules.mdc)
**/__test__/components/**/*.ts: Component tests should use mount from @vue/test-utils
Stub complex child components that are not the focus of a component test
Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Use createTestingPinia() to mock Pinia stores when testing Vue components
Test that expected elements are rendered and verify component output rather than internals
Test component interactions (clicks, inputs) using await element.trigger and setValue, and verify emitted events via wrapper.emitted()
Prefer semantic queries like find('button') or CSS selectors; avoid relying on data-test IDs unless necessary
Assert rendered text with wrapper.text(), check attributes via element.attributes(), and verify element existence with expect(element.exists()).toBe(true)
In component tests that use real store implementations, configure createTestingPinia with stubActions: false when you need real action behavior
Use findComponent(ComponentName) for locating child components and findAll for collections
Files:
web/__test__/components/SsoButton.test.ts
🧠 Learnings (18)
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Prefer semantic queries like find('button') or CSS selectors; avoid relying on data-test IDs unless necessary
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Test component interactions (clicks, inputs) using await element.trigger and setValue, and verify emitted events via wrapper.emitted()
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should mock external dependencies and services
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Component tests should use mount from vue/test-utils
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should stub complex child components not under test
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Use createTestingPinia() to mock Pinia stores when testing Vue components
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should use mount from Vue Test Utils
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Stub complex child components that are not the focus of a component test
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should use createTestingPinia() for mocking stores in components
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/**/*.{test,spec}.{ts,js,tsx,jsx} : Store testing with Pinia: place all mock declarations at top level; use factory functions for module mocks; clear mocks between tests
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/store/**/*.ts : In store tests, mock external dependencies used by the store and verify interactions with those mocks
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use vi.spyOn() to specify return values or behavior of methods under test
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/mocks/**/*.ts : Frequently used mocks can also be placed under __test__/mocks
Applied to files:
web/__test__/components/SsoButton.test.ts
🔇 Additional comments (8)
web/__test__/components/SsoButton.test.ts (8)
12-21: Good stubbing of child componentThe SsoProviderButtonStub properly follows the testing guideline to stub complex child components that are not the focus of the test.
83-121: Proper test setup and cleanupGood use of vi.clearAllTimers(), vi.useFakeTimers(), and vi.restoreAllMocks() for test isolation, following the testing guidelines.
123-156: Well-structured test for provider renderingThe test properly uses flushPromises() and fake timers to handle async operations, following the testing guidelines for Vue component tests.
158-179: Good coverage of empty stateThe test properly verifies the behavior when no OIDC providers are configured, using semantic queries as recommended.
181-209: Thorough test of polling behaviorExcellent test coverage of the polling mechanism, properly simulating API unavailability and recovery.
211-252: Comprehensive test of OIDC navigation flowThe test thoroughly verifies the navigation behavior, including state generation and sessionStorage usage.
254-368: Excellent coverage of OIDC callback scenariosThe tests comprehensively cover token-based authentication, error handling, and authorization code flow scenarios, with proper async handling throughout.
370-413: Good test for multi-provider supportThe test effectively verifies that multiple OIDC providers are rendered correctly, using semantic queries as recommended.
elibosley
force-pushed
the
api-feat-oidc-sso
branch
from
3510f8a to
9134ffc
Compare
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (15)
api/generated-schema.graphql (4)
1466-1525: Remove clientSecret from output type and implement Node on OidcProvider.Leaking clientSecret is a security risk; also this top-level resource should implement Node for consistency. Keep clientSecret only on input/mutations.
-type OidcProvider { +type OidcProvider implements Node { """The unique identifier for the OIDC provider""" id: PrefixedID! @@ - """OAuth2 client secret (if required by provider)""" - clientSecret: StringNote: Apply this change in the source SDL/model; this file is generated.
1945-1956: Protect OIDC queries and fix id type for oidcProvider.Add authorization directives and use PrefixedID for id consistency.
"""Get public OIDC provider information for login buttons""" publicOidcProviders: [PublicOidcProvider!]! """Get all configured OIDC providers (admin only)""" - oidcProviders: [OidcProvider!]! + oidcProviders: [OidcProvider!]! @usePermissions(action: READ, resource: "SSO", possession: ANY) """Get a specific OIDC provider by ID""" - oidcProvider(id: String!): OidcProvider + oidcProvider(id: PrefixedID!): OidcProvider @usePermissions(action: READ, resource: "SSO", possession: ANY) """Validate an OIDC session token (internal use for CLI validation)""" - validateOidcSession(token: String!): OidcSessionValidation! + validateOidcSession(token: String!): OidcSessionValidation! @usePermissions(action: READ, resource: "SSO", possession: OWN)Note: Apply in the source schema.
2003-2007: Enforce permissions on mutations and use PrefixedID for delete.Restrict upsert/delete to admins and keep ID types consistent.
"""Create or update an OIDC provider""" - upsertOidcProvider(provider: OidcProviderInput!): OidcProvider! + upsertOidcProvider(provider: OidcProviderInput!): OidcProvider! @usePermissions(action: UPDATE, resource: "SSO", possession: ANY) """Delete an OIDC provider""" - deleteOidcProvider(id: String!): Boolean! + deleteOidcProvider(id: PrefixedID!): Boolean! @usePermissions(action: DELETE, resource: "SSO", possession: ANY)Note: Apply in the source schema.
2050-2110: Make id optional in OidcProviderInput to support create vs update.For upsert semantics, allow clients to omit id on create.
input OidcProviderInput { """The unique identifier for the OIDC provider""" - id: String! + id: StringAdd documentation: absent → create, present → update.
web/pages/login.vue (1)
31-48: Debug modal displaying passwords is acceptable for test viewAs previously discussed and documented in the learnings, this debug modal is intentionally designed as a test view where displaying the password in plain text is acceptable for debugging purposes.
web/components/sso/useSsoAuth.ts (1)
11-23: Improve error handling for missing DOM elementsThe function logs a warning but still returns potentially null elements, which could cause runtime errors in consuming code when these elements are accessed in
enterCallbackTokenIntoFieldand other functions.Consider returning null when elements are missing and handling this case in calling functions:
- const getInputFields = (): { - form: HTMLFormElement; - passwordField: HTMLInputElement; - usernameField: HTMLInputElement; - } => { + const getInputFields = (): { + form: HTMLFormElement; + passwordField: HTMLInputElement; + usernameField: HTMLInputElement; + } | null => { const form = document.querySelector('form[action="/login"]') as HTMLFormElement; const passwordField = document.querySelector('input[name=password]') as HTMLInputElement; const usernameField = document.querySelector('input[name=username]') as HTMLInputElement; if (!form || !passwordField || !usernameField) { console.warn('Could not find form, username, or password field'); + return null; } return { form, passwordField, usernameField }; };Then update the calling functions to handle null:
const enterCallbackTokenIntoField = (token: string) => { - const { form, passwordField, usernameField } = getInputFields(); - if (!passwordField || !usernameField || !form) { - console.warn('Could not find form, username, or password field'); - } else { + const fields = getInputFields(); + if (fields) { + const { form, passwordField, usernameField } = fields; usernameField.value = 'root'; passwordField.value = token; // Submit the form form.requestSubmit(); + } else { + console.warn('Could not find form, username, or password field'); } };api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts (2)
28-43: Duplicate code between OidcAuthorizationRule and OidcAuthorizationRuleInput classes.These classes have identical field definitions, violating the DRY principle.
62-174: Significant code duplication between OidcProvider and OidcProviderInput classes.Both classes have nearly identical field definitions and validation decorators. This duplication increases maintenance burden and risk of inconsistencies.
api/src/unraid-api/rest/rest.controller.ts (1)
61-97: Add parameter validation for providerId.The
providerIdparameter is passed directly to the service without validation, which could lead to injection attacks.Apply this diff to add validation:
async oidcAuthorize( @Param('providerId') providerId: string, @Query('state') state: string, @Req() req: FastifyRequest, @Res() res: FastifyReply ) { try { if (!state) { return res.status(400).send('State parameter is required'); } + + // Validate providerId format to prevent injection attacks + if (!/^[a-zA-Z0-9._-]+$/.test(providerId)) { + return res.status(400).send('Invalid provider ID format'); + } // Get the host from the request headers const host = req.headers.host || undefined;api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts (3)
96-108: Avoid mutating in-memory config when ensuring unraid.net provider.The code mutates the providers array obtained directly from the config service by using unshift, which can cause side effects.
Apply this diff to avoid mutation:
async getProviders(): Promise<OidcProvider[]> { const config = this.configService.get<OidcConfig>(this.configKey()); - const providers = config?.providers || []; + const providers = [...(config?.providers || [])]; // Ensure unraid.net provider is always present const hasUnraidNet = providers.some((p) => p.id === 'unraid.net'); if (!hasUnraidNet) { providers.unshift(this.getUnraidNetSsoProvider()); } // Clean up providers - convert empty strings to undefined return providers.map((provider) => this.cleanProvider(provider)); }
110-120: Incomplete field cleaning in cleanProvider method.The
cleanProvidermethod doesn't clean all optional string fields.jwksUri,buttonText, andbuttonVariantare missing.Apply this diff to ensure consistent behavior:
private cleanProvider(provider: OidcProvider): OidcProvider { // Convert empty strings to undefined for optional fields return { ...provider, clientSecret: provider.clientSecret?.trim() || undefined, authorizationEndpoint: provider.authorizationEndpoint?.trim() || undefined, tokenEndpoint: provider.tokenEndpoint?.trim() || undefined, + jwksUri: provider.jwksUri?.trim() || undefined, buttonIcon: provider.buttonIcon?.trim() || undefined, + buttonText: provider.buttonText?.trim() || undefined, + buttonVariant: provider.buttonVariant?.trim() || undefined, buttonStyle: provider.buttonStyle?.trim() || undefined, }; }
291-304: Strengthen URL validation for Unraid.net issuer check.The issuer validation uses a substring match with
includes('unraid.net')which is insufficient and can match invalid URLs.Apply this diff to properly validate the issuer URL:
for (const provider of processedConfig.providers) { - if (provider.issuer && !provider.issuer.includes('unraid.net')) { - // Skip validation for unraid.net as it uses custom auth flow + if (provider.issuer) { + let isUnraidIssuer = false; + try { + const host = new URL(provider.issuer).hostname; + // Built-in provider runs on account.unraid.net; allow subdomains if needed + isUnraidIssuer = host === 'account.unraid.net' || host.endsWith('.unraid.net'); + } catch { + isUnraidIssuer = false; + } + if (!isUnraidIssuer) { + // Validate non-Unraid issuers via discovery try { const validation = await this.validationService.validateProvider(provider); if (!validation.isValid) { validationErrors.push(`❌ ${provider.name}: ${validation.error}`); } } catch (error) { // Don't fail the save, just warn this.logger.warn(`Failed to validate provider ${provider.id}: ${error}`); } + } } }api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts (3)
518-528: Consider making redirect URI configurable.The redirect URI logic has hardcoded values and fragile localhost detection.
239-242: JWT decode uses base64 instead of base64url encoding.The ID token parsing uses base64 decoding which fails for base64url-encoded tokens. Additionally, it logs full claims which may contain PII.
Apply this diff to fix the decoding and remove PII from logs:
// Decode the JWT manually const payload = tokens.id_token.split('.')[1]; -claims = JSON.parse(Buffer.from(payload, 'base64').toString()); -this.logger.debug(`ID token claims: ${JSON.stringify(claims)}`); +// Properly handle base64url decoding +const normalized = payload.replace(/-/g, '+').replace(/_/g, '/').padEnd(Math.ceil(payload.length / 4) * 4, '='); +claims = JSON.parse(Buffer.from(normalized, 'base64').toString('utf8')); +// Avoid logging full claims to prevent PII exposure +this.logger.debug(`ID token parsed; available claim keys: ${claims ? Object.keys(claims).join(', ') : 'none'}`);
433-502: Consider handling array claims properly in evaluateRule.The method converts claim values to strings, which collapses array claims (e.g., groups) into comma-separated strings, potentially breaking EQUALS and CONTAINS operators.
Apply this diff to properly handle array claims:
private evaluateRule(rule: OidcAuthorizationRule, claims: JwtClaims): boolean { const claimValue = claims[rule.claim]; this.logger.debug( `Evaluating rule for claim ${rule.claim}: ${JSON.stringify({ claimValue, claimType: typeof claimValue, ruleOperator: rule.operator, ruleValues: rule.value, })}` ); if (claimValue === undefined || claimValue === null) { this.logger.debug(`Claim ${rule.claim} not found in token`); return false; } // Log detailed claim analysis if (typeof claimValue === 'object' && claimValue !== null) { this.logger.warn( `unexpected JWT claim value encountered - claim ${rule.claim} is object type: ${JSON.stringify(claimValue)}` ); return false; } if (Array.isArray(claimValue)) { - this.logger.warn( - `unexpected JWT claim value encountered - claim ${rule.claim} is array type: ${JSON.stringify(claimValue)}` - ); - return false; + // Handle array claims by checking if any element matches + this.logger.debug(`Processing array claim ${rule.claim} with ${claimValue.length} values`); + const stringValues = claimValue.map(String); + + switch (rule.operator) { + case AuthorizationOperator.EQUALS: + return rule.value.some((v) => stringValues.includes(v)); + case AuthorizationOperator.CONTAINS: + return stringValues.some((sv) => rule.value.some((v) => sv.includes(v))); + case AuthorizationOperator.STARTS_WITH: + return stringValues.some((sv) => rule.value.some((v) => sv.startsWith(v))); + case AuthorizationOperator.ENDS_WITH: + return stringValues.some((sv) => rule.value.some((v) => sv.endsWith(v))); + default: + this.logger.error(`Unknown authorization operator: ${rule.operator}`); + return false; + } } const value = String(claimValue); this.logger.debug(`Processing claim ${rule.claim} with string value: "${value}"`); let result: boolean; switch (rule.operator) { case AuthorizationOperator.EQUALS: result = rule.value.some((v) => value === v); this.logger.debug( `EQUALS check: "${value}" matches any of [${rule.value.join(', ')}]: ${result}` ); return result; case AuthorizationOperator.CONTAINS: result = rule.value.some((v) => value.includes(v)); this.logger.debug( `CONTAINS check: "${value}" contains any of [${rule.value.join(', ')}]: ${result}` ); return result; case AuthorizationOperator.STARTS_WITH: result = rule.value.some((v) => value.startsWith(v)); this.logger.debug( `STARTS_WITH check: "${value}" starts with any of [${rule.value.join(', ')}]: ${result}` ); return result; case AuthorizationOperator.ENDS_WITH: result = rule.value.some((v) => value.endsWith(v)); this.logger.debug( `ENDS_WITH check: "${value}" ends with any of [${rule.value.join(', ')}]: ${result}` ); return result; default: this.logger.error(`Unknown authorization operator: ${rule.operator}`); return false; } }
🧹 Nitpick comments (10)
unraid-ui/src/forms/VerticalLayout.vue (1)
46-46: Explicit cast to UISchemaElement is fine; consider typing elements to avoid per-use casts.Optional: type the computed so the template doesn’t need casting, and prefer a stable key over the array index if available (e.g., element.scope).
Example (outside the changed lines):
const elements = computed<UISchemaElement[]>(() => layout.layout.value.uischema.elements ?? []);And in template (if available):
:key="(element as any).scope ?? _i"api/src/unraid-api/graph/resolvers/sso/oidc-session-validation.model.ts (1)
1-10: Looks good; consider adding GraphQL descriptions to aid schema introspection.Adding descriptions improves auto-generated docs and discoverability.
-import { Field, ObjectType } from '@nestjs/graphql'; +import { Field, ObjectType } from '@nestjs/graphql'; -@ObjectType() +@ObjectType({ description: 'Result of validating a short-lived OIDC session token' }) export class OidcSessionValidation { - @Field(() => Boolean) + @Field(() => Boolean, { description: 'Whether the provided token/session is valid' }) valid!: boolean; - @Field(() => String, { nullable: true }) + @Field(() => String, { nullable: true, description: 'Username associated with the token when valid' }) username?: string; }api/src/unraid-api/cli/sso/validate-token.command.ts (3)
7-12: Update CLI description to reflect actual output shape (includes username).The command prints username on success, but the description only mentions error and valid.
@SubCommand({ name: 'validate-token', aliases: ['validate', 'v'], - description: 'Returns JSON: { error: string | null, valid: boolean }', + description: 'Returns JSON: { error: string | null, valid: boolean, username?: string }', arguments: '<token>', })
49-52: Unify GraphQL error handling with errorPolicy: 'all'.Apollo Client throws on GraphQL errors by default, so
errorswill often be unset. UsingerrorPolicy: 'all'prevents throwing and ensureserrorsis populated, keeping all error handling in one branch.- const { data, errors } = await client.query({ + const { data, errors } = await client.query({ query: VALIDATE_OIDC_SESSION_QUERY, variables: { token }, + errorPolicy: 'all', });
59-73: Make response handling robust and avoid hardcoding username to 'root'.
- Fail fast if the response is missing (distinguish from an invalid token).
- Only include username when present; avoid defaulting to "root" which can mislead downstream tooling.
Do you rely on "root" as a fallback in any consumer? If not, prefer omitting the field when absent (see diff).
- const validation = data?.validateOidcSession; + const validation = data?.validateOidcSession; - if (validation?.valid) { - this.logger.always( - JSON.stringify({ - error: null, - valid: true, - username: validation.username || 'root', - }) - ); - process.exit(0); - } else { - this.createErrorAndExit('Invalid OIDC session token'); - } + if (!validation || typeof validation.valid !== 'boolean') { + this.createErrorAndExit('Malformed response: validateOidcSession missing or invalid'); + } + if (!validation.valid) { + this.createErrorAndExit('Invalid OIDC session token'); + } + const payload: Record<string, unknown> = { + error: null, + valid: true, + }; + if (validation.username) { + payload.username = validation.username; + } + this.logger.always(JSON.stringify(payload)); + process.exit(0);packages/unraid-shared/src/services/user-settings.ts (1)
126-160: Type-narrowing for fragment values to reduce casts; aggregate warnings looks good.Logic is sound: skips unknown namespaces, OR’s restartRequired, aggregates warnings, returns full snapshot. Minor type-safety improvement can remove the broad cast in Line 140.
- async updateNamespacedValues( - values: Record<string, unknown> - ): Promise<{ restartRequired: boolean; values: Record<string, unknown>; warnings?: string[] }> { + async updateNamespacedValues( + values: Record<string, unknown> + ): Promise<{ restartRequired: boolean; values: Record<string, unknown>; warnings?: string[] }> { let restartRequired = false; let allWarnings: string[] = []; for (const [key, fragmentValues] of Object.entries(values)) { if (!this.settings.has(key as keyof UserSettings)) { // Skip unknown namespaces – they may belong to other consumers continue; } - - const result = await this.updateValues( - key as keyof UserSettings, - fragmentValues as Partial<UserSettings[keyof UserSettings]> - ); + const k = key as keyof UserSettings; + // Narrow the type for this specific namespace; avoids casting to union of all fragments + const result = await this.updateValues(k, fragmentValues as Partial<UserSettings[typeof k]>);If you prefer returning an empty warnings array instead of omitting the field (to stabilize response shape), I can provide a follow-up diff.
api/.env.development (1)
20-20: Align with dotenv-linter: keep keys ordered.Move PATHS_OIDC_JSON before PATHS_PARITY_CHECKS for alphabetical ordering.
PATHS_KEYFILE_BASE=./dev/Unraid.net # Keyfile location PATHS_MACHINE_ID=./dev/data/machine-id -PATHS_PARITY_CHECKS=./dev/states/parity-checks.log +PATHS_OIDC_JSON=./dev/configs/oidc.local.json +PATHS_PARITY_CHECKS=./dev/states/parity-checks.log PATHS_CONFIG_MODULES=./dev/configs PATHS_ACTIVATION_BASE=./dev/activation PATHS_PASSWD=./dev/passwd @@ -PATHS_OIDC_JSON=./dev/configs/oidc.local.jsonapi/dev/configs/README.md (2)
10-17: Clarify gitignore status and use the repo-relative path in PATHS_OIDC_JSON.The file is already ignored in api/.gitignore; no need to edit .gitignore. Also prefer the repo-relative path used in .env.development.
-### Using a Local Configuration (gitignored) -For local testing with real OAuth providers: +### Using a Local Configuration (gitignored) +For local testing with real OAuth providers: -1. Create an `oidc.local.json` file based on `oidc.local.example.json` -2. Add it to your local `.gitignore` to prevent committing secrets -3. Set the environment variable: `PATHS_OIDC_JSON=oidc.local.json` -4. The API will load your local configuration instead of the default +1. Create an `oidc.local.json` file based on `oidc.local.example.json` +2. No action needed for gitignore (already ignored by api/.gitignore) +3. Set the environment variable: `PATHS_OIDC_JSON=./dev/configs/oidc.local.json` +4. The API will load your local configuration instead of the default-Example: +Example: ```bash -PATHS_OIDC_JSON=oidc.local.json pnpm dev +PATHS_OIDC_JSON=./dev/configs/oidc.local.json pnpm dev--- `32-35`: **Minor wording tweak for GitHub section.** Optional, improves clarity. ```diff -1. Go to GitHub Settings > Developer settings > OAuth Apps +1. Go to GitHub Settings → Developer settings → OAuth Apps 2. Create a new OAuth App 3. Set Authorization callback URL: `http://localhost:3000/graphql/api/auth/oidc/callback`unraid-ui/src/forms/ObjectArrayField.vue (1)
126-151: Consider extracting default value initialization logicThe default value initialization logic is complex and could benefit from being extracted into a separate utility function for better maintainability and testability.
Consider extracting this logic into a utility function:
+const getDefaultValueForSchema = (schema: JsonSchema): unknown => { + if (schema.default !== undefined) { + return schema.default; + } + switch (schema.type) { + case 'array': + return []; + case 'string': + return ''; + case 'number': + case 'integer': + return 0; + case 'boolean': + return false; + default: + return undefined; + } +}; const addItem = () => { const schema = control.value.schema; const newItem: Record<string, unknown> = {}; // Initialize with default values if available if (schema?.items && typeof schema.items === 'object' && !Array.isArray(schema.items)) { const properties = schema.items.properties; if (properties && typeof properties === 'object') { Object.entries(properties).forEach(([key, propSchema]) => { const schema = propSchema as JsonSchema; - if (schema.default !== undefined) { - newItem[key] = schema.default; - } else if (schema.type === 'array') { - newItem[key] = []; - } else if (schema.type === 'string') { - newItem[key] = ''; - } else if (schema.type === 'number' || schema.type === 'integer') { - newItem[key] = 0; - } else if (schema.type === 'boolean') { - newItem[key] = false; - } + const defaultValue = getDefaultValueForSchema(schema); + if (defaultValue !== undefined) { + newItem[key] = defaultValue; + } }); } } items.value = [...items.value, newItem]; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
api/src/unraid-api/cli/generated/gql.tsis excluded by!**/generated/**api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (57)
api/.env.development(1 hunks)api/.gitignore(1 hunks)api/dev/configs/README.md(1 hunks)api/dev/configs/api.json(1 hunks)api/dev/configs/connect.json(1 hunks)api/dev/configs/oidc.json(1 hunks)api/docs/public/index.md(1 hunks)api/docs/public/oidc-provider-examples.md(1 hunks)api/generated-schema.graphql(6 hunks)api/package.json(2 hunks)api/src/unraid-api/app/app.module.ts(1 hunks)api/src/unraid-api/cli/queries/validate-oidc-session.query.ts(1 hunks)api/src/unraid-api/cli/sso/validate-token.command.ts(2 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(2 hunks)api/src/unraid-api/graph/resolvers/settings/settings.model.ts(3 hunks)api/src/unraid-api/graph/resolvers/settings/settings.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/settings/settings.resolver.ts(6 hunks)api/src/unraid-api/graph/resolvers/settings/settings.service.ts(4 hunks)api/src/unraid-api/graph/resolvers/settings/sso-settings.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-session-validation.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-session.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-validation.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/public-oidc-provider.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/sso-settings.types.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/sso.module.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts(1 hunks)api/src/unraid-api/graph/utils/form-utils.ts(1 hunks)api/src/unraid-api/rest/rest.controller.ts(2 hunks)api/src/unraid-api/rest/rest.module.ts(1 hunks)packages/unraid-shared/package.json(1 hunks)packages/unraid-shared/src/services/user-settings.ts(3 hunks)unraid-ui/src/components/common/index.ts(1 hunks)unraid-ui/src/components/form/index.ts(1 hunks)unraid-ui/src/forms/HorizontalLayout.vue(2 hunks)unraid-ui/src/forms/ObjectArrayField.vue(1 hunks)unraid-ui/src/forms/SteppedLayout.vue(2 hunks)unraid-ui/src/forms/UnraidSettingsLayout.vue(2 hunks)unraid-ui/src/forms/VerticalLayout.vue(2 hunks)unraid-ui/src/forms/renderers.ts(4 hunks)web/__test__/components/SsoButton.test.ts(5 hunks)web/components/SsoButton.ce.vue(1 hunks)web/components/queries/oidc-providers.query.ts(1 hunks)web/components/queries/public-oidc-providers.query.ts(1 hunks)web/components/sso/SsoButtons.vue(1 hunks)web/components/sso/SsoProviderButton.vue(1 hunks)web/components/sso/useSsoAuth.ts(1 hunks)web/components/sso/useSsoProviders.ts(1 hunks)web/composables/gql/gql.ts(3 hunks)web/composables/gql/graphql.ts(16 hunks)web/nuxt.config.ts(1 hunks)web/pages/login.vue(9 hunks)web/server/api/debug/validate-token.post.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- api/dev/configs/api.json
- api/docs/public/oidc-provider-examples.md
🚧 Files skipped from review as they are similar to previous changes (36)
- packages/unraid-shared/package.json
- api/src/unraid-api/graph/resolvers/sso/sso.module.ts
- web/nuxt.config.ts
- api/package.json
- api/src/unraid-api/rest/rest.module.ts
- api/src/unraid-api/graph/resolvers/settings/sso-settings.model.ts
- api/src/unraid-api/graph/resolvers/sso/public-oidc-provider.model.ts
- unraid-ui/src/forms/SteppedLayout.vue
- api/src/unraid-api/graph/resolvers/resolvers.module.ts
- api/src/unraid-api/cli/queries/validate-oidc-session.query.ts
- api/src/unraid-api/graph/resolvers/settings/settings.model.ts
- web/components/queries/oidc-providers.query.ts
- api/dev/configs/connect.json
- api/src/unraid-api/graph/resolvers/sso/sso-settings.types.ts
- api/src/unraid-api/graph/utils/form-utils.ts
- api/.gitignore
- api/dev/configs/oidc.json
- web/composables/gql/gql.ts
- unraid-ui/src/forms/UnraidSettingsLayout.vue
- unraid-ui/src/components/form/index.ts
- api/src/unraid-api/app/app.module.ts
- api/src/unraid-api/graph/resolvers/sso/oidc-session.service.ts
- api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts
- web/components/sso/SsoProviderButton.vue
- api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts
- web/components/sso/SsoButtons.vue
- unraid-ui/src/components/common/index.ts
- web/components/SsoButton.ce.vue
- api/src/unraid-api/graph/resolvers/settings/settings.service.ts
- web/server/api/debug/validate-token.post.ts
- api/src/unraid-api/graph/resolvers/sso/oidc-validation.service.ts
- api/src/unraid-api/graph/resolvers/settings/settings.module.ts
- web/test/components/SsoButton.test.ts
- web/components/sso/useSsoProviders.ts
- web/composables/gql/graphql.ts
- api/src/unraid-api/graph/resolvers/settings/settings.resolver.ts
🧰 Additional context used
📓 Path-based instructions (9)
web/**/*.query.ts
📄 CodeRabbit Inference Engine (.cursor/rules/web-graphql.mdc)
GraphQL queries must be placed in
.query.tsfilesIn the web app, GraphQL queries must live in files ending with .query.ts
Files:
web/components/queries/public-oidc-providers.query.ts
web/**/*.{query,mutation}.ts
📄 CodeRabbit Inference Engine (.cursor/rules/web-graphql.mdc)
All GraphQL under
web/must follow this naming convention
Files:
web/components/queries/public-oidc-providers.query.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
web/components/queries/public-oidc-providers.query.tsapi/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsweb/components/sso/useSsoAuth.tspackages/unraid-shared/src/services/user-settings.tsapi/src/unraid-api/graph/resolvers/sso/oidc-session-validation.model.tsapi/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/oidc-provider.model.tsunraid-ui/src/forms/renderers.tsapi/src/unraid-api/cli/sso/validate-token.command.tsapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.tsapi/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
web/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All web GraphQL code must follow the .query.ts/.mutation.ts filename conventions (do not place operations in other file types)
Files:
web/components/queries/public-oidc-providers.query.tsweb/components/sso/useSsoAuth.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
web/components/queries/public-oidc-providers.query.tsunraid-ui/src/forms/HorizontalLayout.vueapi/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsunraid-ui/src/forms/VerticalLayout.vueweb/components/sso/useSsoAuth.tspackages/unraid-shared/src/services/user-settings.tsapi/src/unraid-api/graph/resolvers/sso/oidc-session-validation.model.tsapi/src/unraid-api/rest/rest.controller.tsweb/pages/login.vueunraid-ui/src/forms/ObjectArrayField.vueapi/src/unraid-api/graph/resolvers/sso/oidc-provider.model.tsunraid-ui/src/forms/renderers.tsapi/src/unraid-api/cli/sso/validate-token.command.tsapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.tsapi/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.tsapi/src/unraid-api/graph/resolvers/sso/oidc-session-validation.model.tsapi/src/unraid-api/rest/rest.controller.tsapi/src/unraid-api/graph/resolvers/sso/oidc-provider.model.tsapi/src/unraid-api/cli/sso/validate-token.command.tsapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.tsapi/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
🧠 Learnings (5)
📚 Learning: 2025-08-11T15:41:23.573Z
Learnt from: elibosley
PR: unraid/api#1563
File: web/pages/login.vue:31-48
Timestamp: 2025-08-11T15:41:23.573Z
Learning: In the Unraid API codebase, debug/test views (such as the debug modal in `web/pages/login.vue`) may intentionally display sensitive information like passwords in plain text for debugging and testing purposes. This is an accepted practice for development tools in this project.
Applied to files:
web/pages/login.vue
📚 Learning: 2025-04-25T13:29:07.868Z
Learnt from: elibosley
PR: unraid/api#1369
File: web/components/Activation/WelcomeModal.ce.vue:40-40
Timestamp: 2025-04-25T13:29:07.868Z
Learning: The `#confirmPassword` selector in the WelcomeModal.ce.vue component is intentionally looking for an element that exists in the Unraid OS login page (.login.php), not within the component itself. This is used as part of a font-size adjustment workaround to handle different CSS between login and authenticated pages.
Applied to files:
web/pages/login.vue
📚 Learning: 2024-12-09T15:45:46.492Z
Learnt from: pujitm
PR: unraid/api#975
File: web/components/Notifications/TabList.vue:1-4
Timestamp: 2024-12-09T15:45:46.492Z
Learning: In our Nuxt.js setup for the `web` project, it's not necessary to explicitly import `computed` from `vue` in Vue components, as it's globally available.
Applied to files:
web/pages/login.vue
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/{store,stores}/**/*.ts : In store files, explicitly import Vue reactivity utilities (e.g., computed, ref, watchEffect); do not rely on Nuxt auto-imports
Applied to files:
web/pages/login.vue
📚 Learning: 2025-03-12T13:35:43.900Z
Learnt from: pujitm
PR: unraid/api#1211
File: web/codegen.ts:14-14
Timestamp: 2025-03-12T13:35:43.900Z
Learning: The JSON scalar type in web/codegen.ts was temporarily changed from 'string' to 'any' for compatibility with JsonForms integration. This change facilitates the implementation of the Connect settings web component.
Applied to files:
unraid-ui/src/forms/renderers.ts
🧬 Code Graph Analysis (7)
web/components/queries/public-oidc-providers.query.ts (1)
web/composables/gql/gql.ts (1)
graphql(276-278)
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts (2)
web/components/sso/useSsoProviders.ts (1)
OidcProvider(5-12)web/composables/gql/graphql.ts (1)
OidcProvider(1145-1175)
packages/unraid-shared/src/services/user-settings.ts (1)
api/src/unraid-api/graph/resolvers/settings/settings.resolver.ts (1)
values(99-101)
api/src/unraid-api/graph/resolvers/sso/oidc-session-validation.model.ts (3)
api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts (2)
ObjectType(28-43)ObjectType(62-174)api/src/unraid-api/graph/resolvers/sso/public-oidc-provider.model.ts (1)
ObjectType(3-22)api/src/unraid-api/cli/generated/graphql.ts (1)
OidcSessionValidation(1255-1259)
api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts (4)
api/src/unraid-api/graph/resolvers/sso/public-oidc-provider.model.ts (1)
ObjectType(3-22)api/src/unraid-api/cli/generated/graphql.ts (3)
OidcAuthorizationRule(1169-1177)OidcProvider(1188-1220)OidcProviderInput(1222-1253)web/components/sso/useSsoProviders.ts (1)
OidcProvider(5-12)web/composables/gql/graphql.ts (2)
OidcProvider(1145-1175)OidcProviderInput(1177-1206)
api/src/unraid-api/cli/sso/validate-token.command.ts (3)
web/helpers/create-apollo-client.ts (1)
client(93-96)api/src/unraid-api/cli/queries/validate-oidc-session.query.ts (1)
VALIDATE_OIDC_SESSION_QUERY(3-10)api/src/unraid-api/cli/log.service.ts (1)
error(65-69)
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts (4)
web/components/sso/useSsoProviders.ts (1)
OidcProvider(5-12)web/composables/gql/graphql.ts (1)
OidcProvider(1145-1175)api/src/unraid-api/types/json-forms.ts (1)
SettingSlice(38-46)api/src/unraid-api/graph/utils/form-utils.ts (1)
createSimpleLabeledControl(8-45)
🪛 dotenv-linter (3.3.0)
api/.env.development
[warning] 20-20: [UnorderedKey] The PATHS_OIDC_JSON key should go before the PATHS_PARITY_CHECKS key
🪛 LanguageTool
api/dev/configs/README.md
[grammar] ~10-~10: There might be a mistake here.
Context: ...Using a Local Configuration (gitignored) For local testing with real OAuth provid...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ... ### Setting up OAuth Apps #### Google 1. Go to [Google Cloud Console](https://con...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...google.com/) 2. Create a new project or select existing 3. Enable Google+ API 4. Creat...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ... Create a new project or select existing 3. Enable Google+ API 4. Create OAuth 2.0 c...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...hql/api/auth/oidc/callback` #### GitHub 1. Go to GitHub Settings > Developer settin...
(QB_NEW_EN)
api/docs/public/index.md
[grammar] ~25-~25: There might be a mistake here.
Context: ...xamples for SSO authentication providers - [Upcoming Features](./upcoming-features.m...
(QB_NEW_EN)
🪛 GitHub Check: CodeQL
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts
[failure] 292-292: Incomplete URL substring sanitization
'unraid.net' can be anywhere in the URL, and arbitrary hosts may come before or after it.
🪛 Gitleaks (8.27.2)
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts
91-91: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🔇 Additional comments (20)
unraid-ui/src/forms/VerticalLayout.vue (1)
18-18: Type-only UISchemaElement import is correct and consistent with other layouts.Matches the typing pattern used across the layouts; no runtime impact.
unraid-ui/src/forms/HorizontalLayout.vue (2)
18-18: Type-only UISchemaElement import aligns with the shared typing approach.Consistent with other form layouts; no runtime behavior change.
38-38: Casting uischema to UISchemaElement is appropriate here.Straightforward and consistent with VerticalLayout; safe for DispatchRenderer typing.
web/components/queries/public-oidc-providers.query.ts (1)
3-14: Query shape matches the PublicOidcProvider GraphQL type.Fields align with the backend model; placement under web/**/*.query.ts follows the convention.
packages/unraid-shared/src/services/user-settings.ts (1)
20-23: Propagate warnings from fragments: good addition.Adding warnings?: string[] to fragment updates enables end-to-end surfacing of validation messages. This aligns with the schema’s UpdateSettingsResponse.warnings.
api/src/unraid-api/graph/resolvers/sso/oidc-session.service.spec.ts (3)
91-91: Hardcoded JWT in test is acceptable for testing purposesThe hardcoded JWT token detected by static analysis is intentionally used as test data. Since this is a test file and the token is not a real credential, this is an acceptable practice.
1-7: Correct import extension for TypeScript/ESM compatibilityThe import statement correctly uses the
.jsextension for ESM compatibility, which aligns with the TypeScript configuration requirements.
34-120: Well-structured test coverage for one-time token validationThe test suite comprehensively covers the critical security aspects of one-time token validation:
- Successful first validation
- Prevention of token reuse
- Invalid token format handling
- Non-existent session handling
- Expired session handling
The tests properly mock the cache manager and verify the expected behavior without brittle assertions on exact error messages.
web/pages/login.vue (2)
2-2: Explicit Vue imports follow framework patternsThe explicit imports of
refandonMountedfrom Vue are appropriate here as this is a page component, not a store file where explicit imports would be required according to the coding guidelines.
50-79: CLI command execution properly handles response typingThe function correctly handles the response from the debug endpoint with runtime type checking using the
inoperator to safely access properties. This approach is appropriate for handling the dynamic response structure.web/components/sso/useSsoAuth.ts (2)
1-2: Correct import usage for Vue composition utilitiesThe explicit imports from
vueandvue-i18nare appropriate for this composable function, following Vue 3 composition API patterns.
64-75: State management and CSRF protection implementation looks goodThe
navigateToProviderfunction correctly:
- Generates a cryptographically secure state token for CSRF protection
- Stores both the state and provider ID in sessionStorage
- Properly encodes URI components when constructing the redirect URL
unraid-ui/src/forms/ObjectArrayField.vue (2)
199-211: Good fix for dynamic item type in remove buttonThe remove button now correctly uses
getItemLabel(item, index)instead of the hardcoded "Remove Provider" text, making the component properly reusable for different item types.
32-43: Tab management logic handles edge cases correctlyThe watch effect properly manages the active tab index when items are added or removed, ensuring the UI always shows a valid tab.
unraid-ui/src/forms/renderers.ts (3)
51-55: Object array detection logic is correctThe
isObjectArrayfunction correctly identifies arrays of objects by checking both the array type and the nested items type.
57-71: String schema detection handles optional URL pattern correctlyThe
isStringOrAnyOfStringfunction properly identifies string fields including the special case of optional URL fields using theanyOfpattern withminLengthandmaxLengthconstraints.
102-103: Renderer priority adjustments ensure correct precedenceThe updated renderer priorities ensure that:
- Object arrays (priority 5) are handled before generic controls
- Enum controls (priority 6) have the highest priority to avoid conflicts with string renderers
- The new string renderer (priority 4) doesn't interfere with enums due to the explicit enum exclusion in
isStringOrAnyOfStringAlso applies to: 105-106, 112-115, 124-127
api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts (1)
3-3: Correct import extension for TypeScript ESM compatibility.The import specifier should use
.jsextension for ESM compatibility in TypeScript source files, not.ts.Apply this diff to correct the import extension:
-import { PrefixedID } from '@unraid/shared/prefixed-id-scalar.js'; +import { PrefixedID } from '@unraid/shared/prefixed-id-scalar.js';Wait, the import already has
.jsextension which is correct. Let me reconsider.api/src/unraid-api/rest/rest.controller.ts (2)
116-119: Properly honor X-Forwarded headers.The code correctly retrieves reverse proxy headers for constructing the callback URL.
139-156: Secure error handling implementation.The error handling correctly avoids exposing sensitive information to users while logging detailed errors for debugging purposes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web/__test__/components/SsoButton.test.ts (1)
13-21: Improve type safety of the component stub.The stub's
thistyping in thehandleClickmethod could be more robust by using proper component interfaces.const SsoProviderButtonStub = { template: '<button @click="handleClick" :disabled="disabled">{{ provider.buttonText || `Sign in with ${provider.name}` }}</button>', props: ['provider', 'disabled', 'onClick'], methods: { - handleClick(this: { onClick: (id: string) => void; provider: { id: string } }) { + handleClick(this: { onClick: (id: string) => void; provider: { id: string; name: string; buttonText?: string } }) { this.onClick(this.provider.id); } } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/__test__/components/SsoButton.test.ts(5 hunks)web/components/queries/public-oidc-providers.query.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/components/queries/public-oidc-providers.query.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
web/__test__/components/SsoButton.test.ts
web/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All web GraphQL code must follow the .query.ts/.mutation.ts filename conventions (do not place operations in other file types)
Files:
web/__test__/components/SsoButton.test.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
web/__test__/components/SsoButton.test.ts
web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx}: Web (Vue) component tests should use mount from Vue Test Utils
Web (Vue) component tests should stub complex child components not under test
Web (Vue) component tests should mock external dependencies and services
Web (Vue) component tests should query elements semantically (e.g., find('button')) rather than using data-test IDs
Web (Vue) component tests should use createTestingPinia() for mocking stores in components
Web (Vue) component tests should use await nextTick() for DOM updates and await all async operations before assertions
Files:
web/__test__/components/SsoButton.test.ts
web/**/*.{test,spec}.{ts,js,tsx,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
web/**/*.{test,spec}.{ts,js,tsx,jsx}: Store testing with Pinia: use createPinia() and setActivePinia; only use createTestingPinia when you need its features
Store testing with Pinia: let stores initialize with natural defaults; do not mock the store under test
Store testing with Pinia: place all mock declarations at top level; use factory functions for module mocks; clear mocks between tests
Files:
web/__test__/components/SsoButton.test.ts
web/__test__/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests for the web app are located under web/test
Files:
web/__test__/components/SsoButton.test.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
web/__test__/components/SsoButton.test.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
web/__test__/components/SsoButton.test.ts
**/__test__/components/**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/web-testing-rules.mdc)
**/__test__/components/**/*.ts: Component tests should use mount from @vue/test-utils
Stub complex child components that are not the focus of a component test
Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Use createTestingPinia() to mock Pinia stores when testing Vue components
Test that expected elements are rendered and verify component output rather than internals
Test component interactions (clicks, inputs) using await element.trigger and setValue, and verify emitted events via wrapper.emitted()
Prefer semantic queries like find('button') or CSS selectors; avoid relying on data-test IDs unless necessary
Assert rendered text with wrapper.text(), check attributes via element.attributes(), and verify element existence with expect(element.exists()).toBe(true)
In component tests that use real store implementations, configure createTestingPinia with stubActions: false when you need real action behavior
Use findComponent(ComponentName) for locating child components and findAll for collections
Files:
web/__test__/components/SsoButton.test.ts
🧠 Learnings (18)
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Prefer semantic queries like find('button') or CSS selectors; avoid relying on data-test IDs unless necessary
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Test component interactions (clicks, inputs) using await element.trigger and setValue, and verify emitted events via wrapper.emitted()
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should mock external dependencies and services
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Component tests should use mount from vue/test-utils
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should stub complex child components not under test
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Use createTestingPinia() to mock Pinia stores when testing Vue components
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should use mount from Vue Test Utils
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Stub complex child components that are not the focus of a component test
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should use createTestingPinia() for mocking stores in components
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/**/*.{test,spec}.{ts,js,tsx,jsx} : Store testing with Pinia: place all mock declarations at top level; use factory functions for module mocks; clear mocks between tests
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/store/**/*.ts : In store tests, mock external dependencies used by the store and verify interactions with those mocks
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use vi.spyOn() to specify return values or behavior of methods under test
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/mocks/**/*.ts : Frequently used mocks can also be placed under __test__/mocks
Applied to files:
web/__test__/components/SsoButton.test.ts
🔇 Additional comments (9)
web/__test__/components/SsoButton.test.ts (9)
84-85: Good testing practice - using fake timers for time-dependent behavior.The use of fake timers to control time-based behavior in tests is excellent. This ensures deterministic testing of polling and timeout mechanisms.
122-155: Comprehensive test for provider rendering with proper async handling.The test properly handles async operations with
flushPromises()andvi.runAllTimers(), ensuring all promises and timers are resolved before assertions. The test coverage includes both the provider data and UI rendering verification.
180-208: Excellent test coverage for polling behavior.This test effectively verifies the polling mechanism by mocking a failed first attempt followed by a successful one, and properly tests the "checking" state display. The timer advancement logic correctly simulates the polling interval.
210-251: Thorough navigation test with proper state management verification.The test comprehensively verifies the SSO flow including sessionStorage interactions, URL construction, and navigation behavior. Good use of
expect.any(String)for the generated state parameter.
253-289: Well-structured token callback test.The test properly simulates the token callback scenario and verifies all expected side effects: form hiding, credential population, form submission, and URL cleanup. The mock setup is comprehensive and realistic.
291-331: Comprehensive error handling test.This test effectively covers the error callback scenario, verifying both the error display and that the form remains visible for user interaction. The URL cleanup verification is a nice touch.
333-367: Good coverage of code/state callback redirect flow.The test properly verifies the OAuth2 authorization code flow redirection to the callback endpoint with proper parameter encoding.
369-411: Excellent multi-provider test coverage.This test validates the component's ability to handle multiple OIDC providers, ensuring both buttons are rendered and the correct text is displayed. The test data includes varied provider configurations which is realistic.
1-413: Overall test quality assessment.This is a comprehensive test suite that follows Vue testing best practices:
- Proper use of
mount()from Vue Test Utils- Appropriate stubbing of child components
- Comprehensive mocking of external dependencies
- Good async operation handling with
flushPromises()and fake timers- Semantic element queries using
find('button')- Thorough coverage of component behavior including edge cases
The test suite effectively covers the multi-provider SSO functionality with realistic scenarios and proper mock isolation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/__test__/components/SsoButton.test.ts (2)
13-21: Consider extracting the stub component for reusabilityThe
SsoProviderButtonStubcomponent is well-structured but could be extracted to a separate test utilities file for potential reuse across other tests.Consider moving this stub to
web/__test__/mocks/sso-provider-button.stub.tsto follow the testing conventions and make it reusable across other test files that might need to stub the SsoProviderButton component.
243-245: Consider adding assertion for state formatWhile the test verifies that a state is generated and stored, it could also validate the format or length of the generated state value to ensure it meets security requirements (e.g., sufficient entropy).
// Should set state and provider in sessionStorage expect(sessionStorage.setItem).toHaveBeenCalledWith('sso_state', expect.any(String)); expect(sessionStorage.setItem).toHaveBeenCalledWith('sso_provider', 'unraid-net'); + +// Validate state has sufficient entropy (e.g., at least 32 characters) +const generatedState = (sessionStorage.setItem as Mock).mock.calls[0][1]; +expect(generatedState.length).toBeGreaterThanOrEqual(32);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/__test__/components/SsoButton.test.ts(5 hunks)web/components/queries/public-oidc-providers.query.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/components/queries/public-oidc-providers.query.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
web/__test__/components/SsoButton.test.ts
web/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All web GraphQL code must follow the .query.ts/.mutation.ts filename conventions (do not place operations in other file types)
Files:
web/__test__/components/SsoButton.test.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
web/__test__/components/SsoButton.test.ts
web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx}: Web (Vue) component tests should use mount from Vue Test Utils
Web (Vue) component tests should stub complex child components not under test
Web (Vue) component tests should mock external dependencies and services
Web (Vue) component tests should query elements semantically (e.g., find('button')) rather than using data-test IDs
Web (Vue) component tests should use createTestingPinia() for mocking stores in components
Web (Vue) component tests should use await nextTick() for DOM updates and await all async operations before assertions
Files:
web/__test__/components/SsoButton.test.ts
web/**/*.{test,spec}.{ts,js,tsx,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
web/**/*.{test,spec}.{ts,js,tsx,jsx}: Store testing with Pinia: use createPinia() and setActivePinia; only use createTestingPinia when you need its features
Store testing with Pinia: let stores initialize with natural defaults; do not mock the store under test
Store testing with Pinia: place all mock declarations at top level; use factory functions for module mocks; clear mocks between tests
Files:
web/__test__/components/SsoButton.test.ts
web/__test__/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests for the web app are located under web/test
Files:
web/__test__/components/SsoButton.test.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
web/__test__/components/SsoButton.test.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
web/__test__/components/SsoButton.test.ts
**/__test__/components/**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/web-testing-rules.mdc)
**/__test__/components/**/*.ts: Component tests should use mount from @vue/test-utils
Stub complex child components that are not the focus of a component test
Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Use createTestingPinia() to mock Pinia stores when testing Vue components
Test that expected elements are rendered and verify component output rather than internals
Test component interactions (clicks, inputs) using await element.trigger and setValue, and verify emitted events via wrapper.emitted()
Prefer semantic queries like find('button') or CSS selectors; avoid relying on data-test IDs unless necessary
Assert rendered text with wrapper.text(), check attributes via element.attributes(), and verify element existence with expect(element.exists()).toBe(true)
In component tests that use real store implementations, configure createTestingPinia with stubActions: false when you need real action behavior
Use findComponent(ComponentName) for locating child components and findAll for collections
Files:
web/__test__/components/SsoButton.test.ts
🧠 Learnings (18)
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Prefer semantic queries like find('button') or CSS selectors; avoid relying on data-test IDs unless necessary
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should mock external dependencies and services
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Test component interactions (clicks, inputs) using await element.trigger and setValue, and verify emitted events via wrapper.emitted()
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Component tests should use mount from vue/test-utils
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should stub complex child components not under test
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Use createTestingPinia() to mock Pinia stores when testing Vue components
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should use mount from Vue Test Utils
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Stub complex child components that are not the focus of a component test
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should use createTestingPinia() for mocking stores in components
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/**/*.{test,spec}.{ts,js,tsx,jsx} : Store testing with Pinia: place all mock declarations at top level; use factory functions for module mocks; clear mocks between tests
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/store/**/*.ts : In store tests, mock external dependencies used by the store and verify interactions with those mocks
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use vi.spyOn() to specify return values or behavior of methods under test
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/mocks/**/*.ts : Frequently used mocks can also be placed under __test__/mocks
Applied to files:
web/__test__/components/SsoButton.test.ts
🔇 Additional comments (7)
web/__test__/components/SsoButton.test.ts (7)
84-86: Good practice: Using fake timers for polling behaviorExcellent use of fake timers to test the polling mechanism. This ensures tests run quickly and deterministically without waiting for real timeouts.
122-155: Well-structured test for OIDC providers renderingThe test properly validates the rendering of provider buttons and includes appropriate waiting for async operations with
flushPromises()and timer advancement.
180-208: Comprehensive polling state testGood coverage of the polling behavior, including error handling and retry logic. The test effectively simulates API unavailability and recovery.
253-289: Good test coverage for token-based authentication flowThe test properly validates the token handling flow, including form manipulation and submission. The cleanup of URL parameters is also correctly tested.
291-331: Thorough error handling testExcellent coverage of the error scenario, including UI feedback (red error text) and URL cleanup while preserving the form's visibility.
333-367: Well-implemented OAuth code flow testThe test correctly validates the redirect behavior when OAuth authorization code and state are present in the URL parameters.
369-411: Comprehensive multi-provider testExcellent test that validates the rendering and behavior with multiple OIDC providers, ensuring the component can handle various provider configurations including custom styling and icons.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web/__test__/components/SsoButton.test.ts (2)
13-21: Consider using a simpler stub implementation.The current stub has a complex method binding. Consider simplifying it to avoid potential
thiscontext issues.const SsoProviderButtonStub = { template: '<button @click="handleClick" :disabled="disabled">{{ provider.buttonText || `Sign in with ${provider.name}` }}</button>', props: ['provider', 'disabled', 'onClick'], methods: { - handleClick(this: { onClick: (id: string) => void; provider: { id: string } }) { - this.onClick(this.provider.id); + handleClick() { + this.onClick?.(this.provider?.id); } } };
180-208: Consider adding error state assertions.The test verifies the polling behavior and message display, but could be more robust by asserting the error state during failed API calls.
const wrapper = mount(SsoButtons, { global: { stubs: { SsoProviderButton: SsoProviderButtonStub, Button: { template: '<button><slot /></button>' } }, }, }); expect(wrapper.text()).toContain('Checking authentication options...'); // Advance timers to trigger the polling await flushPromises(); + // Verify the first failed API call was made + expect(refetchMock).toHaveBeenCalledTimes(1); vi.advanceTimersByTime(2000); await flushPromises(); + // Verify the retry was made after delay + expect(refetchMock).toHaveBeenCalledTimes(2); // After successful API response, checking message should disappear expect(wrapper.text()).not.toContain('Checking authentication options...');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/__test__/components/SsoButton.test.ts(5 hunks)web/components/queries/public-oidc-providers.query.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/components/queries/public-oidc-providers.query.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
web/__test__/components/SsoButton.test.ts
web/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All web GraphQL code must follow the .query.ts/.mutation.ts filename conventions (do not place operations in other file types)
Files:
web/__test__/components/SsoButton.test.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
web/__test__/components/SsoButton.test.ts
web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx}: Web (Vue) component tests should use mount from Vue Test Utils
Web (Vue) component tests should stub complex child components not under test
Web (Vue) component tests should mock external dependencies and services
Web (Vue) component tests should query elements semantically (e.g., find('button')) rather than using data-test IDs
Web (Vue) component tests should use createTestingPinia() for mocking stores in components
Web (Vue) component tests should use await nextTick() for DOM updates and await all async operations before assertions
Files:
web/__test__/components/SsoButton.test.ts
web/**/*.{test,spec}.{ts,js,tsx,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
web/**/*.{test,spec}.{ts,js,tsx,jsx}: Store testing with Pinia: use createPinia() and setActivePinia; only use createTestingPinia when you need its features
Store testing with Pinia: let stores initialize with natural defaults; do not mock the store under test
Store testing with Pinia: place all mock declarations at top level; use factory functions for module mocks; clear mocks between tests
Files:
web/__test__/components/SsoButton.test.ts
web/__test__/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests for the web app are located under web/test
Files:
web/__test__/components/SsoButton.test.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
web/__test__/components/SsoButton.test.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
web/__test__/components/SsoButton.test.ts
**/__test__/components/**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/web-testing-rules.mdc)
**/__test__/components/**/*.ts: Component tests should use mount from @vue/test-utils
Stub complex child components that are not the focus of a component test
Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Use createTestingPinia() to mock Pinia stores when testing Vue components
Test that expected elements are rendered and verify component output rather than internals
Test component interactions (clicks, inputs) using await element.trigger and setValue, and verify emitted events via wrapper.emitted()
Prefer semantic queries like find('button') or CSS selectors; avoid relying on data-test IDs unless necessary
Assert rendered text with wrapper.text(), check attributes via element.attributes(), and verify element existence with expect(element.exists()).toBe(true)
In component tests that use real store implementations, configure createTestingPinia with stubActions: false when you need real action behavior
Use findComponent(ComponentName) for locating child components and findAll for collections
Files:
web/__test__/components/SsoButton.test.ts
🧠 Learnings (18)
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should mock external dependencies and services
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Prefer semantic queries like find('button') or CSS selectors; avoid relying on data-test IDs unless necessary
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Component tests should use mount from vue/test-utils
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should stub complex child components not under test
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Use createTestingPinia() to mock Pinia stores when testing Vue components
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should use mount from Vue Test Utils
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Test component interactions (clicks, inputs) using await element.trigger and setValue, and verify emitted events via wrapper.emitted()
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Stub complex child components that are not the focus of a component test
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should use createTestingPinia() for mocking stores in components
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/**/*.{test,spec}.{ts,js,tsx,jsx} : Store testing with Pinia: place all mock declarations at top level; use factory functions for module mocks; clear mocks between tests
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/store/**/*.ts : In store tests, mock external dependencies used by the store and verify interactions with those mocks
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use vi.spyOn() to specify return values or behavior of methods under test
Applied to files:
web/__test__/components/SsoButton.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/mocks/**/*.ts : Frequently used mocks can also be placed under __test__/mocks
Applied to files:
web/__test__/components/SsoButton.test.ts
🔇 Additional comments (5)
web/__test__/components/SsoButton.test.ts (5)
123-155: LGTM! Well-structured test for single provider rendering.The test properly uses fake timers, waits for async operations, and verifies both the "or" separator text and the provider button text.
253-289: LGTM! Comprehensive token callback handling test.The test thoroughly validates token-based authentication flow including form manipulation, auto-submission, and URL cleanup.
291-331: LGTM! Proper error handling verification.The test correctly verifies error message display, form visibility preservation, and URL cleanup when authentication fails.
333-367: LGTM! Well-tested OAuth redirect flow.The test properly validates the redirect behavior when authorization code and state parameters are present.
369-411: LGTM! Good coverage of multiple provider scenarios.The test effectively validates rendering and interaction with multiple OIDC providers, including proper button count and text verification.
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (1)
web/components/RClone/RCloneConfig.vue (1)
54-55: Fix timer types for browser and clear timeouts to prevent leaksUsing NodeJS.Timeout in browser builds is brittle (setTimeout returns number). Also, pending timers should be cleared on unmount.
Apply this diff:
-let refetchTimeout: NodeJS.Timeout | null = null; +let refetchTimeout: ReturnType<typeof setTimeout> | null = null; @@ -let changeTimeout: NodeJS.Timeout | null = null; +let changeTimeout: ReturnType<typeof setTimeout> | null = null;And add unmount cleanup:
-import { computed, provide, ref, watch } from 'vue'; +import { computed, provide, ref, watch, onBeforeUnmount } from 'vue'; @@ ); +onBeforeUnmount(() => { + if (refetchTimeout) clearTimeout(refetchTimeout); + if (changeTimeout) clearTimeout(changeTimeout); +});Also applies to: 160-171
♻️ Duplicate comments (15)
api/docs/public/oidc-provider-examples.md (4)
455-461: Add language to fenced block (MD040) and include production HTTPS guidanceAlso, present both local and production redirect URIs here for consistency and visibility.
Apply this diff:
-``` -http://YOUR_UNRAID_IP:3001/graphql/api/auth/oidc/callback -``` - -Replace `YOUR_UNRAID_IP` with your actual server IP address. +```text +# Local development (HTTP): +http://YOUR_UNRAID_IP:3001/graphql/api/auth/oidc/callback + +# Production (HTTPS): +https://YOUR_DOMAIN_OR_IP/graphql/api/auth/oidc/callback +``` + +> ⚠️ Security Warning: Always use HTTPS in production to prevent token interception. HTTP should only be used for local development. + +Replace `YOUR_UNRAID_IP` or `YOUR_DOMAIN_OR_IP` with your actual server address.
108-111: Emphasize HTTPS for production in Google OAuth setupAdd a short warning right where the redirect URI is introduced.
Apply this diff:
- - Authorized JavaScript origins: `http://YOUR_UNRAID_IP:3001` - - Authorized redirect URIs: `http://YOUR_UNRAID_IP:3001/graphql/api/auth/oidc/callback` + - Authorized JavaScript origins: `http://YOUR_UNRAID_IP:3001` + - Authorized redirect URIs: + - Local: `http://YOUR_UNRAID_IP:3001/graphql/api/auth/oidc/callback` + - Production: `https://YOUR_DOMAIN_OR_IP/graphql/api/auth/oidc/callback` + > Use HTTPS for production deployments.
238-239: Azure AD: show both local and production redirect URIsAligns with earlier guidance and reduces misconfiguration.
Apply this diff:
- - Redirect URI: `http://YOUR_UNRAID_IP:3001/graphql/api/auth/oidc/callback` + - Redirect URI: + - Local: `http://YOUR_UNRAID_IP:3001/graphql/api/auth/oidc/callback` + - Production: `https://YOUR_DOMAIN_OR_IP/graphql/api/auth/oidc/callback`
276-277: Keycloak: include HTTPS production example for Valid Redirect URIsKeeps security guidance consistent across providers.
Apply this diff:
- - Valid Redirect URIs: `http://YOUR_UNRAID_IP:3001/graphql/api/auth/oidc/callback` + - Valid Redirect URIs: + - Local: `http://YOUR_UNRAID_IP:3001/graphql/api/auth/oidc/callback` + - Production: `https://YOUR_DOMAIN_OR_IP/graphql/api/auth/oidc/callback`api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts (2)
39-71: Deduplicate OidcAuthorizationRule and OidcAuthorizationRuleInput definitionsThe rule ObjectType and InputType share identical fields. Extract shared fields into a base class to reduce duplication and maintenance.
As previously suggested:
+abstract class OidcAuthorizationRuleBase { + @Field(() => String, { description: 'The claim to check (e.g., email, sub, groups, hd)' }) + @IsString() + @IsNotEmpty() + claim!: string; + @Field(() => AuthorizationOperator, { description: 'The comparison operator' }) + @IsEnum(AuthorizationOperator) + operator!: AuthorizationOperator; + @Field(() => [String], { description: 'The value(s) to match against' }) + @IsArray() + @IsString({ each: true }) + value!: string[]; +} @ObjectType() -export class OidcAuthorizationRule { - // duplicated fields... -} +export class OidcAuthorizationRule extends OidcAuthorizationRuleBase {} @InputType() -export class OidcAuthorizationRuleInput { - // duplicated fields... -} +export class OidcAuthorizationRuleInput extends OidcAuthorizationRuleBase {}
73-195: Deduplicate OidcProvider and OidcProviderInput field definitionsBoth classes repeat nearly all fields and decorators, violating DRY and increasing drift risk. Extract common fields into a base, and override differences (e.g., id scalar vs string, rule input vs object) as needed.
Pattern:
- OidcProviderBase with shared fields (name, clientId, clientSecret, issuer, endpoints, scopes, rules, ruleMode, button* fields, customAuthParams).
- OidcProvider extends base and overrides id with PrefixedID.
- OidcProviderInput extends base and overrides id as string; rules typed with OidcAuthorizationRuleInput.
If decorators differ, use protected members + getters with appropriate @field or split minimal overrides.
Also applies to: 197-319
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts (3)
236-266: Fix base64url decoding and redact PII when logging ID token claimsManual Buffer(..., 'base64') fails for base64url tokens and current logs include sensitive claims. Decode using base64url-safe normalization and log only claim keys.
- // Parse ID token to get user info + // Parse ID token to get user info let claims: JwtClaims | null = null; if (tokens.id_token) { try { - // Decode the JWT manually - const payload = tokens.id_token.split('.')[1]; - claims = JSON.parse(Buffer.from(payload, 'base64').toString()); - this.logger.debug(`ID token claims: ${JSON.stringify(claims)}`); - - // Log all claim types for debugging - if (claims) { - for (const [key, value] of Object.entries(claims)) { - const valueType = Array.isArray(value) ? 'array' : typeof value; - this.logger.debug( - `Claim '${key}': type=${valueType}, value=${JSON.stringify(value)}` - ); - - // Check for unexpected claim types early - if (valueType === 'object' && value !== null && !Array.isArray(value)) { - this.logger.error( - `unexpected JWT claim value encountered - claim '${key}' is complex object: ${JSON.stringify(value)}` - ); - } - } - } + // Decode the JWT payload (base64url-safe) + const payload = tokens.id_token.split('.')[1]!; + const normalized = payload + .replace(/-/g, '+') + .replace(/_/g, '/') + .padEnd(Math.ceil(payload.length / 4) * 4, '='); + claims = JSON.parse(Buffer.from(normalized, 'base64').toString('utf8')); + // Avoid logging PII; log only claim keys + this.logger.debug( + `ID token parsed; available claim keys: ${ + claims ? Object.keys(claims).join(', ') : 'none' + }` + ); } catch (e) { this.logger.warn(`Failed to parse ID token: ${e}`); } } else { this.logger.error('No ID token received from provider'); }
541-551: Make redirect URI configurable; drop hardcoded localhost and base URL logicRedirect handling should be driven by configuration to avoid fragile env detection and hardcoded ports/hosts.
private getRedirectUri(requestHost?: string): string { - // Always use the proxied path through /graphql to match production - if (requestHost && requestHost.includes('localhost')) { - // In development, use the Nuxt proxy at port 3000 - return `http://localhost:3000/graphql/api/auth/oidc/callback`; - } - - // In production, use the configured base URL with /graphql prefix - const baseUrl = this.configService.get('BASE_URL', 'http://tower.local'); - return `${baseUrl}/graphql/api/auth/oidc/callback`; + // Prefer explicit callback URL if provided + const configured = this.configService.get<string>('OIDC_CALLBACK_URL'); + if (configured) return configured; + // Fallback to BASE_URL + fixed path + const baseUrl = this.configService.get<string>('BASE_URL', 'http://tower.local'); + return `${baseUrl.replace(/\/$/, '')}/graphql/api/auth/oidc/callback`; }
473-525: Array claims are incorrectly handled (and array check is unreachable)The object check runs before Array.isArray, so the array branch is never hit. Also, array-valued claims (e.g., groups) should be supported, not rejected.
private evaluateRule(rule: OidcAuthorizationRule, claims: JwtClaims): boolean { const claimValue = claims[rule.claim]; this.logger.debug( `Evaluating rule for claim ${rule.claim}: ${JSON.stringify({ claimValue, claimType: typeof claimValue, ruleOperator: rule.operator, ruleValues: rule.value, })}` ); if (claimValue === undefined || claimValue === null) { this.logger.debug(`Claim ${rule.claim} not found in token`); return false; } - // Log detailed claim analysis - if (typeof claimValue === 'object' && claimValue !== null) { - this.logger.warn( - `unexpected JWT claim value encountered - claim ${rule.claim} is object type: ${JSON.stringify(claimValue)}` - ); - return false; - } - - if (Array.isArray(claimValue)) { - this.logger.warn( - `unexpected JWT claim value encountered - claim ${rule.claim} is array type: ${JSON.stringify(claimValue)}` - ); - return false; - } - - const value = String(claimValue); - this.logger.debug(`Processing claim ${rule.claim} with string value: "${value}"`); + // Normalize to a string array of values for uniform matching + const values: string[] = Array.isArray(claimValue) + ? (claimValue as unknown[]).map((v) => String(v)) + : [String(claimValue)]; + this.logger.debug( + `Processing claim ${rule.claim} with values: [${values.map((v) => `"${v}"`).join(', ')}]` + ); let result: boolean; switch (rule.operator) { case AuthorizationOperator.EQUALS: - result = rule.value.some((v) => value === v); + result = rule.value.some((v) => values.includes(v)); this.logger.debug( - `EQUALS check: "${value}" matches any of [${rule.value.join(', ')}]: ${result}` + `EQUALS check: any of [${values.join(', ')}] matches any of [${rule.value.join(', ')}]: ${result}` ); return result; case AuthorizationOperator.CONTAINS: - result = rule.value.some((v) => value.includes(v)); + result = values.some((val) => rule.value.some((v) => val.includes(v))); this.logger.debug( - `CONTAINS check: "${value}" contains any of [${rule.value.join(', ')}]: ${result}` + `CONTAINS check: any of [${values.join(', ')}] contains any of [${rule.value.join(', ')}]: ${result}` ); return result; case AuthorizationOperator.STARTS_WITH: - result = rule.value.some((v) => value.startsWith(v)); + result = values.some((val) => rule.value.some((v) => val.startsWith(v))); this.logger.debug( - `STARTS_WITH check: "${value}" starts with any of [${rule.value.join(', ')}]: ${result}` + `STARTS_WITH check: any of [${values.join(', ')}] starts with any of [${rule.value.join(', ')}]: ${result}` ); return result; case AuthorizationOperator.ENDS_WITH: - result = rule.value.some((v) => value.endsWith(v)); + result = values.some((val) => rule.value.some((v) => val.endsWith(v))); this.logger.debug( - `ENDS_WITH check: "${value}" ends with any of [${rule.value.join(', ')}]: ${result}` + `ENDS_WITH check: any of [${values.join(', ')}] ends with any of [${rule.value.join(', ')}]: ${result}` ); return result; default: this.logger.error(`Unknown authorization operator: ${rule.operator}`); return false; } }api/generated-schema.graphql (4)
2064-2066: Make id optional in OidcProviderInput to support create semanticsClients shouldn’t be forced to generate IDs. Optional id implies create; present id implies update.
input OidcProviderInput { """The unique identifier for the OIDC provider""" - id: String! + id: String
1466-1530: Do not expose clientSecret and implement Node on OidcProviderExposing clientSecret leaks sensitive credentials. Also, to align with schema consistency, OidcProvider should implement Node.
-type OidcProvider { +type OidcProvider implements Node { """The unique identifier for the OIDC provider""" id: PrefixedID! @@ - """OAuth2 client secret (if required by provider)""" - clientSecret: String + # clientSecret intentionally omitted from API output for security
1958-1969: Protect admin/internal OIDC queries with authorization directivesAdd @usePermissions to admin and internal queries to prevent unauthorized enumeration and probing. Leave public providers open.
"""Get public OIDC provider information for login buttons""" publicOidcProviders: [PublicOidcProvider!]! """Get all configured OIDC providers (admin only)""" -oidcProviders: [OidcProvider!]! +oidcProviders: [OidcProvider!]! @usePermissions(action: READ, resource: "SSO", possession: ANY) """Get a specific OIDC provider by ID""" -oidcProvider(id: String!): OidcProvider +oidcProvider(id: PrefixedID!): OidcProvider @usePermissions(action: READ, resource: "SSO", possession: ANY) """Validate an OIDC session token (internal use for CLI validation)""" -validateOidcSession(token: String!): OidcSessionValidation! +validateOidcSession(token: String!): OidcSessionValidation! @usePermissions(action: READ, resource: "SSO", possession: OWN)
2016-2020: Protect mutations and use consistent ID typeRestrict provider mutations to authorized users and use PrefixedID consistently.
"""Create or update an OIDC provider""" -upsertOidcProvider(provider: OidcProviderInput!): OidcProvider! +upsertOidcProvider(provider: OidcProviderInput!): OidcProvider! @usePermissions(action: UPDATE, resource: "SSO", possession: ANY) """Delete an OIDC provider""" -deleteOidcProvider(id: String!): Boolean! +deleteOidcProvider(id: PrefixedID!): Boolean! @usePermissions(action: DELETE, resource: "SSO", possession: ANY)web/composables/gql/graphql.ts (2)
1208-1241: Remove clientSecret from OidcProvider output type to prevent credential leaksExposing
clientSecretin an output type is a hard no. It will be visible via introspection and can be queried accidentally or maliciously. Restrict it to input-only, or return a masked/null value through a dedicated admin-only resolver type.Recommended actions:
- Schema: Remove
clientSecretfromOidcProvider(output) and, if absolutely necessary, create a separate admin-only payload (e.g.,OidcProviderSecretView) used only in mutation responses and always masked.- Resolver: If you cannot remove immediately, override the field resolver to always return
"***REDACTED***"ornull.- Frontend: Keep excluding
clientSecretin all queries (current OidcProvidersDocument does this correctly).Apply this minimal stopgap diff to avoid exposing secrets in the generated surface (note: update the backend schema; this file will regenerate accordingly):
export type OidcProvider = { __typename?: 'OidcProvider'; @@ /** OAuth2 client ID registered with the provider */ clientId: Scalars['String']['output']; - /** OAuth2 client secret (if required by provider) */ - clientSecret?: Maybe<Scalars['String']['output']>; /** Use custom parameter names for authorization (e.g., callbackUrl instead of redirect_uri) */ customAuthParams?: Maybe<Scalars['Boolean']['output']>;If you want, I can draft a schema/resolver refactor to implement this safely end-to-end. Do you want me to open a tracking issue?
1413-1421: Sanitize or constrain buttonStyle to prevent CSS injection
buttonStyleaccepts arbitrary CSS and is exposed in the public type. Without strict server-side sanitization or a whitelist, this is susceptible to style injection/abuse.Recommended:
- Backend: Sanitize
buttonStyleon write or read; alternatively replace with a safe, explicit enum/JSON of allowed tokens (e.g., colors, gradients, radius).- Frontend: Avoid binding raw
buttonStyledirectly tostyleuntil sanitization is guaranteed.Run this to verify whether any sanitization exists server-side:
#!/bin/bash # Search for buttonStyle handling and sanitization rg -n -A3 -B3 'buttonStyle' api/ | sed -n '1,200p' rg -n -A3 -B3 'sanitize.*buttonStyle|buttonStyle.*sanitize' api/ || true
🧹 Nitpick comments (25)
api/docs/public/images/README.md (4)
42-46: Fix bare URL (MD034) and clarify path formattingWrap the URL in angle brackets and avoid leading slash in the path reference to keep paths consistently repo-root relative and satisfy markdownlint.
Apply this diff:
-1. Navigate to the OIDC configuration page at http://localhost:3000 +1. Navigate to the OIDC configuration page at <http://localhost:3000> @@ -4. Place them in this directory (/api/docs/public/images/) +4. Place them in this directory (`api/docs/public/images/`)
30-35: Consistently format operator names as codeOperators read better and avoid ambiguity when wrapped in backticks.
Apply this diff:
- - Displays multiple rules with different operators (equals, contains, endsWith, startsWith) + - Displays multiple rules with different operators (`equals`, `contains`, `endsWith`, `startsWith`) @@ -- **Rule operators**: Demonstrate equals, contains, endsWith, startsWith +- **Rule operators**: Demonstrate `equals`, `contains`, `endsWith`, `startsWith`Also applies to: 49-56
5-17: Tighten phrasing and expectations for screenshotsMinor editorial improvements to reduce ambiguity: specify that “Simple authorization mode” is selected, and prefer “tabs visible” to ensure the correct UI state is captured.
Apply this diff:
- - Simple authorization mode selected + - Simple authorization mode selected (visible in the Authorization Mode dropdown) @@ - - Unraid.net provider tab selected + - Unraid.net provider tab selected (tab visible/active)
62-69: Add recommended image resolution guidanceHigh-DPI captures help readability in dark theme docs across devices.
Would you like me to add a short “Recommended resolution” note (e.g., capture at 2x, minimum width 1440px) here?
api/docs/public/oidc-provider-examples.md (7)
10-16: Fix nested list indentation (MD007)The nested entries under “Provider Configuration Examples” should be indented by two spaces to satisfy markdownlint.
Apply this diff:
-- [Provider Configuration Examples](#provider-configuration-examples) - - [Unraid.net Provider](#unraidnet-provider) - - [Google](#google) - - [Authelia](#authelia) - - [Microsoft/Azure AD](#microsoftazure-ad) - - [Keycloak](#keycloak) - - [Authentik](#authentik) - - [Okta](#okta) +- [Provider Configuration Examples](#provider-configuration-examples) + - [Unraid.net Provider](#unraidnet-provider) + - [Google](#google) + - [Authelia](#authelia) + - [Microsoft/Azure AD](#microsoftazure-ad) + - [Keycloak](#keycloak) + - [Authentik](#authentik) + - [Okta](#okta)
141-144: Avoid hotlinking external iconsLinking to
https://www.google.com/favicon.icocan break or violate terms. Prefer bundling the icon or referencing a locally hosted asset.Would you like me to add a note pointing to a locally stored icon under
api/docs/public/images/and update the example accordingly?
149-156: Clarify Google Workspace hd claim limitationsThe
hdclaim is only present/reliable when the account is part of a Google Workspace domain and the app/org policies allow it. Add a brief caveat.Proposed addition after the rule:
+Note: The `hd` claim is set for Google Workspace accounts. It may be absent for consumer accounts or if not included per your org’s policies. Verify tokens in your environment before enforcing this rule.Would you like me to apply this note?
431-435: Fix sub-list indentation (MD007) under Simple/Advanced fieldsIndentation for nested bullets under “Simple Authorization Fields” and “Advanced Authorization Fields” should be two spaces.
Apply this diff:
- - Helper text: "Users with emails ending in these domains can login" + - Helper text: "Users with emails ending in these domains can login" @@ - - Helper text: "Only these exact email addresses can login" + - Helper text: "Only these exact email addresses can login" @@ - - **Claim**: The JWT claim to check - - **Operator**: How to compare (equals, contains, endsWith, startsWith) - - **Value**: What to match against + - **Claim**: The JWT claim to check + - **Operator**: How to compare (`equals`, `contains`, `endsWith`, `startsWith`) + - **Value**: What to match againstAlso applies to: 443-446
449-452: Production hardening note for Developer SandboxIf
/graphqlexposes an interactive sandbox, call out that it should be disabled in production.Proposed addition:
- **Enable Developer Sandbox**: Toggle to enable GraphQL sandbox at `/graphql` + - For security, disable the sandbox in production environments.
198-203: Secrets handling: clarify hashed vs unhashed and rotationGood call noting “unhashed secret” for the Unraid config. Add a brief best-practice note about storing secrets securely and rotating regularly.
Proposed addition:
- **Client Secret**: Your unhashed secret + - Store and handle this secret securely. Rotate periodically and avoid committing it to source control.
517-518: Suggest adding a failure-handling hint to the testing flowIf the SSO button doesn’t appear or login fails, reference Troubleshooting to close the loop.
Proposed addition:
5. Click to test the login flow +6. If the button is missing or login fails, see the Troubleshooting section above.api/src/unraid-api/graph/utils/form-utils.ts (3)
24-35: Minor cleanups: use property shorthand and avoid emitting undefined options
- Prefer property shorthand for
scope.- Only include
optionswhen values are provided to avoidoptions: undefined/description: undefined.Apply this diff:
{ type: 'Label', text: label, - options: { - description, - }, + options: description ? { description } : undefined, } as LabelElement, { type: 'Control', - scope: scope, - options: controlOptions, + scope, + ...(controlOptions ? { options: controlOptions } : {}), } as ControlElement,
39-39: Remove obvious comment per guidelinesThe “Add rule if provided” comment states the obvious and can be removed to align with the repo guideline on avoiding unnecessary comments.
Apply this diff:
- // Add rule if provided
101-110: Broadenelementstyping forcreateAccordionLayoutto increase flexibilityLimiting accordion items to
Layoutprevents using non-layout UI schema elements (e.g., a single Control). If your renderer supports it, consider widening toUISchemaElementto make this helper more broadly applicable.Apply this diff:
- elements: Array< - Layout & { - options?: Layout['options'] & { + elements: Array< + UISchemaElement & { + options?: UISchemaElement['options'] & { accordion?: { title?: string; description?: string; }; }; } >;Additionally, add
UISchemaElementto the import (outside the selected range):// at line 1 import type { ControlElement, LabelElement, Layout, Rule, UISchemaElement } from '@jsonforms/core';unraid-ui/src/forms/config.ts (2)
20-26: AJV options: consider removing unsupported options and enabling formatscreateAjv already configures Ajv for JSON Forms. The verbose option is no longer supported in Ajv v8; keep allErrors and strict, and ensure formats are enabled. JSON Forms’ createAjv typically adds ajv-formats, so no extra work needed if you rely on that. If you do need custom formats/keywords, expose a hook here.
Apply this diff to drop verbose and clarify intent:
export function createJsonFormsAjv(): JsonFormsAjv { return createAjv({ allErrors: true, - verbose: true, - strict: false, + // Use strict mode for better diagnostics; switch to 'false' only if necessary + strict: true, }); }
32-41: Freeze and re-use the shared instances to avoid accidental mutationGiven these are global singletons used across many forms, freezing helps prevent accidental runtime mutation of shared config.
Apply this diff:
-export const jsonFormsAjv: JsonFormsAjv = createJsonFormsAjv(); +export const jsonFormsAjv: JsonFormsAjv = createJsonFormsAjv(); export const defaultJsonFormsConfig: JsonFormsConfig = { restrict: false, trim: false, ajv: jsonFormsAjv, }; + +Object.freeze(jsonFormsAjv); +Object.freeze(defaultJsonFormsConfig);web/components/RClone/RCloneConfig.vue (1)
75-81: Guard noisy console logs behind dev checksUnscoped console logging can clutter production logs. Wrap logs with a dev guard or remove.
Apply this diff:
- console.log('[RCloneConfig] providerType changed:', newValue.type); + if (import.meta.env?.DEV) { + console.log('[RCloneConfig] providerType changed:', newValue.type); + } @@ - console.log('[RCloneConfig] Refetching form schema'); + if (import.meta.env?.DEV) { + console.log('[RCloneConfig] Refetching form schema'); + } @@ - console.log('[RCloneConfig] onChange received data:', JSON.stringify(data)); + if (import.meta.env?.DEV) { + console.log('[RCloneConfig] onChange received data:', JSON.stringify(data)); + }Also applies to: 169-170
web/components/ConnectSettings/ConnectSettings.ce.vue (2)
83-86: Reuse centralized default JSON Forms config to avoid driftYou already share Ajv; do the same for JSON Forms config to keep behavior consistent across forms.
Apply this diff:
-import { BrandButton, jsonFormsRenderers, jsonFormsAjv, Label, SettingsGrid } from '@unraid/ui'; +import { BrandButton, jsonFormsRenderers, jsonFormsAjv, defaultJsonFormsConfig, Label, SettingsGrid } from '@unraid/ui'; @@ -const jsonFormsConfig = { - restrict: false, - trim: false, -}; +const jsonFormsConfig = defaultJsonFormsConfig;Ajv binding looks correct:
<JsonForms ... :ajv="jsonFormsAjv" />Also applies to: 11-13, 118-129
91-96: Optional: remove or guard console logsLeaving console logs in a CE might propagate to host pages. Guard or remove to reduce noise.
- console.log('[ConnectSettings] trying to update settings to', formState.value); + if (import.meta.env?.DEV) { + console.log('[ConnectSettings] trying to update settings to', formState.value); + }unraid-ui/src/forms/AccordionLayout.vue (1)
60-84: Use the host-provided Ajv if available, fallback to shared ajvSome forms may pass a custom Ajv via JsonForms config. Respect it before falling back to the shared instance.
Apply this diff:
-import { jsonFormsAjv } from '@/forms/config'; +import { jsonFormsAjv } from '@/forms/config'; @@ - // Filter elements based on visibility rules + // Filter elements based on visibility rules return allElements.filter((element: UISchemaElement & Record<string, unknown>) => { @@ - try { + try { // Get the root data from JSONForms context for rule evaluation const rootData = jsonFormsContext?.core?.data || {}; const formData = props.data || layout.data || rootData; const formPath = props.path || layout.path || ''; - - const visible = isVisible(element as UISchemaElement, formData, formPath, jsonFormsAjv); + // Prefer ajv from layout/config if provided, else use shared instance + const ajvFromConfig = (layout as unknown as { config?: { ajv?: unknown } })?.config?.ajv; + const ajvToUse = (ajvFromConfig as unknown) || jsonFormsAjv; + const visible = isVisible(element as UISchemaElement, formData, formPath, ajvToUse as any); return visible;api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts (1)
61-66: Testing private methods via casting is brittleThese unit tests reach into private methods via
(service as any). Prefer covering behavior through the public method (checkAuthorization) to avoid white-box coupling.unraid-ui/src/forms/renderers.ts (1)
58-73: Broaden string schema detection to cover common optional patternsCurrent anyOf detection only matches minLength=1 and maxLength=0. Optional string patterns often use const '' or nullable. Consider supporting those to increase renderer applicability.
const isStringOrAnyOfString = (schema: JsonSchema): boolean => { if (!schema || typeof schema !== 'object' || Array.isArray(schema)) return false; // Exclude enum fields - they should use select renderer if (schema.enum) return false; // Handle direct string type (but not enums) if (schema.type === 'string') return true; - // Handle anyOf with all string types (for optional URL fields) - if (schema.anyOf && Array.isArray(schema.anyOf) && schema.anyOf.length === 2) { - // Check if it's the pattern we expect: [{type: 'string', minLength: 1}, {type: 'string', maxLength: 0}] - const hasMinLength = schema.anyOf.some((s: JsonSchema) => s.type === 'string' && s.minLength === 1); - const hasMaxLength = schema.anyOf.some((s: JsonSchema) => s.type === 'string' && s.maxLength === 0); - return hasMinLength && hasMaxLength; - } + // Handle common optional string patterns via anyOf/oneOf: + const list = (schema.anyOf ?? schema.oneOf) as JsonSchema[] | undefined; + if (Array.isArray(list) && list.length >= 2) { + const hasString = list.some((s) => s && typeof s === 'object' && !Array.isArray(s) && s.type === 'string'); + const hasEmptyConst = list.some((s: any) => s && typeof s === 'object' && s.const === ''); + const hasEmptyMaxLen = list.some((s: any) => s && typeof s === 'object' && s.type === 'string' && s.maxLength === 0); + return hasString && (hasEmptyConst || hasEmptyMaxLen); + } return false; };web/composables/gql/graphql.ts (3)
1460-1471: Avoid duplicative access paths for OIDC providers (Query.oidcProviders vs Settings.sso.oidcProviders)Two surfaces expose provider lists: root
oidcProvidersandsettings.sso.oidcProviders. This invites divergent permissions/caching and client confusion.Propose deprecating one path (likely the root field) and consolidating on a single canonical field with clear access control and caching semantics. Mark the deprecated field in schema and provide a migration note in developer docs.
1106-1109: Confirm create semantics for upsert: OidcProviderInput.id is required
upsertOidcProviderrequiresprovider.id: string. For pure create flows, is the ID user-defined (e.g., slug)? If backend generates IDs, consider makingidoptional on create or document that clients must supply it.If creation requires a user-defined key, document allowed charset/length and collision behavior in the schema description.
1251-1258: Model buttonVariant as an enum to prevent invalid values
buttonVariant?: Stringshifts validation to clients. Converting to a GraphQL enum aligned with Reka UI variants prevents invalid inputs and enables better tooling.Example:
enum ButtonVariant { DEFAULT OUTLINE GHOST LINK # ...align with supported Reka UI variants } # In OidcProvider / OidcProviderInput: buttonVariant: ButtonVariantRegenerate types after schema change.
api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.test.ts
Outdated
Show resolved
Hide resolved
…mentation - Eliminated the customAuthParams field from the OidcProvider and OidcProviderInput types in the GraphQL schema. - Updated the OidcAuthService to use standard OAuth2 parameters, removing the handling for custom parameter names. - Adjusted related documentation comments for clarity and consistency. - Modified REST controller routes to streamline OIDC authorization and callback handling based on the environment.
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (10)
api/generated-schema.graphql (4)
2058-2128: Makeidoptional and usePrefixedIDin OidcProviderInputFor upsert flows, clients shouldn’t be forced to pre-generate IDs. Use:
- absent id → create
- present id → update
Also align input type with the
PrefixedIDscalar used across the API.input OidcProviderInput { """The unique identifier for the OIDC provider""" - id: String! + id: PrefixedID @@ """OAuth2 client secret (if required by provider)""" clientSecret: StringOptional follow-up: If you expect clients to use human-readable slugs (e.g., "google"), keep
Stringbut still make it optional and document it clearly.
1466-1525: Do not expose clientSecret; and implement Node for OidcProviderLeaking
clientSecretin the public type is a security risk (easy accidental disclosure). Also, top-level resources in this API generally implementNode. Align OIDC with that convention.Apply the following schema changes:
-type OidcProvider { +type OidcProvider implements Node { """The unique identifier for the OIDC provider""" id: PrefixedID! @@ - """OAuth2 client secret (if required by provider)""" - clientSecret: String + """ + Indicates whether a client secret has been configured. + The actual secret value is never returned by the API. + """ + hasClientSecret: Boolean!Follow-up:
- Ensure resolvers omit the secret from responses and compute
hasClientSecretfrom server-side storage.- If UI needs to allow rotating the secret, keep
clientSecretonly in the input type (as already done).
1953-1963: Add permission guards to OIDC queries and fix argument typeAdmin/config queries should be protected. Also keep
oidcProviderargument consistent with thePrefixedIDused on the type."""Get public OIDC provider information for login buttons""" publicOidcProviders: [PublicOidcProvider!]! """Get all configured OIDC providers (admin only)""" - oidcProviders: [OidcProvider!]! + oidcProviders: [OidcProvider!]! @usePermissions(action: READ, resource: "SSO", possession: ANY) """Get a specific OIDC provider by ID""" - oidcProvider(id: String!): OidcProvider + oidcProvider(id: PrefixedID!): OidcProvider @usePermissions(action: READ, resource: "SSO", possession: ANY) """Validate an OIDC session token (internal use for CLI validation)""" - validateOidcSession(token: String!): OidcSessionValidation! + validateOidcSession(token: String!): OidcSessionValidation! @usePermissions(action: READ, resource: "SSO", possession: OWN)Note:
publicOidcProvidersremains public intentionally.
2011-2015: Protect mutations with permissions and use PrefixedID for deleteMutations must be admin-only, and ID types should be consistent.
"""Create or update an OIDC provider""" - upsertOidcProvider(provider: OidcProviderInput!): OidcProvider! + upsertOidcProvider(provider: OidcProviderInput!): OidcProvider! @usePermissions(action: UPDATE, resource: "SSO", possession: ANY) """Delete an OIDC provider""" - deleteOidcProvider(id: String!): Boolean! + deleteOidcProvider(id: PrefixedID!): Boolean! @usePermissions(action: DELETE, resource: "SSO", possession: ANY)api/src/unraid-api/rest/rest.controller.ts (6)
68-76: Validate providerId to prevent malformed input reaching the serviceAdd a simple allowlist-based check for providerId (defense in depth) before invoking the service.
try { if (!state) { return res.status(400).send('State parameter is required'); } + // Validate providerId format (defense in depth) + if (!/^[A-Za-z0-9._-]{1,64}$/.test(providerId)) { + return res.status(400).send('Invalid provider ID format'); + }
78-81: Honor reverse proxy headers when deriving host for authorizeBehind proxies, req.headers.host may be wrong. Prefer X-Forwarded-Host if present (mirrors your callback handler).
- // Get the host from the request headers - const host = req.headers.host || undefined; + // Get the host from reverse-proxy-aware headers + const host = + (req.headers['x-forwarded-host'] as string) || + req.headers.host || + undefined;
115-118: Ensure state is server-generated and cryptographically validatedController accepts client-supplied state and passes it through. Please verify that OidcAuthService signs or stores/validates state robustly (prevents CSRF/state-fixation).
#!/bin/bash # Inspect OidcAuthService for secure state generation and verification fd -a 'oidc-auth.service.ts' | while read -r f; do echo "----- $f -----" rg -n -C3 $'class\\s+OidcAuthService\\b|getAuthorizationUrl\\(|handleCallback\\(|extractProviderFromState\\(|state|nonce|verify|sign|hmac|jose|jwt|pkce|code_verifier' "$f" doneAlso applies to: 121-121
139-145: Prevent caching of the redirect that carries the token (even in fragment)Add no-store to avoid intermediaries caching the redirect.
// Manually set redirect headers for better proxy compatibility res.status(302); + res.header('Cache-Control', 'no-store'); res.header('Location', loginUrl); return res.send();
82-82: Do not log full authorization URL (contains sensitive query like state/nonce)Logging the full URL can leak state/nonce/client_id to logs. Log only safe context.
- this.logger.log(`Redirecting to OIDC provider: ${authUrl}`); + this.logger.log(`Redirecting to OIDC provider ${providerId}`);
123-129: Avoid logging the full callback URL (contains code and state)Log only a sanitized path to prevent leaking authorization code and state.
- this.logger.debug(`Full callback URL from request: ${fullUrl}`); + const { pathname } = new URL(fullUrl); + this.logger.debug(`OIDC callback received for provider ${providerId} at path: ${pathname}`);
🧹 Nitpick comments (4)
api/generated-schema.graphql (3)
1447-1465: Authorization rules DSL is clear and minimalThe rule and operator modeling is straightforward and flexible.
If you have a canonical set of supported claim names (e.g., email, sub, groups, hd), consider documenting them in the description to reduce guesswork for clients.
1535-1538: Consider adding an error reason for invalid sessionsOptional: add an error code/message to aid debugging (e.g., expired, signature_invalid, audience_mismatch). Keeps UI/CLI from guessing.
If you want, I can propose a minimal
reason: Stringorcode: Stringaddition with a short list of canonical values.
1540-1547: Public provider surface is minimal and safeOnly UI-facing fields are exposed, which is good. Consider clarifying the semantics of
id(slug vs. internal id) in the description.api/src/unraid-api/rest/rest.controller.ts (1)
159-163: Also add no-store to error redirectPrevents caching of error redirect pages that include error context in the fragment or query.
res.status(302); + res.header('Cache-Control', 'no-store'); res.header('Location', loginUrl); return res.send();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**
📒 Files selected for processing (5)
api/dev/configs/oidc.json(1 hunks)api/generated-schema.graphql(6 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts(1 hunks)api/src/unraid-api/rest/rest.controller.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/src/unraid-api/graph/resolvers/sso/oidc-auth.service.ts
- api/dev/configs/oidc.json
- api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/rest/rest.controller.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/rest/rest.controller.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/rest/rest.controller.ts
🧠 Learnings (1)
📚 Learning: 2024-11-15T16:22:12.460Z
Learnt from: elibosley
PR: unraid/api#942
File: api/src/unraid-api/auth/auth.module.ts:94-96
Timestamp: 2024-11-15T16:22:12.460Z
Learning: In `CookieService.defaultOpts()` in `api/src/unraid-api/auth/cookie.service.ts`, use `sameSite: 'lax'` for session cookie configuration.
Applied to files:
api/src/unraid-api/rest/rest.controller.ts
🧬 Code Graph Analysis (1)
api/src/unraid-api/rest/rest.controller.ts (2)
api/src/unraid-api/auth/public.decorator.ts (1)
Public(4-4)api/src/unraid-api/types/fastify.ts (2)
FastifyRequest(25-30)FastifyReply(33-33)
🔇 Additional comments (8)
api/generated-schema.graphql (4)
1403-1408: SSO settings type looks good and follows Node conventionSensible separation of SSO config under Settings;
implements Nodekeeps it consistent with the rest of the schema.
1430-1431: Good addition: warnings channel in update responseNon-blocking field for config validation warnings is useful and won’t break existing clients.
1440-1442: Settings.sso exposure is reasonableSurface looks correct; assuming the query-level auth for
settingsis already enforced server-side.If
settingsrequires admin privileges elsewhere, consider documenting that in the schema description for thesettingsquery to avoid misuse by clients. Do you want a PR to add a short note?
1527-1534: Rule mode enum is appropriateNaming and semantics are clear.
api/src/unraid-api/rest/rest.controller.ts (4)
1-1: ESM import specifiers with .js extensions look goodImports comply with our ESM guideline. No action needed.
Also applies to: 8-8
14-17: Constructor injection of OidcAuthService is correct and consistentDependency is injected cleanly; visibility and readonly usage look good.
89-99: Good: generic error response for authorizeError path returns a generic message and avoids leaking details. Nice.
147-157: Good: sanitized error handling in callbackOnly generic messages are exposed; detailed info is logged at debug level. This addresses prior concerns about leaking internals.
- Changed the id field type for the oidcProvider query and related mutations from String to PrefixedID for better type safety. - Removed the OidcProviderInput and OidcAuthorizationRuleInput types from the schema, streamlining the OIDC provider management. - Adjusted the OIDC provider resolver to reflect the updated schema and removed unnecessary mutation methods for upserting and deleting OIDC providers.
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Eliminated the delete and upsert mutations for OIDC providers from the GraphQL schema to simplify the API. - Removed associated input types and comments to streamline the OIDC provider management. - Updated the SSO resolver to include permission checks for validating OIDC sessions.
- Added a warning to the OIDC provider setup documentation emphasizing the importance of using HTTPS for production redirect URIs to ensure secure communication and prevent credential interception.
- Eliminated the customAuthParams field from the OIDC provider configuration in the OidcConfigPersistence class to streamline the provider setup. - Updated related logic to ensure compatibility with the revised configuration structure.
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts (1)
96-101: Add authorization to validateOidcSession (security)This endpoint is described as “internal use,” but it currently lacks explicit access control. If exposed, it could be abused to probe token validity. Protect it with permissions (preferred) or mark it @public only if you’ve explicitly designed for unauthenticated CLI usage and considered rate-limiting and token characteristics (short TTL, one-time use).
Recommended fix:
@Query(() => OidcSessionValidation, { description: 'Validate an OIDC session token (internal use for CLI validation)', }) - public async validateOidcSession(@Args('token') token: string): Promise<OidcSessionValidation> { + @UsePermissions({ + action: AuthActionVerb.READ, + resource: 'sso', + possession: AuthPossession.OWN, + }) + public async validateOidcSession(@Args('token') token: string): Promise<OidcSessionValidation> { return await this.oidcSessionService.validateSession(token); }api/generated-schema.graphql (2)
1959-1961: ID type consistency resolved for oidcProvider argumentThe argument now uses PrefixedID!, matching the OidcProvider.id field. This addresses the prior mismatch.
1962-1963: validateOidcSession is publicly visible in schema; ensure it’s guardedAs generated, it has no directive and the resolver currently lacks an authorization decorator. This endpoint should be restricted (e.g., @usePermissions READ/OWN on SSO/session) unless explicitly designed to be public with compensating controls.
🧹 Nitpick comments (2)
api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts (1)
34-62: Re-evaluate “must-have rules” gating for public visibilityCurrently, providers with no authorizationRules are hidden from the login page. If “no rules” is intended to mean “unrestricted,” this would inadvertently hide legitimate providers. Consider either:
- Treating empty/missing rules as “visible” and only validate rule completeness when rules are present, or
- Making this gating configurable (e.g., a visiblePublicly flag), or pushing the policy into OidcConfigPersistence to centralize visibility logic.
Suggested in-place tweak (if “no rules” should be allowed):
- const providersWithRules = providers.filter((provider) => { - // Check if provider has authorization rules - if (!provider.authorizationRules || provider.authorizationRules.length === 0) { - this.logger.debug( - `Hiding provider ${provider.id} from login page - no authorization rules configured` - ); - return false; - } - - // Check if at least one rule is complete and valid - const hasValidRules = provider.authorizationRules.some( + const providersWithRules = providers.filter((provider) => { + const rules = provider.authorizationRules; + // No rules configured -> allow visibility; rely on downstream auth where applicable + if (!rules || rules.length === 0) { + return true; + } + + // Check if at least one rule is complete and valid + const hasValidRules = rules.some( (rule) => rule.claim && // Has a claim specified rule.operator && // Has an operator specified rule.value && // Has values array rule.value.length > 0 && // Has at least one value rule.value.some((v) => v && v.trim() !== '') // At least one non-empty value );api/generated-schema.graphql (1)
1956-1958: Admin query appears without @usePermissions directive in schema outputThe resolvers do enforce permissions, but the generated schema doesn’t show @usePermissions. If you intend for clients to see directives in SDL (e.g., for tooling or docs), ensure your code-first directive mapping is configured to emit them during schema generation. Otherwise, this is acceptable since runtime guards exist.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**
📒 Files selected for processing (5)
api/generated-schema.graphql(4 hunks)api/src/__test__/graphql/resolvers/rclone-api.service.test.ts(11 hunks)api/src/unraid-api/graph/resolvers/rclone/rclone-api.service.ts(3 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/graph/resolvers/sso/oidc-provider.model.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/graph/resolvers/sso/sso.resolver.tsapi/src/__test__/graphql/resolvers/rclone-api.service.test.tsapi/src/unraid-api/graph/resolvers/rclone/rclone-api.service.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/sso/sso.resolver.tsapi/src/unraid-api/graph/resolvers/rclone/rclone-api.service.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/graph/resolvers/sso/sso.resolver.tsapi/src/__test__/graphql/resolvers/rclone-api.service.test.tsapi/src/unraid-api/graph/resolvers/rclone/rclone-api.service.ts
api/**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Prefer not to mock simple dependencies in API tests
Files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
🧠 Learnings (11)
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/store/**/*.ts : In store tests, mock external dependencies used by the store and verify interactions with those mocks
Applied to files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/components/**/*.ts : Mock external dependencies and services in component tests (e.g., vi.mock for helper modules)
Applied to files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to **/__test__/mocks/**/*.ts : Frequently used mocks can also be placed under __test__/mocks
Applied to files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to api/**/*.{test,spec}.{ts,tsx,js,jsx} : Prefer not to mock simple dependencies in API tests
Applied to files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/__test__/**/*.{test,spec}.{ts,js,tsx,jsx} : Web (Vue) component tests should mock external dependencies and services
Applied to files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
📚 Learning: 2025-08-11T15:06:30.877Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T15:06:30.877Z
Learning: Applies to web/**/*.{test,spec}.{ts,js,tsx,jsx} : Store testing with Pinia: place all mock declarations at top level; use factory functions for module mocks; clear mocks between tests
Applied to files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Applied to files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Applied to files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Applied to files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Applied to files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: Applies to {**/*.test.ts,**/__test__/{components,store}/**/*.ts} : Use vi.spyOn() to specify return values or behavior of methods under test
Applied to files:
api/src/__test__/graphql/resolvers/rclone-api.service.test.ts
🧬 Code Graph Analysis (2)
api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts (4)
api/src/unraid-api/cli/generated/graphql.ts (4)
Query(1429-1485)PublicOidcProvider(1409-1417)OidcProvider(1207-1239)OidcSessionValidation(1274-1278)web/composables/gql/graphql.ts (4)
Query(1433-1489)PublicOidcProvider(1413-1421)OidcProvider(1207-1241)OidcSessionValidation(1278-1282)api/src/unraid-api/auth/public.decorator.ts (1)
Public(4-4)web/components/sso/useSsoProviders.ts (1)
OidcProvider(5-12)
api/src/unraid-api/graph/resolvers/rclone/rclone-api.service.ts (1)
api/src/environment.ts (1)
ENVIRONMENT(81-83)
🔇 Additional comments (17)
api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts (5)
1-16: ESM import specifiers look correctAll TypeScript imports use .js extensions and the file resides under api/src/unraid-api/, matching the repo’s ESM and placement guidelines.
27-33: Public query surface for login buttons is appropriateMarking publicOidcProviders as @public and returning a constrained PublicOidcProvider shape avoids leaking sensitive fields while serving the UI needs.
64-71: Mapping to public shape is minimal and safeOnly UI-facing fields are returned; no config or secret data is exposed via this query.
74-83: Admin listing is properly protected@UsePermissions on oidcProviders aligns with admin-only expectations. Looks good.
84-94: ID type consistency and access control are correctUsing PrefixedID for the id argument and enforcing permissions on the single-provider query is consistent and secure.
api/generated-schema.graphql (2)
1403-1408: SsoSettings type addition is aligned with designSsoSettings implements Node and exposes oidcProviders for the settings surface. Matches the PR intent.
1440-1442: Settings.sso field surfaces SSO under Settings as expectedClear integration point for SSO in the unified Settings surface.
api/src/unraid-api/graph/resolvers/rclone/rclone-api.service.ts (3)
13-13: LGTM! Proper import for environment-based configuration.The import follows the correct ESM syntax with .js extension as required by the coding guidelines.
48-53: Production guard implementation looks correct.The environment-based early return properly prevents RClone initialization in production environments and correctly sets the initialized state to false.
366-368: URL construction normalization is well-implemented.The endpoint normalization correctly handles both cases - endpoints with and without leading slashes - ensuring consistent URL formation for Unix socket communication. This addresses the potential issue where base URLs ending with
:could create malformed URLs.api/src/__test__/graphql/resolvers/rclone-api.service.test.ts (7)
37-45: Environment and file system mocks properly configured.The mocks correctly simulate the development environment and file existence checks needed for the tests. The environment mock ensures the production guard doesn't interfere with test execution.
75-75: Dynamic import follows proper patterns.The dynamic import for file-exists utility is consistent with other mocked modules and properly typed.
82-87: Mock setup properly simulates RClone socket initialization.The mock configuration correctly simulates a successful socket check with a PID response and ensures file existence checks pass, enabling proper test isolation.
102-103: Good practice: Mock reset after initialization.Clearing the mock after initialization prevents test interference while preserving the initialization behavior needed for service setup.
120-120: URL patterns correctly updated to reflect endpoint normalization.All test expectations properly validate the new URL format without leading slashes after the Unix socket path. The URLs consistently use the pattern
http://unix:/tmp/rclone.sock:endpointwhich matches the service implementation.Also applies to: 152-152, 183-183, 222-222, 252-252, 278-278, 305-305, 332-332, 357-357
125-127: Authorization headers correctly validated.The tests properly validate the presence of Basic authentication headers using regex matching, which ensures the authentication mechanism is working without exposing sensitive credentials in test output.
Also applies to: 157-159, 188-190, 231-233, 260-262, 283-285, 314-316, 337-339, 362-364
124-124: Request options properly validated.All tests correctly validate the essential request options including
enableUnixSockets: trueandresponseType: 'json', ensuring the HTTP client is configured correctly for Unix socket communication.Also applies to: 156-156, 187-187, 229-229, 258-258, 281-281, 312-312, 335-335, 360-360
- Updated the timeout for the CloudService.hardCheckCloud integration test to 10 seconds to accommodate longer execution times and improve test reliability.
- Introduced a new dynamic introspection plugin to control introspection queries based on sandbox mode. - Updated the GraphQL server configuration to utilize the new introspection plugin and adjusted the introspection setting to always return true. - Refactored the settings service to simplify the update logic and removed unnecessary comments for clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (13)
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts (8)
99-127: Avoid mutating providers array when ensuring the default providerUnshifting mutates the array returned by configService.get(). Clone first and build a new array to prevent side effects.
Apply this diff:
- const config = this.configService.get<OidcConfig>(this.configKey()); - const providers = config?.providers || []; + const config = this.configService.get<OidcConfig>(this.configKey()); + const providers = [...(config?.providers || [])]; @@ - const hasUnraidNet = providers.some((p) => p.id === 'unraid.net'); - if (!hasUnraidNet) { - providers.unshift(this.getUnraidNetSsoProvider()); - } + const hasUnraidNet = providers.some((p) => p.id === 'unraid.net'); + const withDefault = hasUnraidNet + ? providers + : [this.getUnraidNetSsoProvider(), ...providers]; @@ - const updatedProviders = providers.map((provider) => { + const updatedProviders = withDefault.map((provider) => {
129-139: Clean all optional string fields consistently (jwksUri/buttonText/buttonVariant missing)Currently missing cleaning for jwksUri, buttonText, and buttonVariant. Trim and set to undefined for empty strings to keep serialization consistent.
Apply this diff:
private cleanProvider(provider: OidcProvider): OidcProvider { // Convert empty strings to undefined for optional fields return { ...provider, clientSecret: provider.clientSecret?.trim() || undefined, authorizationEndpoint: provider.authorizationEndpoint?.trim() || undefined, tokenEndpoint: provider.tokenEndpoint?.trim() || undefined, + jwksUri: provider.jwksUri?.trim() || undefined, buttonIcon: provider.buttonIcon?.trim() || undefined, + buttonText: provider.buttonText?.trim() || undefined, + buttonVariant: provider.buttonVariant?.trim() || undefined, buttonStyle: provider.buttonStyle?.trim() || undefined, }; }
146-149: Accept resolver input type in upsertProvider to avoid unsafe castsAlign the service API with the resolver input type to centralize normalization and avoid casts upstream.
Add the missing import near the top:
+import { OidcProviderInput } from '@app/unraid-api/graph/resolvers/sso/oidc-provider.model.js';Then change the signature:
- async upsertProvider( - provider: OidcProvider & { authorizationMode?: string; simpleAuthorization?: any } - ): Promise<OidcProvider> { + async upsertProvider( + provider: OidcProviderInput & { authorizationMode?: string; simpleAuthorization?: any } + ): Promise<OidcProvider> {
955-971: UI show-rule condition compares lowercase 'and'If users set AND mode, the advanced section will not show because the const is lowercase. Use uppercase to match enum.
Apply this diff:
{ scope: '#/properties/authorizationRuleMode', - schema: { const: 'and' }, + schema: { const: 'AND' }, },
158-174: authorizationRuleMode is not persistedThe cleanedProvider mapping omits authorizationRuleMode; changes to AND/OR will be lost.
Apply this diff:
const cleanedProvider: OidcProvider = { id: provider.id, name: provider.name, clientId: provider.clientId, clientSecret: provider.clientSecret, issuer: provider.issuer, authorizationEndpoint: provider.authorizationEndpoint, tokenEndpoint: provider.tokenEndpoint, jwksUri: provider.jwksUri, scopes: provider.scopes, authorizationRules: provider.authorizationRules, + authorizationRuleMode: provider.authorizationRuleMode, buttonText: provider.buttonText, buttonIcon: provider.buttonIcon, buttonVariant: provider.buttonVariant, buttonStyle: provider.buttonStyle, };
310-317: Built-in issuer skip check is incorrect (compares to 'unraid.net' instead of account.unraid.net)The current check never skips validation for the built-in Unraid provider. Parse the hostname and allow account.unraid.net (and optionally subdomains) to avoid false warnings.
Apply this diff:
- // Parse the issuer URL and check if hostname is exactly 'unraid.net' - const issuerUrl = new URL(provider.issuer); - if (issuerUrl.hostname === 'unraid.net') { - // Skip validation for unraid.net as it uses custom auth flow - continue; - } + // Parse the issuer URL and skip validation for built-in Unraid issuer(s) + const issuerUrl = new URL(provider.issuer); + const host = issuerUrl.hostname; + const isUnraidIssuer = + host === 'account.unraid.net' || host.endsWith('.unraid.net'); + if (isUnraidIssuer) { + // Skip validation for Unraid as it uses a custom auth flow + continue; + }
574-581: Rule mode enum must match GraphQL enum (uppercase) to round-trip correctlyLowercase values will not match AuthorizationRuleMode enum and will break UI round-trip and persistence.
Apply this diff:
authorizationRuleMode: { type: 'string', title: 'Rule Mode', - enum: ['or', 'and'], - default: 'or', + enum: ['OR', 'AND'], + default: 'OR', description: 'How to evaluate multiple rules: OR (any rule passes) or AND (all rules must pass)', },
593-596: Operator enum must match GraphQL enum (uppercase) to avoid validation mismatchLowercase operators won’t match AuthorizationOperator and will fail rule editing/evaluation.
Apply this diff:
operator: { type: 'string', title: 'Operator', - enum: ['equals', 'contains', 'endsWith', 'startsWith'], + enum: ['EQUALS', 'CONTAINS', 'ENDS_WITH', 'STARTS_WITH'], },web/composables/gql/graphql.ts (2)
1185-1216: Do not expose clientSecret on the OidcProvider output typeEven if not selected in admin queries, exposing clientSecret on the public type is a footgun. Remove it from the schema or always resolve to null/REDACTED, then regenerate this file.
I can open a follow-up to adjust the backend GraphQL schema/resolvers to omit or mask clientSecret and regenerate the frontend types accordingly.
2590-2590: Drop buttonStyle from public providers query until backend sanitization is enforcedAvoid passing arbitrary CSS to the client from a public endpoint. Remove buttonStyle from the PublicOidcProviders operation and regenerate this file.
Update the source operation (web/components/queries/public-oidc-providers.query.ts):
query PublicOidcProviders { publicOidcProviders { id name buttonText buttonIcon buttonVariant - buttonStyle } }Then regenerate GraphQL types so this generated DocumentNode reflects the change.
api/docs/public/oidc-provider-setup.md (3)
38-40: Align “Simple Mode” domain examples with the “without @” guidanceUse bare domains in examples and clarify the explanatory sentence.
Apply this diff:
- - Allow specific email domains (e.g., @company.com) + - Allow specific email domains (e.g., company.com) @@ - **Allowed Email Domains**: Enter `company.com` -- This allows anyone with @company.com email +- This allows anyone with email addresses ending in `@company.com`Also applies to: 78-80
204-210: Reword Issuer URL guidance; don’t imply validation is disabled with full discovery URLPrefer the issuer base URL, but avoid stating that validation is “disabled.”
Apply this diff:
-The **Issuer URL** field accepts both formats, but **base URL is strongly recommended** for security: +The **Issuer URL** field accepts either the issuer base URL or the full discovery URL; prefer the issuer base URL (not the `/.well-known/...` path): @@ -**⚠️ Security Note**: Always use the base URL format when possible. The system automatically appends `/.well-known/openid-configuration` for OIDC discovery. Using the full discovery URL directly disables important issuer validation checks and is not recommended by the OpenID Connect specification. +**⚠️ Security note**: Prefer the issuer base URL. The system appends `/.well-known/openid-configuration` during discovery. Providing the full discovery URL also works but may skip issuer URL normalization in our implementation; token issuer validation remains enforced. Using the base URL reduces configuration mistakes.
374-379: Okta Issuer usually requires /oauth2/default (or a custom authorization server)Use the default auth server path, and add a note for custom servers.
Apply this diff:
- - **Issuer URL**: `https://YOUR_DOMAIN.okta.com` + - **Issuer URL**: `https://YOUR_DOMAIN.okta.com/oauth2/default` + - If you use a custom authorization server, replace `default` with your server ID.
🧹 Nitpick comments (3)
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts (1)
158-174: Consider sanitizing buttonStyle to avoid CSS injection risksbuttonStyle is free-form CSS that will be rendered on the login page. Whitelist safe properties or sanitize the string before persisting/serving to the client.
If helpful, I can add a minimal sanitizer utility (e.g., permit background-color, border-color, color, border, border-radius, transition) and apply it here before save and when building the public provider DTOs.
api/docs/public/oidc-provider-setup.md (2)
196-199: Add a language to the fenced code block (markdownlint MD040)Specify a language for the redirect URI block.
Apply this diff:
-``` +```text http://YOUR_UNRAID_IP:3001/graphql/api/auth/oidc/callback--- `167-172`: **Fix nested list indentation (markdownlint MD007)** Indent nested bullets by two spaces. Apply this diff: ```diff - **Allowed Email Domains**: Enter domains without @ (e.g., `company.com`) - - Helper text: "Users with emails ending in these domains can login" + - Helper text: "Users with emails ending in these domains can login" - **Specific Email Addresses**: Add individual email addresses - - Helper text: "Only these exact email addresses can login" + - Helper text: "Only these exact email addresses can login" @@ - **For each rule**: - - **Claim**: The JWT claim to check - - **Operator**: How to compare (equals, contains, endsWith, startsWith) - - **Value**: What to match against + - **Claim**: The JWT claim to check + - **Operator**: How to compare (equals, contains, endsWith, startsWith) + - **Value**: What to match againstAlso applies to: 179-183
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
api/src/unraid-api/cli/generated/graphql.tsis excluded by!**/generated/**
📒 Files selected for processing (8)
api/docs/public/oidc-provider-setup.md(1 hunks)api/src/__test__/graphql/resolvers/rclone-api.service.test.ts(11 hunks)api/src/unraid-api/graph/resolvers/rclone/rclone-api.service.ts(3 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts(1 hunks)api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts(1 hunks)api/src/unraid-api/graph/utils/form-utils.ts(2 hunks)unraid-ui/src/forms/AccordionLayout.vue(1 hunks)web/composables/gql/graphql.ts(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- api/src/unraid-api/graph/utils/form-utils.ts
- unraid-ui/src/forms/AccordionLayout.vue
- api/src/unraid-api/graph/resolvers/sso/sso.resolver.ts
- api/src/unraid-api/graph/resolvers/rclone/rclone-api.service.ts
- api/src/test/graphql/resolvers/rclone-api.service.test.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.tsweb/composables/gql/graphql.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.tsweb/composables/gql/graphql.ts
web/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All web GraphQL code must follow the .query.ts/.mutation.ts filename conventions (do not place operations in other file types)
Files:
web/composables/gql/graphql.ts
🧬 Code Graph Analysis (2)
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts (4)
api/src/unraid-api/cli/generated/graphql.ts (2)
OidcProvider(1184-1216)OidcAuthorizationRule(1174-1182)web/components/sso/useSsoProviders.ts (1)
OidcProvider(5-12)api/src/unraid-api/types/json-forms.ts (2)
SettingSlice(38-46)mergeSettingSlices(69-69)api/src/unraid-api/graph/utils/form-utils.ts (2)
createAccordionLayout(95-128)createSimpleLabeledControl(7-44)
web/composables/gql/graphql.ts (2)
api/src/unraid-api/cli/generated/graphql.ts (4)
ResolvedOrganizerV1(1597-1601)Scalars(11-75)OidcProvider(1184-1216)ResolvedOrganizerView(1603-1609)web/components/sso/useSsoProviders.ts (1)
OidcProvider(5-12)
🪛 LanguageTool
api/docs/public/oidc-provider-setup.md
[grammar] ~78-~78: There might be a mistake here.
Context: ...on: - Allowed Email Domains: Enter company.com - This allows anyone with @company.com ema...
(QB_NEW_EN)
[grammar] ~101-~101: There might be a mistake here.
Context: ...n Rule Mode** to AND 2. Add two rules: - Rule 1: - Claim: email -...
(QB_NEW_EN)
[grammar] ~102-~102: There might be a mistake here.
Context: ... to AND 2. Add two rules: - Rule 1: - Claim: email - Operator: `e...
(QB_NEW_EN)
[grammar] ~103-~103: There might be a mistake here.
Context: ...o rules: - Rule 1: - Claim: email - Operator: endsWith - Value:...
(QB_NEW_EN)
[grammar] ~104-~104: There might be a mistake here.
Context: ...Claim: email - Operator: endsWith - Value: @company.com - Rule 2: ...
(QB_NEW_EN)
[grammar] ~105-~105: There might be a mistake here.
Context: ...perator**: endsWith - Value: @company.com - Rule 2: - Claim: `email_verifie...
(QB_NEW_EN)
[grammar] ~106-~106: There might be a mistake here.
Context: ...- Value: @company.com - Rule 2: - Claim: email_verified - **Opera...
(QB_NEW_EN)
[grammar] ~107-~107: There might be a mistake here.
Context: ...any.com - Rule 2: - **Claim**:email_verified - **Operator**:equals - **Value**:...
(QB_NEW_EN)
[grammar] ~108-~108: There might be a mistake here.
Context: ...: email_verified - Operator: equals - Value: true This ensures users must...
(QB_NEW_EN)
[grammar] ~118-~118: There might be a mistake here.
Context: ...(default) 2. Add rules for each group: - **Claim**:groups - **Operator**:co...
(QB_NEW_EN)
[grammar] ~119-~119: There might be a mistake here.
Context: ...d rules for each group: - Claim: groups - Operator: contains - Value: `...
(QB_NEW_EN)
[grammar] ~120-~120: There might be a mistake here.
Context: ... Claim: groups - Operator: contains - Value: admins Or add another ...
(QB_NEW_EN)
[grammar] ~123-~123: There might be a mistake here.
Context: ...**: admins Or add another rule: - Claim: groups - Operator: `co...
(QB_NEW_EN)
[grammar] ~124-~124: There might be a mistake here.
Context: ... Or add another rule: - Claim: groups - Operator: contains - Value: `...
(QB_NEW_EN)
[grammar] ~125-~125: There might be a mistake here.
Context: ... Claim: groups - Operator: contains - Value: developers Users in either `...
(QB_NEW_EN)
[grammar] ~132-~132: There might be a mistake here.
Context: ...d. #### Multiple Domains - Claim: email - Operator: endsWith - Values: Add...
(QB_NEW_EN)
[grammar] ~133-~133: There might be a mistake here.
Context: ...s - Claim: email - Operator: endsWith - Values: Add multiple domains (e.g., `c...
(QB_NEW_EN)
[grammar] ~141-~141: There might be a mistake here.
Context: .... Add multiple rules that ALL must pass: - Email must be from company domain - E...
(QB_NEW_EN)
[grammar] ~160-~160: There might be a mistake here.
Context: ...thorization (recommended for most users) - advanced: For complex claim-based rule...
(QB_NEW_EN)
[grammar] ~179-~179: There might be a mistake here.
Context: ...e claim-based rules - For each rule: - Claim: The JWT claim to check - **...
(QB_NEW_EN)
[grammar] ~206-~206: There might be a mistake here.
Context: ...ecurity: - Base URL (recommended): https://accounts.google.com - Full discovery URL: `https://accounts....
(QB_NEW_EN)
[grammar] ~211-~211: There might be a mistake here.
Context: ...ation. Examples of correct base URLs: - Google: https://accounts.google.com - ...
(QB_NEW_EN)
[grammar] ~212-~212: There might be a mistake here.
Context: ...mples of correct base URLs:** - Google: https://accounts.google.com - Microsoft/Azure: `https://login.microsof...
(QB_NEW_EN)
[grammar] ~213-~213: There might be a mistake here.
Context: ...accounts.google.com- Microsoft/Azure:https://login.microsoftonline.com/YOUR_TENANT_ID/v2.0` - Keycloak: `https://keycloak.example.com/...
(QB_NEW_EN)
[grammar] ~214-~214: There might be a mistake here.
Context: ...ne.com/YOUR_TENANT_ID/v2.0- Keycloak:https://keycloak.example.com/realms/YOUR_REALM` - Authelia: https://auth.yourdomain.com ...
(QB_NEW_EN)
[grammar] ~240-~240: There might be a mistake here.
Context: ...orrectly (without @) - In Advanced Mode: - Verify claim names match exactly what yo...
(QB_NEW_EN)
[grammar] ~269-~269: There might be a mistake here.
Context: ...gs for: - Received claims from provider - Authorization rule evaluation - Token va...
(QB_NEW_EN)
[grammar] ~270-~270: There might be a mistake here.
Context: ...provider - Authorization rule evaluation - Token validation errors ## Security Bes...
(QB_NEW_EN)
[grammar] ~286-~286: There might be a mistake here.
Context: ...ify network connectivity between Unraid and provider ## Provider-Specific Setup #...
(QB_NEW_EN)
[grammar] ~294-~294: There might be a mistake here.
Context: ...ules in the interface. Configuration: - Issuer URL: Pre-configured (built-in p...
(QB_NEW_EN)
[grammar] ~295-~295: There might be a mistake here.
Context: ...RL**: Pre-configured (built-in provider) - Client ID/Secret: Pre-configured (buil...
(QB_NEW_EN)
[grammar] ~296-~296: There might be a mistake here.
Context: ...et**: Pre-configured (built-in provider) - Redirect URI: `http://YOUR_UNRAID_IP:3...
(QB_NEW_EN)
[grammar] ~307-~307: There might be a mistake here.
Context: ...gle.com/). Create a new OAuth client ID with Web application type. **Configuration:...
(QB_NEW_EN)
[grammar] ~309-~309: There might be a mistake here.
Context: ... Web application type. Configuration: - Issuer URL: `https://accounts.google.c...
(QB_NEW_EN)
[grammar] ~310-~310: There might be a mistake here.
Context: .... Configuration: - Issuer URL: https://accounts.google.com - Client ID/Secret: From Google Cloud Co...
(QB_NEW_EN)
[grammar] ~311-~311: There might be a mistake here.
Context: ...m Google Cloud Console OAuth credentials - Required Scopes: openid, profile, ...
(QB_NEW_EN)
[grammar] ~312-~312: There might be a mistake here.
Context: ...Required Scopes**: openid, profile, email - Redirect URI: `http://YOUR_UNRAID_IP:3...
(QB_NEW_EN)
[grammar] ~321-~321: There might be a mistake here.
Context: ...hash-password command. Configuration: - Issuer URL: `https://auth.yourdomain.c...
(QB_NEW_EN)
[grammar] ~322-~322: There might be a mistake here.
Context: .... Configuration: - Issuer URL: https://auth.yourdomain.com - Client ID: unraid-api (or as configu...
(QB_NEW_EN)
[grammar] ~323-~323: There might be a mistake here.
Context: ...raid-api` (or as configured in Authelia) - Client Secret: Your unhashed secret - ...
(QB_NEW_EN)
[grammar] ~324-~324: There might be a mistake here.
Context: ... Client Secret: Your unhashed secret - Required Scopes: openid, profile, ...
(QB_NEW_EN)
[grammar] ~325-~325: There might be a mistake here.
Context: ...Scopes**: openid, profile, email, groups - Redirect URI: `http://YOUR_UNRAID_IP:3...
(QB_NEW_EN)
[grammar] ~334-~334: There might be a mistake here.
Context: ...d note your tenant ID. Configuration: - Issuer URL: `https://login.microsofton...
(QB_NEW_EN)
[grammar] ~335-~335: There might be a mistake here.
Context: .... Configuration: - Issuer URL: https://login.microsoftonline.com/YOUR_TENANT_ID/v2.0 - Client ID: Your Application (client) I...
(QB_NEW_EN)
[grammar] ~336-~336: There might be a mistake here.
Context: ...lient ID**: Your Application (client) ID - Client Secret: Generated client secret...
(QB_NEW_EN)
[grammar] ~337-~337: There might be a mistake here.
Context: ...Client Secret**: Generated client secret - Required Scopes: openid, profile, ...
(QB_NEW_EN)
[grammar] ~338-~338: There might be a mistake here.
Context: ...Required Scopes**: openid, profile, email - Redirect URI: `http://YOUR_UNRAID_IP:3...
(QB_NEW_EN)
[grammar] ~345-~345: There might be a mistake here.
Context: ...cloak Create a new confidential client in Keycloak Admin Console with `openid-con...
(QB_NEW_EN)
[grammar] ~347-~347: There might be a mistake here.
Context: ...m the Credentials tab. Configuration: - Issuer URL: `https://keycloak.example....
(QB_NEW_EN)
[grammar] ~348-~348: There might be a mistake here.
Context: .... Configuration: - Issuer URL: https://keycloak.example.com/realms/YOUR_REALM - Client ID: unraid-api (or as configu...
(QB_NEW_EN)
[grammar] ~349-~349: There might be a mistake here.
Context: ...raid-api` (or as configured in Keycloak) - Client Secret: From Keycloak Credentia...
(QB_NEW_EN)
[grammar] ~350-~350: There might be a mistake here.
Context: ... Secret**: From Keycloak Credentials tab - Required Scopes: openid, profile, ...
(QB_NEW_EN)
[grammar] ~351-~351: There might be a mistake here.
Context: ...Required Scopes**: openid, profile, email - Redirect URI: `http://YOUR_UNRAID_IP:3...
(QB_NEW_EN)
[grammar] ~360-~360: There might be a mistake here.
Context: ...nk it to the provider. Configuration: - Issuer URL: `https://authentik.example...
(QB_NEW_EN)
[grammar] ~361-~361: There might be a mistake here.
Context: .... Configuration: - Issuer URL: https://authentik.example.com/application/o/unraid-api/ - Client ID: From Authentik provider con...
(QB_NEW_EN)
[grammar] ~362-~362: There might be a mistake here.
Context: ...*: From Authentik provider configuration - Client Secret: From Authentik provider...
(QB_NEW_EN)
[grammar] ~363-~363: There might be a mistake here.
Context: ...*: From Authentik provider configuration - Required Scopes: openid, profile, ...
(QB_NEW_EN)
[grammar] ~364-~364: There might be a mistake here.
Context: ...Required Scopes**: openid, profile, email - Redirect URI: `http://YOUR_UNRAID_IP:3...
(QB_NEW_EN)
[grammar] ~371-~371: There might be a mistake here.
Context: ...Okta Create a new OIDC Web Application in Okta Admin Console and assign appropria...
(QB_NEW_EN)
[grammar] ~373-~373: There might be a mistake here.
Context: ...riate users or groups. Configuration: - Issuer URL: `https://YOUR_DOMAIN.okta....
(QB_NEW_EN)
[grammar] ~374-~374: There might be a mistake here.
Context: .... Configuration: - Issuer URL: https://YOUR_DOMAIN.okta.com - Client ID: From Okta application confi...
(QB_NEW_EN)
[grammar] ~375-~375: There might be a mistake here.
Context: ...D**: From Okta application configuration - Client Secret: From Okta application c...
(QB_NEW_EN)
[grammar] ~376-~376: There might be a mistake here.
Context: ...t**: From Okta application configuration - Required Scopes: openid, profile, ...
(QB_NEW_EN)
[grammar] ~377-~377: There might be a mistake here.
Context: ...Required Scopes**: openid, profile, email - Redirect URI: `http://YOUR_UNRAID_IP:3...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
api/docs/public/oidc-provider-setup.md
168-168: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
170-170: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
180-180: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
181-181: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
182-182: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
196-196: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (1)
web/composables/gql/graphql.ts (1)
1449-1451: Good improvement: use PrefixedID for provider lookupsoidcProvider now accepts PrefixedID input, ensuring consistent ID normalization. LGTM.
- Added a preHandler hook to block GET requests to the /graphql endpoint when the sandbox mode is disabled, returning a 403 error with a descriptive message. - Refactored the sandbox plugin to remove the isSandboxEnabled parameter, simplifying its usage. - Updated the ConnectSettings component to reflect changes in restart logic based on mutation response instead of computed properties.
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
api/src/unraid-api/graph/resolvers/settings/settings.service.ts (1)
54-68: Fix: allow clearing SSO users and extra origins (don’t gate on truthiness)Using truthiness prevents setting empty arrays, so users can’t clear lists. Check for undefined instead.
- if (settings.ssoSubIds) { + if (settings.ssoSubIds !== undefined) { await this.ssoUserService.setSsoUsers(settings.ssoSubIds); } - if (settings.extraOrigins) { + if (settings.extraOrigins !== undefined) { // @ts-expect-error - this is correct, but the configService typescript implementation is too narrow this.configService.set('api.extraOrigins', settings.extraOrigins); }api/src/unraid-api/graph/graph.module.ts (1)
30-31: Bug: Boolean('false') yields true — parse sandbox flag explicitlyUsing Boolean(configService.get('api.sandbox')) treats the string "false" as true. Parse the value explicitly to avoid enabling features unintentionally.
- const isSandboxEnabled = () => Boolean(configService.get('api.sandbox')); + const isSandboxEnabled = () => { + const v = configService.get<boolean | string>('api.sandbox'); + return v === true || v === 'true' || v === 1 || v === '1'; + };
♻️ Duplicate comments (8)
api/src/unraid-api/graph/resolvers/settings/settings.service.ts (1)
31-42: Refactor migration check: avoid dynamic import and improve clarity/perfImporting fs/promises inside the method on every call is unnecessary. Use a static import and call access() directly.
Apply within-range changes:
- try { - const { access } = await import('fs/promises'); - await access(this.oidcConfig.configPath()); + try { + await access(this.oidcConfig.configPath());Add the missing import at the top of the file:
+import { access } from 'fs/promises';api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts (7)
129-139: Clean all optional strings: add jwksUri, buttonText, buttonVariantThese fields can also be empty strings; normalize to undefined for consistency.
return { ...provider, clientSecret: provider.clientSecret?.trim() || undefined, authorizationEndpoint: provider.authorizationEndpoint?.trim() || undefined, tokenEndpoint: provider.tokenEndpoint?.trim() || undefined, + jwksUri: provider.jwksUri?.trim() || undefined, + buttonText: provider.buttonText?.trim() || undefined, buttonIcon: provider.buttonIcon?.trim() || undefined, + buttonVariant: provider.buttonVariant?.trim() || undefined, buttonStyle: provider.buttonStyle?.trim() || undefined, };
9-13: Align service API with resolver input type (accept OidcProviderInput)Let the service accept the GraphQL input + UI fields to avoid unsafe casts in callers; normalization here already builds a proper OidcProvider.
Imports:
-} from '@app/unraid-api/graph/resolvers/sso/oidc-provider.model.js'; + OidcProviderInput, +} from '@app/unraid-api/graph/resolvers/sso/oidc-provider.model.js';Signature:
- async upsertProvider( - provider: OidcProvider & { authorizationMode?: string; simpleAuthorization?: any } - ): Promise<OidcProvider> { + async upsertProvider( + provider: OidcProviderInput & { authorizationMode?: string; simpleAuthorization?: any } + ): Promise<OidcProvider> {Also applies to: 146-149
158-174: Fix: authorizationRuleMode is dropped during upsertThe cleaned object omits authorizationRuleMode, so AND/OR won’t persist.
const cleanedProvider: OidcProvider = { id: provider.id, name: provider.name, clientId: provider.clientId, clientSecret: provider.clientSecret, issuer: provider.issuer, authorizationEndpoint: provider.authorizationEndpoint, tokenEndpoint: provider.tokenEndpoint, jwksUri: provider.jwksUri, scopes: provider.scopes, authorizationRules: provider.authorizationRules, + authorizationRuleMode: provider.authorizationRuleMode, buttonText: provider.buttonText, buttonIcon: provider.buttonIcon, buttonVariant: provider.buttonVariant, buttonStyle: provider.buttonStyle, };
312-317: Correct issuer host check for Unraid; skip validation for account.unraid.net and subdomainsExact equality to 'unraid.net' is wrong. Use proper host parsing and allow account.unraid.net or *.unraid.net per the custom auth flow.
- // Parse the issuer URL and check if hostname is exactly 'unraid.net' - const issuerUrl = new URL(provider.issuer); - if (issuerUrl.hostname === 'unraid.net') { - // Skip validation for unraid.net as it uses custom auth flow - continue; - } + const host = new URL(provider.issuer).hostname; + const isUnraidIssuer = + host === 'account.unraid.net' || host.endsWith('.unraid.net'); + if (isUnraidIssuer) { + // Skip validation for Unraid built-in/custom flow + continue; + }
574-581: Enum mismatch: use uppercase for Rule Mode to match GraphQL (OR/AND)Lowercase values won’t round-trip with the AuthorizationRuleMode enum.
- authorizationRuleMode: { + authorizationRuleMode: { type: 'string', title: 'Rule Mode', - enum: ['or', 'and'], - default: 'or', + enum: ['OR', 'AND'], + default: 'OR', description: 'How to evaluate multiple rules: OR (any rule passes) or AND (all rules must pass)', - }, + },
593-596: Enum mismatch: use uppercase operator values to match GraphQL enumThese must match AuthorizationOperator (EQUALS, CONTAINS, ENDS_WITH, STARTS_WITH).
- operator: { + operator: { type: 'string', title: 'Operator', - enum: ['equals', 'contains', 'endsWith', 'startsWith'], + enum: ['EQUALS', 'CONTAINS', 'ENDS_WITH', 'STARTS_WITH'], - }, + },
99-110: Avoid mutating in-memory providers array returned from config serviceUnshifting into providers mutates the array obtained from config, causing side-effects. Clone and build a new array.
- const providers = config?.providers || []; + const providers = [...(config?.providers || [])]; @@ - if (!hasUnraidNet) { - providers.unshift(this.getUnraidNetSsoProvider()); - } + const withDefault = hasUnraidNet + ? providers + : [this.getUnraidNetSsoProvider(), ...providers]; @@ - const updatedProviders = providers.map((provider) => { + const updatedProviders = withDefault.map((provider) => {
🧹 Nitpick comments (5)
packages/unraid-api-plugin-connect/src/__test__/cloud.service.test.ts (1)
41-45: Avoid asserting on error message substrings; make the test deterministicCatching and special-casing errors by checking
error.message.includes('Timeout')is brittle and violates the testing guideline to avoid relying on exact error messages. Prefer making the test deterministic by stubbing/mocking the network path so you can consistently assert:
- Resolution path:
await expect(promise).resolves.toMatchObject({ status: 'error' }), or- Rejection path:
await expect(promise).rejects.toThrow()(without checking the message).This will remove the need for the try/catch and the flakiness from real network timeouts.
api/src/unraid-api/graph/resolvers/settings/settings.service.ts (1)
156-164: Nit: “IDs” vs “ID's” and consistency across label/descriptionUse “IDs” (plural) not “ID's” (possessive) and keep it consistent in both places.
- description: `Provide a list of Unique Unraid Account ID's. Find yours at <a href="https://account.unraid.net/settings" target="_blank" rel="noopener noreferrer">account.unraid.net/settings</a>.`, + description: `Provide a list of Unique Unraid Account IDs. Find yours at <a href="https://account.unraid.net/settings" target="_blank" rel="noopener noreferrer">account.unraid.net/settings</a>.`,api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts (1)
37-46: Use path.basename for env override; avoid manual splitManual split is brittle and not Windows-safe. Use node:path basename.
Apply within-range change:
- const envPath = process.env.PATHS_OIDC_JSON; - if (envPath) { - // Extract just the filename from the path - const parts = envPath.split('/'); - return parts[parts.length - 1]; - } + const envPath = process.env.PATHS_OIDC_JSON; + if (envPath) { + return basename(envPath); + }Add missing import at top:
+import { basename } from 'node:path';api/src/unraid-api/graph/sandbox-plugin.ts (1)
64-68: Make CSRF token retrieval fault-tolerant with a safe fallback (per prior learning)Per the team’s prior agreement, sandbox should remain accessible even if CSRF retrieval fails. Add a silent fallback to an empty token to avoid breaking the landing page.
- const { getters } = await import('@app/store/index.js'); - const csrfToken = getters.emhttp().var.csrfToken; - const plugin = await getSandboxPlugin(csrfToken); + let csrfToken = ''; + try { + const { getters } = await import('@app/store/index.js'); + csrfToken = getters.emhttp()?.var?.csrfToken ?? ''; + } catch { + // Intentionally ignore to keep sandbox accessible (dev tool) + } + const plugin = await getSandboxPlugin(csrfToken);api/src/unraid-api/graph/graph.module.ts (1)
38-39: Note: introspection=true with plugin gating is acceptable, but AST-based detection strengthens itGiven the server-level introspection is always on, the companion plugin must robustly detect introspection. Pair this with the AST-based detection suggested in introspection-plugin.ts to close edge cases where operationName is renamed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
api/src/unraid-api/graph/graph.module.ts(3 hunks)api/src/unraid-api/graph/introspection-plugin.ts(1 hunks)api/src/unraid-api/graph/resolvers/settings/settings.service.ts(7 hunks)api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts(1 hunks)api/src/unraid-api/graph/sandbox-plugin.ts(3 hunks)api/src/unraid-api/main.ts(2 hunks)packages/unraid-api-plugin-connect/src/__test__/cloud.service.test.ts(1 hunks)web/components/ConnectSettings/ConnectSettings.ce.vue(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/components/ConnectSettings/ConnectSettings.ce.vue
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
TypeScript source files must use import specifiers with .js extensions for ESM compatibility
Files:
packages/unraid-api-plugin-connect/src/__test__/cloud.service.test.tsapi/src/unraid-api/graph/introspection-plugin.tsapi/src/unraid-api/graph/sandbox-plugin.tsapi/src/unraid-api/main.tsapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.tsapi/src/unraid-api/graph/resolvers/settings/settings.service.tsapi/src/unraid-api/graph/graph.module.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: General testing: use .rejects.toThrow() without arguments when asserting thrown errors
General testing: focus on behavior, not exact error message wording
General testing: avoid brittle tests tied to non-essential details (exact messages, log formats)
General testing: use mocks as nouns (objects), not verbs (behavioral overuse)
Files:
packages/unraid-api-plugin-connect/src/__test__/cloud.service.test.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Avoid unnecessary or obvious comments; only add comments when needed for clarity
Files:
packages/unraid-api-plugin-connect/src/__test__/cloud.service.test.tsapi/src/unraid-api/graph/introspection-plugin.tsapi/src/unraid-api/graph/sandbox-plugin.tsapi/src/unraid-api/main.tsapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.tsapi/src/unraid-api/graph/resolvers/settings/settings.service.tsapi/src/unraid-api/graph/graph.module.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
packages/unraid-api-plugin-connect/src/__test__/cloud.service.test.ts
api/src/unraid-api/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Place new API source files in api/src/unraid-api/ rather than legacy locations
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/graph/introspection-plugin.tsapi/src/unraid-api/graph/sandbox-plugin.tsapi/src/unraid-api/main.tsapi/src/unraid-api/graph/resolvers/sso/oidc-config.service.tsapi/src/unraid-api/graph/resolvers/settings/settings.service.tsapi/src/unraid-api/graph/graph.module.ts
🧠 Learnings (4)
📚 Learning: 2025-01-15T21:34:00.006Z
Learnt from: pujitm
PR: unraid/api#1047
File: api/src/unraid-api/graph/sandbox-plugin.ts:57-57
Timestamp: 2025-01-15T21:34:00.006Z
Learning: In the GraphQL sandbox (api/src/unraid-api/graph/sandbox-plugin.ts), CSRF token validation should fail silently with a fallback value to maintain sandbox accessibility, as it's a development tool where strict security measures aren't required.
Applied to files:
api/src/unraid-api/graph/sandbox-plugin.tsapi/src/unraid-api/main.ts
📚 Learning: 2025-04-23T20:19:42.542Z
Learnt from: pujitm
PR: unraid/api#1367
File: packages/unraid-api-plugin-connect/src/pubsub/user.service.ts:44-52
Timestamp: 2025-04-23T20:19:42.542Z
Learning: The project uses a custom or extended implementation of NestJS ConfigService that includes a `set()` method for runtime configuration mutation, unlike the standard nestjs/config package which only provides getter methods.
Applied to files:
api/src/unraid-api/main.ts
📚 Learning: 2025-01-27T14:31:42.305Z
Learnt from: elibosley
PR: unraid/api#1063
File: web/components/SsoButton.ce.vue:5-8
Timestamp: 2025-01-27T14:31:42.305Z
Learning: In the Unraid API web components, SSO-related props are intentionally provided in both camelCase (`ssoEnabled`) and lowercase (`ssoenabled`) variants to support interchangeable usage across different contexts (e.g., HTML attributes vs Vue props).
Applied to files:
api/src/unraid-api/graph/resolvers/settings/settings.service.ts
📚 Learning: 2025-03-12T13:48:14.850Z
Learnt from: pujitm
PR: unraid/api#1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
Applied to files:
api/src/unraid-api/graph/graph.module.ts
🧬 Code Graph Analysis (4)
api/src/unraid-api/graph/sandbox-plugin.ts (1)
api/src/store/index.ts (1)
getters(18-23)
api/src/unraid-api/graph/resolvers/sso/oidc-config.service.ts (5)
api/src/unraid-api/cli/generated/graphql.ts (2)
OidcProvider(1184-1216)OidcAuthorizationRule(1174-1182)web/composables/gql/graphql.ts (2)
OidcProvider(1184-1216)OidcAuthorizationRule(1174-1182)web/components/sso/useSsoProviders.ts (1)
OidcProvider(5-12)api/src/unraid-api/types/json-forms.ts (2)
SettingSlice(38-46)mergeSettingSlices(69-69)api/src/unraid-api/graph/utils/form-utils.ts (2)
createAccordionLayout(95-128)createSimpleLabeledControl(7-44)
api/src/unraid-api/graph/resolvers/settings/settings.service.ts (3)
packages/unraid-shared/src/services/sso.ts (1)
SsoUserService(1-14)api/src/unraid-api/types/json-forms.ts (2)
SettingSlice(38-46)mergeSettingSlices(69-69)api/src/unraid-api/graph/utils/form-utils.ts (1)
createLabeledControl(49-89)
api/src/unraid-api/graph/graph.module.ts (2)
api/src/unraid-api/graph/introspection-plugin.ts (1)
createDynamicIntrospectionPlugin(3-30)api/src/unraid-api/graph/sandbox-plugin.ts (1)
createSandboxPlugin(85-90)
🔇 Additional comments (5)
api/src/unraid-api/graph/resolvers/settings/settings.service.ts (1)
55-62: Confirm intentional behavior: ignoring restart flag from setSsoUserssetSsoUsers returns a boolean indicating if a restart is required. You now ignore it and hardcode restartRequired=false. If that’s intentional due to OIDC changes (no restart needed), all good; otherwise, wire this through.
If it should propagate, adjust as:
- const restartRequired = false; + let restartRequired = false; ... - if (settings.ssoSubIds) { - await this.ssoUserService.setSsoUsers(settings.ssoSubIds); + if (settings.ssoSubIds !== undefined) { + restartRequired = await this.ssoUserService.setSsoUsers(settings.ssoSubIds); }api/src/unraid-api/graph/introspection-plugin.ts (1)
3-6: Overall approach LGTMThe dynamic plugin cleanly gates introspection at response time and composes well with GraphModule’s plugin wiring.
Also applies to: 29-30
api/src/unraid-api/graph/sandbox-plugin.ts (2)
31-47: LGTM: Local-default landing page plugin configurationDynamic import and embed headers look correct; includeCookies + initial document are appropriate defaults.
85-90: LGTM: Plugin wrapper is straightforwardWrapping renderLandingPage via serverWillStart is clean and keeps concerns isolated.
api/src/unraid-api/graph/graph.module.ts (1)
47-50: LGTM: Plugin composition and dynamic introspectionThe split between dynamic introspection gating and sandbox rendering is clean and aligns with per-request control. Order is fine.
- Improved the sandbox access control by robustly coercing the sandbox configuration to a boolean value. - Updated the introspection plugin to detect introspection requests more effectively, allowing for better control over GraphQL introspection based on sandbox mode. - Standardized the error code for introspection denial to 'INTROSPECTION_DISABLED' for clarity.
…oding - Updated the introspection plugin to more accurately identify introspection queries by checking for the presence of __schema at the root level. - Replaced manual JWT decoding with the jose library for better reliability and security. - Adjusted logging to ensure sensitive information is not exposed while still providing useful debugging information. - Reduced session timeout for one-time tokens to enhance security.
- Introduced a new test suite for the dynamic introspection plugin, covering various scenarios based on sandbox mode. - Validated that introspection queries are allowed when sandbox mode is enabled and blocked when disabled. - Ensured proper handling of edge cases, including missing or empty queries, and dynamic sandbox status checks. - Enhanced test coverage for regular queries, mutations, and subscriptions to confirm expected behavior.
- Renamed the OIDC provider examples section to "OIDC Provider Setup" for better clarity. - Enhanced the Google OAuth setup instructions with detailed steps and a warning about domain requirements for redirect URIs. - Updated various sections to ensure consistency and clarity in the documentation. - Made minor adjustments to the introspection plugin tests for type safety and improved error handling.
…d validation - Added OidcStateService to handle secure state generation and validation for OIDC flows. - Integrated state management into OidcAuthService, replacing the previous state handling logic. - Implemented unit tests for OidcStateService to ensure secure state generation, validation, and extraction of provider information. - Updated SsoModule to include OidcStateService for dependency injection.
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
- Added OidcStateService to the test suite for OidcAuthService. - Updated tests to verify that the state parameter in authorization URLs includes a secure token format. - Ensured that state handling logic aligns with the recent implementation of secure state management.
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
2 participants
This will require an Account app update, as the account app is not fully OIDC compliant at this point.
Summary by CodeRabbit
New Features
UI / Settings
Documentation
Chores