feat(web): rm api-key validation from connect sign in

[pujitm]

This will now happen at the mothership layer.

---

• To see the specific tasks where the Asana app for GitHub is being used, see below:
  • https://app.asana.com/0/0/1208980070046457

Summary by CodeRabbit

• New Features

  • Streamlined user login process by removing API key validation.
  • Enhanced error handling for missing user attributes.
  • Improved local API key management during login.
  • Introduced new functionality for mothership subscriptions.
  • Added methods for initializing and stopping ping timeout jobs.

• Bug Fixes

  • Resolved issues related to error handling in user login flow.

• Chores

  • Removed unused API key validation logic from multiple components.
  • Deleted obsolete test coverage for API key validation scenarios.
  • Simplified test cases for API key check functionality.
  • Enhanced logging and error handling in various components.
  • Removed API key-related reducers and getters from the store.

[coderabbitai]

Walkthrough

The pull request introduces significant modifications to several files related to API key validation and user authentication. The connectSignIn function has removed the API key validation logic, focusing instead on JWT decoding and user attribute checks. Similar changes were made in the apiKeyCheckJob and enableLocalApiKeyListener functions, which also eliminated API key validation. The validateApiKeyWithKeyServer function has been completely removed, along with its associated test cases, impacting test coverage. Additionally, the Redux store configuration has been adjusted to remove references to the API key state management.

Changes

File	Change Summary
api/src/graphql/resolvers/mutation/connect/connect-sign-in.ts	Removed conditional API key validation logic; simplified user authentication flow; focused on JWT token decoding and user attribute verification.
api/src/mothership/jobs/api-key-check-jobs.ts	Removed API key validation logic; eliminated the apiKeyCheckJob function, simplifying control flow.
api/src/store/listeners/local-api-key-listener.ts	Removed API key validation logic; directly create local API key without validation; retained error handling for local API key creation.
api/src/__test__/mothership/api-key/validate-api-key-with-keyserver.test.ts	Entire test file removed, eliminating test coverage for API key validation scenarios.
api/src/mothership/api-key/validate-api-key-with-keyserver.ts	Removed the validateApiKeyWithKeyServer function, which handled API key validation and associated error handling.
api/src/__test__/mothership/api-key/api-key-check-jobs.test.ts	Refactored tests for apiKeyCheckJob; removed detailed error handling tests; streamlined assertions for success and invalid length scenarios.
api/src/store/index.ts	Removed apiKey reducer from Redux store configuration; updated getters accordingly.
api/src/store/listeners/api-key-listener.ts	Removed the validateApiKeyIfPresent function, which managed API key validation based on application state.
api/src/store/modules/apikey.ts	Removed Redux slice for managing API key state, including its reducer and initial state.

Possibly related PRs

• fix(web): notifications consistency on unraid servers #961: The removal of API key validation logic in the NotificationsService is related to the overall changes in API key handling in the main PR.

Suggested reviewers

• mdatelle
• zackspear

Poem

🐰 A Rabbit's Authentication Tale 🔑

No more keys to validate with care,
Tokens decoded with a bunny's flair.
Streamlined login, simple and neat,
Authentication now a hopping feat!

Hop hop hooray! 🎉

Tip

CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command @coderabbitai generate docstrings to have CodeRabbit automatically generate docstrings for your pull request. We would love to hear your feedback on Discord. This feature will be included in our new Pro Plan when released, so please consider upgrading your current plan.

---

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between d4da204 and 881c41c.

📒 Files selected for processing (4)

• api/src/graphql/resolvers/mutation/connect/connect-sign-in.ts (0 hunks)
• api/src/mothership/api-key/api-key-types.ts (0 hunks)
• api/src/mothership/utils/get-mothership-websocket-headers.ts (2 hunks)
• api/src/store/getters/index.ts (0 hunks)

💤 Files with no reviewable changes (3)

• api/src/mothership/api-key/api-key-types.ts
• api/src/graphql/resolvers/mutation/connect/connect-sign-in.ts
• api/src/store/getters/index.ts

🔇 Additional comments (3)

api/src/mothership/utils/get-mothership-websocket-headers.ts (3)

Line range hint 1-53: Overall changes look good

The modifications consistently implement the architectural change of moving API key validation to the mothership layer while maintaining clean code structure and proper typing. The symmetrical treatment of API keys in both WebSocket headers and connection parameters is a good approach.

