Conversation
|
Claude finished @jrmccannon's task in 4m 21s —— View job Re-Review: PM-18236 - Use Single Org RequirementScopeRe-review triggered by new commit Changes Since Last ReviewThe new commit contains only cosmetic changes:
Previous Findings Status
Full PR VerificationVerified correctness across all six modified command files:
The All changes are properly gated behind the No New FindingsThe code is correct and well-tested with 531 lines of unit tests covering all edge cases. No issues identified. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| public Error? IsCompliantForOrganizationToJoin(Guid targetOrganizationId, OrganizationUser organizationUser) => | ||
| !IsEnabledForTargetOrganization(targetOrganizationId) | ||
| || policyDetails.Single(x => x.OrganizationUserId == organizationUser.Id) | ||
| .HasRole([OrganizationUserType.Admin, OrganizationUserType.Owner]) | ||
| ? null | ||
| : new UserIsAMemberOfAnotherOrganizationError(); | ||
|
|
||
| public Error? IsEnabledForOtherOrganizationsUserIsAPartOf(Guid targetOrganizationId) => | ||
| policyDetails.Any(p => p.OrganizationId != targetOrganizationId) | ||
| ? new UserIsAMemberOfAnOrganizationThatHasSingleOrgPolicy() |
There was a problem hiding this comment.
IsCompliantForOrganizationToJoin returns an error when the target organization has SingleOrg policy enabled and the user is not an Admin/Owner, but it never checks whether the user actually has memberships in other organizations. This changes the behavior relative to the old code path.
The old code in all three callers (AcceptOrgUserCommand, ConfirmOrganizationUserCommand, RestoreOrganizationUserCommand) guards with:
if (allOrgUsers.Any(ou => ou.OrganizationId != orgUser.OrganizationId)
&& singleOrgPolicies.Any(p => p.OrganizationId == orgUser.OrganizationId))That is, both conditions must be true: (1) the target org has SingleOrg policy, and (2) the user belongs to at least one other org. This is correct -- SingleOrg means "you may only be in one organization," so a user joining their first and only org should be allowed.
The new code only checks condition (1). A user with zero other org memberships will now be blocked with "Member cannot join the organization until they leave or remove all other organizations" -- which is self-contradictory since they have no other organizations to leave.
This regression affects:
- Accept flow (
AcceptOrgUserCommand.ValidateSingleOrganizationPolicyAsync) - Confirm flow (
ConfirmOrganizationUserCommand.CheckPoliciesAsync, line 220) - Restore flow (
RestoreOrganizationUserCommand.CheckPoliciesBeforeRestoreAsync, line 302)
Additionally, the HasRole([Admin, Owner]) check on line 30 is effectively dead code because the factory's default ExemptRoles already filters out Admin/Owner entries from policyDetails before the requirement class sees them.
The test AcceptOrgUserAsync_WithPolicyRequirementsEnabled_UserJoiningOrgWithSingleOrgPolicy_ThrowsBadRequest validates this incorrect behavior -- SetupCommonAcceptOrgUserMocks returns [] for GetManyByUserAsync (no other orgs), so the old code would not throw here.
The requirement needs access to either the count of other org memberships or a way to determine whether the user is only in the target org, so it can preserve the old semantics.
| // handle all this filtering in the req | ||
|
|
||
| // can return specific errors for if the org applies | ||
|
|
||
| // think about who is joining and what their role is and what their current status is |
There was a problem hiding this comment.
🎨 SUGGESTED: Remove development notes before merge
These look like working notes from design/implementation. They should be cleaned up before this ships.
| // handle all this filtering in the req | |
| // can return specific errors for if the org applies | |
| // think about who is joining and what their role is and what their current status is |
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6999 +/- ##
==========================================
+ Coverage 56.28% 56.39% +0.10%
==========================================
Files 1986 1990 +4
Lines 87667 87919 +252
Branches 7816 7854 +38
==========================================
+ Hits 49345 49583 +238
- Misses 36491 36501 +10
- Partials 1831 1835 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
1 participant
🎟️ Tracking
PM-18236
📔 Objective
In order to help encapsulate all business logic around Policies, we are moving the logic into requirements that can take the raw data and provide meaningful methods to discern the state of the organization and what actions can be performed by a user.
For single organization, this captures that a user cannot be another member of an organization in order to join an exisitng org that has Single Org enabled. If a user is a member of anotehr orgh with Single Organization Policy enabled, they cannot join or create another org.
For more information see here
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes