fix(auth-admin): Hide client secret native clients #16763
Conversation
WalkthroughThe changes introduce modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
magnearun
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16763 +/- ##
==========================================
- Coverage 36.44% 36.43% -0.02%
==========================================
Files 6852 6852
Lines 143559 143796 +237
Branches 40972 41089 +117
==========================================
+ Hits 52326 52388 +62
- Misses 91233 91408 +175
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 78 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
|
![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
🧹 Outside diff range and nitpick comments (3)
apps/services/auth/admin-api/src/app/v2/clients/me-clients.controller.ts (1)
104-107: Consider moving client type check to ClientSecretsServiceFor better separation of concerns, consider moving the client type validation to the
ClientSecretsService. This would:
- Encapsulate client secret creation rules in one place
- Make the controller thinner and more focused
- Make it easier to maintain and test
Example refactor:
async create( @CurrentUser() user: User, @Param('tenantId') tenantId: string, @Body() input: AdminCreateClientDto, ): Promise<AdminClientDto> { const client = await this.clientsService.create(input, user, tenantId) - if (client.clientType === ClientType.native) { - return client - } await this.clientsSecretsService.create(tenantId, client.clientId) return client }And in
ClientSecretsService:async create(tenantId: string, clientId: string): Promise<void> { const client = await this.clientsService.findByTenantIdAndClientId(tenantId, clientId); if (client.clientType === ClientType.native) { return; } // existing secret creation logic }libs/portals/admin/ids-admin/src/screens/Client/EditClient.tsx (1)
129-129: Consider adding prop type validation.To improve type safety and reusability across NextJS apps (as per coding guidelines), consider adding PropTypes or TypeScript prop validation for the
clientTypeprop in theBasicInfocomponent.- clientType={client.clientType} + clientType={client.clientType as AuthAdminClientType}libs/portals/admin/ids-admin/src/screens/Client/components/BasicInfo.tsx (1)
39-41: Consider adding a comment to explain the business logic.While the logic is correct, adding a comment would help future maintainers understand why native clients don't need client secrets.
+// Native clients don't require client secrets as per OAuth 2.0 specifications const hasClientSecrets = Boolean(clientSecrets && clientSecrets.length > 0) && clientType !== AuthAdminClientType.native
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/services/auth/admin-api/src/app/v2/clients/me-clients.controller.ts(2 hunks)libs/portals/admin/ids-admin/src/screens/Client/EditClient.tsx(1 hunks)libs/portals/admin/ids-admin/src/screens/Client/components/BasicInfo.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/services/auth/admin-api/src/app/v2/clients/me-clients.controller.ts (2)
Pattern apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/portals/admin/ids-admin/src/screens/Client/EditClient.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/ids-admin/src/screens/Client/components/BasicInfo.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (6)
apps/services/auth/admin-api/src/app/v2/clients/me-clients.controller.ts (2)
20-20: LGTM: Import addition follows best practices
The ClientType import is correctly grouped with other imports from the same module and is necessary for the new type check.
104-107: Verify impact on existing native clients
The implementation correctly prevents client secret creation for native clients. However, this is a breaking change that needs verification.
Let's check for existing native clients with secrets:
✅ Verification successful
The initial search revealed that native clients are used in the system, but we need to verify the historical handling of client secrets for native clients to ensure this change aligns with existing patterns and won't break existing implementations.
Change is safe and aligns with existing architecture
The code analysis reveals this change is safe and consistent with the existing architecture for the following reasons:
-
Native clients are already configured to not require client secrets:
- In
createClient.ts: Client secrets are only required for 'web' and 'machine' clients - In client forms:
requireClientSecretis explicitly set tofalsefor native clients
- In
-
The change follows established patterns:
- Native clients use PKCE for security instead of client secrets
- UI already handles native clients differently in secret management flows
- Migration scripts and seeders show native clients are grouped with SPA clients, which don't use secrets
The change simply moves an existing architectural requirement into an explicit guard clause, making the behavior more obvious and maintainable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to native clients and their secrets
# to assess the impact of this change
# Look for native client type usage
echo "Searching for native client type usage..."
rg -A 5 "clientType.*native"
# Look for related tests
echo "Searching for related test files..."
fd -e spec.ts -e test.ts | xargs rg -l "native.*client"
Length of output: 10216
Script:
#!/bin/bash
# Let's verify if there are any existing native clients with secrets in the database migrations or seeders
# Search for native client configurations in seeders
echo "Searching seeder data for native clients..."
fd seeder -e json -e js | xargs rg "clientType.*native" -B 2 -A 5
# Search for client secret related code specifically for native clients
echo "Searching for native client secret handling..."
rg -A 5 "requireClientSecret.*native|clientSecret.*native"
# Search for PKCE requirements for native clients
echo "Searching for PKCE requirements..."
rg "requirePkce.*native|PKCE.*native"
Length of output: 13491
libs/portals/admin/ids-admin/src/screens/Client/EditClient.tsx (1)
129-129: LGTM! The change aligns with PR objectives.
The addition of the clientType prop to BasicInfo enables the component to handle native clients differently, which directly implements the PR's goal of hiding client secrets for native clients.
libs/portals/admin/ids-admin/src/screens/Client/components/BasicInfo.tsx (3)
7-7: LGTM! Type definitions and imports are well structured.
The changes properly introduce the necessary types and maintain good TypeScript practices.
Also applies to: 10-10, 16-16
23-23: LGTM! Props destructuring is properly updated.
The clientType prop is correctly added to the component's parameters.
Line range hint 1-200: Component meets all coding guidelines requirements.
✅ The component:
- Is reusable across different NextJS apps
- Uses TypeScript effectively for props and type exports
- Has no tree-shaking concerns
* hide client secret from ui for native clients * not create secrets for native clients * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
https://www.notion.so/Bug-Remove-ClientSecret-from-Native-apps-in-IDS-7efd495d0290439bb9df3aea265a4b88?pvs=4
https://www.notion.so/Bug-Secret-created-for-Native-app-client-ff4918b4682e4ababb8723fe0c9b8e9c?pvs=4
What
Hide ui or client secret in ids admin since native client should not have client secrets but until now we have being adding them regardless
Stop creating client secrets for native clients
Why
Native clients do not require client secrets
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
nativeclient types more efficiently.clientTypeprop to theBasicInfocomponent for improved data handling.Bug Fixes
BasicInfoto refine the criteria for displaying client secrets based onclientType.Documentation