---

Line range hint 43-51: Ensure proper WebSocket connection error handling

The connection parameters now include the API key without validation, consistent with the WebSocket headers approach. Ensure that WebSocket connection failures due to invalid API keys are properly handled and provide meaningful feedback to users.

Let's verify the WebSocket error handling implementation:

✅ Verification successful

Based on the code analysis, I can now generate the final response:

WebSocket error handling is properly implemented

The codebase has comprehensive WebSocket error handling implemented through multiple layers:

• Error handling for invalid API keys with proper user feedback
• Network error handling with automatic retries and exponential backoff
• Connection status monitoring with ping/timeout detection
• Proper cleanup of WebSocket resources on disconnection

Key implementations:

• GraphQLClient class handles WebSocket lifecycle and error events
• PingTimeoutJobs monitors connection health and handles timeouts
• Error status propagation through Redux store with MinigraphStatus enum
• Automatic reconnection with configurable retry policy

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for WebSocket connection error handling implementation

# Search for WebSocket error handling
rg -A 5 "websocket.*error|socket.*error|connection.*error" --type typescript

# Search for error messages related to invalid API keys in WebSocket context
rg -A 5 "invalid.*api.?key.*socket|socket.*invalid.*api.?key" --type typescript

Length of output: 230

---

Script:

#!/bin/bash
# Description: Check for WebSocket connection error handling implementation

# Search for WebSocket error handling
rg -A 5 "websocket.*error|socket.*error|connection.*error"

# Search for error messages related to invalid API keys in WebSocket context
rg -A 5 "invalid.*api.?key|socket.*invalid"

# Look for WebSocket connection handling code
ast-grep --pattern 'new WebSocket($$$)'

# Search for error event listeners in WebSocket context
ast-grep --pattern 'socket.on("error", $$$)'

# Look for error handling in the mothership utils directory
fd . api/src/mothership -t f -x cat {}

Length of output: 27769

---

Line range hint 19-29: Verify mothership layer API key validation implementation

The removal of API key validation here aligns with moving validation to the mothership layer. However, we should ensure proper error handling exists for invalid API keys at the mothership layer.

Let's verify the mothership layer implementation:

✅ Verification successful

Based on the search results, I can now generate a final response as we have sufficient evidence about the API key validation implementation:

API key validation is properly implemented in the mothership layer

The codebase shows clear evidence of API key validation being handled at the mothership layer:

• The authentication service (auth.service.ts) implements validateApiKeyCasbin which validates API keys and throws UnauthorizedException for invalid keys
• GraphQL schema includes ApiKeyResponse type with valid and error fields to handle validation responses
• WebSocket connection headers include the API key which gets validated at connection time
• Error handling for invalid API keys is implemented with proper HTTP 401 responses

The removal of local validation is safe as the validation has been properly moved to the mothership layer, maintaining security while centralizing the validation logic.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for API key validation implementation in mothership layer
# and error handling for invalid keys

# Search for API key validation in mothership-related files
rg -A 5 "apiKey.*valid|validateApiKey" --type typescript

# Search for error handling related to invalid API keys
rg -A 5 "invalid.*api.?key|api.?key.*invalid" --type typescript

Length of output: 194

---

Script:

#!/bin/bash
# Let's try without the typescript file type restriction and search for API key validation patterns

# Search for API key validation in mothership-related files
rg -A 5 "apiKey.*valid|validateApiKey" 

# Search for error handling related to invalid API keys
rg -A 5 "invalid.*api.?key|api.?key.*invalid"

# Search for mothership-related files that might handle API key validation
fd mothership | xargs rg -l "apiKey|api.?key"

# Search for WebSocket connection error handling
rg -A 5 "WebSocket.*error|connection.*error" api/src/mothership

Length of output: 30436

---

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[github-actions]

This plugin has been deployed to Cloudflare R2 and is available for testing.
Download it at this URL: https://preview.dl.unraid.net/unraid-api/pr/986/dynamix.unraid.net.staging.plg

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 631fb4a and 615a3c5.

📒 Files selected for processing (4)

• api/src/__test__/mothership/api-key/validate-api-key-with-keyserver.test.ts (1 hunks)
• api/src/graphql/resolvers/mutation/connect/connect-sign-in.ts (0 hunks)
• api/src/mothership/jobs/api-key-check-jobs.ts (1 hunks)
• api/src/store/listeners/local-api-key-listener.ts (0 hunks)

💤 Files with no reviewable changes (2)

• api/src/store/listeners/local-api-key-listener.ts
• api/src/graphql/resolvers/mutation/connect/connect-sign-in.ts

🔇 Additional comments (1)

api/src/mothership/jobs/api-key-check-jobs.ts (1)

39-39: Verify the impact of returning true unconditionally

By adding return true; at this point, the function will return true whenever the API state data is fully loaded, regardless of API key validity checks. Please ensure this aligns with the intended behavior and does not introduce security risks by bypassing necessary validations.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

api/src/mothership/jobs/api-key-check-jobs.ts (1)

Line range hint 26-39: Improve code structure and validation flow

The current code structure can be improved for better efficiency and clarity:

1. The dev environment check could be moved earlier to avoid unnecessary validations
2. API key length validation should occur before checking if the key is valid

Consider restructuring the code like this:

     if (isAPIStateDataFullyLoaded(state)) {
+        if (['development'].includes(NODE_ENV)) {
+            keyServerLogger.debug('In dev environment, marking API Key as Valid');
+            dispatch(setApiKeyState(API_KEY_STATUS.API_KEY_VALID));
+            return true;
+        }
+
+        if (!isApiKeyCorrectLength(state.config.remote.apikey)) {
+            keyServerLogger.error('API Key has invalid length, logging you out.');
+            await dispatch(logoutUser({ reason: 'API Key has invalid length' }));
+            return false;
+        }
+
         if (isApiKeyValid(state)) {
             return true;
         }
-
-        if (!isApiKeyCorrectLength(state.config.remote.apikey)) {
-            keyServerLogger.error('API Key has invalid length, logging you out.');
-            await dispatch(logoutUser({ reason: 'API Key has invalid length' }));
-            return false;
-        }
-
-        if (['development'].includes(NODE_ENV)) {
-            keyServerLogger.debug('In dev environment, marking API Key as Valid');
-            dispatch(setApiKeyState(API_KEY_STATUS.API_KEY_VALID));
-            return true;
-        }

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 615a3c5 and 29399b4.

📒 Files selected for processing (5)

• api/src/__test__/mothership/api-key/validate-api-key-with-keyserver.test.ts (1 hunks)
• api/src/graphql/resolvers/mutation/connect/connect-sign-in.ts (0 hunks)
• api/src/mothership/api-key/validate-api-key-with-keyserver.ts (0 hunks)
• api/src/mothership/jobs/api-key-check-jobs.ts (1 hunks)
• api/src/store/listeners/local-api-key-listener.ts (0 hunks)

💤 Files with no reviewable changes (3)

• api/src/store/listeners/local-api-key-listener.ts
• api/src/graphql/resolvers/mutation/connect/connect-sign-in.ts
• api/src/mothership/api-key/validate-api-key-with-keyserver.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• api/src/test/mothership/api-key/validate-api-key-with-keyserver.test.ts

🔇 Additional comments (1)

api/src/mothership/jobs/api-key-check-jobs.ts (1)

Line range hint 1-45: Verify the impact of removing API key validation

The removal of the API key validation logic needs careful consideration:

1. Ensure the mothership layer is properly handling the validation
2. Verify error handling and user feedback mechanisms are in place
3. Confirm that security is not compromised during this transition

Let's verify the implementation of validation in the mothership layer:

✅ Verification successful

Let me gather more information about the validation implementation and security measures.

---

Let me check one more crucial piece - the actual validation implementation with the keyserver.

---

Based on the gathered information, I can now provide the final response:

API key validation is properly handled in multiple layers with adequate security measures

The codebase shows a robust multi-layer validation approach:

• Auth service (auth.service.ts) implements server-side validation through validateApiKeyCasbin
• Comprehensive test coverage demonstrates proper handling of various scenarios including invalid keys, network errors, and invalid responses
• The auth service throws UnauthorizedException for invalid keys
• The API key check job includes proper error handling and user feedback through logging
• Development environment checks are properly isolated

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for mothership validation implementation
echo "Searching for mothership validation implementation..."
rg -l "mothership.*validation|validateApiKey" --type ts

# Check for potential security implications
echo "Checking for other API key validation points..."
ast-grep --pattern 'validateApiKey|API_KEY_STATUS.API_KEY_VALID'

# Look for related test coverage
echo "Checking test coverage..."
fd -e test.ts -e spec.ts | xargs rg "apiKeyCheckJob|API key validation"

Length of output: 2420

---

Script:

#!/bin/bash
# Check the implementation of API key validation in auth service
echo "Checking auth service implementation..."
rg -A 10 "validateApiKey" api/src/unraid-api/auth/auth.service.ts

# Check API key listener implementation
echo "Checking API key listener..."
rg -A 10 "API_KEY_STATUS" api/src/store/listeners/api-key-listener.ts

# Check the test implementation for validation scenarios
echo "Checking test scenarios..."
cat api/src/__test__/mothership/api-key/api-key-check-jobs.test.ts

Length of output: 5444

---

Script:

#!/bin/bash
# Check the keyserver validation implementation
echo "Checking keyserver validation..."
rg -A 15 "validateApiKeyWithKeyServer" api/src/mothership/api-key/validate-api-key-with-keyserver.ts

# Check if there are any other validation points in the mothership layer
echo "Checking other mothership validation points..."
fd . api/src/mothership -e ts | xargs rg "API_KEY_STATUS.API_KEY_VALID" -A 5

Length of output: 964

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

api/src/mothership/jobs/api-key-check-jobs.ts (1)

39-39: ⚠️ Potential issue

Critical: Unconditional return true creates security vulnerability

The unconditional return true statement completely bypasses API key validation in production environments. While the PR objective indicates moving validation to the mothership layer, there's no evidence of this validation being implemented. This could potentially allow unauthorized access.

Please clarify:

1. Where exactly in the mothership layer is the validation implemented?
2. How is this validation being invoked?
3. What's preventing this code from accepting invalid API keys?

Consider implementing proper validation logic:

-        return true;
+        // Implement mothership validation logic
+        const isValid = await validateWithMothership(state.config.remote.apikey);
+        if (!isValid) {
+            keyServerLogger.error('Invalid API key detected by mothership validation');
+            await dispatch(logoutUser({ reason: 'Invalid API key' }));
+            return false;
+        }
+        return true;

🧹 Nitpick comments (1)

api/src/mothership/jobs/api-key-check-jobs.ts (1)

Line range hint 11-45: Architectural Concern: Job doesn't fulfill its intended purpose

The apiKeyCheckJob name implies this job validates API keys, but the implementation doesn't perform any actual validation. This creates a misleading security boundary where:

1. Callers expect validation to occur
2. Logs suggest validation is happening
3. But no real validation takes place

This could lead to:

• False sense of security in monitoring
• Difficulty in debugging API key issues
• Potential security vulnerabilities if other systems rely on this validation

Consider either:

1. Implementing proper mothership validation in this job
2. Removing this job entirely if validation truly belongs elsewhere
3. Renaming to better reflect its current purpose (e.g., apiKeyStateCheckJob)

Please clarify the intended architecture and how API key validation is meant to work in the new design.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 29399b4 and c3ca2ae.

📒 Files selected for processing (5)

• api/src/__test__/mothership/api-key/validate-api-key-with-keyserver.test.ts (1 hunks)
• api/src/graphql/resolvers/mutation/connect/connect-sign-in.ts (0 hunks)
• api/src/mothership/api-key/validate-api-key-with-keyserver.ts (0 hunks)
• api/src/mothership/jobs/api-key-check-jobs.ts (1 hunks)
• api/src/store/listeners/local-api-key-listener.ts (0 hunks)

💤 Files with no reviewable changes (3)

• api/src/store/listeners/local-api-key-listener.ts
• api/src/graphql/resolvers/mutation/connect/connect-sign-in.ts
• api/src/mothership/api-key/validate-api-key-with-keyserver.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• api/src/test/mothership/api-key/validate-api-key-with-keyserver.test.ts

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 34e770e and 9f2f54e.

📒 Files selected for processing (1)

• api/src/__test__/mothership/api-key/api-key-check-jobs.test.ts (1 hunks)

🔇 Additional comments (2)

api/src/__test__/mothership/api-key/api-key-check-jobs.test.ts (2)

1-7: LGTM: Clean imports aligned with new architecture

The imports are properly scoped to the current testing needs after the removal of API key validation logic.

---

1-35: Verify test coverage for removed scenarios

With the removal of several test cases (NETWORK_ERROR, INVALID_RESPONSE, etc.), we should verify that these scenarios are properly tested in the mothership layer.

Let's check for related test coverage:

✅ Verification successful

Based on the shell script results, I can now generate the final response:

Test coverage is comprehensive in the mothership layer

The removed test scenarios from api-key-check-jobs.test.ts are well covered through:

• Retry mechanism tests in retry-validate-api-key.test.ts that handle network errors and invalid responses with backoff logic
• Extensive API key validation tests in api-key.service.spec.ts covering:
  • Invalid key formats and structures
  • Network and file read errors
  • Authentication failures
  • JSON parsing errors
  • Various edge cases

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Look for mothership validation tests
fd -e test.ts -e spec.ts . | grep -i "mothership.*api.*key" | xargs cat

# Check overall test coverage for API key related code
fd -e ts . | grep -i "api.*key" | xargs rg -A 5 "describe|it\("

Length of output: 26336

[github-actions]

This plugin has been deployed to Cloudflare R2 and is available for testing.
Download it at this URL: https://preview.dl.unraid.net/unraid-api/pr/986/dynamix.unraid.net.staging.plg

[elibosley]

One piece of feedback - let's see if we can't just eliminate the validity flag on the token?

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (11)

api/src/mothership/utils/get-mothership-websocket-headers.ts (2)

Line range hint 6-6: Remove unused import isApiKeyValid

The isApiKeyValid import is no longer used after removing API key validation.

-import { isApiKeyValid } from '@app/store/getters/index';

---

Line range hint 44-52: Ensure proper error handling at mothership layer

The function now sends connection parameters without local validation. To maintain system reliability:

1. Ensure the mothership layer provides clear error responses for invalid API keys
2. Consider adding error handling documentation for failed connections

Consider implementing a retry mechanism with exponential backoff for failed connections, as removing local validation might lead to more connection attempts with invalid keys.

api/src/store/modules/minigraph.ts (1)

55-57: LGTM! Improved debug logging format.

The change from string concatenation to using the %o format specifier is a good improvement for better object logging. This will make debugging connection status changes more readable.

Note: This change appears unrelated to the PR's main objective of removing API key validation.

api/src/mothership/graphql-client.ts (3)

Line range hint 40-53: Update the doc comment to match the new state logic.
The JSDoc for isAPIStateDataFullyLoaded still references keyserver validation requirements that no longer exist. Consider revising it to avoid confusion about any lingering “API Key has passed Validation from Keyserver” references.

 /**
- * Checks that API_VERSION, config.remote.apiKey, emhttp.var.flashGuid, and emhttp.var.version are all set before returning true\
- * Also checks that the API Key has passed Validation from Keyserver
+ * Checks that API_VERSION, config.remote.apiKey, emhttp.var.flashGuid, and emhttp.var.version are all set before returning true.
  *
  * @returns boolean
  */

---

140-147: Revise documentation of createApolloLinks to remove API key mentions.
The doc block still references “... including API key validation,” but that logic has been removed. Ensure the JSDoc precisely reflects the current behavior.

---

201-207: Reconsider immediate logout on invalid API key errors.
If the mothership or other layers are now responsible for API key checks, an immediate logout may be disruptive if the error is transient or incorrectly flagged. Evaluate whether a more graceful fallback or user prompt is preferable.

api/src/store/listeners/mothership-subscription-listener.ts (2)

10-23: Improve clarity of logic for newConnectionParams and apiKey checks.
The function logic is clear, but consider clarifying the difference between a missing apiKey vs. changed connection parameters for future maintainers. Logging can specify which condition triggered the reconnect more explicitly.

---

25-33: Simplify status-based reconnection checks.
By grouping MinigraphStatus.PING_FAILURE and MinigraphStatus.PRE_INIT conditions, the code is simpler, but you might add an inline comment explaining why these statuses are singled out to avoid confusion.

api/src/store/listeners/listener-middleware.ts (1)

33-33: Initiating Mothership job listener earlier.
Placing enableMothershipJobsListener near the top ensures immediate subscription readiness. Verify that any dependencies (e.g., store state for config.remote.apikey) are already loaded when this fires to avoid missing initial events.

api/src/mothership/subscribe-to-mothership.ts (2)

3-4: Maintain consistent naming.
The new imports and references to RemoteGraphQL_Fragment maintain code clarity. Some code segments use “Mothership” while others use “minigraph.” Consider consistently referencing the same conceptual layer to help unify the naming.

---

78-89: Robustness of setupNewMothershipSubscription.
Clearing the client instance and re-creating it ensures a clean slate but may cause short-lived disconnections. Consider whether a short re-connection delay is needed in case of transient disruptions or if any graceful handoff is necessary between old and new instances.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 217a1ca and d4da204.

📒 Files selected for processing (18)

• api/src/__test__/mothership/api-key/api-key-check-jobs.test.ts (0 hunks)
• api/src/__test__/mothership/api-key/retry-validate-api-key.test.ts (0 hunks)
• api/src/graphql/resolvers/query/cloud/check-api.ts (1 hunks)
• api/src/index.ts (4 hunks)
• api/src/mothership/api-key/retry-validate-api-key.ts (0 hunks)
• api/src/mothership/graphql-client.ts (5 hunks)
• api/src/mothership/jobs/api-key-check-jobs.ts (0 hunks)
• api/src/mothership/jobs/ping-timeout-jobs.ts (5 hunks)
• api/src/mothership/subscribe-to-mothership.ts (4 hunks)
• api/src/mothership/utils/get-mothership-websocket-headers.ts (2 hunks)
• api/src/store/index.ts (0 hunks)
• api/src/store/listeners/api-key-listener.ts (0 hunks)
• api/src/store/listeners/listener-middleware.ts (1 hunks)
• api/src/store/listeners/mothership-subscription-listener.ts (1 hunks)
• api/src/store/modules/apikey.ts (0 hunks)
• api/src/store/modules/config.ts (2 hunks)
• api/src/store/modules/minigraph.ts (1 hunks)
• api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts (3 hunks)

💤 Files with no reviewable changes (7)

• api/src/test/mothership/api-key/api-key-check-jobs.test.ts
• api/src/store/listeners/api-key-listener.ts
• api/src/mothership/jobs/api-key-check-jobs.ts
• api/src/store/modules/apikey.ts
• api/src/test/mothership/api-key/retry-validate-api-key.test.ts
• api/src/store/index.ts
• api/src/mothership/api-key/retry-validate-api-key.ts

✅ Files skipped from review due to trivial changes (2)

• api/src/graphql/resolvers/query/cloud/check-api.ts
• api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts

🔇 Additional comments (13)

api/src/mothership/utils/get-mothership-websocket-headers.ts (1)

Line range hint 20-31: Verify architectural changes for WebSocket security

The function now sends the API key without local validation, assuming validation occurs at the mothership layer. While this aligns with the PR objectives, we should verify:

1. The mothership layer is properly validating these headers
2. There's no security gap during the WebSocket connection establishment

✅ Verification successful

API key validation is handled properly at the mothership layer

The code analysis reveals a robust security architecture:

1. The WebSocket connection is secured through multiple layers:

   • Headers are only sent when API state data is fully loaded (isAPIStateDataFullyLoaded)
   • The mothership validates the API key and disconnects on invalid keys (isInvalidApiKeyError triggers logout)
   • Connection health is monitored through ping messages
   • The system maintains connection state tracking and reconnection logic

2. The architectural change aligns with the new design where:

   • API key validation is centralized at the mothership layer
   • Local validation is removed to prevent duplicate checks
   • Invalid keys trigger immediate disconnection and user logout

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for API key validation in mothership-related files
rg -A 5 "x-api-key|apiKey" --type ts src/mothership

# Look for WebSocket connection handling in mothership
ast-grep --pattern 'class $_ {
  $$$
  connect($$$) {
    $$$
  }
  $$$
}'

