feat(api/secrets): view secret value permission #3128
Conversation
WalkthroughThe update introduces extensive changes across backend and frontend modules to expand and refine the permission model related to secret management. A new permission, readValue, has been added, and existing functions, tests, API routes, and schema validations have been updated to accept and enforce this new permission. Database migration scripts now include logic to insert and remove the readValue action from secret permissions. Permission enums have been refined, replacing generic project permission actions with secret-specific ones across various services, including service tokens, secret rotations, approvals, and integrations. Changes also incorporate enhanced error handling, new environment setup exports, and updates to API documentation. On the frontend, components, hooks, and page elements now account for secret visibility by using properties like secretValueHidden and parameters such as viewSecretValue, ensuring that secret display is controlled based on user permissions. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (2)
✨ 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 (
|
DanielHougaard
force-pushed
the
daniel/view-secret-value-permission
branch
from
71a39ac to
c5d92dd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
frontend/src/components/v2/SecretInput/SecretInput.tsx (1)
49-53: 🛠️ Refactor suggestionClarify the relationship between
isVisibleandvalueHiddenprops.The component has two props that seem to control visibility:
isVisibleandvalueHidden. This could lead to confusion about which prop takes precedence and how they interact.Consider:
- Consolidating these props into a single visibility control.
- Or clearly documenting the difference between them and their interaction pattern.
- Or renaming them to be more descriptive of their specific purposes.
🧹 Nitpick comments (12)
frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx (2)
146-158: Verify the permission precedence logic.The
isReadOnlycondition now includes three checks:
- Can describe secret
- Cannot edit secret
- Cannot read secret value
This implementation means a user who can't read the value but can edit other fields would see the entire form as read-only. Consider separating the read-only state for the secret value from other fields.
- const isReadOnly = - permission.can( - ProjectPermissionSecretActions.DescribeSecret, - subject(ProjectPermissionSub.Secrets, { - environment, - secretPath, - secretName: secretKey, - secretTags: selectTagSlugs - }) - ) && - cannotEditSecret && - cannotReadSecretValue; + const isReadOnly = cannotEditSecret; + const isSecretValueReadOnly = isReadOnly || cannotReadSecretValue;
292-293: Consider making secret visibility configurable.The secret value is always initially hidden (
isVisible={false}). Consider making this configurable based on user preferences while respecting the permission constraints.- isVisible={false} + isVisible={userPreferences?.defaultSecretVisibility ?? false}frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretOverviewTableRow.tsx (1)
226-230: Consider using a distinct representation for hidden secrets.Currently, both hidden secrets and empty secrets are represented as empty strings, which could make it difficult to distinguish between them in the UI. Consider using a placeholder like
"[hidden]"for hidden secrets to provide better user feedback.defaultValue={ secret?.secretValueHidden - ? "" + ? "[hidden]" : secret?.valueOverride || secret?.value || importedSecret?.secret?.value }backend/src/server/routes/v3/secret-router.ts (1)
2310-2310: Improve the clarity of the comment.The current comment "? No work needed, does not expose secret value." is unclear. Consider making it more descriptive to better explain why this endpoint doesn't need modifications.
- // ? No work needed, does not expose secret value. + // This endpoint doesn't need the viewSecretValue parameter since it doesn't expose any secret valuesbackend/src/db/migrations/20250218020306_backfill-secret-permissions-with-readvalue.ts (2)
32-53: Consider unifying$updatePermissionsUpfunctionality for clarity.
The loop logic to detect and appendSecretActions.ReadValueis readable but might grow unwieldy if more conditions are introduced later. Extracting the iteration logic into a small helper function could promote maintainability.
84-150: DRY opportunity inupfunction for repeated.reduce()&.onConflict()blocks.
The role updates, identity additional privileges, and user additional privileges share very similar code. You could consolidate them with a higher-order pattern or helper method to reduce duplication.backend/src/ee/services/permission/project-permission.ts (1)
859-865: Conditional readValue logic for secrets subject.
Nicely ensures least-privilege approach. Test thoroughly so you don’t inadvertently allow or deny reads under complex path patterns.backend/src/services/secret/secret-fns.ts (2)
366-373: Logical placeholder usage but check consistency.
While hiding the secret value with"<hidden-by-infisical>"works as intended, note that the newly introduced function uses a slightly different placeholder string ("hidden-by-infisical>"). Consider unifying placeholders for consistency.- : "<hidden-by-infisical>"; + : "hidden-by-infisical>";
1204-1226: New functionconditionallyHideSecretValueis a good modular approach.
Having a helper to transform secret ciphertext to placeholders localizes the logic well. However, the placeholder string here differs slightly fromdecryptSecretRaw's placeholder. Recommend aligning both.backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (1)
1368-1411: Use of transaction for batch insertion.
Wrapping the entire insert logic in a transaction is a good safety measure. Consider logging partial failures or rolled-back states for debugging.backend/src/lib/errors/index.ts (1)
63-68: LGTM! Consider adding a default error message.The new error class follows the established pattern. However, consider adding a more specific default error message to help users understand why their secret read access was denied.
export class ForbiddenReadSecretError extends ForbiddenRequestError { constructor({ error, message }: { message?: string; error?: unknown } = {}) { - super({ message, error }); + super({ message: message ?? "You do not have permission to read this secret's value", error }); this.name = "ForbiddenReadSecretError"; } }backend/src/server/routes/v2/service-token-router.ts (1)
116-126: Consider enhancing audit logging.The audit log entry could be more informative by including the assigned permissions in the metadata.
await server.services.auditLog.createAuditLog({ ...req.auditLogInfo, projectId: serviceToken.projectId, event: { type: EventType.CREATE_SERVICE_TOKEN, metadata: { name: serviceToken.name, - scopes: req.body.scopes + scopes: req.body.scopes, + permissions: req.body.permissions } } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
backend/e2e-test/routes/v2/service-token.spec.ts(5 hunks)backend/e2e-test/vitest-environment-knex.ts(0 hunks)backend/src/db/migrations/20250218020306_backfill-secret-permissions-with-readvalue.ts(1 hunks)backend/src/ee/routes/v1/secret-router.ts(2 hunks)backend/src/ee/services/permission/project-permission.ts(9 hunks)backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts(5 hunks)backend/src/ee/services/secret-rotation/secret-rotation-service.ts(2 hunks)backend/src/ee/services/secret-snapshot/secret-snapshot-service.ts(5 hunks)backend/src/lib/api-docs/constants.ts(3 hunks)backend/src/lib/errors/index.ts(2 hunks)backend/src/server/routes/v1/dashboard-router.ts(8 hunks)backend/src/server/routes/v2/service-token-router.ts(1 hunks)backend/src/server/routes/v3/secret-router.ts(20 hunks)backend/src/services/external-migration/external-migration-fns.ts(1 hunks)backend/src/services/external-migration/external-migration-queue.ts(1 hunks)backend/src/services/integration/integration-service.ts(3 hunks)backend/src/services/project/project-service.ts(2 hunks)backend/src/services/secret-import/secret-import-service.ts(7 hunks)backend/src/services/secret-sync/secret-sync-service.ts(3 hunks)backend/src/services/secret-tag/secret-tag-dal.ts(1 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.ts(4 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts(52 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts(6 hunks)backend/src/services/secret/secret-fns.ts(6 hunks)backend/src/services/secret/secret-service.ts(34 hunks)backend/src/services/secret/secret-types.ts(2 hunks)backend/src/services/service-token/service-token-service.ts(2 hunks)backend/src/services/service-token/service-token-types.ts(1 hunks)frontend/src/components/v2/InfisicalSecretInput/InfisicalSecretInput.tsx(2 hunks)frontend/src/components/v2/SecretInput/SecretInput.tsx(2 hunks)frontend/src/context/ProjectPermissionContext/types.ts(2 hunks)frontend/src/hooks/api/dashboard/queries.tsx(3 hunks)frontend/src/hooks/api/dashboard/types.ts(1 hunks)frontend/src/hooks/api/secretApprovalRequest/queries.tsx(1 hunks)frontend/src/hooks/api/secretImports/queries.tsx(2 hunks)frontend/src/hooks/api/secretSnapshots/queries.tsx(1 hunks)frontend/src/hooks/api/secrets/queries.tsx(1 hunks)frontend/src/hooks/api/secrets/types.ts(3 hunks)frontend/src/pages/project/RoleDetailsBySlugPage/components/GeneralPermissionPolicies.tsx(1 hunks)frontend/src/pages/project/RoleDetailsBySlugPage/components/ProjectRoleModifySection.utils.tsx(5 hunks)frontend/src/pages/secret-manager/OverviewPage/components/CreateSecretForm/CreateSecretForm.tsx(2 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretEditRow.tsx(3 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretOverviewTableRow.tsx(1 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretRenameRow.tsx(2 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SelectionPanel/SelectionPanel.tsx(3 hunks)frontend/src/pages/secret-manager/OverviewPage/components/SelectionPanel/components/MoveSecretsDialog/MoveSecretsDialog.tsx(2 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/SecretDashboardPage.tsx(3 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx(4 hunks)frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretItem.tsx(8 hunks)
💤 Files with no reviewable changes (1)
- backend/e2e-test/vitest-environment-knex.ts
✅ Files skipped from review due to trivial changes (2)
- frontend/src/components/v2/InfisicalSecretInput/InfisicalSecretInput.tsx
- frontend/src/pages/project/RoleDetailsBySlugPage/components/GeneralPermissionPolicies.tsx
👮 Files not reviewed due to content moderation or server errors (4)
- frontend/src/pages/secret-manager/SecretDashboardPage/SecretDashboardPage.tsx
- backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.ts
- backend/src/services/secret-import/secret-import-service.ts
- frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretItem.tsx
🔇 Additional comments (121)
frontend/src/pages/secret-manager/SecretDashboardPage/components/SecretListView/SecretDetailSidebar.tsx (3)
12-13: LGTM: Import changes align with new permission model.The addition of
ProjectPermissionSecretActionsimport andfaTriangleExclamationicon supports the new permission model and improves user feedback.Also applies to: 48-48
126-144: LGTM: Permission checks are correctly implemented.The implementation of
cannotReadSecretValuefollows the same pattern ascannotEditSecret, ensuring consistent permission checking across the component. The check uses the newProjectPermissionSecretActions.ReadValuepermission as specified in the PR objectives.
275-303: LGTM: Secret value visibility is properly handled.The implementation correctly:
- Shows a warning message when the user lacks read value permission
- Uses the warning icon for better visibility
- Maintains the existing functionality while adding the new permission check
frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretOverviewTableRow.tsx (1)
224-224: LGTM! The secretValueHidden prop implementation looks good.The implementation safely handles undefined secrets and maintains backward compatibility with a default value of
false.backend/src/server/routes/v3/secret-router.ts (3)
35-40: LGTM! Well-designed utility function.The
convertStringBooleanfunction provides a clean, type-safe way to handle boolean query parameters consistently across routes. The implementation correctly transforms string "true"/"false" values to booleans with a configurable default.
75-75: LGTM! Consistent implementation of theviewSecretValueparameter.The
viewSecretValueparameter has been properly added to all relevant routes with consistent defaults and documentation. This aligns well with the PR objective of controlling secret value visibility through permissions.Also applies to: 259-259, 408-408, 937-937
275-275: LGTM! Consistent implementation of thesecretValueHiddenresponse field.The
secretValueHiddenboolean field has been properly added to all relevant response schemas, ensuring consistent visibility information in the API responses. This complements theviewSecretValueparameter and provides clear feedback about the secret value visibility state.Also applies to: 415-415, 652-654, 750-752, 1211-1211, 1381-1386, 2074-2074, 2196-2201
frontend/src/components/v2/SecretInput/SecretInput.tsx (1)
86-86: Align with PR's permission model for secret value visibility.Based on the PR objectives, this component should respect the new "read value" permission. Currently, the visibility is controlled by
isVisible || isSecretFocused, which might not align with the permission model.Consider:
- Adding a prop that reflects the user's "read value" permission.
- Updating the visibility logic to respect this permission:
-{syntaxHighlight(value, isVisible || isSecretFocused, isImport)} +{syntaxHighlight(value, (isVisible || isSecretFocused) && hasReadValuePermission, isImport)}backend/src/db/migrations/20250218020306_backfill-secret-permissions-with-readvalue.ts (2)
10-17: Enums appear consistent and aligned with existing permission patterns.
No apparent issues in the new enums, and naming is straightforward.
152-220: Verify rollback logic indownfunction mirrors theupadditions.
The rollback logic appropriately removesReadValueif present. However, confirm that your environment or production data won't encounter partial migrations that skip the “up” step. If that occurs, thedownfunction might remove unrelated data.Would you like a script to double-check your database for partial migrations?
backend/src/ee/services/secret-snapshot/secret-snapshot-service.ts (6)
25-29: ImportedProjectPermissionSecretActionsrecognized.
This ensures improved granularity for secret value reads. Good adoption.
104-106: Enhanced permission check forprojectSecretSnapshotCount.
EnforcingReadValueis a strong move to guard secret retrieval at the snapshot level. Make sure any UI component or CLI usage explicitly handles the error if the user lacks this permission.
141-143: EnsuringReadValuepermission forlistSnapshots.
Consistency across snapshot listing and counting is good. Tests should confirm listing fails gracefully when users lackReadValue.
168-169: Minor addition ofshouldUseBridgevariable.
No direct issues found. Verify logic ensures that legacy vs. new bridging is consistently handled in upper call sites.
178-208: Refined decryption flow in the v3 snapshot code path.
Enforcing theReadValuepermission on each secret version is an excellent security measure. The loop-based checks look solid.
212-262: Conditional decryption for older snapshots.
The complementary “else” block is logically consistent with the new approach. Ensure unit tests cover a scenario where an old snapshot is accessed with restrictedReadValuepermission.backend/src/ee/services/permission/project-permission.ts (6)
20-26: NewProjectPermissionSecretActionsenum is clear.
Having “DescribeSecret” plus “ReadValue” clarifies the difference between listing a secret and accessing its value.
424-424: Retain legacy schema note.
The comment indicates no further modifications should be made here. Good to preserve this for historical reference.
456-458: Transition toProjectPermissionSecretActionsin the V2 schema.
This helps unify secrets-related permissions into a dedicated enum. Great approach for future expansions.
607-611: Extended member permissions now include full secret access.
Members can read, create, edit, and delete secrets — but also read values. Consider restricting certain roles from reading sensitive values if needed.
783-784: Viewer role questions regarding secret values.
Currently, viewers cannot read secret values. Confirm if that meets the intended user story.
828-828: Service tokens now check forreadValue.
The additionalcanReadValueflag is a helpful improvement. Keep in mind tokens must handle partial read access gracefully if only partial scopes are assigned.backend/src/services/secret/secret-fns.ts (4)
16-16: New import aligns with updated secret permission structure.
This import reference toProjectPermissionSecretActionsandProjectPermissionSubis consistent with the new permission model and helps differentiate secret actions from other project actions.
193-193: Ensure consistent usage ofReadValue.
Switching from a generic read permission toProjectPermissionSecretActions.ReadValueis correct for restricting full secret-value access. Confirm that any scenarios needing only metadata view continue to use corresponding "describe" or "list" permissions so as not to over-restrict.
347-347: Helpful addition ofsecretValueHiddenproperty.
This boolean flag on the secret object makes it straightforward to handle cases where the decrypted secret value must not be rendered.
391-391:secretValueHiddenoutput clarifies secret state.
Adding this field to the returned object is helpful for clients to know whether the value was actually decrypted.backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (20)
14-18: Split permissions import is aligned with new secret permission model.
ImportingProjectPermissionSecretActionsalongsideProjectPermissionActionsandProjectPermissionSubclarifies your updated perm checks for secret usage.
23-23: Refined error imports for more precise exceptions.
The addition ofForbiddenReadSecretErrorenhances clarity around scenarios where a user lacks secret-value viewing permissions.
259-259: Switching toCreatefromProjectPermissionSecretActions.
This is consistent with the new granular secret actions. Confirm that all calls requiring secret creation (and not merely metadata creation) are updated similarly.
399-399: Refined permission check toProjectPermissionSecretActions.Edit.
Shifting to a dedicatedEditpermission helps separate general read from write operations.
416-416: Use ofProjectPermissionSecretActions.Editis appropriate.
This helps maintain consistent and precise checks for secret modifications.
433-433: Another correct usage ofEditaction.
Good to see consistent usage of the new action enumerations.
516-536: Conditionally hiding secret value is well-handled.
The approach correctly checksReadValuepermission and only then exposes the full secret data in the response. This ensures compliance with more granular secret-read policies.
625-649: Appropriate read-value permission usage upon deleting a secret.
This snippet properly checks whether the user can see the secret content on deletion. Retaining the placeholder if not allowed is a good security practice.
678-680:DescribeSecretensures listing vs. actual value read.
This block uses the less-privileged describe action for simply enumerating secrets, distinct fromReadValue. This separation enforces minimal access.
759-760: Fallback toReadValueis a sensible default.
Automatically settingfilterByActiontoReadValueensures consistent permission checks if not explicitly provided.
763-784: Granular check with fallback action is sound.
The code checks whether the user can perform the requested action (filterByAction) or can proceed with read-value. This ensures the correct boundary of access for each user.
868-868: NewviewSecretValueparameter.
Introducing this parameter clarifies whether to return plain text or masked secrets. Confirm that your documentation and front-end usage handle this parameter neatly.
884-891:DescribeSecretcheck aligns with high-level listing.
This ensures the user can only request secret metadata in this context. Looks good.
929-982: ConditionalviewSecretValuegating is well-organized.
The.filter(...)logic ensures each secret is only returned in plaintext if the user providesviewSecretValueand has theReadValuepermission. This significantly reduces the risk of accidental exposure.
1037-1058: Permission-based function for import references.
hasSecretAccesschecks both read-value and describe for each expanded reference, which is consistent with your new model.
1078-1078:viewSecretValueingetSecretByName.
This parameter is consistent with the approach used in other endpoints, offering a unified way to control secret output.
1237-1258: Corner case handling for personal secrets.
SettingsecretValueHidden = trueby default and then verifyingReadValuefor non-personal secrets prevents unintended leaks.
1261-1272: Consistent final reshape with optional hiding.
The final returned secret object properly accounts for hidden or revealed values. This is a neat, centralized place for such logic.
1422-1444: Return path with hidden or unhidden values.
This snippet effectively ensures thatsecretValueHiddenis computed per secret. Great job bridging it to the new permission checks.
1487-1497: Extended structure for secret updates.
Augmenting the updated secrets with additional fields (includingsecretPathand typedtags) is beneficial for clarity.backend/src/services/secret/secret-service.ts (30)
16-20: No concerns with the updated import statements.
These imports now differentiate between generic project actions and secret-specific actions, which is a welcome improvement to permission granularity.
55-55: Confirmed import ofconditionallyHideSecretValue.
Looks good. Ensure to maintain thorough tests covering its usage.
212-214: Correct transition toProjectPermissionSecretActions.Create.
This aligns secret creation with the new permission scheme.
330-332: Correct transition toProjectPermissionSecretActions.Edit.
This consistently enforces secret-specific edit permissions.
452-460: Good approach to determine if the secret value should be hidden.
Using!permission.can(...)forReadValuecentralizes visibility checks.
462-467: Neatly applies theconditionallyHideSecretValuehelper.
Combining the updated secret with hidden-value logic simplifies the return object.
490-492: Switch to secret-specificDeletepermission is appropriate.
No issues found.
563-566: Good consistency forsecretValueHiddenlogic.
Maintaining a uniform pattern for visibility checks helps reduce mistakes.
568-575: Clear use of conditional hiding before returning the deleted secret.
Ensures consistent security posture.
624-626: ApplyingReadValuefor recursion is correct.
Ensures a user must explicitly have permission to read secret values.
649-654: Filtering imports viaReadValueaction is consistent.
Prevents unauthorized access to imported secrets’ values.
706-708: Valid check forReadValueingetSecretByName.
Maintains the principle of least privilege.
756-761: Appropriately enforcingReadValueon imported references.
This is in line with the new permission semantics.
774-775: Explicitly exposingsecretValueHidden: false.
Since the import is already filtered by permission checks, forcing it false here is consistent.
785-791: Justified always-falsesecretValueHidden.
Checked early in the function, so no further checks needed.
813-815: Addingcreatepermission check for bulk secrets.
Matches the single-secret creation flow.
901-903: EnsuringEditpermission on bulk updates.
Correct and consistent with single-update approach.
943-945: Initiating transactional update procedure.
No issues; transaction usage helps maintain atomicity.
974-985: Hiding secret values for updated secrets.
Important security measure for partial read permissions.
1019-1021: Correctly alignedDeletepermission check for bulk deletion.
Maintains the new secret-centric permission model.
1071-1080: ApplyingconditionallyHideSecretValueto multiple secrets.
Ensures consistent privacy logic across an array of deleted items.
1296-1296: Introduction ofviewSecretValueparameter.
Enhances flexibility for partial secret retrieval.
1312-1313: Properly passingviewSecretValueinto V2 bridging service.
Looks consistent with the new parameter.
1356-1362: ForcingsecretValueHidden: falseon decrypted import secrets.
Ensure source permission checks have already filtered unauthorized reads.
1457-1458: Correct pass-through ofviewSecretValueto secret V2 logic.
No problems identified.
1477-1478: ExtendingviewSecretValueusage for secret-by-name retrieval.
Aligns with the rest of the code.
1527-1531: ReturningsecretMetadata: undefinedavoids exposing data by default.
A safe fallback for older consumption patterns.
1681-1691: Decryption with forced hidden=false.
Ensures that newly created secret is fully exposed to the creator if permitted.
2086-2087: Consistently setssecretValueHidden: falsefor newly inserted secrets.
Matches the creation pattern.
2723-2750: Robust permission checks before moving secrets.
VerifyingDelete,Create, andEditensures correct coverage of the move operation.backend/src/services/service-token/service-token-types.ts (1)
10-10: Adding"readValue"to service token permissions is a good extension.
It aligns with the newly introduced secret read-value permission model.backend/src/services/secret-tag/secret-tag-dal.ts (1)
50-60: New methods for managing secret tags (V1 and V2) are well-structured.
This consolidated approach (save/delete for standard vs. V2) keeps the DAL organized.frontend/src/hooks/api/dashboard/types.ts (1)
72-72: LGTM! Type definition aligns with PR objectives.The addition of
viewSecretValueboolean flag is well-placed and correctly typed, enabling granular control over secret value visibility as intended.backend/src/ee/routes/v1/secret-router.ts (1)
3-3: LGTM! Enhanced permission granularity for secrets.The switch to
ProjectPermissionSecretActionsimproves type safety by ensuring only secret-specific permissions are allowed in the access list.Also applies to: 12-12
backend/src/server/routes/v2/service-token-router.ts (1)
97-97: LGTM! Service token permissions updated.The addition of "readValue" to the permissions enum aligns with the new granular secret access control.
backend/src/services/external-migration/external-migration-queue.ts (1)
32-32: LGTM! Type signature updated correctly.The addition of the
findmethod tosecretTagDALis well-structured and maintains type safety.frontend/src/hooks/api/secrets/types.ts (1)
22-22: LGTM! Types updated consistently.The
secretValueHiddenproperty has been correctly added to all relevant types (EncryptedSecret,SecretV3RawSanitized, andSecretV3Raw), maintaining consistency in the type system.Also applies to: 41-41, 65-65
frontend/src/hooks/api/secretApprovalRequest/queries.tsx (1)
82-82: LGTM! Correctly propagating secret visibility state.The implementation properly preserves the
secretValueHiddenstate from the encrypted secret during decryption.frontend/src/context/ProjectPermissionContext/types.ts (2)
10-16: LGTM! Well-structured permission model for secrets.The new
ProjectPermissionSecretActionsenum provides granular control over secret operations, with a clear distinction between describing a secret (read) and viewing its value (readValue). This aligns perfectly with the PR objectives.
140-140: LGTM! Proper type update for secret permissions.The
ProjectPermissionSettype correctly usesProjectPermissionSecretActionsfor theSecretssub-permission, ensuring type safety for the new permission model.backend/src/services/service-token/service-token-service.ts (1)
74-74: LGTM! Consistent permission check update.The permission check now correctly uses
ProjectPermissionSecretActions.Createinstead of the genericProjectPermissionActions.Create, maintaining consistency with the new granular permission model for secrets.frontend/src/pages/secret-manager/OverviewPage/components/SelectionPanel/SelectionPanel.tsx (1)
62-62: LGTM! Consistent permission checks for secret deletion.The permission checks for secret deletion have been correctly updated to use
ProjectPermissionSecretActions.Delete, maintaining consistency with the new granular permission model.Also applies to: 114-114
frontend/src/hooks/api/secretImports/queries.tsx (1)
140-140: LGTM! Added support for secret value visibility control.The
secretValueHiddenproperty has been consistently added to secret objects in both single environment and all environments queries, supporting the new permission model's ability to control secret value visibility.Also applies to: 180-180
frontend/src/hooks/api/secrets/queries.tsx (1)
71-71: LGTM! The secret value visibility property is correctly propagated.The addition of
secretValueHiddenproperty aligns with the PR's objective to introduce granular control over secret value access. The property is correctly propagated from the raw secret to the decrypted secret.frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretRenameRow.tsx (1)
13-14: LGTM! Permission checks correctly updated to use secret-specific actions.The changes appropriately transition from generic project permissions to secret-specific permissions, aligning with the PR's objective to introduce more granular control over secret access. The permission checks now use
ProjectPermissionSecretActions.DescribeSecretandProjectPermissionSecretActions.Editfor more precise control.Also applies to: 54-55
backend/src/services/secret-v2-bridge/secret-v2-bridge-types.ts (4)
40-40: LGTM! The viewSecretValue property is correctly typed.The addition of
viewSecretValuetoTGetSecretsDTOaligns with the PR's objective to control secret value visibility.
62-62: LGTM! The viewSecretValue property is consistently added.The addition of
viewSecretValuetoTGetASecretDTOmaintains consistency withTGetSecretsDTOand supports the secret value visibility feature.
338-338: LGTM! The filterByAction property enhances permission control.The addition of
filterByActiontoTGetSecretsRawByFolderMappingsDTOcorrectly supports filtering secrets based on permission actions.
172-172: LGTM! The secretTagDAL type is correctly updated.The addition of the
findmethod tosecretTagDALtype in bothTFnSecretBulkInsertandTFnSecretBulkUpdateenhances the tag handling capabilities.Also applies to: 196-196
frontend/src/hooks/api/dashboard/queries.tsx (1)
210-211: LGTM! The new parameters are correctly propagated through the query chain.The addition of
viewSecretValueandincludeImportsparameters touseGetProjectSecretsDetailsis correctly implemented:
- Parameters are added to the function signature
- Parameters are included in the query key
- Parameters are passed to the fetch function
This aligns with the PR's objective to control secret value visibility and handle imports.
Also applies to: 235-236, 252-253
frontend/src/pages/secret-manager/OverviewPage/components/SecretOverviewTableRow/SecretEditRow.tsx (2)
41-42: LGTM! New prop for controlling secret value visibility.The
secretValueHiddenprop aligns with the PR's objective to enhance granularity of secret value access.
145-174: Well-implemented conditional rendering for secret value visibility.The implementation effectively handles both cases:
- When
secretValueHiddenis true: Shows a blurred placeholder with a helpful tooltip.- When
secretValueHiddenis false: Renders the editable secret input.The UX is user-friendly with clear feedback about permission requirements.
frontend/src/pages/secret-manager/OverviewPage/components/CreateSecretForm/CreateSecretForm.tsx (2)
22-22: LGTM! New import for secret-specific permissions.The addition of
ProjectPermissionSecretActionsaligns with the PR's objective to enhance permission granularity.
278-280: LGTM! Updated permission check to use secret-specific action.The permission check now uses
ProjectPermissionSecretActions.Createinstead of the genericProjectPermissionActions.Create, providing better permission granularity.backend/src/ee/services/secret-rotation/secret-rotation-service.ts (2)
5-9: LGTM! Imports updated to include secret-specific permissions.The addition of
ProjectPermissionSecretActionsaligns with the PR's objective to enhance permission granularity.
113-114: LGTM! Updated permission check to use secret-specific action.The permission check now uses
ProjectPermissionSecretActions.Editinstead of the genericProjectPermissionActions.Edit, providing better permission granularity.backend/src/services/integration/integration-service.ts (2)
5-9: LGTM! Imports updated to include secret-specific permissions.The addition of
ProjectPermissionSecretActionsaligns with the PR's objective to enhance permission granularity.
98-100: LGTM! Updated permission checks to use secret-specific read value action.The permission checks in both
createIntegrationandupdateIntegrationnow useProjectPermissionSecretActions.ReadValueinstead of the generic read action, correctly implementing the PR's objective to control access to secret values.Also applies to: 181-183
frontend/src/pages/secret-manager/OverviewPage/components/SelectionPanel/components/MoveSecretsDialog/MoveSecretsDialog.tsx (1)
28-29: LGTM! Permission check updated to use the new secret-specific actions.The changes correctly update the permission check to use
ProjectPermissionSecretActions.Deleteinstead ofProjectPermissionActions.Delete, aligning with the new granular permission model for secrets.Also applies to: 99-99
backend/src/services/secret/secret-types.ts (1)
183-183: LGTM! Added viewSecretValue property to support granular secret value access.The changes correctly add the
viewSecretValueproperty to bothTGetSecretsRawDTOandTGetASecretRawDTOtypes, enabling the implementation of the newread valuepermission.Also applies to: 209-209
backend/e2e-test/routes/v2/service-token.spec.ts (1)
9-9: LGTM! Test cases updated to include the new readValue permission.The changes correctly update the test cases to include the new
readValuepermission, ensuring that service token permissions are properly tested with the new granular permission model.Also applies to: 142-142, 499-499, 521-521, 543-543
backend/src/services/secret-sync/secret-sync-service.ts (1)
6-6: LGTM! Permission checks updated to use the new secret-specific actions.The changes correctly update the permission checks to use
ProjectPermissionSecretActions.ReadValueinstead ofProjectPermissionActions.Read, aligning with the new granular permission model for secrets.Also applies to: 182-182, 273-273
frontend/src/pages/project/RoleDetailsBySlugPage/components/ProjectRoleModifySection.utils.tsx (4)
25-31: LGTM! Well-structured schema for secret permissions.The new
SecretPolicyActionSchemacorrectly separates "describe secret" and "read value" permissions, with clear comments indicating their purposes.
117-120: LGTM! Clean integration of the new secret policy schema.The
SecretPolicyActionSchemais properly integrated into the project role form schema while maintaining support for conditions and inversion flags.
286-305: LGTM! Comprehensive permission conversion logic.The permission conversion correctly handles all secret-related actions, including the new
readValuepermission, while maintaining proper type safety.
485-488: LGTM! Clear and descriptive permission labels.The permission labels "Describe Secret" and "Read Value" clearly communicate their distinct purposes to users.
backend/src/services/external-migration/external-migration-fns.ts (1)
36-36: LGTM! Type definition properly extended.The
secretTagDALtype correctly includes thefindmethod alongside existing methods.backend/src/server/routes/v1/dashboard-router.ts (3)
119-119: LGTM! Response schema properly handles secret visibility.The
secretValueHiddenboolean field is consistently added to all relevant response schemas.Also applies to: 416-417
398-399: LGTM! API parameters properly handle secret visibility control.The
viewSecretValueparameter is consistently added to relevant routes with appropriate defaults.Also applies to: 870-871
298-299: LGTM! Service calls properly implement secret visibility.The service layer calls are consistently updated to pass the
viewSecretValueparameter.Also applies to: 611-612, 905-906
backend/src/services/project/project-service.ts (1)
754-757: LGTM! Permission check updated to use new action type.The permission check is correctly updated to use
ProjectPermissionSecretActions.DescribeSecretinstead of the generic read action.backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts (3)
61-61: LGTM! Import of granular secret permissions.The change from
ProjectPermissionActionstoProjectPermissionSecretActionsaligns with the new granular permission model for secrets.
1367-1371: LGTM! Proper permission mapping for secret operations.The mapping of operations to appropriate permissions is correct:
- Create → ProjectPermissionSecretActions.Create
- Update → ProjectPermissionSecretActions.Edit
- Delete → ProjectPermissionSecretActions.Delete
918-920:✅ Verification successful
Verify the permission check for secret value access.
The permission check now uses
ProjectPermissionSecretActions.ReadValuewhich aligns with the PR objectives. However, ensure that this change is consistently applied across all secret-related endpoints.
🏁 Script executed:
#!/bin/bash # Description: Check for consistent usage of ReadValue permission across the codebase # Search for permission checks related to secret values rg -A 3 "ProjectPermissionSecretActions\.(ReadValue|Create|Edit|Delete)"Length of output: 35526
Verified: Consistent use of ReadValue permission across secret-related endpoints
The update in
backend/src/ee/services/secret-approval-request/secret-approval-request-service.tscorrectly appliesProjectPermissionSecretActions.ReadValuewith the appropriate subject for secret value access. A repository-wide search confirms that endpoints meant to handle secret reads are consistently usingReadValue(while other operations appropriately use Create, Edit, or Delete as needed).
- File
backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts(lines 918-920): Correct usage of ReadValue.- Other secret endpoints: Similar permission patterns are observed where secret read access is involved.
No further action is needed regarding this permission change.
backend/src/lib/api-docs/constants.ts (1)
667-667: LGTM! Consistent documentation of viewSecretValue parameter.The
viewSecretValueparameter is properly documented across all relevant endpoints:
- SECRETS.ATTACH_TAGS
- RAW_SECRETS.LIST
- RAW_SECRETS.GET
The description "Whether or not to retrieve the secret value" clearly communicates the parameter's purpose.
Also applies to: 691-691, 720-720
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
frontend/src/pages/project/AccessControlPage/components/ServiceTokenTab/components/ServiceTokenSection/AddServiceTokenModal.tsx (1)
304-317: Add tooltips to clarify permission differences.While the permission labels are clear, users might benefit from tooltips explaining:
- What information is visible with "Describe Secret"
- What additional access "Read Value" provides
Consider adding tooltips to the checkboxes:
<Checkbox id={String(value[optionValue])} key={optionValue} className="data-[state=checked]:bg-primary" isChecked={value[optionValue]} isDisabled={optionValue === "read"} + title={optionValue === "read" + ? "Allows viewing secret metadata without revealing the actual value" + : optionValue === "readValue" + ? "Allows viewing the actual secret value" + : "Allows creating and updating secrets"} onCheckedChange={(state) => { onChange({ ...value, [optionValue]: state }); }} > {label} </Checkbox>backend/src/db/migrations/20250218020306_backfill-secret-permissions-with-readvalue.ts (4)
10-17: Consider documenting the new enums to clarify usage.These enums represent crucial permission granularity for secrets. Adding JSDoc comments clarifying each action’s intent (e.g., “readValue” vs. “read”) will help future maintainers.
84-122: Check performance when updating a large set of service tokens.The logic updates service tokens with a CASE statement. While functional, it could be slow if the table has many tokens. Consider batching or chunking if you expect extremely large data sets to avoid potential database timeouts.
115-122: Refine TypeScript definitions to avoid@ts-expect-error.The raw query usage may be typed more precisely with a suitable cast or extended type definitions so that manual ignore comments aren’t required.
183-221: Symmetry in down-migration is good, but verify database consistency after potential partial updates.The
$updatePermissionsDownfunction neatly removesReadValue. If an error or interruption occurs mid-migration, partial merges could occur. Consider wrapping the process in a transaction for atomicity.backend/src/ee/services/permission/project-permission.ts (3)
20-26: Clarify naming for “DescribeSecret” vs. “ReadValue”.“DescribeSecret” is mapped to
"read". This can be confusing since normal “read” actions exist for other resources. Consider a more explicit name (e.g., “ListSecretMetadata”) to differentiate from reading the value in code references.
456-456: Properly highlight new secret actions in docs.Updating the
ProjectPermissionV2Schemato useProjectPermissionSecretActionsis a key change. Ensure this is reflected in user-facing docs so end-users understand the new permission capability.
859-865: Check for permission collisions with subject-based conditions.This conditionally grants
ReadValueifcanReadValueis set. If you plan to unifyDescribeSecretwith partial maybe-later expansions, there could be conflicts or confusion. Keep them separate if you want strict control over who sees raw data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/src/db/migrations/20250218020306_backfill-secret-permissions-with-readvalue.ts(1 hunks)backend/src/ee/services/permission/project-permission.ts(9 hunks)frontend/src/pages/project/AccessControlPage/components/ServiceTokenTab/components/ServiceTokenSection/AddServiceTokenModal.tsx(2 hunks)
🔇 Additional comments (10)
frontend/src/pages/project/AccessControlPage/components/ServiceTokenTab/components/ServiceTokenSection/AddServiceTokenModal.tsx (2)
58-65: LGTM! Schema updated correctly for new permission.The schema has been properly extended to include the new
readValuepermission while maintaining consistency with the existing permission structure.
296-303: LGTM! Default values set appropriately.The default values are correctly configured:
readValue: falseensures explicit opt-in for reading secret values- Maintains backward compatibility
- Preserves existing default permissions
backend/src/db/migrations/20250218020306_backfill-secret-permissions-with-readvalue.ts (4)
19-30: Validate unpacked permission arrays in calling code or handle parse failures.The function
$unpackPermissionsthrows on invalid data, which is correct from a schema perspective. However, ensure that the calling code or migration logic properly handles or logs potential parse failures, preventing partial or silent failures during migrations.
32-53: EnsureReadValueis appended only where appropriate.The
$updatePermissionsUpfunction automatically appendsReadValuewheneverReadis present under theSecretssubject. This is aligned with the PR’s goal, but confirm whether there are edge cases (e.g., conditions restricting read scope) whereReadValueshould not automatically be appended.
124-163: Confirm migrations preserve existing permission conditions.You are parsing and repacking the permission arrays. Ensure any existing conditions (like environment-based restrictions) remain intact through
$updatePermissionsUp, especially if they must carry over for partial read grants.
223-263: Confirm default read / readValue usage.Removing
ReadValuefrom roles and privileges is correct for rollback, but double-check logical contrasts with partial privileges that might have been appended. Some records might end up with noReadorReadValueat all, impacting usage flows.backend/src/ee/services/permission/project-permission.ts (4)
425-425: Retain V1 schema for specific backward compatibility needs only.A cautionary comment is provided here, which is helpful. Ensure you create a new schema if you foresee more additions to secrets.
549-559: Granting bothDescribeSecretandReadValueto admins.This is intentional to give full control. No concerns, but ensure you keep an eye on any possible friction if other roles should not have both.
603-611: Revisit member permissions if restricting secret visibility.Members are also given both
DescribeSecretandReadValue. Confirm that you truly want members to see full secret values by default.
827-828: Check string match for “readValue” in uppercase vs. lowercase usage.You rely on
permission.includes("readValue"). Confirm that any environment or existing code references do not store this permission incorrectly capitalized or spelled differently (e.g., “readvalue”).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
backend/src/lib/errors/index.ts (2)
94-104: Align naming convention with other error classes.The class name
DisableRotationErrorsuses plural form ("Errors") while all other error classes use singular form ("Error"). Consider renaming to maintain consistency.-export class DisableRotationErrors extends Error { +export class DisableRotationError extends Error { name: string; error: unknown; constructor({ name, error, message }: { message: string; name?: string; error?: unknown }) { super(message); - this.name = name || "DisableRotationErrors"; + this.name = name || "DisableRotationError"; this.error = error; } }
99-99: Consider providing a default message for consistency.Unlike other error classes,
DisableRotationErrorsrequires a message parameter without providing a default value. Consider making it optional with a default message to maintain consistency with other error classes.- constructor({ name, error, message }: { message: string; name?: string; error?: unknown }) { + constructor({ name, error, message }: { message?: string; name?: string; error?: unknown }) { super(message || "Failed to disable rotation"); this.name = name || "DisableRotationError"; this.error = error; }backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (2)
1251-1254: Consider standardizing error messages across all operations.While the error handling is clear, consider standardizing the error message format across all operations for better maintainability and user experience.
Apply this pattern consistently:
- message: `You do not have permission to view secret value on secret with name '${secretName}'`, + message: `Unable to view secret value for secret '${secretName}': insufficient permissions`, name: "ForbiddenReadSecretError"Also applies to: 2459-2462
1037-1057: Consider extracting permission check logic into a helper function.The permission check logic is duplicated across multiple operations. Consider extracting it into a helper function to improve maintainability and reduce duplication.
Example implementation:
const canReadSecretValue = ( permission: PureAbility, environment: string, secretPath: string, secretName: string, secretTags: string[] ): boolean => { return permission.can( ProjectPermissionSecretActions.ReadValue, subject(ProjectPermissionSub.Secrets, { environment, secretPath, secretName, secretTags }) ); };Also applies to: 1772-1797
backend/src/services/secret/secret-fns.ts (1)
1207-1225: Add JSDoc documentation for the new function.The
conditionallyHideSecretValuefunction provides a clean abstraction for hiding secret values. Consider adding JSDoc documentation to explain its purpose, parameters, and return value.+/** + * Conditionally masks secret value fields based on the provided flag. + * @param shouldHideValue - Flag indicating whether to hide the secret value + * @param secretValueFields - Object containing the secret value fields to potentially mask + * @returns Object containing either the original or masked secret value fields and a hidden flag + */ export const conditionallyHideSecretValue = ( shouldHideValue: boolean, {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/src/lib/errors/index.ts(1 hunks)backend/src/server/routes/v3/secret-router.ts(19 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.ts(5 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts(53 hunks)backend/src/services/secret/secret-fns.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/services/secret-v2-bridge/secret-v2-bridge-fns.ts
🔇 Additional comments (8)
backend/src/server/routes/v3/secret-router.ts (3)
35-40: LGTM! Well-structured utility function.The
convertStringBooleanfunction is a clean abstraction that improves code maintainability by centralizing string-to-boolean conversion logic. It properly handles type safety using zod's enum and transform.
259-259: LGTM! Consistent implementation of viewSecretValue parameter.The
viewSecretValueparameter is properly added to all relevant routes with:
- Consistent usage of the
convertStringBooleanutility- Default value of true for backward compatibility
- Proper API documentation references
Also applies to: 409-409, 938-938
275-275: LGTM! Comprehensive response schema updates.The
secretValueHiddenfield is consistently added to all relevant response schemas. Good handling of imports with the comment explaining why imported secrets don't need this field - they're filtered out if the user lacks read value permission.Note: The comment on lines 295-297 provides valuable context:
// No `secretValueHidden` on imports, because imported secrets that the user doesn't have read value permission on, will be filtered out. // Therefore all returned secret imports will have the value present.Also applies to: 295-297, 416-416, 653-655, 751-753, 814-818, 1075-1075, 2197-2201
backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts (2)
14-18: LGTM! Well-structured permission model.The introduction of
ProjectPermissionSecretActionsenhances the granularity of secret-specific permissions, aligning with the PR objectives.
1236-1258: LGTM! Comprehensive secret value visibility handling.The implementation correctly:
- Handles personal secrets visibility
- Enforces
ReadValuepermission checks- Provides clear error messages for unauthorized access
backend/src/services/secret/secret-fns.ts (3)
16-16: LGTM! The changes align with the new permission model.The import change and the new constant support the implementation of the new
read valuepermission by providing the necessary tools for handling hidden secret values.Also applies to: 54-54
191-201: LGTM! The permission check correctly implements the new ReadValue permission.The function now properly filters paths based on the new
ReadValuepermission, ensuring that users can only access secret values they have permission to view.
347-409: LGTM! The decryption function properly handles secret value visibility.The function now correctly implements the masking of secret values based on the
secretValueHiddenflag, which is essential for enforcing the newread valuepermission.
Description 📣
This PR adds a new permission on the secrets subject, called
read value. This aims to make secret value access more granular, and to allow users to specify weather or not a role should have access to purely describe a secret, or reveal it's value entirely.A few important notes:
Read Valuepermission.POSTsecret endpoints (endpoints used to create new secrets), will still expose the secret value regardless if the user has theRead Valuepermission or not. This is because the person creating the secret will already know the value that they're creating, therefore there's no point trying to enforce theRead Valuepermission here.viewSecretValueparameter (which defaults totrue). If this parameter is set totrue, and the user doesn't have theRead Valuepermission, it will throw an API error. If it's set tofalse, the API will return the secret without the value. The value is masked with<hidden-by-infisical>.Read Valuepermission on the secret(s). This is inline with our current logic for secret imports.a. The same thing applies to recursive mode.
expandSecretReferencesis true and the user doesn't have theRead Valuepermission, it will result in an API error.Type ✨
Summary by CodeRabbit