fix my account rules #4717
fix my account rules #4717
Conversation
WalkthroughThe pull request introduces several key changes across three files, focusing on enhancing asynchronous handling of license validation, updating authorization rules with new credential definitions, and modifying filtering logic for entitlement usage. The Changes
Possibly related PRs
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
|
![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 and nitpick comments (2)
src/domain/space/account/account.service.authorization.ts (1)
295-298: Significant change in authorization strategyThe authorization for creating resources is being shifted from host-based (using
hostCredentials) to feature-based (usingvcCampaignCredentialandbetaTesterCredential). This is a fundamental change that affects who can create resources under an account.Consider documenting this change in architecture decision records (ADR) to track the rationale behind this authorization strategy shift.
src/domain/space/account/account.resolver.mutations.ts (1)
Line range hint
591-599: Enhance error message clarityThe error message could be more user-friendly by providing clearer guidance.
- throw new ValidationException( - `Unable to create ${licenseType} on account: ${account.id}. Entitlement limit of ${entitlementLimit} of type ${licenseType} reached`, - LogContext.ACCOUNT - ); + throw new ValidationException( + `License limit reached: Cannot create ${licenseType} on this account. You have reached the maximum limit of ${entitlementLimit} ${licenseType}s. Please upgrade your license to add more.`, + LogContext.ACCOUNT + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/domain/space/account/account.resolver.mutations.ts(6 hunks)src/domain/space/account/account.service.authorization.ts(2 hunks)src/services/infrastructure/license-entitlement-usage/license.entitlement.usage.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/domain/space/account/account.resolver.mutations.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/domain/space/account/account.service.authorization.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
src/services/infrastructure/license-entitlement-usage/license.entitlement.usage.service.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs. Ensure that the code adheres to TypeScript's typing system and modern standards.
🔇 Additional comments (7)
src/domain/space/account/account.service.authorization.ts (2)
302-302: Verify impact on existing users and add tests
The authorization rules for creating spaces, virtual contributors, and innovation packs now require either VC campaign or beta tester credentials. This change might affect existing users who previously had access through host credentials.
#!/bin/bash
# Search for existing tests covering these authorization rules
rg -l "createSpace|createVC|createInnovationPack" --type ts --glob "**/*.spec.ts"
# Search for existing usages that might be affected
rg "CREATE_SPACE|CREATE_VIRTUAL_CONTRIBUTOR|CREATE_INNOVATION_PACK" --type tsWould you like me to help generate test cases to verify:
- Access is granted with VC campaign credentials
- Access is granted with beta tester credentials
- Access is denied without these credentials
- Existing host credentials no longer grant access
Also applies to: 310-310, 319-319
285-293: Verify the empty resourceID in credential definitions
The resourceID is set to an empty string for both credentials. While this might be intentional for platform-wide credentials, please verify this is the expected behavior.
✅ Verification successful
Let me try another search pattern to specifically look for credential definitions.
Let me try one more search with a simpler pattern to find credential definitions.
Based on the extensive search results, I can see that empty resourceID is commonly used for platform-wide credentials throughout the codebase. Specifically:
- The
BETA_TESTERandVC_CAMPAIGNcredentials are used in platform-wide authorization rules, similar to other global credentials likeGLOBAL_ADMIN,GLOBAL_SUPPORT, etc. - Other instances of platform-wide credentials (e.g.,
GLOBAL_REGISTERED,GLOBAL_ADMIN,GLOBAL_SUPPORT) consistently use empty resourceID. - Only role-specific credentials (like
SPACE_MEMBER,ORGANIZATION_ADMIN) require a specific resourceID.
Empty resourceID is correct for platform-wide credentials
The empty resourceID in both vcCampaignCredential and betaTesterCredential is intentional and follows the established pattern for platform-wide authorization credentials in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other credential definitions to verify the pattern
rg -A 2 "type:\s*AuthorizationCredential.*,\s*resourceID:" --type ts
Length of output: 2757
Script:
#!/bin/bash
# Search for credential definitions with type and resourceID
rg "type:\s*AuthorizationCredential\.[A-Z_]+,\s*resourceID:" -B 2 -A 2 --type ts
Length of output: 82
Script:
#!/bin/bash
# Search for ICredentialDefinition usage
rg "ICredentialDefinition" -B 2 -A 5 --type ts
# Also search for AuthorizationCredential enum usage
rg "AuthorizationCredential\." -B 2 -A 2 --type ts
Length of output: 118641
src/domain/space/account/account.resolver.mutations.ts (5)
88-91: LGTM! Proper async handling of license validation
The validation call is correctly awaited and the parameters are properly passed.
Line range hint 152-156: LGTM! Proper async handling of license validation
The validation call is correctly awaited and the parameters are properly passed.
Line range hint 195-199: LGTM! Proper async handling of license validation
The validation call is correctly awaited and the parameters are properly passed.
Line range hint 255-259: LGTM! Proper async handling of license validation
The validation call is correctly awaited and the parameters are properly passed.
584-591: Verify platform admin bypass behavior
The platform admin check bypasses the license entitlement validation. This is a critical path that should be thoroughly tested.
src/services/infrastructure/license-entitlement-usage/license.entitlement.usage.service.ts
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Improvements
Bug Fixes