feat: customer feedback sdk permissions#3068
Conversation
chronark
requested review from
MichaelUnkey,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: f2f3376 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThis pull request introduces updates for permissions and roles management. A changelog document now details a minor version change for the "@unkey/api" package. New tests have been added and updated for the key creation functionality to validate both roles and permissions assignment, including checks for insufficient permissions. The API route for key creation now accepts additional parameters ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant RBAC
participant DB
Client->>API: POST /v1/keys.createKey (with roles & permissions)
API->>DB: Retrieve existing roles and permissions
alt All entities exist
API-->>Client: Return created key
else Some entities missing
API->>RBAC: Evaluate requested permissions/roles
alt Evaluation fails (insufficient permissions)
API-->>Client: 403 Error (INSUFFICIENT_PERMISSIONS)
else Evaluation passes
API->>DB: Create missing roles/permissions
API-->>Client: Return created key with combined IDs
end
end
sequenceDiagram
participant App
participant Unkey
participant API
App->>Unkey: Call permissions.addPermissions(...)
Unkey->>API: HTTP POST /v1/keys.addPermissions
API-->>Unkey: Return result
Unkey-->>App: Deliver response
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/api/src/routes/v1_keys_createKey.ts (3)
538-587: Consider refactoring to reduce duplication with getRoleIds function.The getPermissionIds and getRoleIds functions have very similar implementations which could be extracted into a more generic function to reduce code duplication.
+async function getEntityIds<T extends { id: string; name: string }>( + entityType: "permission" | "role", + auth: { permissions: Array<string> }, + rbac: RBAC, + db: Database, + workspaceId: string, + entityNames: Array<string>, + findEntities: (db: Database, workspaceId: string, names: string[]) => Promise<T[]>, + createEntities: (workspaceId: string, names: string[]) => Array<T> +): Promise<Array<string>> { + if (entityNames.length === 0) { + return []; + } + + const entities = await findEntities(db, workspaceId, entityNames); + + if (entities.length === entityNames.length) { + return entities.map((e) => e.id); + } + + const permissionQuery = entityType === "permission" + ? "rbac.*.create_permission" + : "rbac.*.create_role"; + + const { val, err } = rbac.evaluatePermissions( + buildUnkeyQuery(({ or }) => or("*", permissionQuery)), + auth.permissions, + ); + + if (err) { + throw new UnkeyApiError({ + code: "INTERNAL_SERVER_ERROR", + message: `Failed to evaluate permissions: ${err.message}`, + }); + } + + if (!val.valid) { + throw new UnkeyApiError({ code: "INSUFFICIENT_PERMISSIONS", message: val.message }); + } + + const missingNames = entityNames.filter( + (name) => !entities.some((entity) => entity.name === name) + ); + + const newEntities = createEntities(workspaceId, missingNames); + + await db.insert(entityType === "permission" ? schema.permissions : schema.roles) + .values(newEntities); + + return [...entities, ...newEntities].map((entity) => entity.id); +}Then update the existing functions to use this generic implementation.
556-586: Handle case-sensitivity in permission name matching.The current implementation doesn't explicitly handle case-sensitivity when matching permission names. If there are permissions with similar names but different casing, the length check might pass but the wrong permissions could be matched.
Consider adding a more robust matching mechanism, such as:
- if (permissions.length === permissionNames.length) { + // Check if every requested permission exists with exact name match + const allPermissionsExist = permissionNames.every(name => + permissions.some(p => p.name === name) + ); + if (allPermissionsExist) {
589-636: Similar refactoring needed for role handling.The getRoleIds function has the same potential issues and refactoring opportunities as getPermissionIds. Apply the same improvements for consistency.
Follow the same pattern recommended for the getPermissionIds function:
- Use the generic helper function to reduce duplication
- Add more robust name matching to handle case sensitivity
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/witty-spoons-flow.md(1 hunks)apps/api/src/routes/v1_keys_createKey.happy.test.ts(2 hunks)apps/api/src/routes/v1_keys_createKey.security.test.ts(1 hunks)apps/api/src/routes/v1_keys_createKey.ts(5 hunks)packages/api/src/client.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
apps/api/src/routes/v1_keys_createKey.security.test.ts (1)
apps/api/src/routes/v1_keys_createKey.ts (1)
V1KeysCreateKeyRequest(254-256)
apps/api/src/routes/v1_keys_createKey.happy.test.ts (1)
apps/api/src/routes/v1_keys_createKey.ts (2)
V1KeysCreateKeyRequest(254-256)V1KeysCreateKeyResponse(257-259)
packages/api/src/client.ts (1)
packages/api/src/openapi.d.ts (1)
paths(15-149)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
🔇 Additional comments (10)
.changeset/witty-spoons-flow.md (1)
1-6: LGTM - Changeset file correctly documents the API changes.The changeset file properly indicates a minor version bump for the "@unkey/api" package, which aligns with adding new permissions functionality. This follows semantic versioning conventions for feature additions.
apps/api/src/routes/v1_keys_createKey.security.test.ts (2)
113-139: Well-structured security test for role creation permissions.This test properly validates that users without the necessary permissions cannot create roles. It follows the same pattern as other security tests in the file and checks for the correct error response.
141-167: Security test properly validates permission creation restrictions.This test ensures that users without the proper permissions cannot create permissions, maintaining the security model integrity. The test structure is consistent with the other security tests.
apps/api/src/routes/v1_keys_createKey.happy.test.ts (3)
225-240: Test correctly checks role association.The updated test properly verifies that roles are connected to the created key, with appropriate assertions.
241-279: Good test for dynamic role creation and association.This test validates that roles can be created dynamically when they don't exist, and properly associated with the key. It includes the correct permission setup and verification of the results.
328-366: Test properly validates permission upsert functionality.This test ensures that permissions can be dynamically created and associated with keys when the user has the necessary authorization. The assertion logic correctly verifies the relationships.
apps/api/src/routes/v1_keys_createKey.ts (2)
13-13: LGTM - Adding RBAC import.Correctly imports the RBAC type and buildUnkeyQuery function from the @unkey/rbac package.
338-344: LGTM - Updated permission and role handling in Promise.all.The Promise.all correctly passes the auth and rbac parameters to the getPermissionIds and getRoleIds functions.
packages/api/src/client.ts (2)
230-307: Well-implemented permission and role management methods for keysThese new methods extend the
keysnamespace with comprehensive management capabilities for permissions and roles. The implementation follows the established pattern in the codebase, with consistent error handling and type safety through OpenAPI definitions.The methods include:
- addPermissions, setPermissions, removePermissions for permission management
- addRoles, setRoles, removeRoles for role management
Each method properly uses the fetch utility and maintains type safety through the OpenAPI definitions.
442-523: Clean implementation of permissions management namespaceThe new
permissionsgetter provides a well-structured API for managing roles and permissions directly:
- Role management: createRole, getRole, deleteRole
- Permission management: createPermission, getPermission, deletePermission
Each method follows the established pattern in the codebase, maintaining consistency with other parts of the SDK. The implementation leverages TypeScript's type system through OpenAPI definitions to ensure type safety.
chronark
changed the title
2 participants
Summary by CodeRabbit