feat: session issues #1087
feat: session issues #1087
Conversation
WalkthroughThe pull request introduces enhancements to API key management by adding an Changes
Sequence DiagramsequenceDiagram
participant Client
participant Resolver
participant ApiKeyService
participant Storage
Client->>Resolver: Create API Key
Resolver->>ApiKeyService: create(input)
alt Key exists and overwrite=false
ApiKeyService-->>Resolver: Return existing key
else Key doesn't exist or overwrite=true
ApiKeyService->>Storage: Save API Key
Storage-->>ApiKeyService: Confirm save
ApiKeyService-->>Resolver: Return new key
end
Resolver-->>Client: API Key Response
Possibly related PRs
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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.
Actionable comments posted: 4
🧹 Nitpick comments (5)
web/composables/gql/graphql.ts (1)
354-360: Thar be new memories and overwrites on deck!
‘memory’ and ‘overwrite’ fields let ye craft ephemeral or replaceable API keys. Exercise caution when ephemeral keys vanish or get replaced, me matey—make sure the rest o’ the code handles it gracefully.api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.ts (1)
Line range hint
89-102: Yer tests could use a bit more muscle, cap'n!While the test cases be solid, we could strengthen our assertions to ensure the API key creation behaves exactly as expected.
Add these additional assertions to yer test:
expect(apiKeyService.create).toHaveBeenCalledWith({ name: input.name, description: input.description, roles: input.roles, permissions: [], + memory: false, + overwrite: false }); +expect(result.id).toBeDefined(); +expect(result.key).toBeDefined(); +expect(result.createdAt).toBeDefined();api/src/unraid-api/auth/api-key.service.ts (2)
106-113: Add more details to yer ship's log, sailor!While ye've added the new
memoryparameter, we could use more documentation about its behavior and limitations.Add JSDoc comments to explain:
+/** + * Creates a new API key with the specified parameters. + * @param {Object} params - The API key creation parameters + * @param {boolean} [params.memory=false] - If true, stores the key in memory only + * @param {boolean} [params.overwrite=false] - If true, overwrites existing key + * @throws {GraphQLError} When validation fails + * @returns {Promise<ApiKeyWithSecret>} The created or existing API key + */ async create({
131-133: Add a log entry when returnin' an existin' key, ye scallywag!When we find an existin' key and return it instead of throwin' an error, we should log this for the ship's records.
if (!overwrite && existingKey) { + this.logger.debug(`Returning existing API key for name: ${sanitizedName}`); return existingKey; }api/src/graphql/schema/types/api-key/api-key.graphql (1)
30-33: Yer schema documentation be ship-shape, but we could use some constraints!The documentation for the new fields be clear as Caribbean waters! However, we might want to add some default values to help our crew.
Consider adding default values in the schema:
""" This will replace the existing key if one already exists with the same name, otherwise returns the existing key """ -overwrite: Boolean +overwrite: Boolean = false """ Whether to create the key in memory only (true), or on disk (false) - memory only keys will not persist through reboots of the API """ -memory: Boolean +memory: Boolean = false
📜 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 (11)
api/dev/states/myservers.cfg(1 hunks)api/src/graphql/schema/types/api-key/api-key.graphql(1 hunks)api/src/unraid-api/auth/api-key.service.ts(3 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.ts(2 hunks)api/src/unraid-api/graph/resolvers/resolvers.module.ts(3 hunks)web/components/UserProfile.ce.vue(1 hunks)web/composables/gql/graphql.ts(10 hunks)web/helpers/create-apollo-client.ts(3 hunks)web/helpers/urls.ts(0 hunks)web/pages/index.vue(3 hunks)
💤 Files with no reviewable changes (1)
- web/helpers/urls.ts
✅ Files skipped from review due to trivial changes (2)
- web/helpers/create-apollo-client.ts
- web/components/UserProfile.ce.vue
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build and Test API
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (14)
web/pages/index.vue (2)
5-5: Arr, the import be ship-shape!Ye've properly imported the SsoButtonCe from the components treasure chest.
163-163: Aye, the SSO button be properly mounted!The SsoButtonCe component be correctly bound to the serverState's ssoEnabled flag. Smooth sailin' ahead!
web/composables/gql/graphql.ts (5)
47-47: Arr, multiple actions be a glorious bounty!
Switchin' from a single action to an array of actions grants more fine-grained permissions, me hearty. This be a worthy enhancement.
71-71: Aye, adding 'permissions' to the ApiKey type be smooth sailin'
This array structure neatly captures a treasure chest of permission details.
88-88: Shipshape expansion of 'ApiKeyWithSecret'
Ye new 'permissions' array ensures consistent scallywag-proofing across the code.
607-607: Yarr, typed arrays be better than uncharted JSON
Switchin’ toArray<Permission>gives clear structure over arbitrary JSON. This reduces confusion among the crew.
1036-1041: Hoist the sails, a new 'Permission' type be upon ye!
Definin’actionsas an array and specifying aresourcebe a grand approach for robust access control.api/dev/states/myservers.cfg (1)
22-22: Ye changed the minigraph to 'ERROR_RETRYING'?
Beware the seas of infinite retries, me buccaneer. Ensure the code signals errors properly and does not swirl into a maelstrom.api/src/unraid-api/graph/resolvers/resolvers.module.ts (4)
7-7: Avast, 'ApiKeyResolver' import be a new recruit!
This signals we be shippin’ out from the old 'AuthResolver' approach.
14-14: Thar be 'MeResolver' still on deck
But the changes speak of removing 'AuthResolver' in favor of 'ApiKeyResolver'. Verify if 'MeResolver' stays or if it was also meant to walk the plank.
28-28: Aye, providers now list 'ApiKeyResolver'
This lines up with the new key-based approach for authentication. Smooth sailing.
46-46: Exports array parted ways with old treasure
ReplacingAuthResolverwithApiKeyResolverensures the new system be fully operational.api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.ts (2)
19-22: Aye, a fine renamin' of the resolver class, matey!The change from
AuthResolvertoApiKeyResolverbe a proper move, followin' the single responsibility principle like a true pirate follows their treasure map!
Line range hint
1-100: Ye've kept a tidy ship here, cap'n!The overall structure of this resolver be ship-shape, with proper decorators, clean method organization, and good use of types.
| onMounted(() => { | ||
| document.cookie = 'unraid_session_cookie=mockusersession'; | ||
| }); |
There was a problem hiding this comment.
Blimey! This cookie be vulnerable to pirates!
Arr matey, setting cookies directly in the frontend code be askin' for trouble! Any scurvy dog with a browser console could manipulate this session.
Consider these security measures to protect yer treasure:
- Move cookie settin' to the backend crow's nest
- Add security flags like
httpOnly,secure, andsameSite - Use proper session management instead of hardcoded values
Here's a safer way to handle sessions, ye landlubber:
-onMounted(() => {
- document.cookie = 'unraid_session_cookie=mockusersession';
-});Move this to your backend authentication route instead!
| memory: input.memory ?? false, | ||
| overwrite: input.overwrite ?? false |
There was a problem hiding this comment.
💡 Codebase verification
Aye, ye concerns be valid as a compass pointin' north!
After diggin' through the ship's logs, there be no safety nets preventin' these dangerous waters:
- A memory key could overwrite a disk key, leavin' us with ghost keys
- A disk key could replace a memory key, causin' unwanted persistence
Here be the spots needin' attention:
api-key.service.ts: Add validation in thecreate()method before the memory/disk operations to prevent these conflictin' flags from causin' trouble
Example validation needed:
if (memory && overwrite && existingKey && !existingKey.memory) {
throw new GraphQLError('Cannot overwrite a disk key with a memory key');
}🔗 Analysis chain
Ahoy! We be needin' some guard rails on these new parameters!
While ye've added the new memory and overwrite flags, we should be checkin' that they don't conflict with each other in ways that could sink our ship!
Consider these edge cases:
- What happens if both
memoryandoverwritebe true? - Should we allow overwritin' a disk-stored key with a memory key?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Let's search for any existing validation logic
ast-grep --pattern 'if ($memory && $overwrite) {
$$$
}'
Length of output: 59
Script:
#!/bin/bash
# Let's find the complete createApiKey implementation
ast-grep --pattern 'createApiKey($$$) {
$$$
}'
# Also search for the input type definition
ast-grep --pattern 'input CreateApiKeyInput {
$$$
}'
# And check for any validation decorators or functions
rg -A 5 "memory.*overwrite|overwrite.*memory" --type typescript
Length of output: 208
Script:
#!/bin/bash
# Read the complete resolver file
cat api/src/unraid-api/graph/resolvers/api-key/api-key.resolver.ts
# Look for related test files
fd "api-key.*test" --type f
Length of output: 3030
Script:
#!/bin/bash
# Check the API key service implementation
cat api/src/unraid-api/auth/api-key.service.ts
Length of output: 11002
| describe('ApiKeyResolver', () => { | ||
| let resolver: ApiKeyResolver; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Arrr! We be missin' some important test cases here, matey!
The test suite be lackin' coverage for the new memory and overwrite parameters in the createApiKey mutation.
Add these test cases to yer test suite:
describe('createApiKey', () => {
+ it('should create new API key in memory when memory flag is true', async () => {
+ const input = {
+ name: 'Memory API Key',
+ roles: [Role.GUEST],
+ memory: true
+ };
+ // Add test implementation
+ });
+
+ it('should overwrite existing API key when overwrite flag is true', async () => {
+ const input = {
+ name: 'Existing Key',
+ roles: [Role.GUEST],
+ overwrite: true
+ };
+ // Add test implementation
+ });
});Committable suggestion skipped: line range outside the PR's diff.
| if (memory) { | ||
| this.memoryApiKeys.push(apiKey as ApiKeyWithSecret) | ||
| } else { | ||
| await this.saveApiKey(apiKey as ApiKeyWithSecret); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ye need to swab the deck of old memory keys, cap'n!
The memory storage implementation be lackin' a way to clean up old keys. We should add methods to manage the memory-stored keys.
Consider adding these methods:
+private cleanupMemoryKeys(): void {
+ // Clean up memory keys older than X time
+ const now = new Date();
+ this.memoryApiKeys = this.memoryApiKeys.filter(key => {
+ const keyDate = new Date(key.createdAt);
+ const hoursDiff = (now.getTime() - keyDate.getTime()) / (1000 * 60 * 60);
+ return hoursDiff < 24; // Keep keys younger than 24 hours
+ });
+}Committable suggestion skipped: line range outside the PR's diff.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores