Jason/arch unit additions #343
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideAligns the AdminRole domain model under the User context, adds permission/visa plumbing so only authorized users can modify admin roles, updates persistence wiring and tests to the new namespace, extends user passports/visas for admin-role access, expands admin role permissions, and adjusts GraphQL resolver typing and architecture/code-quality tests to the sthrift layout. Sequence diagram for updating an AdminRole with permission-based visa checksequenceDiagram
actor AdminUser
participant AppService as AdminUserApplicationService
participant Repo as AdminRoleRepository
participant Role as AdminRole
participant Passport as AdminUserUserPassport
participant Visa as AdminUserUserVisa
AdminUser->>AppService: updateAdminRole(command)
AppService->>Repo: getById(roleId)
Repo-->>AppService: AdminRole instance
AppService->>Role: set roleName(newName)
activate Role
Role->>Role: validateVisa()
Role->>Passport: forAdminRole(Role)
Passport-->>Role: Visa
Role->>Visa: determineIf(permissions => permissions.canManageUserRoles)
Visa-->>Role: boolean
alt not authorized
Role-->>AdminUser: throw PermissionError
else authorized
Role->>Role: update props.roleName
Role-->>AppService: updated AdminRole
AppService->>Repo: save(updated AdminRole)
Repo-->>AppService: persisted AdminRole
AppService-->>AdminUser: success
end
deactivate Role
Class diagram for updated AdminRole and user passport/visa permissionsclassDiagram
class AdminRole {
-boolean isNew
-UserVisa visa
+AdminRole(props, passport)
+getNewInstance(newProps, passport, roleName, isDefault) AdminRole
+roleName string
+isDefault boolean
-validateVisa() void
}
class AdminRoleProps {
<<interface>>
+string roleName
+boolean isDefault
}
class AdminRolePermissionsProps {
<<interface>>
+UserDomainPermissions userPermissions
}
class UserDomainPermissions {
<<interface>>
+boolean canBlockUsers
+boolean canUnblockUsers
+boolean canBlockListings
+boolean canUnblockListings
+boolean canRemoveListings
+boolean canViewListingReports
+boolean canViewUserReports
+boolean canManageUserRoles
+boolean isEditingOwnAccount
}
class RoleDomainPermissions {
<<interface>>
+boolean canCreateRole
+boolean canUpdateRole
+boolean canDeleteRole
+boolean canAssignRole
}
class UserVisa {
<<interface>>
+determineIf(func) boolean
}
class RoleVisa {
<<interface>>
+determineIf(func) boolean
}
class UserPassport {
<<interface>>
+forPersonalUser(root) UserVisa
+forAdminUser(root) UserVisa
+forAdminRole(root) UserVisa
}
class GuestUserPassport {
+forPersonalUser(root) UserVisa
+forAdminUser(root) UserVisa
+forAdminRole(root) UserVisa
}
class SystemUserPassport {
-UserDomainPermissions permissions
+forPersonalUser(root) UserVisa
+forAdminUser(root) UserVisa
+forAdminRole(root) UserVisa
}
class PersonalUserUserPassport {
-PersonalUserEntityReference _user
+forPersonalUser(root) UserVisa
+forAdminUser(root) UserVisa
+forAdminRole(root) UserVisa
}
class AdminUserUserPassport {
-AdminUserEntityReference _user
+forPersonalUser(root) UserVisa
+forAdminUser(root) UserVisa
+forAdminRole(root) UserVisa
}
class AdminRoleDomainAdapter {
+roleName string
+isDefault boolean
+permissions AdminRolePermissionsDomainAdapter
}
class AdminRolePermissionsDomainAdapter {
+userPermissions AdminRoleUserPermissionsDomainAdapter
}
AdminRole --> AdminRoleProps : props
AdminRole --> UserVisa : uses
AdminRole ..> UserPassport : via passport
AdminRole --> UserDomainPermissions : permission checks
RoleVisa --> RoleDomainPermissions
UserVisa --> UserDomainPermissions
GuestUserPassport ..|> UserPassport
SystemUserPassport ..|> UserPassport
PersonalUserUserPassport ..|> UserPassport
AdminUserUserPassport ..|> UserPassport
AdminRoleDomainAdapter ..|> AdminRoleProps
AdminRolePermissionsDomainAdapter ..|> AdminRolePermissionsProps
AdminRolePermissionsDomainAdapter --> AdminRoleDomainAdapter
AdminRoleDomainAdapter --> AdminRolePermissionsDomainAdapter
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Changing
AdminRole.isNewfromprotectedtoprivateand never resetting it tofalsemeans newly created roles will continue to bypassvalidateVisa()even after persistence, so the permission gate will never run for them—please either expose the flag to the base class/unit of work or explicitly flip it off after the first save. - The new
validateVisa()hook only guards theroleNameandisDefaultsetters; all other mutable properties (notably the nested permission structures exposed throughthis.props.permissions) remain writable without any visa check, so you should ensure those mutation paths also callvalidateVisa()to actually enforce the new authorization policy.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Changing `AdminRole.isNew` from `protected` to `private` and never resetting it to `false` means newly created roles will continue to bypass `validateVisa()` even after persistence, so the permission gate will never run for them—please either expose the flag to the base class/unit of work or explicitly flip it off after the first save.
- The new `validateVisa()` hook only guards the `roleName` and `isDefault` setters; all other mutable properties (notably the nested permission structures exposed through `this.props.permissions`) remain writable without any visa check, so you should ensure those mutation paths also call `validateVisa()` to actually enforce the new authorization policy.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce authorization and context restructuring for admin roles and update architecture tests and paths accordingly.
New Features:
Bug Fixes:
Enhancements:
Tests: