feat(ids-admin): General Mandate delegation for companies #17026
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
# Conflicts: # apps/services/auth/ids-api/src/app/delegations/test/delegations.controller.spec.ts # libs/portals/admin/delegation-admin/src/screens/Root.tsx
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17026 +/- ##
==========================================
- Coverage 35.74% 35.71% -0.04%
==========================================
Files 6925 6949 +24
Lines 147571 148114 +543
Branches 42010 42186 +176
==========================================
+ Hits 52747 52895 +148
- Misses 94824 95219 +395
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 258 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 14 Total Test Services: 0 Failed, 12 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (2) |
![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
🧹 Outside diff range and nitpick comments (14)
apps/services/auth/ids-api/test/stubs/genericStubs.ts (1)
9-10: Consider consolidating national ID generation approachesThe file now has two different approaches for generating national IDs:
getFakeNationalId: Uses faker to generate 10 random digitsgetFakeCompanyNationalIdandgetFakePerson: UsecreateNationalIdwith type parameterWhile this is acceptable based on the retrieved learning about
getFakeNationalId, consider documenting the reason for these different approaches to avoid confusion.Add a comment explaining the difference:
export const getFakeNationalId = () => faker.helpers.replaceSymbolWithNumber('##########') +// Uses createNationalId to ensure proper format for company identifiers export const getFakeCompanyNationalId = () => createNationalId('company')apps/services/auth/delegation-api/project.json (1)
34-38: Consider enhancing the development configurationThe command configuration could be improved by:
- Adding environment variables for development settings
- Including watch mode flags if supported
"commands": [ { - "command": "yarn start services-auth-delegation-api" + "command": "yarn start services-auth-delegation-api --watch", + "env": { + "NODE_ENV": "development", + "LOG_LEVEL": "debug" + } } ],apps/services/auth/admin-api/project.json (1)
61-70: Simplify the dev target configuration.The current configuration uses parallel command execution with only a single command, which adds unnecessary complexity. Unless there are plans to add more commands soon, consider keeping the simpler configuration.
"dev": { "executor": "nx:run-commands", "options": { - "commands": [ - { - "command": "yarn start --project services-auth-admin-api" - } - ], - "parallel": true + "command": "yarn start --project services-auth-admin-api" } }If you're planning to add more commands later, please add a TODO comment explaining the future additions to help other developers understand the parallel configuration.
libs/auth-api-lib/src/lib/delegations/delegation-scope.service.ts (2)
8-8: Optimize import for better tree-shakingConsider importing only the required function instead of the entire module to improve tree-shaking capabilities.
-import * as kennitala from 'kennitala' +import { isCompany } from 'kennitala'
213-220: Consider caching the isCompany resultThe
isCompanycheck might be called multiple times for the same national ID. Consider caching the result to improve performance.+private readonly companyCache = new Map<string, boolean>(); + +private isCompany(nationalId: string): boolean { + if (!this.companyCache.has(nationalId)) { + this.companyCache.set(nationalId, kennitala.isCompany(nationalId)); + } + return this.companyCache.get(nationalId)!; +} delegationType: { - [Op.or]: kennitala.isCompany(fromNationalId) + [Op.or]: this.isCompany(fromNationalId)libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (1)
Line range hint
281-285: Add validation for fromNationalIdWhile the validation correctly ensures the delegate (toNationalId) is a person, it should also validate that the delegator (fromNationalId) is either a valid person or company national ID.
Apply this diff to add the missing validation:
if (!kennitala.isPerson(toNationalId)) { throw new BadRequestException({ message: 'National Ids are not valid', error: ErrorCodes.INPUT_VALIDATION_INVALID_PERSON, }) } +if (!kennitala.isPerson(delegation.fromNationalId) && !kennitala.isCompany(delegation.fromNationalId)) { + throw new BadRequestException({ + message: 'Delegator national ID must be a valid person or company', + error: ErrorCodes.INPUT_VALIDATION_INVALID_NATIONAL_ID, + }) +}libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts (3)
199-204: Enhance method documentationThe JSDoc comment should include the @returns tag to document the return type and describe what the method returns.
/** * Finds all companies that have a general mandate for the user. * @param user * @param clientAllowedApiScopes * @param requireApiScopes + * @returns Promise<MergedDelegationDTO[]> Array of company delegations with general mandate */
220-226: Update method documentation for new parameterThe JSDoc comment should document the new
supportedDelegationTypesparameter and its default value./** * Finds all individuals that have a general mandate for the user. * @param user * @param clientAllowedApiScopes * @param requireApiScopes + * @param supportedDelegationTypes - Array of delegation types to filter by, defaults to [AuthDelegationType.GeneralMandate] */
231-240: Consider improving type safetyThe type casting of delegation types using
as AuthDelegationTypesuggests potential type safety issues. Consider using type guards or validation to ensure type safety at runtime.- supportedDelegationTypes.includes( - dt.delegationType as AuthDelegationType, - ), + supportedDelegationTypes.includes(dt.delegationType) && + Object.values(AuthDelegationType).includes(dt.delegationType),libs/auth-api-lib/src/lib/delegations/delegations-incoming.service.ts (2)
315-319: Consider moving duplicate removal logic earlierWhile the implementation correctly removes duplicates using Set, consider moving this logic to the individual delegation providers to prevent potential duplicate processing earlier in the chain.
- mergedDelegationMap.forEach((delegation) => { - delegation.types = Array.from(new Set(delegation.types)) - }) + // Add this method to the class + private removeDuplicateTypes(types: AuthDelegationType[]): AuthDelegationType[] { + return Array.from(new Set(types)) + } + // Use it in each provider's mapping, e.g.: + ds.map((d) => ({ + ...DelegationDTOMapper.toMergedDelegationDTO(d), + types: this.removeDuplicateTypes(d.types) + }))
Line range hint
179-319: Architecture feedback: Good alignment with coding guidelinesThe implementation:
- Maintains reusability by keeping business logic in the service layer
- Uses proper TypeScript types (AuthDelegationType, MergedDelegationDTO)
- Follows existing patterns for feature flags and delegation handling
apps/services/auth/ids-api/src/app/delegations/test/delegations.controller.spec.ts (3)
64-65: Consider enhancing mock implementationsThe mock implementations for national registry APIs could be more robust:
- They always return the same user data regardless of the input ID
- They don't handle error cases
Consider adding these test cases:
// Add error cases nationalRegistryApiSpy = jest .spyOn(nationalRegistryApi, 'getIndividual') .mockImplementation(async (id) => { if (id === 'invalid') { throw new Error('User not found') } return createNationalRegistryUser({ nationalId: id === representeeNationalId ? representeeNationalId : id, }) })Also applies to: 85-85, 94-95, 137-156
167-168: Enhance spy cleanupThe spy cleanup could be more thorough by resetting all calls and implementations.
- nationalRegistryV3ApiSpy.mockClear() - nationalRegistryApiSpy.mockClear() + nationalRegistryV3ApiSpy.mockReset() + nationalRegistryApiSpy.mockReset()
226-251: Refactor duplicate cleanup codeThe cleanup code is duplicated across different describe blocks. Consider extracting it into a reusable helper function.
const cleanupModels = async (...models) => { for (const model of models) { await model.destroy({ where: {}, cascade: true, truncate: true, force: true, }) } } // Usage in afterAll blocks: await cleanupModels( apiScopeDelegationTypeModel, apiScopeModel, delegationDelegationTypeModel, delegationModel, )Also applies to: 379-408
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
apps/services/auth/admin-api/project.json(1 hunks)apps/services/auth/delegation-api/project.json(1 hunks)apps/services/auth/ids-api/project.json(1 hunks)apps/services/auth/ids-api/src/app/delegations/test/delegations.controller.spec.ts(7 hunks)apps/services/auth/ids-api/test/stubs/genericStubs.ts(2 hunks)libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts(5 hunks)libs/auth-api-lib/src/lib/delegations/delegation-scope.service.ts(2 hunks)libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts(1 hunks)libs/auth-api-lib/src/lib/delegations/delegations-incoming.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
apps/services/auth/admin-api/project.json (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."
apps/services/auth/delegation-api/project.json (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."
apps/services/auth/ids-api/project.json (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."
apps/services/auth/ids-api/src/app/delegations/test/delegations.controller.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."
apps/services/auth/ids-api/test/stubs/genericStubs.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/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.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."
libs/auth-api-lib/src/lib/delegations/delegation-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."
libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.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."
libs/auth-api-lib/src/lib/delegations/delegations-incoming.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."
📓 Learnings (2)
apps/services/auth/ids-api/src/app/delegations/test/delegations.controller.spec.ts (1)
Learnt from: saevarma
PR: island-is/island.is#16257
File: apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.auth.spec.ts:178-201
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `apps/services/auth/admin-api/src/app/v2/delegations/test/delegation-admin.auth.spec.ts`, ensure that test case names accurately describe the scenario being tested, particularly in cases involving invalid request signatures.
apps/services/auth/ids-api/test/stubs/genericStubs.ts (1)
Learnt from: xDivisionByZerox
PR: island-is/island.is#17058
File: apps/services/auth/ids-api/test/stubs/genericStubs.ts:6-6
Timestamp: 2024-11-27T19:52:35.776Z
Learning: In `apps/services/auth/ids-api/test/stubs/genericStubs.ts`, the `getFakeNationalId` function generates a 10-digit numeric string for testing purposes and does not need to produce valid Icelandic national IDs (kennitala). Another custom national ID generation function in the codebase handles generating valid kennitala when required.
🔇 Additional comments (10)
apps/services/auth/ids-api/test/stubs/genericStubs.ts (1)
23-23: LGTM!
The new function is correctly added to the default exports, maintaining consistency with the module's export pattern.
apps/services/auth/delegation-api/project.json (1)
31-41: Verify impact of parallel execution on related services
The new "dev" target configuration looks correct, but let's verify how it interacts with other services mentioned in the AI summary.
✅ Verification successful
Parallel execution configuration is consistent with related services
The parallel execution configuration in the new "dev" target is aligned with other auth services:
services-auth-admin-apialso usesparallel: true- Both services use independent
yarn startcommands without interdependencies services-auth-ids-apidoesn't have a comparable dev configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check configuration of related services to ensure consistent parallel execution setup
# Find all project.json files in auth services
fd project.json apps/services/auth -x cat {} | jq -c 'select(.targets.dev != null) | {name: .name, parallel: .targets.dev.options.parallel}'
Length of output: 300
Script:
#!/bin/bash
# Let's check if these services have any dependencies between them in their project.json files
fd project.json apps/services/auth -x cat {} | jq -c '.targets.dev.options.commands[].command'
Length of output: 436
apps/services/auth/admin-api/project.json (1)
60-60: LGTM! Valid NX workspace pattern for Docker filtering.
The empty docker-express target is a common pattern used in NX workspaces to filter projects with specific capabilities.
libs/auth-api-lib/src/lib/delegations/delegation-scope.service.ts (2)
213-220:
Add error handling for invalid national IDs
The isCompany check might fail with invalid national IDs. Consider adding validation and error handling.
delegationType: {
[Op.or]: kennitala.isCompany(fromNationalId)
+ ? isValidNationalId(fromNationalId)
+ ? [AuthDelegationType.GeneralMandate, AuthDelegationType.ProcurationHolder]
+ : (() => { throw new Error('Invalid national ID format') })()
? [
AuthDelegationType.GeneralMandate,
AuthDelegationType.ProcurationHolder,
]
: [AuthDelegationType.GeneralMandate],
},213-220: Verify access control for company delegations
Ensure that proper access controls are in place when allowing ProcurationHolder delegations for companies.
✅ Verification successful
Let me gather more information about the access control mechanisms for ProcurationHolder by checking the implementation and validation logic.
Let me try a different approach to find the access control mechanisms.
Based on the analysis of the codebase, I can now provide a final response:
Access controls for ProcurationHolder delegations are properly implemented
The codebase shows comprehensive access control mechanisms for ProcurationHolder delegations:
- Validation of delegation types and providers through
validateDelegationTypeAndProvider - Specific access guards in
UserAccessGuardthat handle ProcurationHolder context - Proper scope validation in
DelegationResourcesServicethat checks ProcurationHolder permissions - Additional validation in
DelegationsIncomingCompanyServicefor company-specific procuration holder access
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing access control mechanisms for ProcurationHolder
rg -A 10 "ProcurationHolder.*authorization|validateProcurationHolder"
# Check for any security-related tests
fd -e spec.ts -e test.ts | xargs rg "ProcurationHolder"
Length of output: 9207
Script:
#!/bin/bash
# Check for ProcurationHolder validation and access control implementation
ast-grep --pattern 'function $FUNC($$$) {
$$$
ProcurationHolder
$$$
}'
# Check for any guard or middleware implementations
rg -A 5 "class.*Guard.*ProcurationHolder|middleware.*ProcurationHolder"
# Check for any validation in the delegation service
ast-grep --pattern 'class DelegationService {
$$$
$FUNC($$$) {
$$$
ProcurationHolder
$$$
}
$$$
}'
Length of output: 272
Script:
#!/bin/bash
# Search for files containing ProcurationHolder validation logic
rg -l "ProcurationHolder" | xargs rg -A 10 "validate|check|verify|authorize"
# Look for guard implementations in auth related files
fd "guard.ts" | xargs rg -A 5 "ProcurationHolder"
# Check delegation scope implementation
rg -A 10 "delegationType.*ProcurationHolder" libs/auth-api-lib/
Length of output: 47234
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (2)
15-15: LGTM: Clean dependency injection for company registry service
The CompanyRegistryClientService is properly imported and injected, following the established dependency injection pattern.
Also applies to: 49-49
317-317: Verify the business rule for general mandate domain names
The code assumes general mandate delegations should have null domain names. This business rule should be verified and documented.
Run the following script to verify this assumption:
✅ Verification successful
Business rule for general mandate domain names is verified
Based on the codebase analysis, the assumption that general mandate delegations should have null domain names is correct. This is evidenced by:
- The
DelegationAdminCustomServiceexplicitly setsdomainName: nullwhen creating general mandate delegations - The UI components handle general mandate delegations differently from other types, specifically omitting domain name displays
- The feature is controlled by a feature flag
isGeneralMandateDelegationEnabled - The codebase consistently treats general mandates as domain-independent delegations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any general mandate delegations with non-null domain names
# This could indicate an inconsistency in the business rule implementation
ast-grep --pattern $'class DelegationAdminCustomService {
$$$
insertDelegation($$$) {
$$$
delegationDelegationTypes: [
{
delegationTypeId: AuthDelegationType.GeneralMandate,
$$$
}
]
$$$
}
$$$
}'
# Also check for any related tests
rg -l "GeneralMandate.*domainName" -t ts
Length of output: 290
Script:
#!/bin/bash
# Let's try a different approach to verify the business rule
# Search for general mandate related code and domain name handling
rg "GeneralMandate" -B 2 -A 2 -t ts
# Search for domain name handling in delegation-related tests
fd "delegation.*spec" -t f -e ts --exec rg "domainName" {}
# Look for domain name validation or business rules
rg "domainName.*null" -B 2 -A 2 -t ts
# Check for any documentation about general mandate
fd "README|CONTRIBUTING" -t f --exec rg -i "general.*mandate|domain.*name" {}
Length of output: 72487
libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts (1)
205-218: Verify delegation type compatibility
The method uses AuthDelegationType.ProcurationHolder when calling findAllAvailableGeneralMandate, but this might cause type compatibility issues since the called method is primarily designed for general mandates.
Let's verify the delegation type handling:
✅ Verification successful
No type compatibility issues with ProcurationHolder delegation type
The findAllAvailableGeneralMandate method is designed to handle different delegation types through its supportedDelegationTypes parameter, which has a default value but can accept other delegation types. The method is correctly used with AuthDelegationType.ProcurationHolder as it's explicitly passed as an argument, overriding the default value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how ProcurationHolder type is handled in the codebase
# Search for ProcurationHolder usage
rg -A 5 "ProcurationHolder"
# Search for delegation type validation
ast-grep --pattern 'supportedDelegationTypes.includes($_)'
Length of output: 101899
Script:
#!/bin/bash
# Let's check how findAllAvailableGeneralMandate handles delegation types
ast-grep --pattern 'findAllAvailableGeneralMandate($$$)'
# Also check for any validation of delegation types in the service
rg -A 5 "findAllAvailableGeneralMandate" libs/auth-api-lib/src/lib/delegations/delegations-incoming-custom.service.ts
Length of output: 601
libs/auth-api-lib/src/lib/delegations/delegations-incoming.service.ts (1)
179-196: LGTM: Well-structured feature flag integration
The implementation correctly handles the ProcurationHolder delegation type by:
- Checking if it's included in supported types
- Verifying the feature flag status
- Integrating with the existing delegation system
Let's verify the feature flag usage consistency:
✅ Verification successful
Let me gather more context about the feature flag definition and its usage pattern.
Based on the comprehensive search results, I can now generate the final response:
Feature flag usage is consistent and well-integrated
The feature flag isGeneralMandateDelegationEnabled is:
- Properly defined in the Features enum
- Consistently used for controlling general mandate delegation functionality
- Correctly integrated with the existing delegation system
- Used in conjunction with the ProcurationHolder type check, maintaining proper authorization control
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of the general mandate feature flag
rg -A 2 "Features.isGeneralMandateDelegationEnabled" --type ts
Length of output: 659
Script:
#!/bin/bash
# Search for the Features enum definition
ast-grep --pattern 'enum Features {
$$$
}'
# Search for any other references to general mandate in the codebase
rg "generalMandate|GeneralMandate" --type ts -A 2
Length of output: 50137
apps/services/auth/ids-api/src/app/delegations/test/delegations.controller.spec.ts (1)
4-4: LGTM: Import statements are well-organized
The new imports for createNationalId and getFakeCompanyNationalId are appropriately added to support the new test cases for company-related delegations.
Also applies to: 30-30, 36-39
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts
Show resolved
Hide resolved
apps/services/auth/ids-api/src/app/delegations/test/delegations.controller.spec.ts
Show resolved
Hide resolved
* initial push for company general mandate * added tests for company general mandate * merge conflict with main resolved * chore: nx format:write update dirty files * fix executor command for dev --------- Co-authored-by: andes-it <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
...
Attach a link to issue if relevant
What
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
Release Notes
New Features
devtarget for multiple services, allowing parallel execution of commands during development.Bug Fixes
Tests
Documentation