feat: initial version of modification service #1078
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of the Single Sign-On (SSO) and authentication request handling in the Unraid API. The changes involve removing existing SSO setup files, creating a new Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 3
🧹 Nitpick comments (7)
api/src/unraid-api/unraid-file-modifier/modifications/sso.modification.ts (2)
18-19: Verify file existence checks
Inapply(), the code first attempts to restore the file, then creates a backup. Confirm that the correct file content is backed up if the file did not exist or was already in a modified state. Consider validating file existence and logging any anomalies to ensure the workflow is clearly traceable.Also applies to: 49-53
84-90: Test coverage recommendation
shouldApply()is a crucial decision point controlling the final applied outcome. Please ensure there's unit or integration test coverage validating both branches of this logic (SSO configured vs. not configured).api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts (4)
12-18: Interface documentation
TheFileModificationinterface is straightforward, but consider thoroughly documenting each method's expected side effects (e.g., file manipulations, backups) to guide implementers on best practices.
22-25: Potential concurrency concerns
If multiple modules attempt to apply modifications concurrently, the sharedhistoryarray isn’t protected. Consider thread safety measures or concurrency locks if the system is expected to modify files in parallel.
71-74: Skip reason logging
WhenshouldApplyis false, the reason is logged, which is good. However, you may want to log in "debug" level if the skip is a normal condition, or in "warn" level if you anticipate that a skip is unusual or noteworthy in production.
94-119: Rollback consistency
Rolling back in reverse order of applied modifications is logical. Confirm that eachrollback()method can handle partial rollbacks gracefully if an earlier modification fails to revert. Consider adding a fallback strategy (e.g. log an error, continue rolling back the rest).api/src/unraid-api/unraid-file-modifier/modifications/auth-request.modification.ts (1)
11-12: Consider making file paths configurable.Hard-coded file paths make the code less maintainable and harder to test. Consider:
- Moving paths to configuration
- Adding environment variable support
- Creating path utility functions
-const AUTH_REQUEST_FILE = '/usr/local/emhttp/auth-request.php' as const; -const WEB_COMPS_DIR = '/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components/_nuxt/' as const; +import { ConfigService } from '@nestjs/config'; + +constructor( + private readonly logger: Logger, + private readonly config: ConfigService, +) { + this.logger = logger; +} + +private get authRequestFile(): string { + return this.config.get('AUTH_REQUEST_FILE') ?? '/usr/local/emhttp/auth-request.php'; +} + +private get webCompsDir(): string { + return this.config.get('WEB_COMPS_DIR') ?? '/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components/_nuxt/'; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (10)
api/src/core/sso/auth-request-setup.ts(0 hunks)api/src/core/sso/sso-remove.ts(0 hunks)api/src/core/sso/sso-setup.ts(0 hunks)api/src/index.ts(1 hunks)api/src/unraid-api/app/app.module.ts(2 hunks)api/src/unraid-api/unraid-file-modifier/modifications/auth-request.modification.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/sso.modification.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.module.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.spec.ts(1 hunks)
💤 Files with no reviewable changes (3)
- api/src/core/sso/sso-remove.ts
- api/src/core/sso/auth-request-setup.ts
- api/src/core/sso/sso-setup.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Test API
- GitHub Check: Build Web App
🔇 Additional comments (7)
api/src/unraid-api/unraid-file-modifier/modifications/sso.modification.ts (2)
32-44: Security considerations for shell execution
Whileescapeshellargprovides some level of sanitization, relying on system commands can be risky. Double-check that the environment, the command used to callexec, and the JWT validation logic are sufficiently locked down to avoid command injection.
46-48: Confirm availability of$docroot
Before injecting<?php include "$docroot/..."; ?>, verify that$docrootis defined within the context of.login.php. Otherwise, this include path might fail.api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts (1)
126-135: Backup file overwriting
When performingbackupFile, if a backup already exists, this will overwrite it. Recommend verifying that overwriting is intended behavior or incorporate versioned backups to preserve prior states.api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.module.ts (1)
1-6: Proper encapsulation in module
ProvidingFileModificationServiceas a provider is a clean design choice. Ensure that any additional file modifications are also added or dynamically loaded through this module to centralize file management logic.api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.spec.ts (1)
5-19: Enhance test coverage for UnraidFileModifierService.The current test suite only verifies service instantiation. Consider adding tests for:
- File modification operations
- Error handling scenarios
- Edge cases (file permissions, non-existent files)
- Rollback functionality
Would you like me to help generate additional test cases?
api/src/unraid-api/app/app.module.ts (1)
14-14: LGTM! Clean integration of UnraidFileModifierModule.The module is properly imported and integrated into the AppModule.
Also applies to: 34-34
api/src/index.ts (1)
102-104: LGTM! Proper async handling in exit hook.The exit hook is correctly modified to handle async operations, ensuring proper cleanup before process exit.
api/src/unraid-api/unraid-file-modifier/modifications/sso.modification.ts
Show resolved
Hide resolved
| async apply(): Promise<void> { | ||
| const JS_FILES = await getJsFiles(WEB_COMPS_DIR); | ||
| this.logger.debug(`Found ${JS_FILES.length} .js files in ${WEB_COMPS_DIR}`); | ||
|
|
||
| const FILES_TO_ADD = ['/webGui/images/partner-logo.svg', ...JS_FILES]; | ||
|
|
||
| if (existsSync(AUTH_REQUEST_FILE)) { | ||
| const fileContent = await readFile(AUTH_REQUEST_FILE, 'utf8'); | ||
|
|
||
| if (fileContent.includes('$arrWhitelist')) { | ||
| FileModificationService.backupFile(AUTH_REQUEST_FILE, true); | ||
| this.logger.debug(`Backup of ${AUTH_REQUEST_FILE} created.`); | ||
|
|
||
| const filesToAddString = FILES_TO_ADD.map((file) => ` '${file}',`).join('\n'); | ||
|
|
||
| const updatedContent = fileContent.replace( | ||
| /(\$arrWhitelist\s*=\s*\[)/, | ||
| `$1\n${filesToAddString}` | ||
| ); | ||
|
|
||
| await writeFile(AUTH_REQUEST_FILE, updatedContent); | ||
| this.logger.debug( | ||
| `Default values and .js files from ${WEB_COMPS_DIR} added to $arrWhitelist.` | ||
| ); | ||
| } else { | ||
| this.logger.debug(`$arrWhitelist array not found in the file.`); | ||
| } | ||
| } else { | ||
| this.logger.debug(`File ${AUTH_REQUEST_FILE} not found.`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add robust error handling for file operations.
The current implementation lacks proper error handling for file operations. Consider:
- Adding try-catch blocks
- Validating file permissions
- Checking file content format
- Adding file operation timeouts
async apply(): Promise<void> {
+ try {
+ await this.validateFilePermissions(AUTH_REQUEST_FILE);
+
const JS_FILES = await getJsFiles(WEB_COMPS_DIR);
this.logger.debug(`Found ${JS_FILES.length} .js files in ${WEB_COMPS_DIR}`);
const FILES_TO_ADD = ['/webGui/images/partner-logo.svg', ...JS_FILES];
if (existsSync(AUTH_REQUEST_FILE)) {
const fileContent = await readFile(AUTH_REQUEST_FILE, 'utf8');
+
+ if (!this.isValidPhpContent(fileContent)) {
+ throw new Error('Invalid PHP file format');
+ }
if (fileContent.includes('$arrWhitelist')) {
FileModificationService.backupFile(AUTH_REQUEST_FILE, true);
this.logger.debug(`Backup of ${AUTH_REQUEST_FILE} created.`);
const filesToAddString = FILES_TO_ADD.map((file) => ` '${file}',`).join('\n');
const updatedContent = fileContent.replace(
/(\$arrWhitelist\s*=\s*\[)/,
`$1\n${filesToAddString}`
);
- await writeFile(AUTH_REQUEST_FILE, updatedContent);
+ await this.writeFileWithTimeout(AUTH_REQUEST_FILE, updatedContent);
this.logger.debug(
`Default values and .js files from ${WEB_COMPS_DIR} added to $arrWhitelist.`
);
} else {
- this.logger.debug(`$arrWhitelist array not found in the file.`);
+ throw new Error('$arrWhitelist array not found in the file');
}
} else {
- this.logger.debug(`File ${AUTH_REQUEST_FILE} not found.`);
+ throw new Error(`File ${AUTH_REQUEST_FILE} not found`);
}
+ } catch (error) {
+ this.logger.error(`Failed to apply auth request modification: ${error.message}`);
+ throw error;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| async rollback(): Promise<void> { | ||
| // No rollback needed, this is safe to preserve | ||
| } |
There was a problem hiding this comment.
Implement proper rollback functionality.
The current rollback implementation is empty with a comment suggesting it's safe to preserve. This needs proper implementation or justification.
async rollback(): Promise<void> {
- // No rollback needed, this is safe to preserve
+ try {
+ const backupFile = FileModificationService.getBackupFilePath(AUTH_REQUEST_FILE);
+ if (existsSync(backupFile)) {
+ await this.validateFilePermissions(backupFile);
+ await this.validateFilePermissions(AUTH_REQUEST_FILE);
+ await copyFile(backupFile, AUTH_REQUEST_FILE);
+ this.logger.debug(`Rolled back ${AUTH_REQUEST_FILE} to backup`);
+ } else {
+ this.logger.warn(`No backup file found for ${AUTH_REQUEST_FILE}`);
+ }
+ } catch (error) {
+ this.logger.error(`Failed to rollback auth request modification: ${error.message}`);
+ throw error;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts (6)
12-19: Consider adding modification ordering.
TheFileModificationinterface is clearly defined. However, if modifications need to be applied in a predictable order, apriorityfield might be beneficial.
26-34: Consider partial rollback on initialization errors.
If some modifications succeed but a later one fails, the service does not immediately roll back the successful ones. Consider partial rollback logic to avoid leaving half-applied modifications.
35-42: Confirm rollback on module shutdown.
Rolling back all modifications on module destroy might be intentional, but ensure it aligns with the desired lifecycle: if persistent changes are needed, consider a different approach.
44-58: Static load is not fully dynamic.
Despite mentioning “dynamically load all file modifications,” this method uses a hard-coded array. Consider discovering modification classes at runtime for easier scalability.
125-140: Backup method handles missing file scenarios.
Consider checking for an existing backup file to avoid accidental overwrites or for supporting multiple backups.
141-161: Restore method unlinks backup automatically.
If you need to preserve backup files for debugging or auditing, consider a configurable toggle rather than always deleting them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.module.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.spec.ts
- api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.module.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts (7)
1-3: Imports look good.
No issues with these imports; they are consistent with NestJS and Node's asynchronous filesystem methods.
4-5: Absolute path imports are clear and consistent.
No immediate issues; they help maintain clarity for referencing modifications.
7-10: Good approach for providing context on applicability.
ThisShouldApplyWithReasoninterface appropriately encapsulates logic and explanation for skipping or applying modifications.
20-25: Service class name is descriptive.
Declaring this as an injectable service is good design for dependency injection within NestJS.
60-64: Sequential application is straightforward.
This method’s loop ensures linear application. If performance becomes a concern, batching or parallelization could be considered, though it may complicate rollback.
66-96: Robust error handling in modification application.
Properly logs errors, rethrows them, and avoids partial state confusion. Pushing the modification to history after successful application is appropriate.
98-123: Rollback logic is clear and complete.
Rolling back in reverse order is essential. It’s good that each rollback is attempted, even if one fails.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
api/src/unraid-api/cli/log.service.ts (1)
14-20: Consider caching log level index for better performance.The
shouldLogmethod performs array index lookup on every call. Consider caching the index ofLOG_LEVELduring initialization for better performance.export class LogService { private logger = console; + private readonly configuredLogLevelIndex: number; + + constructor() { + this.configuredLogLevelIndex = levels.indexOf(LOG_LEVEL.toLowerCase() as LogLevel); + } shouldLog(level: LogLevel): boolean { - const logLevelsLowToHigh = levels; - return ( - logLevelsLowToHigh.indexOf(level) >= - logLevelsLowToHigh.indexOf(LOG_LEVEL.toLowerCase() as LogLevel) - ); + return levels.indexOf(level) >= this.configuredLogLevelIndex; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (12)
api/ecosystem.config.json(1 hunks)api/src/core/logrotate/setup-logrotate.ts(0 hunks)api/src/index.ts(1 hunks)api/src/unraid-api/cli/log.service.ts(2 hunks)api/src/unraid-api/cli/start.command.ts(2 hunks)api/src/unraid-api/cron/cron.module.ts(1 hunks)api/src/unraid-api/cron/log-cleanup.service.ts(0 hunks)api/src/unraid-api/unraid-file-modifier/modifications/auth-request.modification.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/sso.modification.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.spec.ts(1 hunks)api/src/utils.ts(2 hunks)
💤 Files with no reviewable changes (2)
- api/src/unraid-api/cron/log-cleanup.service.ts
- api/src/core/logrotate/setup-logrotate.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/unraid-api/unraid-file-modifier/modifications/auth-request.modification.ts
- api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts
🧰 Additional context used
📓 Learnings (1)
api/src/unraid-api/unraid-file-modifier/modifications/sso.modification.ts (1)
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.
⏰ 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 (6)
api/src/unraid-api/cron/cron.module.ts (1)
7-7: Verify the removal of log cleanup functionality.The removal of
LogCleanupServicefrom providers eliminates the scheduled log management tasks. Please confirm:
- Is this removal intentional as part of the SSO refactoring?
- How will log rotation be handled going forward to prevent unconstrained log growth?
Let's check if log management has been moved elsewhere:
✅ Verification successful
Log cleanup is properly handled by PM2 logrotate
The removal of
LogCleanupServiceis valid as log rotation is managed by PM2's logrotate module, which is configured to:
- Retain 2 log files
- Compress old logs
- Rotate at 1MB file size
This process-level log management is more robust than the previous cron-based solution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any new log management implementations rg -l "logrotate|log.*cleanup|log.*rotation" --type ts # Check for any systemd timer or cron job configurations that might handle logs fd -e timer -e service -e conf "log"Length of output: 150
Script:
#!/bin/bash # Check content of files with log-related matches echo "=== Content of api/src/index.ts ===" rg -A 5 -B 5 "logrotate|log.*cleanup|log.*rotation" api/src/index.ts echo -e "\n=== Content of api/src/unraid-api/cli/start.command.ts ===" rg -A 5 -B 5 "logrotate|log.*cleanup|log.*rotation" api/src/unraid-api/cli/start.command.ts # Look for logging configuration echo -e "\n=== Logging configuration patterns ===" rg -A 5 "createLogger|winston|logging\..*Config" --type tsLength of output: 2318
api/ecosystem.config.json (2)
2-2: LGTM! Schema declaration improves configuration validation.Adding the PM2 ecosystem schema helps catch configuration errors early.
10-10: Verify impact of reduced listen timeout.The listen_timeout has been reduced from 30s to 15s. This could affect long-running operations.
Please confirm that 15 seconds is sufficient for all API operations to complete, particularly during high load or when dealing with large datasets.
api/src/unraid-api/unraid-file-modifier/modifications/sso.modification.ts (1)
84-90: LGTM! Clean implementation of shouldApply.The shouldApply method correctly checks configuration and provides clear reasons.
api/src/index.ts (1)
Line range hint
100-108: LGTM! Improved shutdown handling.The async exit hook ensures proper server cleanup before shutdown.
api/src/unraid-api/cli/log.service.ts (1)
40-43: Consider standardizing error handling across logging methods.The
errormethod accepts an optional trace parameter, but it's not consistently used across error logging calls in the codebase.
| describe('FileModificationService', () => { | ||
| let service: UnraidFileModificationService; | ||
|
|
||
| beforeEach(async () => { | ||
| const module: TestingModule = await Test.createTestingModule({ | ||
| providers: [UnraidFileModificationService], | ||
| }).compile(); | ||
|
|
||
| service = module.get<UnraidFileModificationService>(UnraidFileModificationService); | ||
| }); | ||
|
|
||
| it('should be defined', () => { | ||
| expect(service).toBeDefined(); | ||
| }); | ||
|
|
||
| it('should load modifications', async () => { | ||
| const mods = await service.loadModifications(); | ||
| expect(mods).toHaveLength(2); | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for FileModificationService.
While the basic tests are good, consider adding test cases for:
- Error handling scenarios
- File modification operations (apply/rollback)
- Edge cases in shouldApply logic
Example test structure:
it('should handle errors during modification application', async () => {
// Test setup with invalid file paths or permissions
await expect(service.apply()).rejects.toThrow();
});
it('should correctly rollback modifications', async () => {
// Test backup and restore functionality
await service.apply();
await service.rollback();
// Verify original state
});
it('should correctly evaluate shouldApply conditions', async () => {
// Test various configuration scenarios
const result = await service.shouldApply();
expect(result.shouldApply).toBeDefined();
expect(result.reason).toBeDefined();
});
api/src/unraid-api/unraid-file-modifier/modifications/sso.modification.ts
Show resolved
Hide resolved
| // Restore the original file if exists | ||
| await restoreFile(this.loginFilePath, false); | ||
| // Backup the original content | ||
| await backupFile(this.loginFilePath, true); | ||
|
|
||
| // Read the file content | ||
| let fileContent = await readFile(this.loginFilePath, 'utf-8'); | ||
|
|
||
| // Add new function after the opening PHP tag (<?php) | ||
| fileContent = fileContent.replace(/<\?php\s*(\r?\n|\r)*/, `<?php\n\n${newFunction}\n`); | ||
|
|
||
| // Replace the old function call with the new function name | ||
| const functionCallPattern = /!verifyUsernamePassword\(\$username, \$password\)/g; | ||
| fileContent = fileContent.replace( | ||
| functionCallPattern, | ||
| '!verifyUsernamePasswordAndSSO($username, $password)' | ||
| ); | ||
|
|
||
| // Inject the PHP include tag after the closing </form> tag | ||
| fileContent = fileContent.replace(/<\/form>/i, `</form>\n${tagToInject}`); | ||
|
|
||
| // Write the updated content back to the file | ||
| await writeFile(this.loginFilePath, fileContent); | ||
|
|
There was a problem hiding this comment.
Add error handling for file operations.
File operations lack proper error handling which could lead to incomplete modifications or corrupted files.
Add try-catch blocks:
+ try {
// Restore the original file if exists
await restoreFile(this.loginFilePath, false);
// Backup the original content
await backupFile(this.loginFilePath, true);
// Read the file content
let fileContent = await readFile(this.loginFilePath, 'utf-8');
// ... file modifications ...
// Write the updated content back to the file
await writeFile(this.loginFilePath, fileContent);
+ } catch (error) {
+ this.logger.error('Failed to modify login file:', error);
+ await this.rollback();
+ throw error;
+ }Committable suggestion skipped: line range outside the PR's diff.
| async configurePm2(): Promise<void> { | ||
| if (existsSync('/tmp/pm2-configured')) { | ||
| return; | ||
| } | ||
| // Write a temp file when first started to prevent needing to run this again | ||
| // Install PM2 Logrotate | ||
| await execa(PM2_PATH, ['install', 'pm2-logrotate']) | ||
| .then(({ stdout }) => { | ||
| this.logger.debug(stdout); | ||
| }) | ||
| .catch(({ stderr }) => { | ||
| this.logger.error('PM2 Logrotate Error: ' + stderr); | ||
| }); | ||
| // Now set logrotate options | ||
| await execa(PM2_PATH, ['set', 'pm2-logrotate:retain', '2']) | ||
| .then(({ stdout }) => this.logger.debug(stdout)) | ||
| .catch(({ stderr }) => this.logger.error('PM2 Logrotate Set Error: ' + stderr)); | ||
| await execa(PM2_PATH, ['set', 'pm2-logrotate:compress', 'true']) | ||
| .then(({ stdout }) => this.logger.debug(stdout)) | ||
| .catch(({ stderr }) => this.logger.error('PM2 Logrotate Compress Error: ' + stderr)); | ||
| await execa(PM2_PATH, ['set', 'pm2-logrotate:max_size', '1M']) | ||
| .then(({ stdout }) => this.logger.debug(stdout)) | ||
| .catch(({ stderr }) => this.logger.error('PM2 Logrotate Max Size Error: ' + stderr)); | ||
|
|
||
| // PM2 Save Settings | ||
| await execa(PM2_PATH, ['set', 'pm2:autodump', 'true']) | ||
| .then(({ stdout }) => this.logger.debug(stdout)) | ||
| .catch(({ stderr }) => this.logger.error('PM2 Autodump Error: ' + stderr)); | ||
|
|
||
| // Update PM2 | ||
| await execa(PM2_PATH, ['update']) | ||
| .then(({ stdout }) => this.logger.debug(stdout)) | ||
| .catch(({ stderr }) => this.logger.error('PM2 Update Error: ' + stderr)); | ||
|
|
||
| await writeFile('/tmp/pm2-configured', 'true'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance PM2 configuration reliability and performance.
Several improvements can be made to the configurePm2 method:
- The temporary file in
/tmpmight be cleared on reboot - No error handling for writeFile operation
- PM2 commands could be parallelized for better performance
async configurePm2(): Promise<void> {
- if (existsSync('/tmp/pm2-configured')) {
+ const PM2_CONFIG_FILE = '/var/lib/unraid/pm2-configured';
+ if (existsSync(PM2_CONFIG_FILE)) {
return;
}
- // Write a temp file when first started to prevent needing to run this again
- // Install PM2 Logrotate
- await execa(PM2_PATH, ['install', 'pm2-logrotate'])
- .then(({ stdout }) => {
- this.logger.debug(stdout);
- })
- .catch(({ stderr }) => {
- this.logger.error('PM2 Logrotate Error: ' + stderr);
- });
- // Now set logrotate options
- await execa(PM2_PATH, ['set', 'pm2-logrotate:retain', '2'])
- .then(({ stdout }) => this.logger.debug(stdout))
- .catch(({ stderr }) => this.logger.error('PM2 Logrotate Set Error: ' + stderr));
- await execa(PM2_PATH, ['set', 'pm2-logrotate:compress', 'true'])
- .then(({ stdout }) => this.logger.debug(stdout))
- .catch(({ stderr }) => this.logger.error('PM2 Logrotate Compress Error: ' + stderr));
- await execa(PM2_PATH, ['set', 'pm2-logrotate:max_size', '1M'])
- .then(({ stdout }) => this.logger.debug(stdout))
- .catch(({ stderr }) => this.logger.error('PM2 Logrotate Max Size Error: ' + stderr));
-
- // PM2 Save Settings
- await execa(PM2_PATH, ['set', 'pm2:autodump', 'true'])
- .then(({ stdout }) => this.logger.debug(stdout))
- .catch(({ stderr }) => this.logger.error('PM2 Autodump Error: ' + stderr));
-
- // Update PM2
- await execa(PM2_PATH, ['update'])
- .then(({ stdout }) => this.logger.debug(stdout))
- .catch(({ stderr }) => this.logger.error('PM2 Update Error: ' + stderr));
-
- await writeFile('/tmp/pm2-configured', 'true');
+ try {
+ // Install PM2 Logrotate and configure settings in parallel
+ const commands = [
+ ['install', 'pm2-logrotate'],
+ ['set', 'pm2-logrotate:retain', '2'],
+ ['set', 'pm2-logrotate:compress', 'true'],
+ ['set', 'pm2-logrotate:max_size', '1M'],
+ ['set', 'pm2:autodump', 'true'],
+ ];
+
+ await Promise.all(
+ commands.map(async (args) => {
+ try {
+ const { stdout } = await execa(PM2_PATH, args);
+ this.logger.debug(stdout);
+ } catch (error) {
+ this.logger.error(`PM2 Command Error: ${args.join(' ')}`, error.stderr);
+ throw error;
+ }
+ })
+ );
+
+ // Update PM2 after all settings are configured
+ const { stdout } = await execa(PM2_PATH, ['update']);
+ this.logger.debug(stdout);
+
+ await writeFile(PM2_CONFIG_FILE, 'true').catch((error) => {
+ this.logger.error('Failed to write PM2 config file', error);
+ throw error;
+ });
+ } catch (error) {
+ this.logger.error('PM2 configuration failed', error);
+ throw error;
+ }
}📝 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.
| async configurePm2(): Promise<void> { | |
| if (existsSync('/tmp/pm2-configured')) { | |
| return; | |
| } | |
| // Write a temp file when first started to prevent needing to run this again | |
| // Install PM2 Logrotate | |
| await execa(PM2_PATH, ['install', 'pm2-logrotate']) | |
| .then(({ stdout }) => { | |
| this.logger.debug(stdout); | |
| }) | |
| .catch(({ stderr }) => { | |
| this.logger.error('PM2 Logrotate Error: ' + stderr); | |
| }); | |
| // Now set logrotate options | |
| await execa(PM2_PATH, ['set', 'pm2-logrotate:retain', '2']) | |
| .then(({ stdout }) => this.logger.debug(stdout)) | |
| .catch(({ stderr }) => this.logger.error('PM2 Logrotate Set Error: ' + stderr)); | |
| await execa(PM2_PATH, ['set', 'pm2-logrotate:compress', 'true']) | |
| .then(({ stdout }) => this.logger.debug(stdout)) | |
| .catch(({ stderr }) => this.logger.error('PM2 Logrotate Compress Error: ' + stderr)); | |
| await execa(PM2_PATH, ['set', 'pm2-logrotate:max_size', '1M']) | |
| .then(({ stdout }) => this.logger.debug(stdout)) | |
| .catch(({ stderr }) => this.logger.error('PM2 Logrotate Max Size Error: ' + stderr)); | |
| // PM2 Save Settings | |
| await execa(PM2_PATH, ['set', 'pm2:autodump', 'true']) | |
| .then(({ stdout }) => this.logger.debug(stdout)) | |
| .catch(({ stderr }) => this.logger.error('PM2 Autodump Error: ' + stderr)); | |
| // Update PM2 | |
| await execa(PM2_PATH, ['update']) | |
| .then(({ stdout }) => this.logger.debug(stdout)) | |
| .catch(({ stderr }) => this.logger.error('PM2 Update Error: ' + stderr)); | |
| await writeFile('/tmp/pm2-configured', 'true'); | |
| } | |
| async configurePm2(): Promise<void> { | |
| const PM2_CONFIG_FILE = '/var/lib/unraid/pm2-configured'; | |
| if (existsSync(PM2_CONFIG_FILE)) { | |
| return; | |
| } | |
| try { | |
| // Install PM2 Logrotate and configure settings in parallel | |
| const commands = [ | |
| ['install', 'pm2-logrotate'], | |
| ['set', 'pm2-logrotate:retain', '2'], | |
| ['set', 'pm2-logrotate:compress', 'true'], | |
| ['set', 'pm2-logrotate:max_size', '1M'], | |
| ['set', 'pm2:autodump', 'true'], | |
| ]; | |
| await Promise.all( | |
| commands.map(async (args) => { | |
| try { | |
| const { stdout } = await execa(PM2_PATH, args); | |
| this.logger.debug(stdout); | |
| } catch (error) { | |
| this.logger.error(`PM2 Command Error: ${args.join(' ')}`, error.stderr); | |
| throw error; | |
| } | |
| }) | |
| ); | |
| // Update PM2 after all settings are configured | |
| const { stdout } = await execa(PM2_PATH, ['update']); | |
| this.logger.debug(stdout); | |
| await writeFile(PM2_CONFIG_FILE, 'true').catch((error) => { | |
| this.logger.error('Failed to write PM2 config file', error); | |
| throw error; | |
| }); | |
| } catch (error) { | |
| this.logger.error('PM2 configuration failed', error); | |
| throw error; | |
| } | |
| } |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
api/src/index.ts (1)
SSO functionality is still active and needs to be removed
The SSO functionality is still actively integrated in the codebase:
UnraidFileModificationServiceactively loads and applies SSO modifications- Complete set of SSO-related CLI commands and supporting files exist at
api/src/unraid-api/cli/sso/- SSO modification implementation at
api/src/unraid-api/unraid-file-modifier/modifications/sso.modification.ts🔗 Analysis chain
Line range hint
1-97: Verify complete removal of SSO functionality.The SSO setup logic has been removed, but we should verify that all related code and dependencies have been cleaned up.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining SSO-related code echo "Searching for SSO-related files and code..." rg -l 'sso|auth.*request|single.*sign.*on' --type ts # Search for any remaining SSO-related imports echo "Searching for SSO-related imports..." ast-grep --pattern 'import { $_ } from $path' | rg -i 'sso|auth.*request'Length of output: 1370
Script:
#!/bin/bash # Check for active usage of SSO components echo "Checking imports of SSO modifications..." rg -p "import.*sso\.modification" -B2 -A2 echo "Checking for SSO CLI command registration..." rg -p "SsoCommand" -B2 -A2 echo "Checking for SSO config usage..." ast-grep --pattern 'sso: $_' echo "Checking for active SSO functionality calls..." rg -p "\.sso\." -B2 -A2Length of output: 839
Script:
#!/bin/bash # Check usage of SSOFileModification in the service echo "Checking usage of SSO modification in service..." rg -p "SSOFileModification" -B5 -A10 "unraid-file-modifier.service.ts" # Also check for any references to this service echo "Checking references to UnraidFileModifierService..." rg -p "UnraidFileModifierService" -B2 -A2Length of output: 402
Script:
#!/bin/bash # Verify the exact path and content echo "Checking exact file paths..." fd -t f "unraid-file-modifier.service.ts" echo "Checking content if found..." find . -name "unraid-file-modifier.service.ts" -type f -exec cat {} \; echo "Double checking SSO files with full paths..." fd -t f -e ts "sso"Length of output: 5681
♻️ Duplicate comments (1)
api/src/unraid-api/cli/start.command.ts (1)
23-59:⚠️ Potential issueImprove PM2 configuration reliability.
Previous review already covered performance optimizations. However, there are critical reliability concerns that need to be addressed:
- Using
/tmp/pm2-configuredis unreliable as it may be cleared on system reboot- No error handling for the
writeFileoperation- Failed PM2 commands don't prevent writing the configuration flag
Consider moving the flag file to a persistent location like
/var/lib/unraid/and implementing proper error handling.
🧹 Nitpick comments (3)
api/src/environment.ts (2)
15-17: Align ENVIRONMENT types with NODE_ENV.The
ENVIRONMENTtype is more restrictive thanNODE_ENV. Consider adding 'test' to maintain consistency:- ? (process.env.ENVIRONMENT as 'production' | 'staging' | 'development') + ? (process.env.ENVIRONMENT as 'production' | 'staging' | 'development' | 'test')
30-34: Consider extracting URLs to configuration.While the implementation is functional, consider these improvements:
- Extract URLs to a configuration object/file for easier maintenance
- Add URL validation when using custom MOTHERSHIP_GRAPHQL_LINK
Example approach:
const MOTHERSHIP_URLS = { staging: 'https://staging.mothership.unraid.net/ws', production: 'https://mothership.unraid.net/ws' } as const; export const MOTHERSHIP_GRAPHQL_LINK = process.env.MOTHERSHIP_GRAPHQL_LINK ? new URL(process.env.MOTHERSHIP_GRAPHQL_LINK).toString() // Validates URL : MOTHERSHIP_URLS[ENVIRONMENT === 'staging' ? 'staging' : 'production'];api/src/dotenv.ts (1)
Line range hint
1-16: Consider enhancing error handling and type safety.The current implementation could benefit from improved error handling and type safety:
- No explicit error handling for failed config loading
- No type checking for loaded environment variables
Consider applying these improvements:
import { config } from 'dotenv'; +import { DotenvConfigOutput } from 'dotenv'; + +function loadConfig(options: Record<string, any>): DotenvConfigOutput { + const result = config(options); + if (result.error) { + throw new Error(`Failed to load environment config: ${result.error.message}`); + } + return result; +} const env = process.env.NODE_ENV === 'development' || process.env.NODE_ENV === 'test' - ? config({ + ? loadConfig({ debug: true, path: `./.env.${process.env.NODE_ENV}`, encoding: 'utf-8', override: true, }) - : config({ + : loadConfig({ path: '/usr/local/unraid-api/.env', encoding: 'utf-8', }); export default env; + +// Optional: Add type checking for required environment variables +export function validateEnv() { + const required = ['NODE_ENV']; // Add your required variables + for (const key of required) { + if (!process.env[key]) { + throw new Error(`Missing required environment variable: ${key}`); + } + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
api/src/dotenv.ts(1 hunks)api/src/environment.ts(1 hunks)api/src/index.ts(1 hunks)api/src/unraid-api/cli/log.service.ts(2 hunks)api/src/unraid-api/cli/start.command.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/cli/log.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
api/src/environment.ts (2)
6-7: LGTM! Type-safe environment variable handling.The type assertion before the nullish coalescing operator provides better type safety while maintaining the production default.
25-29: LGTM! Well-structured logging configuration.The implementation provides good defaults while ensuring consistent casing of log levels.
api/src/dotenv.ts (1)
5-10: Verify the implications of enabling environment variable override.The addition of
override: trueallows.envfile variables to override existing environment variables in development and test environments. This could lead to unexpected behavior if system-level variables are unintentionally overridden.Run this script to check for potential conflicts:
✅ Verification successful
Environment variable override is safely implemented
The
override: trueoption is safe because:
- It's only enabled in development and test environments
- All system-critical variables have hardcoded fallbacks
- The codebase consistently uses the nullish coalescing operator (
??) for safe defaults- No security-sensitive variables are affected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for environment variables that might be affected by override # First, check if there are any .env files echo "Checking for .env files..." fd -t f ".env" -x echo "Found .env file: {}" # Then, look for environment variable declarations in these files echo -e "\nChecking environment variables in .env files..." fd -t f ".env" -x sh -c 'echo "Variables in {}: "; grep -v "^#" {} | grep "="' # Finally, check if any of these variables are already set in the system echo -e "\nChecking for potential conflicts with system variables..." fd -t f ".env" -x sh -c 'grep -v "^#" {} | cut -d"=" -f1 | while read var; do if [ ! -z "${!var}" ]; then echo "Warning: $var is already set in the system with value: ${!var}" fi done'Length of output: 2851
Script:
#!/bin/bash # Search for environment variable usage patterns echo "Checking environment variable usage patterns..." rg "process\.env\." --type ts --type js -A 2 -B 2 echo -e "\nChecking for direct environment variable access..." ast-grep --pattern 'process.env.$_'Length of output: 13992
api/src/unraid-api/cli/start.command.ts (3)
Line range hint
1-22: LGTM! Clean imports and class setup.The imports are well-organized, and the command class is properly structured with dependency injection.
Line range hint
61-77: LGTM! Well-structured run method.The method properly configures PM2, handles environment variables, and includes appropriate error handling and process management.
Line range hint
79-89: LGTM! Safe log level parsing.The method implements proper validation and fallback handling for log levels.
api/src/index.ts (1)
98-100: LGTM! Improved shutdown reliability.Making the exit hook async and awaiting server closure ensures clean shutdown.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.spec.ts (4)
29-31: Enhance test coverage by making shouldApply configurable.The
shouldApplymethod is hardcoded to always return true, which limits test scenarios. Consider making it configurable likeapplyandrollback.- async shouldApply() { - return { shouldApply: true, reason: 'Always Apply this mod' }; - } + constructor( + public applyImplementation?: () => Promise<void>, + public rollbackImplementation?: () => Promise<void>, + public shouldApplyImplementation?: () => Promise<{ shouldApply: boolean; reason: string }> + ) {} + async shouldApply() { + if (this.shouldApplyImplementation) { + return this.shouldApplyImplementation(); + } + return { shouldApply: true, reason: 'Default test behavior' }; + }
35-41: Extract mockLogger type for reusability.Consider extracting the mockLogger type to improve maintainability and reusability across test files.
type MockLogger = { log: ReturnType<typeof vi.fn>; error: ReturnType<typeof vi.fn>; warn: ReturnType<typeof vi.fn>; debug: ReturnType<typeof vi.fn>; verbose: ReturnType<typeof vi.fn>; };
43-62: Split beforeEach setup for better organization.Consider splitting the setup into smaller, focused beforeEach blocks for better organization and maintainability.
beforeEach(() => { mockLogger = { log: vi.fn(), error: vi.fn(), warn: vi.fn(), debug: vi.fn(), verbose: vi.fn(), }; }); beforeEach(() => { // Mock Logger methods vi.spyOn(Logger.prototype, 'log').mockImplementation(mockLogger.log); vi.spyOn(Logger.prototype, 'error').mockImplementation(mockLogger.error); vi.spyOn(Logger.prototype, 'warn').mockImplementation(mockLogger.warn); vi.spyOn(Logger.prototype, 'debug').mockImplementation(mockLogger.debug); vi.spyOn(Logger.prototype, 'verbose').mockImplementation(mockLogger.verbose); }); beforeEach(async () => { // Setup TestingModule const module: TestingModule = await Test.createTestingModule({ providers: [UnraidFileModificationService], }).compile(); service = module.get<UnraidFileModificationService>(UnraidFileModificationService); });
68-71: Make modification count test more maintainable.The test assumes exactly 2 modifications, which is brittle. Consider making it more flexible or explicit about the expected modifications.
- it('should load modifications', async () => { - const mods = await service.loadModifications(); - expect(mods).toHaveLength(2); - }); + it('should load all available modifications', async () => { + const mods = await service.loadModifications(); + expect(mods.length).toBeGreaterThan(0); + // Verify specific modifications are loaded + expect(mods.map(mod => mod.id)).toEqual( + expect.arrayContaining(['auth-request', 'sso']) + ); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
api/src/unraid-api/unraid-file-modifier/__snapshots__/unraid-file-modifier.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
| it('should rollback all mods', async () => { | ||
| await service.loadModifications(); | ||
| const applyFn = vi.fn(); | ||
| const rollbackFn = vi.fn(); | ||
| await service.applyModification(new TestFileModification(applyFn, rollbackFn)); | ||
| await expect(service.rollbackAll()).resolves.toBe(undefined); | ||
| expect(mockLogger.error).not.toHaveBeenCalled(); | ||
| expect(mockLogger.log).toHaveBeenCalledTimes(5); | ||
| expect(applyFn).toHaveBeenCalled(); | ||
| expect(rollbackFn).toHaveBeenCalled(); | ||
| expect(mockLogger.log).toHaveBeenNthCalledWith(1, 'RootTestModule dependencies initialized'); | ||
| expect(mockLogger.log).toHaveBeenNthCalledWith( | ||
| 2, | ||
| 'Applying modification: test - Always Apply this mod' | ||
| ); | ||
| expect(mockLogger.log).toHaveBeenNthCalledWith(3, 'Modification applied successfully: test'); | ||
| expect(mockLogger.log).toHaveBeenNthCalledWith(4, 'Rolling back modification: test'); | ||
| expect(mockLogger.log).toHaveBeenNthCalledWith(5, 'Modification rolled back successfully: test'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance rollback test coverage.
The rollback tests could be more comprehensive. Consider adding:
- Error handling scenarios
- Concurrent modifications
- Cleanup after tests
it('should handle errors during rollback', async () => {
const errorMod = new TestFileModification(
vi.fn(),
() => Promise.reject(new Error('Rollback failed'))
);
await service.applyModification(errorMod);
await service.rollbackAll();
expect(mockLogger.error).toHaveBeenCalled();
});
it('should handle concurrent modifications', async () => {
const mods = [
new TestFileModification(vi.fn(), vi.fn()),
new TestFileModification(vi.fn(), vi.fn())
];
await Promise.all(mods.map(mod => service.applyModification(mod)));
await service.rollbackAll();
mods.forEach(mod => {
expect(mod.rollbackImplementation).toHaveBeenCalled();
});
});
afterEach(async () => {
await service.rollbackAll();
vi.clearAllMocks();
});
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
api/src/unraid-api/unraid-file-modifier/modifications/sso.modification.ts
Show resolved
Hide resolved
…fication.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
elibosley
force-pushed
the
feat/file-modification-service
branch
from
6dd1ee4 to
f8c2847
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
api/src/unraid-api/cli/log.service.ts (2)
14-21: Consider optimizing the log level comparison.The current implementation works but could be improved for better performance and type safety.
- shouldLog(level: LogLevel): boolean { - const logLevelsLowToHigh = levels; - const shouldLog = ( - logLevelsLowToHigh.indexOf(level) >= - logLevelsLowToHigh.indexOf(LOG_LEVEL.toLowerCase() as LogLevel) - ); - return shouldLog; + private readonly configuredLevelIndex: number; + + constructor() { + this.logger = console; + this.configuredLevelIndex = levels.indexOf(LOG_LEVEL.toLowerCase() as LogLevel); + } + + private shouldLog(level: LogLevel): boolean { + return levels.indexOf(level) >= this.configuredLevelIndex; + }This refactor:
- Caches the configured level index for better performance
- Makes the method private as it's only used internally
- Simplifies the code by removing unnecessary variables
23-50: Consider applying DRY principle to logging methods.The logging methods follow a consistent pattern that could be extracted to reduce duplication.
+ private logWithLevel<T extends any[]>( + level: LogLevel, + method: keyof Console, + message: string | any, + ...args: T + ): void { + if (this.shouldLog(level)) { + (this.logger[method] as any)(message, ...args); + } + } + log(message: string): void { - if (this.shouldLog('info')) { - this.logger.log(message); - } + this.logWithLevel('info', 'log', message); } info(message: string): void { - if (this.shouldLog('info')) { - this.logger.info(message); - } + this.logWithLevel('info', 'info', message); } warn(message: string): void { - if (this.shouldLog('warn')) { - this.logger.warn(message); - } + this.logWithLevel('warn', 'warn', message); } error(message: string, trace: string = ''): void { - if (this.shouldLog('error')) { - this.logger.error(message, trace); - } + this.logWithLevel('error', 'error', message, trace); } debug(message: any, ...optionalParams: any[]): void { - if (this.shouldLog('debug')) { - this.logger.debug(message, ...optionalParams); - } + this.logWithLevel('debug', 'debug', message, ...optionalParams); }api/src/unraid-api/cli/start.command.ts (1)
Line range hint
61-76: Consider consistent error handling approach.While the PM2 start command has proper error handling, it differs from the approach used in
configurePm2. Consider standardizing error handling across both methods.async run(_: string[], options: StartCommandOptions): Promise<void> { this.logger.info('Starting the Unraid API'); - await this.configurePm2(); + try { + await this.configurePm2(); - const envLog = options['log-level'] ? `LOG_LEVEL=${options['log-level']}` : ''; - const { stderr, stdout } = await execa(`${envLog} ${PM2_PATH}`.trim(), [ - 'start', - ECOSYSTEM_PATH, - '--update-env', - ]); - if (stdout) { - this.logger.log(stdout); - } - if (stderr) { - this.logger.error(stderr); - process.exit(1); - } - process.exit(0); + const envLog = options['log-level'] ? `LOG_LEVEL=${options['log-level']}` : ''; + const { stderr, stdout } = await execa(`${envLog} ${PM2_PATH}`.trim(), [ + 'start', + ECOSYSTEM_PATH, + '--update-env', + ]); + + if (stdout) { + this.logger.log(stdout); + } + + if (stderr) { + throw new Error(stderr); + } + + process.exit(0); + } catch (error) { + this.logger.error('Failed to start Unraid API:', error.message); + process.exit(1); + } }api/src/index.ts (1)
98-100: LGTM! Consider adding error handling for server shutdown.The async exit hook implementation is a good improvement, ensuring proper server shutdown. However, consider adding error handling for the server.close() operation.
exitHook(async (signal) => { console.log('exithook', signal); - await server?.close?.(); + try { + await server?.close?.(); + } catch (error) { + logger.error('Error during server shutdown:', error); + } // If port is unix socket, delete socket before exiting unlinkUnixPort();api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts (2)
20-41: Consider enhancing error recovery in lifecycle methods.While the error handling exists, the service might leave the system in an inconsistent state if modifications fail during initialization.
Consider adding a recovery mechanism:
async onModuleInit() { try { this.logger.log('Loading file modifications...'); const mods = await this.loadModifications(); await this.applyModifications(mods); } catch (err) { this.logger.error(`Failed to apply modifications: ${err}`); + // Attempt to roll back any partially applied modifications + try { + await this.rollbackAll(); + } catch (rollbackErr) { + this.logger.error(`Failed to roll back after init failure: ${rollbackErr}`); + } throw err; } }
46-57: Consider making modification loading more dynamic.The current implementation hardcodes modification classes. Consider making it more flexible for future extensions.
Consider using a dynamic loading approach:
async loadModifications(): Promise<FileModification[]> { const modifications: FileModification[] = []; - const modificationClasses: Array<new (logger: Logger) => FileModification> = [ - AuthRequestModification, - SSOFileModification, - ]; + const modificationClasses = await this.discoverModifications(); for (const ModificationClass of modificationClasses) { const instance = new ModificationClass(this.logger); modifications.push(instance); } return modifications; } +private async discoverModifications(): Promise<Array<new (logger: Logger) => FileModification>> { + // This could be implemented to discover modifications dynamically + // For example, by scanning a directory or using dependency injection + return [AuthRequestModification, SSOFileModification]; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
api/src/unraid-api/unraid-file-modifier/__snapshots__/unraid-file-modifier.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (20)
api/ecosystem.config.json(1 hunks)api/src/core/logrotate/setup-logrotate.ts(0 hunks)api/src/core/sso/auth-request-setup.ts(0 hunks)api/src/core/sso/sso-remove.ts(0 hunks)api/src/core/sso/sso-setup.ts(0 hunks)api/src/dotenv.ts(1 hunks)api/src/environment.ts(1 hunks)api/src/index.ts(1 hunks)api/src/unraid-api/app/app.module.ts(2 hunks)api/src/unraid-api/auth/auth.service.spec.ts(1 hunks)api/src/unraid-api/cli/log.service.ts(2 hunks)api/src/unraid-api/cli/start.command.ts(2 hunks)api/src/unraid-api/cron/cron.module.ts(1 hunks)api/src/unraid-api/cron/log-cleanup.service.ts(0 hunks)api/src/unraid-api/unraid-file-modifier/modifications/auth-request.modification.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/sso.modification.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.module.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.spec.ts(1 hunks)api/src/utils.ts(2 hunks)
💤 Files with no reviewable changes (5)
- api/src/core/sso/sso-remove.ts
- api/src/unraid-api/cron/log-cleanup.service.ts
- api/src/core/sso/auth-request-setup.ts
- api/src/core/sso/sso-setup.ts
- api/src/core/logrotate/setup-logrotate.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.module.ts
- api/src/unraid-api/cron/cron.module.ts
- api/src/dotenv.ts
- api/src/unraid-api/app/app.module.ts
- api/ecosystem.config.json
- api/src/unraid-api/unraid-file-modifier/modifications/auth-request.modification.ts
- api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.spec.ts
- api/src/unraid-api/unraid-file-modifier/modifications/sso.modification.ts
- api/src/environment.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
api/src/unraid-api/cli/log.service.ts (1)
3-4: LGTM! Clean import statements.The imports are well-organized and properly typed.
api/src/unraid-api/cli/start.command.ts (2)
1-11: LGTM! Clean import organization.The imports are well-organized, with node built-ins first, followed by external and internal imports.
23-59: Enhance PM2 configuration reliability and performance.The current implementation has several areas for improvement:
- Using
/tmpfor configuration state is unreliable as it may be cleared on reboot- Sequential PM2 commands could be parallelized
- Error handling needs improvement
This was previously identified in past reviews. The suggested improvements are still valid and should be implemented.
api/src/index.ts (1)
Line range hint
1-120: Verify the impact of SSO removal on authentication.The removal of SSO setup logic (
setupSso,setupAuthRequest,removeSso) suggests a significant change in authentication handling. Please ensure:
- Alternative authentication mechanisms are in place
- No security gaps were introduced
- Existing authenticated sessions are handled properly
Let's verify the authentication implementation:
api/src/unraid-api/auth/auth.service.spec.ts (1)
27-27: LGTM! Test coverage enhanced with permissions.The addition of the permissions property to both mock objects improves test coverage by including permission-related scenarios. The empty array in mockApiKey and the specific permission in mockApiKeyWithSecret provide good test cases.
Also applies to: 36-41
api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts (1)
6-17: LGTM! Well-defined interfaces.The interfaces
ShouldApplyWithReasonandFileModificationare well-structured and provide clear contracts for implementing classes.api/src/utils.ts (1)
254-281: LGTM! Robust implementation of file operations.The implementation includes:
- Path validation
- File access checks
- Comprehensive error handling
- Clear error messages based on specific failure cases
Also applies to: 290-319
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts
Outdated
Show resolved
Hide resolved
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web/components/SsoButton.ce.vue (2)
Line range hint
71-84: Enhance error handling with more descriptive messages.The current error handling shows a generic "Error fetching token" message. Consider providing more specific error messages based on different failure scenarios to help users understand and resolve issues.
} catch (err) { console.error('Error fetching token', err); currentState.value = 'error'; - error.value = 'Error fetching token'; + error.value = err instanceof Error + ? `Authentication failed: ${err.message}` + : 'Error fetching token'; reEnableFormOnError(); }
Line range hint
4-8: Standardize prop naming convention.The component accepts two props (
ssoenabledandssoEnabled) that serve the same purpose. This could lead to confusion and maintenance issues.Consider deprecating one variant and standardizing on a single prop name:
export interface Props { - ssoenabled?: boolean | string; - ssoEnabled?: boolean; + ssoEnabled?: boolean | string; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts(1 hunks)web/components/SsoButton.ce.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
web/components/SsoButton.ce.vue (2)
139-143: LGTM! Improved prop naming convention.The template changes follow Vue.js best practices by using kebab-case for prop names in templates (
btn-styleinstead ofbtnStyle). The error state handling and visual structure are well implemented.
Line range hint
29-40: Consider security implications of form manipulation approach.The current implementation of entering the access token into the password field has several potential issues:
- Exposes the access token in the DOM
- Relies on DOM manipulation which could break with layout changes
- May not work with password managers or form validation
Consider implementing a more robust server-side token exchange mechanism instead of client-side form manipulation.
✅ Verification successful
Form manipulation approach is acceptable but could be improved
The current implementation is isolated to SsoButton.ce.vue and the token exposure is temporary during form submission. While a server-side token exchange would be more robust, the current approach has limited security impact because:
- No persistent token storage in DOM
- No XSS vulnerabilities (no innerHTML usage)
- Isolated implementation not used elsewhere in codebase
- Proper backend SSO infrastructure exists
Consider this a low-priority improvement for future refactoring.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential XSS vulnerabilities in form handling rg -l 'document\.querySelector.*innerHTML' . rg -l 'document\.write' . # Look for other instances of direct form manipulation ast-grep --pattern 'document.querySelector("form")'Length of output: 128
Script:
#!/bin/bash # Check for input field value assignments ast-grep --pattern '$_.value = $_' # Look for form-related querySelector patterns rg -l "querySelector\\(('|\").*?(form|input|password)" . # Find SSO-related files fd -e vue -e ts -e js . | rg -i "sso|oauth|auth"Length of output: 11491
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/src/unraid-api/auth/api-key.service.ts (1)
Line range hint
50-54: Consider optimizing the file watch handler.While using the 'all' event type provides comprehensive coverage of file system events, reloading all API keys for every event might impact performance. Consider these optimizations:
- Filter relevant events (add/change/unlink) before reloading
- Reload only the affected key instead of all keys
- watch(this.basePath, { ignoreInitial: false }).on('all', async (path) => { - this.logger.debug(`API key changed: ${path}`); - this.memoryApiKeys = []; - this.memoryApiKeys = await this.loadAllFromDisk(); - }); + watch(this.basePath, { ignoreInitial: false }).on('all', async (event, path) => { + // Filter relevant events + if (!['add', 'change', 'unlink'].includes(event)) { + return; + } + this.logger.debug(`API key ${event}: ${path}`); + + if (event === 'unlink') { + // Remove the deleted key from memory + const keyId = path.split('/').pop()?.replace('.json', ''); + this.memoryApiKeys = this.memoryApiKeys.filter(key => key.id !== keyId); + return; + } + + // Load only the changed/added key + const fileName = path.split('/').pop() as string; + const apiKey = await this.loadApiKeyFile(fileName); + if (apiKey) { + // Update or add the key + const index = this.memoryApiKeys.findIndex(key => key.id === apiKey.id); + if (index !== -1) { + this.memoryApiKeys[index] = apiKey; + } else { + this.memoryApiKeys.push(apiKey); + } + } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/src/unraid-api/auth/api-key.service.ts(1 hunks)api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/unraid-api/unraid-file-modifier/unraid-file-modifier.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
- GitHub Check: Cloudflare Pages
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Summary by CodeRabbit
Release Notes
New Features
Modifications
Infrastructure