Length of output: 185

---

Script:

#!/bin/bash
# Search for API key validation in mothership-related files
rg -A 5 "x-api-key|apiKey" --type ts api/src/mothership

# Look for WebSocket connection handling
rg -A 10 "WebSocket|websocket" --type ts api/src/mothership

Length of output: 13517

api/src/store/modules/minigraph.ts (1)

Line range hint 1-100: Verify connection states remain valid after API key validation removal.

Since API key validation is being removed from the connect sign-in process, we should verify that none of the connection states or error handling in this file were previously dependent on API key validation results.

api/src/mothership/graphql-client.ts (3)

1-9: Imports are consistent with the updated architecture.
All newly introduced imports align with the WebSocket and Apollo Client functionalities. The approach to use specialized imports from graphql-ws and @apollo/client is valid.

---

69-71: Check if we need additional validation beyond the loaded state.
By removing the API key validation requirement, the code simply checks for the presence of config data. If the business logic still mandates a deeper validation, consider introducing a new approach (e.g., token introspection at a different layer) or confirm that this omission is intentional.

---

264-279: Ensure clearing event handlers is appropriate for concurrency.
Replacing each event callback with a no-op effectively silences those events. Ensure no concurrent processes depend on these events being handled. If concurrency or multiple clients can exist simultaneously, verifying that no further event emissions are pending might be beneficial.

api/src/store/listeners/mothership-subscription-listener.ts (2)

2-8: Imports & references.
The newly introduced imports for setupNewMothershipSubscription and MinigraphStatus strongly align with the updated subscription architecture. Confirm that any removed references to API key checks are no longer needed here.

---

37-40: Success logging & subscription flow.
The trace log "Renewing mothership subscription" is a sensible notice before calling setupNewMothershipSubscription. Confirm that other logs won't duplicate this message with higher severity, possibly causing confusion in logs.

api/src/mothership/subscribe-to-mothership.ts (1)

27-31: Efficient error logging in subscription.
The shift to a single-line error log for GraphQL subscription errors is clean and more readable. Confirm that any additional error handling logic (like retry or fallback) is not needed if subscription errors are frequent.

api/src/index.ts (2)

116-118: LGTM! Enhanced error logging

The error handling has been improved with structured logging, providing better visibility into both Error instances and unexpected errors.

---

76-77: Verify initialization sequence

The setupNewMothershipSubscription is called after loading configs but before starting listeners. This sequence seems logical, but we should verify that the subscription setup doesn't depend on any of the subsequent initializations.

✅ Verification successful

Initialization sequence is correctly ordered

The verification shows that setupNewMothershipSubscription is independent of subsequent initializations and is properly placed in the sequence:

• It only depends on the store state and GraphQL client setup
• It runs after startStoreSync and setupLogRotation
• It correctly runs before file watchers (StateManager, Registration, VarRun) as these are independent systems
• The subscription setup is also properly handled in case of connection parameter changes through the store listener

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any dependencies on StateManager, Registration, or VarRun in the subscription setup
ast-grep --pattern 'setupNewMothershipSubscription() {
  $$$
  StateManager.$$$
  $$$
}'

ast-grep --pattern 'setupNewMothershipSubscription() {
  $$$
  setupRegistrationKeyWatch.$$$
  $$$
}'

ast-grep --pattern 'setupNewMothershipSubscription() {
  $$$
  setupVarRunWatch.$$$
  $$$
}'

Length of output: 289

---

Script:

#!/bin/bash
# Let's first find the implementation of setupNewMothershipSubscription
rg -A 10 "setupNewMothershipSubscription" --type ts

# Also check for any imports or dependencies in the file
rg "import.*StateManager|import.*Registration|import.*VarRun" --type ts api/src/index.ts

# Check the order of initialization in index.ts
rg "^[^/].*setup|^[^/].*init|^[^/].*start" api/src/index.ts

Length of output: 4919

api/src/mothership/jobs/ping-timeout-jobs.ts (2)

83-91: LGTM! Well-implemented singleton pattern

The singleton pattern implementation for PingTimeoutJobs is clean and includes proper initialization checks. The return value provides useful feedback about the job's running state.

---

93-102: LGTM! Thorough cleanup implementation

The stopPingTimeoutJobs function implements a complete cleanup sequence:

1. Stops all jobs
2. Clears the instance
3. Nullifies the reference
4. Includes proper logging and null checks

api/src/store/modules/config.ts (1)

110-111: LGTM! Comprehensive logout cleanup

The logout process now includes proper cleanup of background jobs and GraphQL client, which aligns with the PR objective of moving API key validation to the mothership layer.

[github-actions]

This plugin has been deployed to Cloudflare R2 and is available for testing.
Download it at this URL: https://preview.dl.unraid.net/unraid-api/pr/986/dynamix.unraid.net.staging.plg

[mdatelle]

Looks good on my end other than Eli's request.