feat(ids-admin): Enable delegation settings for normal users #15181
Conversation
…ettings on permissions for normal users.
…ields on permissions. Remove isSuperAdmin check in delegation section in application. Remove unused variable.
WalkthroughThe recent changes primarily focus on refining the authorization and delegation functionalities within the authentication API and admin portals. Key modifications include the removal of unnecessary delegation types and API scope fields, updates to consent and access control features, and adjustments to the UI elements and test cases to reflect these changes. The goal is to streamline the authorization process and enhance the clarity and manageability of permissions and client configurations. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
saevarma
added
the
deprecated:automerge
![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)
libs/auth-api-lib/src/lib/resources/admin/admin-scope.service.ts (3)
Line range hint
28-41: Remove invalid decorator usage.The
@InjectModeldecorators are incorrectly placed on class properties, which is not valid in TypeScript. Decorators can only be used on class declarations, class expressions, class methods, or method parameters. To fix this, you should move these decorators inside the constructor and use them on the constructor parameters.- @InjectModel(ApiScope) - private readonly apiScope: typeof ApiScope, - @InjectModel(ApiScopeUserClaim) - private readonly apiScopeUserClaim: typeof ApiScopeUserClaim, - @InjectModel(ApiScopeDelegationType) - private readonly apiScopeDelegationType: typeof ApiScopeDelegationType, + constructor( + @InjectModel(ApiScope) private readonly apiScope: typeof ApiScope, + @InjectModel(ApiScopeUserClaim) private readonly apiScopeUserClaim: typeof ApiScopeUserClaim, + @InjectModel(ApiScopeDelegationType) private readonly apiScopeDelegationType: typeof ApiScopeDelegationType, + private readonly adminTranslationService: AdminTranslationService, + private readonly translationService: TranslationService, + private sequelize: Sequelize, + ) {}
Line range hint
44-63: Consider optimizing the sorting of API scopes.Currently, the API scopes are sorted in the application layer after retrieval from the database. This could potentially lead to performance issues if the number of scopes is large. Consider modifying the SQL query to include an
ORDER BYclause, which would perform the sorting at the database level, likely improving performance.- const apiScopes = await this.apiScope.findAll({ - where: { - domainName: tenantId, - enabled: true, - }, - include: [ - { model: ApiScopeDelegationType, as: 'supportedDelegationTypes' }, - ], - }) + const apiScopes = await this.apiScope.findAll({ + where: { + domainName: tenantId, + enabled: true, + }, + order: [['name', 'ASC']], + include: [ + { model: ApiScopeDelegationType, as: 'supportedDelegationTypes' }, + ], + })Tools
Biome
[error] 41-41: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.
Line range hint
109-150: Enhance error messages for better debugging and user feedback.The error messages in the
createScopemethod could be more descriptive to aid in debugging and provide better feedback to the user. Consider including more context in the messages, such as the invalid input values.- throw new BadRequestException(`Invalid scope name: "${input.name}"`) + throw new BadRequestException(`Invalid scope name provided: "${input.name}". Scope names must start with the tenant ID prefix.`) - throw new BadRequestException(`Scope name "${input.name}" already exists`) + throw new BadRequestException(`The scope name "${input.name}" is already in use. Please choose a different name.`) - throw new BadRequestException(translatedValuesErrorMsg) + throw new BadRequestException(`${translatedValuesErrorMsg}. Please ensure both displayName and description are provided and correctly formatted.`)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- libs/auth-api-lib/src/lib/clients/admin/dto/admin-patch-client.dto.ts (1 hunks)
- libs/auth-api-lib/src/lib/resources/admin/admin-scope.service.ts (2 hunks)
- libs/auth-api-lib/src/lib/resources/admin/dto/admin-patch-scope.dto.ts (1 hunks)
- libs/portals/admin/ids-admin/src/lib/messages.ts (1 hunks)
- libs/portals/admin/ids-admin/src/screens/Client/EditClient.tsx (1 hunks)
- libs/portals/admin/ids-admin/src/screens/Client/components/Delegation.tsx (4 hunks)
- libs/portals/admin/ids-admin/src/screens/Permission/EditPermission.tsx (2 hunks)
Files skipped from review due to trivial changes (2)
- libs/auth-api-lib/src/lib/resources/admin/dto/admin-patch-scope.dto.ts
- libs/portals/admin/ids-admin/src/lib/messages.ts
Additional context used
Path-based instructions (5)
libs/portals/admin/ids-admin/src/screens/Permission/EditPermission.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/auth-api-lib/src/lib/clients/admin/dto/admin-patch-client.dto.ts (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/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/Delegation.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/auth-api-lib/src/lib/resources/admin/admin-scope.service.ts (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."
Biome
libs/auth-api-lib/src/lib/resources/admin/admin-scope.service.ts
[error] 37-37: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.
[error] 39-39: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.
[error] 41-41: Decorators are not valid here. (parse)
Decorators are only valid on class declarations, class expressions, and class methods.
You can enable parameter decorators by setting theunsafeParameterDecoratorsEnabledoption totruein your configuration file.
Additional comments not posted (4)
libs/portals/admin/ids-admin/src/screens/Permission/EditPermission.tsx (1)
22-23: Reordering of components looks good.Please verify the user interface to ensure that the reordering provides a better user experience as intended.
libs/auth-api-lib/src/lib/clients/admin/dto/admin-patch-client.dto.ts (1)
Line range hint
1-100: Addition of new properties for delegation and API scopes is well-documented.Please ensure that these new properties are correctly handled in the backend logic.
libs/portals/admin/ids-admin/src/screens/Client/EditClient.tsx (1)
129-129: Conditional rendering of theDelegationcomponent based on the application type is implemented correctly.Please verify that the conditional rendering functions as expected in different scenarios.
libs/portals/admin/ids-admin/src/screens/Client/components/Delegation.tsx (1)
Line range hint
1-100: Removal of theisSuperAdmincheck to democratize access to delegation settings is aligned with the PR objectives.Please conduct thorough testing to ensure that this change does not introduce any security or business logic issues.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15181 +/- ##
==========================================
- Coverage 37.15% 36.99% -0.17%
==========================================
Files 6475 6418 -57
Lines 131847 130936 -911
Branches 37691 37490 -201
==========================================
- Hits 48987 48435 -552
+ Misses 82860 82501 -359
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 198 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 14 Total Test Services: 0 Failed, 14 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (4) |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- apps/services/auth/admin-api/src/app/v2/clients/test/me-clients.spec.ts (1 hunks)
- apps/services/auth/admin-api/src/app/v2/scopes/test/me-scopes.spec.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- apps/services/auth/admin-api/src/app/v2/scopes/test/me-scopes.spec.ts
Additional context used
Path-based instructions (1)
apps/services/auth/admin-api/src/app/v2/clients/test/me-clients.spec.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."
Additional comments not posted (4)
apps/services/auth/admin-api/src/app/v2/clients/test/me-clients.spec.ts (4)
372-372: The test case for creating a client with default values for 'super admin fields' as a normal user is well-implemented.This test case effectively checks the system's behavior when a normal user tries to create a client with fields typically reserved for super admins. The assertions are correctly checking the expected outcomes.
372-372: The test case for a super user creating a client with correct none default values is well-implemented.This test case effectively checks the system's behavior when a super user creates a client with non-default values. The assertions are correctly checking the expected outcomes, ensuring that the system behaves as expected for super users.
372-372: The test case for creating a client with correct delegation types for a super user is well-implemented.This test case effectively checks the system's behavior when a super user creates a client with specific delegation types. The assertions correctly verify the supported delegation types in the response, ensuring that the system behaves as expected for super users.
372-372: The test case for updating supported delegation types is well-implemented.This test case effectively checks the system's behavior when supported delegation types are updated. The assertions correctly verify the supported delegation types and related boolean fields in the response, ensuring that the system behaves as expected.
What
Why
We are now onboarding more organisations and we want them to be able to configure their own delegation settings.
Screenshots / Gifs
N/A
Checklist:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests