feat: add gui settings field for sso users #1310
Conversation
WalkthroughThis pull request introduces several enhancements focused on managing SSO user IDs and refining the GraphQL schema and state management. The changes include adding a new Changes
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
api/src/store/modules/config.ts (1)
329-342:⚠️ Potential issueMissing setSsoUsers in configUpdateActionsFlash.
The new
setSsoUsersaction is not included in theconfigUpdateActionsFlasharray, which means changes made using this action might not be persisted to flash storage.Add the new action to ensure changes are properly persisted:
export const configUpdateActionsFlash = isAnyOf( addSsoUser, + setSsoUsers, updateUserConfig, updateAccessTokens, updateAllowedOrigins, setUpnpState, setWanPortToValue, setWanAccess, setupRemoteAccessThunk.fulfilled, logoutUser.fulfilled, loginUser.fulfilled, removeSsoUser, setLocalApiKey );
🧹 Nitpick comments (6)
api/src/unraid-api/graph/connect/connect-settings.service.ts (6)
21-21: Incorrect import order.The imports should be arranged alphabetically, which is a common convention in many TypeScript/JavaScript projects. This is also flagged by your static analysis tools.
-import { updateAllowedOrigins, updateUserConfig, setSsoUsers } from '@app/store/modules/config.js'; +import { setSsoUsers, updateAllowedOrigins, updateUserConfig } from '@app/store/modules/config.js';🧰 Tools
🪛 GitHub Check: Build API
[failure] 21-21:
ReplaceupdateAllowedOrigins,·updateUserConfig,·setSsoUserswithsetSsoUsers,·updateAllowedOrigins,·updateUserConfig🪛 GitHub Check: Test API
[failure] 21-21:
ReplaceupdateAllowedOrigins,·updateUserConfig,·setSsoUserswithsetSsoUsers,·updateAllowedOrigins,·updateUserConfig
137-139: Format UUID regex for better readability.The UUID regex is quite long and would be more readable with proper indentation. This is also flagged by your static analysis tools.
- const uuidRegex = /^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$/; + const uuidRegex = + /^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$/;🧰 Tools
🪛 GitHub Check: Build API
[failure] 137-137:
Insert⏎···········🪛 GitHub Check: Test API
[failure] 137-137:
Insert⏎···········
145-145: Fix grammatical error in comment.There's a grammatical issue in the restart condition comment.
- // request a restart if we're there were no sso users before + // request a restart if there were no sso users before
377-379: Incorrect method comment.The method comment for
ssoUsersSliceincorrectly refers to "Extra origins settings slice" instead of SSO users.- /** - * Extra origins settings slice - */ + /** + * SSO users settings slice + */
389-389: Use complete URL with protocol in the description.The URL in the description should include the
https://protocol to make it properly clickable.- description: `Provide a list of Unique Unraid Account ID's. Find yours at <a href="account.unraid.net/settings" target="_blank">account.unraid.net/settings</a>`, + description: `Provide a list of Unique Unraid Account ID's. Find yours at <a href="https://account.unraid.net/settings" target="_blank">account.unraid.net/settings</a>`,
123-146: Consider documenting the restart behavior.The restart behavior is based on whether there were any existing SSO users. Consider adding a more detailed explanation of why this specific condition requires a restart.
/** * Updates the SSO users and returns true if a restart is required * @param userIds - The list of SSO user IDs * @returns true if a restart is required, false otherwise + * @remarks A restart is required only when enabling SSO users for the first time + * because the system needs to initialize the authentication mechanism. */🧰 Tools
🪛 GitHub Check: Build API
[failure] 137-137:
Insert⏎···········🪛 GitHub Check: Test API
[failure] 137-137:
Insert⏎···········
🛑 Comments failed to post (2)
unraid-ui/src/forms/StringArrayField.vue (1)
47-47:
⚠️ Potential issueSecurity risk: Using v-html without sanitization.
Changing from
{{ control.description }}tov-html="control.description"allows rendering HTML content, which can introduce XSS vulnerabilities if the description content is not properly sanitized.Consider one of these approaches:
- Add sanitization before using v-html:
-<p v-if="control.description" v-html="control.description" /> +<p v-if="control.description" v-html="sanitizeHtml(control.description)" />
- Or if rich text is not needed, revert to the safer interpolation:
-<p v-if="control.description" v-html="control.description" /> +<p v-if="control.description">{{ control.description }}</p>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<p v-if="control.description" v-html="sanitizeHtml(control.description)" />api/src/unraid-api/graph/connect/connect-settings.service.ts (1)
132-132: 💡 Verification agent
🧩 Analysis chain
Check the implementation of
symmetricDifference.The
symmetricDifferencemethod is not a standard method on the JavaScriptSetprototype. This code might rely on a custom extension or polyfill that's not visible in this file.
🏁 Script executed:
#!/bin/bash # Search for symmetricDifference implementation rg -A 5 "symmetricDifference" --type tsLength of output: 731
Action: Clarify the source and implementation of the
symmetricDifferencemethod.The code calls
newUserSet.symmetricDifference(currentUserSet)on a JavaScript Set, but this isn’t part of the standard Set API. Our search didn’t reveal any custom implementation or polyfill elsewhere in the TypeScript files, so please verify that this method is being correctly extended (or imported) before relying on its behavior. If it isn’t defined, you might need to implement it or adjust the logic to use a standard approach.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
api/src/store/modules/config.ts (1)
313-324: 🛠️ Refactor suggestionAdd setSsoUsers to configUpdateActionsFlash
The
setSsoUsersaction is properly exported but hasn't been added to theconfigUpdateActionsFlashlist (lines 329-342), unlikeaddSsoUser. This may prevent changes from being persisted to disk when the action is dispatched.export const configUpdateActionsFlash = isAnyOf( addSsoUser, + setSsoUsers, updateUserConfig, updateAccessTokens, updateAllowedOrigins, setUpnpState, setWanPortToValue, setWanAccess, setupRemoteAccessThunk.fulfilled, logoutUser.fulfilled, loginUser.fulfilled, removeSsoUser, setLocalApiKey );
🧹 Nitpick comments (1)
api/src/unraid-api/graph/connect/connect-settings.service.ts (1)
378-380: Fix documentation commentThe comment for the
ssoUsersSlicemethod incorrectly states "Extra origins settings slice".-/** - * Extra origins settings slice - */ +/** + * SSO users settings slice + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
api/src/graphql/generated/api/operations.tsis excluded by!**/generated/**api/src/graphql/generated/api/types.tsis excluded by!**/generated/**
📒 Files selected for processing (7)
api/src/graphql/schema/types/connect/connect.graphql(2 hunks)api/src/store/modules/config.ts(2 hunks)api/src/unraid-api/graph/connect/connect-settings.service.ts(6 hunks)unraid-ui/src/forms/StringArrayField.vue(1 hunks)web/components/ConnectSettings/graphql/settings.query.ts(2 hunks)web/composables/gql/gql.ts(3 hunks)web/composables/gql/graphql.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- unraid-ui/src/forms/StringArrayField.vue
- web/components/ConnectSettings/graphql/settings.query.ts
- api/src/graphql/schema/types/connect/connect.graphql
- web/composables/gql/graphql.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
api/src/store/modules/config.ts (1)
222-224: New reducer to bulk-set SSO user IDsThis addition provides a more efficient way to update multiple SSO user IDs at once, filtering out falsy values and joining valid IDs with commas.
api/src/unraid-api/graph/connect/connect-settings.service.ts (3)
58-58: Added ssoUserIds to current settingsCorrectly adds SSO user IDs to the settings object, converting from comma-separated string to array.
96-98: Added handling for SSO users in syncSettingsProperly integrates the new SSO user functionality with the existing settings synchronization logic.
381-404: New UI schema for SSO usersThe implementation provides a clear UI structure for managing SSO users with appropriate title, description, and input configuration.
web/composables/gql/gql.ts (1)
17-18: Added ssoUserIds to GraphQL queries and mutationsThe GraphQL query, mutation, and associated function signatures are correctly updated to include the new
ssoUserIdsfield, ensuring proper data flow between client and server.Also applies to: 43-44, 86-86, 90-90
| private async updateSSOUsers(userIds: string[]): Promise<boolean> { | ||
| const { ssoUserIds } = await this.getCurrentSettings(); | ||
| const currentUserSet = new Set(ssoUserIds); | ||
| const newUserSet = new Set(userIds); | ||
| if (newUserSet.symmetricDifference(currentUserSet).size === 0) { | ||
| // there's no change, so no need to update | ||
| return false; | ||
| } | ||
| // make sure we aren't adding invalid user ids | ||
| const uuidRegex = | ||
| /^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$/; | ||
| const invalidUserIds = userIds.filter((id) => !uuidRegex.test(id)); | ||
| if (invalidUserIds.length > 0) { | ||
| throw new GraphQLError(`Invalid SSO user ID's: ${invalidUserIds.join(', ')}`); | ||
| } | ||
| const { store } = await import('@app/store/index.js'); | ||
| store.dispatch(setSsoUsers(userIds)); | ||
| // request a restart if we're there were no sso users before | ||
| return currentUserSet.size === 0; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New updateSSOUsers method with validation
The method properly validates UUID format and detects changes between current and new user sets.
The symmetricDifference method used on line 132 isn't a standard Set method in JavaScript/TypeScript. Please verify if this is provided by a utility library or needs to be implemented:
🏁 Script executed:
#!/bin/bash
# Check if Set.prototype.symmetricDifference exists or is imported from a utility library
rg -F "symmetricDifference" --type tsLength of output: 171
Action Required: Implement or Import symmetricDifference Method
The current implementation uses newUserSet.symmetricDifference(currentUserSet) on line 132, but JavaScript/TypeScript’s Set does not provide a built-in symmetricDifference method. There’s no indication from the repository that this method is defined or imported from a utility library. To resolve this:
- Verify if a custom implementation was intended: If the method is defined elsewhere (perhaps in a shared utilities file), please add the appropriate import.
- Implementation suggestion: If it’s missing, consider implementing the functionality manually or adding a helper function to compute the symmetric difference between two sets.
There was a problem hiding this comment.
@pujitm this is sick - didn't realize how far the web set had come, I've been dealing with the node one which is pretty worthless for any operations like this.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| private async updateSSOUsers(userIds: string[]): Promise<boolean> { | ||
| const { ssoUserIds } = await this.getCurrentSettings(); | ||
| const currentUserSet = new Set(ssoUserIds); | ||
| const newUserSet = new Set(userIds); | ||
| if (newUserSet.symmetricDifference(currentUserSet).size === 0) { | ||
| // there's no change, so no need to update | ||
| return false; | ||
| } | ||
| // make sure we aren't adding invalid user ids | ||
| const uuidRegex = | ||
| /^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$/; | ||
| const invalidUserIds = userIds.filter((id) => !uuidRegex.test(id)); | ||
| if (invalidUserIds.length > 0) { | ||
| throw new GraphQLError(`Invalid SSO user ID's: ${invalidUserIds.join(', ')}`); | ||
| } | ||
| const { store } = await import('@app/store/index.js'); | ||
| store.dispatch(setSsoUsers(userIds)); | ||
| // request a restart if we're there were no sso users before | ||
| return currentUserSet.size === 0; | ||
| } |
There was a problem hiding this comment.
@pujitm this is sick - didn't realize how far the web set had come, I've been dealing with the node one which is pretty worthless for any operations like this.
elibosley
force-pushed
the
feat/sso-via-api
branch
from
131df0a to
1ddd9cd
Compare
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/src/unraid-api/graph/connect/connect-settings.service.ts (1)
128-147:⚠️ Potential issueThe
symmetricDifferencemethod is not standardThe current implementation uses
newUserSet.symmetricDifference(currentUserSet)on line 132, but JavaScript/TypeScript'sSetdoes not provide a built-insymmetricDifferencemethod.Here's a suggested implementation:
- if (newUserSet.symmetricDifference(currentUserSet).size === 0) { + // Calculate symmetric difference manually + const difference = new Set([...newUserSet, ...currentUserSet]); + for (const item of newUserSet) { + if (currentUserSet.has(item)) { + difference.delete(item); + } + } + if (difference.size === 0) {
🧹 Nitpick comments (3)
api/src/unraid-api/graph/connect/connect-settings.service.ts (2)
146-146: Minor typo in commentThere's a small grammatical error in the comment.
- // request a restart if we're there were no sso users before + // request a restart if there were no sso users before
378-380: Incorrect comment for SSO users sliceThe comment appears to be copied from the
extraOriginsSlicemethod.- /** - * Extra origins settings slice - */ + /** + * SSO users settings slice + */web/composables/gql/graphql.ts (1)
589-594:DockerMutationstype
Defines core container actions. Consider adding doc comments to clarify usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (2)
api/src/graphql/generated/api/operations.tsis excluded by!**/generated/**api/src/graphql/generated/api/types.tsis excluded by!**/generated/**
📒 Files selected for processing (8)
api/src/graphql/schema/types/connect/connect.graphql(2 hunks)api/src/store/modules/config.ts(3 hunks)api/src/unraid-api/graph/connect/connect-settings.service.ts(7 hunks)api/src/unraid-api/graph/connect/connect.resolver.ts(1 hunks)unraid-ui/src/forms/StringArrayField.vue(1 hunks)web/components/ConnectSettings/graphql/settings.query.ts(2 hunks)web/composables/gql/gql.ts(3 hunks)web/composables/gql/graphql.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- api/src/unraid-api/graph/connect/connect.resolver.ts
- web/components/ConnectSettings/graphql/settings.query.ts
- unraid-ui/src/forms/StringArrayField.vue
- api/src/store/modules/config.ts
- api/src/graphql/schema/types/connect/connect.graphql
- web/composables/gql/gql.ts
🧰 Additional context used
🧬 Code Definitions (1)
api/src/unraid-api/graph/connect/connect-settings.service.ts (4)
api/src/utils.ts (1)
csvStringToArray(264-274)web/composables/gql/graphql.ts (1)
ApiSettingsInput(96-115)api/src/graphql/generated/api/types.ts (1)
ApiSettingsInput(93-112)api/src/store/modules/config.ts (1)
setSsoUsers(222-224)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (20)
api/src/unraid-api/graph/connect/connect-settings.service.ts (5)
21-21: Import statement correctly includessetSsoUsersThe import statement now properly includes the
setSsoUsersaction that will be used to update SSO users in the store.
58-58: Added SSO user IDs to settingsThe
ssoUserIdsfield is now correctly retrieved fromremote.ssoSubIdsand converted from a CSV string to an array using thecsvStringToArrayutility function.
96-98: Added SSO user management to sync settingsThe conditional check for
settings.ssoUserIdsand subsequent call toupdateSSOUserscorrectly handles SSO user synchronization.
183-183: Added SSO users slice to settings schemaThe
ssoUsersSlicemethod is now correctly included in the list of slices used to build the settings schema.
381-404: Well-structured SSO users settings slice implementationThe
ssoUsersSlicemethod properly defines the schema for SSO user settings with appropriate properties and UI elements. The description includes helpful information for users to find their Unraid Account ID.web/composables/gql/graphql.ts (15)
113-114: NewssoUserIdsfield for API settings
This addition cleanly extends theApiSettingsInputtype to accept multiple Unraid account IDs.
199-205: IntroduceArrayDiskInputtype
Defines a disk identifier and optional slot, aligning well with the intended usage.
238-250: NewArrayMutationstype
Adds clear, distinct operations for array management. Please ensure that corresponding resolvers are implemented and tested.
252-255: Check optional input foraddDiskToArray
The input is marked optional. Consider making it mandatory if an ID must always be provided.
257-260:clearArrayDiskStatisticsarguments
Confirms a required disk ID parameter. No issues found.
262-265:mountArrayDiskarguments
Straightforward required ID parameter. Looks good.
267-270: Check optional input forremoveDiskFromArray
If the user must specify a disk ID, consider making this field required.
272-275:setStatearguments
The optional input approach is acceptable if a default handling for no input is defined.
277-280:unmountArrayDiskarguments
Consistent design. ID is required.
317-321:ArrayStateInputtype
Clean definition specifying the desired state. No issues.
322-328:ArrayStateInputStateenum
Clearly enumerates the possible array states. Looks good.
565-565: Addedmutationsfield inDockertype
Includes a reference toDockerMutations. No concerns.
596-599:startContainerarguments
Simple mandatory ID parameter. No issues found.
601-604:stopContainerarguments
Consistent argument structure.
2060-2061: Generated GraphQL documents
These references appear consistent with the new fields.
🤖 I have created a release *beep* *boop* --- ## [4.6.0](v4.5.0...v4.6.0) (2025-04-03) ### Features * add gui settings field for sso users ([#1310](#1310)) ([5ba3fa6](5ba3fa6)) ### Bug Fixes * build ([ed67af9](ed67af9)) * **plugin:** flaky masking of benign warning during pnpm install ([#1313](#1313)) ([1f10b63](1f10b63)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Style & Documentation