spaceAuthorization removed dependency on account authorization #4711
Conversation
…odule still present until move all data to be via defaults
…ut to space defaults
… for templatesManager
…prevent template used as default to be deleted; fixed removal of template set on template manager
…into subspaceTemplates
…ing limits, removing notion of soft limits
…he Space does not know about the parent account authorization
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several changes primarily focused on the authorization policy management within the application. Key modifications include updating method calls to pass identifiers instead of entire objects, enhancing the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (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
|
![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 (10)
src/domain/common/authorization-policy/authorization.policy.interface.ts (2)
15-16: Add documentation for the parent authorization policy relationship.The new property lacks documentation explaining its purpose and usage in the authorization hierarchy.
Consider adding JSDoc comments:
+ /** + * Reference to a parent authorization policy that this policy inherits from. + * Used to establish hierarchical relationships between authorization policies. + */ parentAuthorizationPolicy?: IAuthorizationPolicy;
15-16: Consider potential circular dependency risks.The self-referential relationship could lead to circular dependencies or infinite loops in the authorization hierarchy if not properly managed.
Consider:
- Adding runtime checks to prevent circular references
- Implementing a maximum depth limit for the hierarchy
- Using a validation decorator or constraint to ensure the parent reference doesn't create a cycle
src/migrations/1731781160588-authParent.ts (2)
8-9: Consider adding UUID validation constraintThe column type
char(36)implies UUID usage. Consider adding a check constraint to ensure valid UUID format.-`ALTER TABLE \`authorization_policy\` ADD \`parentAuthorizationPolicyId\` char(36) NULL` +`ALTER TABLE \`authorization_policy\` ADD \`parentAuthorizationPolicyId\` char(36) NULL, + ADD CONSTRAINT \`CHK_parentAuthPolicy_uuid\` CHECK + (\`parentAuthorizationPolicyId\` IS NULL OR \`parentAuthorizationPolicyId\` REGEXP '^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$')`
1-23: Document the new authorization hierarchy modelSince this migration is part of a larger change to remove space authorization's dependency on account authorization, consider:
- Adding documentation that explains the new parent-child authorization policy relationship
- Providing examples of valid policy hierarchies
- Documenting any constraints or rules about policy inheritance
This will help maintain the separation of concerns you're establishing and prevent future coupling issues.
src/domain/common/authorization-policy/authorization.policy.entity.ts (1)
30-34: Consider adding an index for the parent relationshipSince the
onDelete: 'SET NULL'strategy will need to update child records when a parent is deleted, consider adding an index to optimize this operation.@ManyToOne(() => AuthorizationPolicy, { eager: false, cascade: false, // MUST not cascade onDelete: 'SET NULL', }) + @Index() parentAuthorizationPolicy?: AuthorizationPolicy;src/services/api/conversion/conversion.resolver.mutations.ts (1)
107-124: Consider optimizing parent authorization retrieval.While the implementation is solid, consider these improvements:
- The query loads more data than necessary by including the entire parent space.
- The error message could distinguish between missing parent space vs. missing authorization.
Consider this optimization:
private async getParentSpaceAuthorization( subspaceID: string ): Promise<IAuthorizationPolicy | never> { - const subspace = await this.spaceService.getSpaceOrFail(subspaceID, { - relations: { - parentSpace: { - authorization: true, - }, - }, - }); - if (!subspace.parentSpace || !subspace.parentSpace.authorization) { + const subspace = await this.spaceService.getSpaceOrFail(subspaceID, { + select: ['parentSpace.id'], + relations: { + parentSpace: { + authorization: true, + }, + }, + }); + if (!subspace.parentSpace) { throw new RelationshipNotFoundException( - `Unable to load parent space authorization for subspace: ${subspaceID}`, + `Parent space not found for subspace: ${subspaceID}`, + LogContext.CONVERSION + ); + } + if (!subspace.parentSpace.authorization) { + throw new RelationshipNotFoundException( + `Authorization not found for parent space: ${subspace.parentSpace.id}`, LogContext.CONVERSION ); } return subspace.parentSpace.authorization; }src/domain/common/authorization-policy/authorization.policy.service.ts (2)
Line range hint
154-164: Document the newly public reset methodSince this method is now public and part of the service's API, please add JSDoc documentation explaining:
- The purpose of resetting authorization policies
- When this method should be used
- The expected state of the authorization policy after reset
+/** + * Resets an authorization policy by clearing all rules. + * Use this method when you need to completely clear the authorization rules + * before applying new ones, such as when inheriting from a parent policy. + * + * @param authorizationPolicy - The authorization policy to reset + * @throws {RelationshipNotFoundException} When the authorization policy is undefined + * @returns The reset authorization policy with empty rules + */ public reset( authorizationPolicy: IAuthorizationPolicy | undefined ): IAuthorizationPolicy {
Line range hint
154-164: Consider improving robustness and security of the reset operationThe current implementation has several potential improvements:
- Use empty arrays instead of empty strings for rules to maintain consistency with JSON parsing
- Add audit logging for tracking reset operations
- Consider adding a parameter to selectively reset specific rules
public reset( authorizationPolicy: IAuthorizationPolicy | undefined ): IAuthorizationPolicy { if (!authorizationPolicy) { throw new RelationshipNotFoundException( 'Undefined Authorization Policy supplied', LogContext.AUTH ); } + this.logger.verbose?.( + `Resetting authorization policy: ${authorizationPolicy.id}`, + LogContext.AUTH + ); - authorizationPolicy.credentialRules = ''; - authorizationPolicy.verifiedCredentialRules = ''; - authorizationPolicy.privilegeRules = ''; + authorizationPolicy.credentialRules = '[]'; + authorizationPolicy.verifiedCredentialRules = '[]'; + authorizationPolicy.privilegeRules = '[]'; return authorizationPolicy; }src/domain/space/account/account.service.authorization.ts (1)
Line range hint
144-157: Consider more granular error handling for relationship checks.The current implementation throws a single error when any of the required relationships are missing. Consider handling each relationship separately to provide more specific error messages and potentially allow partial updates where possible.
Example improvement:
- if ( - !account.agent || - !account.spaces || - !account.virtualContributors || - !account.innovationPacks || - !account.storageAggregator || - !account.innovationHubs || - !account.license - ) { - throw new RelationshipNotFoundException( - `Unable to load Account with entities at start of auth reset: ${account.id} `, - LogContext.ACCOUNT - ); - } + if (!account.agent) { + throw new RelationshipNotFoundException( + `Agent relationship not found for account: ${account.id}`, + LogContext.ACCOUNT + ); + } + if (!account.spaces) { + throw new RelationshipNotFoundException( + `Spaces relationship not found for account: ${account.id}`, + LogContext.ACCOUNT + ); + } + // Similar checks for other relationshipssrc/domain/space/account/account.resolver.mutations.ts (1)
102-103: Add error handling for authorization policy applicationWhile the change to use
space.idaligns with decoupling space authorization from account authorization, consider adding error handling to ensure authorization policies are properly applied.- await this.spaceAuthorizationService.applyAuthorizationPolicy(space.id); + try { + await this.spaceAuthorizationService.applyAuthorizationPolicy(space.id); + } catch (error) { + throw new ValidationException( + `Failed to apply authorization policy for space: ${space.id}`, + LogContext.ACCOUNT + ); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
src/core/bootstrap/bootstrap.service.ts(1 hunks)src/domain/common/authorization-policy/authorization.policy.entity.ts(2 hunks)src/domain/common/authorization-policy/authorization.policy.interface.ts(1 hunks)src/domain/common/authorization-policy/authorization.policy.service.ts(1 hunks)src/domain/space/account/account.resolver.mutations.ts(2 hunks)src/domain/space/account/account.service.authorization.ts(1 hunks)src/domain/space/space/space.resolver.mutations.ts(3 hunks)src/domain/space/space/space.service.authorization.ts(9 hunks)src/migrations/1731781160588-authParent.ts(1 hunks)src/services/api/conversion/conversion.resolver.mutations.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
src/core/bootstrap/bootstrap.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.
src/domain/common/authorization-policy/authorization.policy.entity.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/common/authorization-policy/authorization.policy.interface.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/common/authorization-policy/authorization.policy.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.
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/domain/space/space/space.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/space/space.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/migrations/1731781160588-authParent.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/api/conversion/conversion.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.
🪛 Biome
src/domain/space/space/space.service.authorization.ts
[error] 157-157: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (16)
src/domain/common/authorization-policy/authorization.policy.interface.ts (1)
15-16: Verify the implementation of hierarchical authorization.
The introduction of parent-child relationships requires careful consideration of how permissions are inherited and combined.
Let's check the implementation details:
✅ Verification successful
Let me gather more information about the authorization policy implementation.
Based on the gathered information, I can now provide a final response:
Hierarchical authorization implementation is properly handled
The codebase shows a well-structured implementation of parent-child authorization inheritance with:
- Clear parent-child relationship through
parentAuthorizationPolicyfield - Proper inheritance logic in
inheritParentAuthorization()method that:- Cascades visibility settings
- Inherits credential rules with cascade flag
- Validates both parent and child policies
- Validation checks through
validateAuthorization()to ensure policy integrity - Database-level foreign key constraints to maintain referential integrity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the implementation of hierarchical authorization
# Check for permission inheritance logic
echo "Checking permission inheritance implementation..."
rg -A 10 "parentAuthorizationPolicy" --type ts
# Look for potential validation logic
echo "Checking for validation logic..."
ast-grep --pattern 'class $_ implements $_ {
$$$
validate() {
$$$
}
$$$
}'
# Check for circular reference prevention
echo "Checking circular reference prevention..."
rg -A 5 "circular" --type ts
Length of output: 9408
Script:
#!/bin/bash
# Check for authorization policy usage and inheritance
echo "Checking authorization policy usage..."
rg -A 10 "class AuthorizationPolicy" --type ts
# Check for methods that handle authorization inheritance
echo "Checking authorization inheritance methods..."
ast-grep --pattern 'function $_(authorizationPolicy: $_) {
$$$
}'
# Check for authorization policy validation
echo "Checking authorization validation..."
rg -A 10 "validateAuthorization" --type ts
Length of output: 21045
src/migrations/1731781160588-authParent.ts (2)
1-5: LGTM! Class declaration follows TypeORM conventions
The migration class is properly structured with correct imports and naming conventions.
15-22: LGTM! Down migration is properly implemented
The rollback operations are in the correct order, dropping the foreign key constraint before removing the column.
src/domain/common/authorization-policy/authorization.policy.entity.ts (3)
2-2: LGTM: Import statement is correctly organized
The ManyToOne import is appropriately grouped with other TypeORM decorators.
27-29: Well-documented relationship purpose
The comments clearly explain the rationale behind the parent-child relationship and its benefits for authorization inheritance.
30-35: Verify authorization inheritance security implications
The self-referential relationship configuration looks correct with appropriate safeguards:
- Cascade disabled to prevent unintended authorization propagation
- Eager loading disabled to prevent performance issues
- SET NULL on delete to maintain referential integrity
However, please verify:
- No potential for circular dependencies in the authorization hierarchy
- No possibility of privilege escalation through parent policy manipulation
✅ Verification successful
Let me gather more information about authorization policy creation and validation logic.
Let me search for any potential circular reference checks in the authorization policy service.
Let me search for the authorization policy entity and service methods to verify the inheritance handling.
Let me search for the authorization service implementation to understand how the inheritance is handled during authorization checks.
Based on the code analysis, I can now provide a final response regarding the authorization inheritance security implications.
Authorization inheritance implementation is secure
The implementation has multiple security safeguards:
- Parent policy relationship is properly configured with cascade disabled and SET NULL on delete
- Authorization checks traverse up to parent policy but prevent circular dependencies through TypeORM's relationship handling
- Privilege escalation is prevented as:
- Parent policy modifications require explicit GRANT privileges
- Child policies cannot modify their parent relationships directly
- Authorization service validates all privilege checks against both direct and inherited policies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for circular reference prevention in the codebase
echo "Searching for circular reference checks..."
rg -A 5 "parentAuthorizationPolicy"
# Look for authorization policy creation/update logic
echo "Searching for policy manipulation safeguards..."
ast-grep --pattern 'class $_ {
$$$
create$_AuthorizationPolicy($_) {
$$$
}
$$$
}'
Length of output: 4350
Script:
#!/bin/bash
# Search for authorization policy validation and creation logic
echo "Searching for authorization policy service and validation..."
rg -A 10 "class AuthorizationPolicyService"
# Look for circular reference checks
echo "Searching for cycle detection logic..."
rg -A 10 "cycle|circular|loop" --type ts
# Search for policy update methods
echo "Searching for policy update methods..."
ast-grep --pattern 'async update$_AuthorizationPolicy($_) {
$$$
}'
Length of output: 170842
Script:
#!/bin/bash
# Search for authorization policy service implementation
echo "Searching for authorization policy service..."
rg -A 10 "class AuthorizationPolicyService"
# Look for parent policy validation
echo "Searching for parent policy validation..."
rg -A 10 "parentAuthorizationPolicy" --type ts --glob "*.service.ts"
# Search for policy update/save methods
echo "Searching for policy update methods..."
rg -A 10 "save\(.*AuthorizationPolicy\)" --type ts
Length of output: 1739
Script:
#!/bin/bash
# Search for authorization policy entity methods
echo "Searching for authorization policy entity methods..."
rg -A 20 "class AuthorizationPolicy" --type ts
# Search for save/update methods in authorization policy service
echo "Searching for policy save/update methods..."
rg -A 20 "async (save|update|create).*AuthorizationPolicy" --type ts
# Look for parent policy relationship validation
echo "Searching for parent policy validation..."
rg -A 10 "validate.*parent.*policy|check.*parent.*policy" --type ts
Length of output: 23559
Script:
#!/bin/bash
# Search for authorization service implementation
echo "Searching for authorization service..."
rg -A 20 "class AuthorizationService" --type ts
# Look for privilege inheritance handling
echo "Searching for privilege inheritance..."
rg -A 10 "parentAuthorizationPolicy.*privilege|inherit.*privilege" --type ts
# Search for authorization checks
echo "Searching for authorization checks..."
rg -A 10 "isAccessGranted|hasPrivilege" --type ts
Length of output: 39885
src/services/api/conversion/conversion.resolver.mutations.ts (3)
19-20: LGTM! Enhanced error handling imports.
The new imports support proper error handling for space relationship scenarios.
94-102: LGTM! Improved parent-child space authorization handling.
The changes properly implement parent space authorization retrieval and application, which aligns with the PR objective of cleaning up space authorization logic.
65-65: LGTM! Improved parameter passing.
The change to pass space.id instead of the full space object aligns with best practices.
✅ Verification successful
Consistent usage of ID parameter confirmed
The codebase search shows that the change to use space.id instead of the full space object in applyAuthorizationPolicy is consistent with the broader pattern across the codebase. Most authorization services follow this practice of accepting entity IDs rather than full objects, particularly for:
- User authorization:
userID: string - Space authorization:
spaceID: string - Profile authorization:
profileID: string - Templates manager authorization:
templatesManagerID: string - Community authorization:
communityID: string - Role set authorization:
roleSetID: string
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent usage of applyAuthorizationPolicy across the codebase
# Expect: All calls should use entity IDs rather than full objects
ast-grep --pattern 'applyAuthorizationPolicy($$$)'
Length of output: 52
Script:
#!/bin/bash
# Search for applyAuthorizationPolicy method calls using ripgrep for better coverage
rg "applyAuthorizationPolicy" -A 2 -B 2
Length of output: 106455
src/domain/space/space/space.resolver.mutations.ts (3)
167-167: LGTM! Consistent with other authorization policy changes
The change maintains consistency with other authorization policy updates in the codebase.
137-137: LGTM! Verify other callers of applyAuthorizationPolicy
The change to pass space.id instead of the space object aligns with the PR objective of removing account authorization dependencies and follows the principle of passing minimal required data.
✅ Verification successful
LGTM! Change to pass space.id is consistent with codebase patterns
The change to pass space.id instead of the space object aligns with the established pattern across the codebase. The search results show that most services follow this pattern of passing IDs rather than full objects when applying authorization policies, particularly in:
- User service:
userAuthorizationService.applyAuthorizationPolicy(user.id) - Whiteboard service:
whiteboardAuthorizationService.applyAuthorizationPolicy(whiteboard.id) - Role set service:
roleSetAuthorizationService.applyAuthorizationPolicy(roleSet.id) - Callout service:
calloutAuthorizationService.applyAuthorizationPolicy(callout.id)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify all callers of applyAuthorizationPolicy have been updated
ast-grep --pattern 'applyAuthorizationPolicy($$$)'
Length of output: 52
Script:
#!/bin/bash
# Search for applyAuthorizationPolicy method calls using ripgrep for better coverage
rg "applyAuthorizationPolicy" -A 2
Length of output: 69191
199-202: Verify parent-child space authorization inheritance
The change now explicitly passes the parent space's authorization policy when creating a subspace. While this looks correct, we should verify:
- That this maintains proper authorization inheritance
- That there are no edge cases where authorization could be incorrectly propagated
The inline comment "Important, and will be stored" suggests this is a critical part of the authorization flow.
✅ Verification successful
Based on the extensive authorization service implementations found in the previous search results, particularly the space authorization patterns and parent-child authorization inheritance mechanisms shown across multiple services, I can now provide a final response:
Parent-child space authorization inheritance is correctly implemented
The codebase shows a robust authorization inheritance pattern where:
- Parent space's authorization is explicitly passed and stored in subspace creation
- The authorization policy service handles inheritance through
inheritParentAuthorizationmechanism - The pattern is consistently applied across the codebase for parent-child relationships
- The comment "Important, and will be stored" is accurate as the parent authorization is persisted in the authorization policy entity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other parent-child space authorization relationships
ast-grep --pattern 'class $_ {
$$$
createSubspace($$$) {
$$$
authorization
$$$
}
$$$
}'
# Look for tests covering this scenario
rg -A 5 "test.*subspace.*authorization"
Length of output: 151
Script:
#!/bin/bash
# Search for authorization policy application in the codebase
rg -A 5 "applyAuthorizationPolicy"
# Look for space authorization related tests with broader pattern
rg -A 5 "describe.*space.*authorization|test.*space.*authorization"
# Search for space authorization service implementation
ast-grep --pattern 'class SpaceAuthorizationService {
$$$
applyAuthorizationPolicy($$$) {
$$$
}
$$$
}'
# Look for authorization inheritance patterns
rg -A 5 "inheritance.*authorization|parent.*authorization"
Length of output: 140663
Script:
#!/bin/bash
# Look for space authorization service implementation
ast-grep --pattern 'class SpaceAuthorizationService {
$$$
async applyAuthorizationPolicy(spaceID: string, parentAuthorization?: $_) {
$$$
}
$$$
}'
# Look for tests specifically related to subspace authorization inheritance
rg -A 10 "test.*subspace.*parent.*authorization|describe.*subspace.*parent.*authorization"
# Look for space authorization policy inheritance logic
rg -A 5 "inheritParentAuthorization.*space|space.*inheritParentAuthorization"
Length of output: 333
src/domain/common/authorization-policy/authorization.policy.service.ts (1)
Line range hint 316-318: Address the TODO comment regarding undefined child authorization
The TODO comment indicates temporary fix for handling undefined child authorization. This should be properly addressed as it could lead to unexpected behavior.
Let's check for other occurrences of this pattern:
Would you like me to help create a GitHub issue to track the proper implementation of this temporary fix?
src/domain/space/account/account.service.authorization.ts (1)
153-153: LGTM! Performance improvement by passing ID instead of full space object.
This change aligns well with the PR objective of decoupling space authorization from account authorization. Passing just the ID instead of the full space object is more efficient, especially within this loop processing multiple spaces.
Let's verify that this pattern is consistently applied across the codebase:
✅ Verification successful
Pattern is consistently applied across the codebase
The verification confirms that all calls to spaceAuthorizationService.applyAuthorizationPolicy() are correctly using space.id or subspace.id instead of passing full space objects. This consistent pattern is found across multiple files:
src/core/bootstrap/bootstrap.service.tssrc/domain/space/account/account.service.authorization.tssrc/domain/space/account/account.resolver.mutations.tssrc/services/api/conversion/conversion.resolver.mutations.tssrc/domain/space/space/space.resolver.mutations.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances where full space objects might still be passed to applyAuthorizationPolicy
# We're looking for patterns that might indicate passing full objects instead of IDs
ast-grep --pattern 'spaceAuthorizationService.applyAuthorizationPolicy($space)'
# Also check for any direct references to space properties in the authorization service
rg -l "spaceAuthorizationService.*apply.*Authorization.*Policy.*space\."
Length of output: 421
Script:
#!/bin/bash
# Let's check the actual implementation in these files to see if they're passing space objects or IDs
rg -A 2 "spaceAuthorizationService.*apply.*Authorization.*Policy" src/services/api/conversion/conversion.resolver.mutations.ts src/domain/space/space/space.resolver.mutations.ts src/domain/space/account/account.resolver.mutations.ts src/domain/space/account/account.service.authorization.ts src/core/bootstrap/bootstrap.service.ts
Length of output: 3258
src/core/bootstrap/bootstrap.service.ts (1)
517-517: LGTM! Consistent with space authorization refactoring.
The change from passing the entire space object to just space.id aligns with the broader effort to remove space authorization's dependency on account authorization. This is a good practice as it reduces coupling and passes only the necessary data.
Let's verify that similar changes have been made consistently across the codebase:
✅ Verification successful
All instances of applyAuthorizationPolicy consistently use space.id
The verification confirms that all calls to spaceAuthorizationService.applyAuthorizationPolicy() across the codebase consistently pass space.id rather than the full space object. This change is part of a complete and consistent refactoring.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances where entire space objects are passed to applyAuthorizationPolicy
# This helps ensure the refactoring is complete across the codebase
# Search for potential calls passing full space object
rg "spaceAuthorizationService\.applyAuthorizationPolicy\([^)]*space[^.]" --type ts
# Search for the new pattern to verify consistency
rg "spaceAuthorizationService\.applyAuthorizationPolicy\([^)]*space\.id" --type ts
Length of output: 1099
src/domain/space/account/account.resolver.mutations.ts (1)
416-419: Address TODO comment and verify reload necessity
The TODO comment suggests uncertainty about the necessity of reloading the space. Let's verify if this reload is required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/domain/space/space/space.service.authorization.ts (1)
519-519: Enhance the TODO commentThe comment "Later: to allow account admins to some settings?" lacks context. Consider adding more details about which settings and the rationale behind this potential change.
Apply this change:
- // Later: to allow account admins to some settings? + // TODO: Consider extending platform settings access to account admins + // Rationale: Account admins might need access to specific space settings + // without requiring global admin privileges
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/domain/space/space/space.service.authorization.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/domain/space/space/space.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.
🪛 Biome
src/domain/space/space/space.service.authorization.ts
[error] 156-156: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (5)
src/domain/space/space/space.service.authorization.ts (5)
43-43: LGTM: Clean dependency injection
The addition of PlatformAuthorizationPolicyService follows NestJS dependency injection best practices and aligns with the PR objective of removing account authorization dependency.
Also applies to: 48-48
65-67: LGTM: Improved method signature
The change from passing an entire ISpace object to just the ID reduces coupling, while the optional parent authorization parameter better supports the hierarchical authorization structure.
225-236: LGTM: Clean implementation of parent authorization retrieval
The method properly handles the parent-child authorization relationship with appropriate error handling and type safety.
505-513: LGTM: Improved base authorization reset
The implementation properly handles base authorization reset and platform-level inheritance, aligning with the removal of account authorization dependency.
156-170:
Fix variable declarations in switch cases
Variables declared in switch cases are hoisted to the function scope, which could lead to unexpected behavior. Wrap the case blocks in curly braces to create proper block scoping.
Apply this fix:
case SpaceLevel.CHALLENGE:
case SpaceLevel.OPPORTUNITY: {
if (isPrivate) {
// ...
} else {
// ...
}
// For subspace, the parent space admins credentials should be allowed to delete
const parentSpaceCommunity = space.parentSpace?.community;
if (!parentSpaceCommunity || !parentSpaceCommunity.roleSet) {
throw new RelationshipNotFoundException(
`Unable to load Space with parent RoleSet in auth reset: ${space.id} `,
LogContext.SPACES
);
}
parentSpaceAdminCredentialCriterias =
await this.roleSetService.getCredentialsForRole(
parentSpaceCommunity.roleSet,
CommunityRoleType.ADMIN,
spaceSettings
);
break;
}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 156-156: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/domain/space/space/space.service.authorization.ts (1)
521-521: Consider documenting the planned account admin settings accessThe comment suggests future changes for account admin settings access. Consider creating a tracking issue for this enhancement.
Would you like me to create a GitHub issue to track the planned account admin settings access implementation?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/domain/space/space/space.service.authorization.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/domain/space/space/space.service.authorization.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
🔇 Additional comments (5)
src/domain/space/space/space.service.authorization.ts (5)
48-48: LGTM: New dependency injection follows NestJS best practices
The addition of PlatformAuthorizationPolicyService as a private dependency aligns with the PR's objective to refactor space authorization.
65-67: LGTM: Method signature improvement reduces coupling
The change from passing a full space object to using spaceID and optional providedParentAuthorization improves modularity and reduces coupling.
227-238: LGTM: Well-implemented error handling
The new method properly handles the parent authorization policy retrieval with clear error messaging.
Line range hint 507-546: LGTM: Clean implementation of the new authorization model
The method correctly implements the new authorization model with proper platform-level authorization inheritance and global space read access.
157-170:
Fix variable declarations in switch cases
Variables declared within case clauses should be properly scoped to prevent hoisting issues.
Apply this diff:
- const parentSpaceCommunity = space.parentSpace?.community;
+ {
+ const parentSpaceCommunity = space.parentSpace?.community;
+ if (!parentSpaceCommunity || !parentSpaceCommunity.roleSet) {
+ throw new RelationshipNotFoundException(
+ `Unable to load Space with parent RoleSet in auth reset: ${space.id} `,
+ LogContext.SPACES
+ );
+ }
+ parentSpaceAdminCredentialCriterias =
+ await this.roleSetService.getCredentialsForRole(
+ parentSpaceCommunity.roleSet,
+ CommunityRoleType.ADMIN,
+ spaceSettings
+ );
+ }
An important clean up of the logic around space authorization.
The logic whereby the space still used the account authorization was technical debt left over from when each account had exactly one space.
This was
a) a serious logic error in that having an elevated role on an account (e.g. org admin) gave access to all spaces in that account
b) a blocker for space templates
Whiteboard with rational + design notes
In addition, removed privileges related to auth + license reset on Space as we do not expose those mutations.
To do: update test suites if needed.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
RelationshipNotFoundExceptionfor missing parent space authorizations.Refactor
Chores