Repository navigation
Exclude tool provider admins from the last-user check - #93
Merged
Merged
Conversation
Fixes the tests to replicate bug in issue #85 No fixture org had a single direct user, so check 5a had no coverage at all, and test_delete_user_role_cannot_delete_last_user pointed at an org with two so it passed via the last-admin check below it. Adds an org with one non-admin direct user and an authorised tool, and repoints that test at it. The test fails at this commit. The tool-derived PROVIDER_ADMIN rows inflate the count, 5a passes, and the organisation's last member is removed. Also pins the sibling last-admin test to its own error message, since both checks return 400.
The last-user check on removing a user from a reporting org counted the result of get_user_associations_for_org, which returns tool-derived PROVIDER_ADMIN associations as well as direct reporting-org roles. A tool's provider admin therefore counted as a member of the organisation, and contributed one association per authorised tool, so the last direct member of an org authorised for a tool could be removed and the org left with no members. This commit adds get_direct_user_associations_for_org to the provider interface and uses it to fix the last user check.
simon-20
approved these changes
Oct 5, 2026
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
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.
Addresses issue #85 - provider admins incorrectly counted in last-user check
What changed
DELETE /users/{user_id}/reporting-org/{org_id}guards against emptying an organisation with a "is this the last user?" check. It counted the result ofget_user_associations_for_org, which returns tool-derivedPROVIDER_ADMINassociations as well as direct reporting-org roles so a tool's provider admin counted as a member of the organisation, and contributed one association per authorised tool. The last real member of an org authorised for a tool could be removed, leaving the org with nobody in it.FineGrainedAuthorisationProvidergainsget_direct_user_associations_for_org, and the check calls that instead. It's a concrete method defined in terms of the existing abstractget_user_associations_for_org, so it inherits all integrity checks and obliges no implementation to supply anything.Two commits:
test:adds the fixture and repoints the test - fails at this commitfix:adds the provider method and switches the function call - greenScope decisions
test_delete_user_role_cannot_delete_last_userwas passing because of the '5b' last-admin check. The '5a - check that the user isn't the last user in the organisation' check was never exercised. Refactoring for testability considered out of scope.Testing
flake8,black,isort,mypyclean;banditzero issues.test:commit the new test fails with200 == 400and the SQL log shows theDELETEexecuting.test_provider_roles_are_correctly_appliedcover an org with one direct user among four provider-admin associations, and an org with none at all despite two provider admins reaching it.Notes for reviewer