Give projects an owning team, and make team role decide what a team confers - #1282
Merged
Merged
Conversation
Projects gain an owning team, the same way challenges already had one. Its owners, admins and managers manage the project, and its approved image becomes the picture on the project's card, derived in the json the way a challenge's is. Null for the projects nobody has assigned, which keep working purely off grants. Ownership is distinct from granting a team a role on the project, which stays what it was: one of several teams helping run it. Ownership is singular, and only it puts a picture on the card. Handing a project to a team requires managing that team -- otherwise anyone could hang their project off another team's name, and its image, by guessing an id -- which is the rule challenges already enforce, generalized here. Teams can also now be granted a role on a single challenge. Users could already be, and hasChallengeGrant would have honoured a team's grant, but there was no service method, endpoint or route to create one. Granting requires admin on the challenge, however that admin is held.
Every "may this user touch this" question was answered independently -- in hasProjectAccess, in the challenge branch, in the managed-project listing -- each consulting a different subset of the ways in. That is how a team's grant came to reach projects (folded into the user's grant list by UserRepository) while reaching challenges not at all, and why adding a route meant finding every caller. Permission.effectiveRole is now the single answer, for a project and for a challenge, and the checks are one comparison against it. Ownership and attachment collapse into the same question: which teams are associated with this work, and what does this person do in them. What a team confers changes with it. A grant to a team used to hand every member the role it named, so a plain member of a team added as Admin administered the project. Members now get the role they hold in the team -- owners and admins administer, managers may edit, plain members get nothing -- which is the line ownership already drew. The role on the grant is no longer read. TeamAccessSpec covers the matrix: each team role against an attached team and an owning team, a stranger, a challenge grant that must not leak to its project, inheritance from the project, and an invitation that confers nothing until accepted.
Handing a challenge to a team already required running that team, but taking one away required nothing of the mover beyond being able to edit the challenge -- so an admin of the parent project could reassign a team's challenge to their own team, or hand it back to nobody, without the team having any say. A change of owning team now needs the mover to run the team it is leaving. Clearing ownership counts as a change: without that, the rule would be a formality, since anyone could clear first and then set. A challenge nobody owns is unaffected, and can still be given to any team the user runs.
requireChallengeOwnership, requireProjectOwnership and validateOwnerTeam all read as though ownership of the team were the bar. It never was: each resolved to isUserTeamManager, so an owner, an admin and a manager all passed and only a plain member was turned away. Three wrappers around one check, each named for a rule none of them applied. They collapse into requireTeamManager, which says the bar out loud, and the controllers' helpers become validateTeamAssignment. No behaviour changes -- the same people could do the same things before this commit.
Access now resolves through Permission.effectiveRole, which asks two things the old checks never did: which teams are attached to the thing being reached for, and what the user does in them. PermissionsSpec mocks its world, and those two services were never stubbed, so both answered null and every write and admin case died with a NullPointerException where it expected to be refused. They answer "no teams involved" now, which is what those specs are about -- the grants a person holds directly.
|
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
1 participant
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.
Merge first: both frontend branches depend on the endpoints and behaviour here.
Projects can be owned by a team
A challenge could already be given to a team; a project could not. Evolution 128
adds
owner_team_idwith the same foreign key and partial index the challengecolumn has. That team's owners, admins and managers run the project, and its
approved image becomes the picture on the project's card, derived in the JSON
exactly as a challenge's is.
Handing a project to a team requires managing that team — otherwise anyone could
hang their project off another team's name, and its image, by guessing an id.
Teams can be granted a role on a challenge
Users could already be. Teams could not, at any layer:
GrantTarget.challengeexisted and
hasChallengeGrantwould have honoured a team's grant, but there wasno service method, controller action or route to create one. Granting requires
admin on the challenge, however that admin is held.
One resolver instead of four
"May this user touch this?" was answered independently in
hasProjectAccess, inthe challenge branch of
hasObjectAccess, and in the managed-project listing —each consulting a different subset of the ways in. That is how a team's grant
came to reach projects (folded into the user's grant list by
UserRepository)while reaching challenges not at all.
Permission.effectiveRoleis now the single answer, and each check is onecomparison against it. Ownership and attachment collapse into the same question:
which teams are associated with this work, and what does this person do in them.
A grant to a team used to hand every member the role it named, so a plain
member of a team added as Admin administered the project. Members now get the
role they hold in the team:
This removes access that exists today. Run
audit.sqlagainst staging andproduction before deploying — it reports, per person, who keeps admin, who drops
to write, and who loses access outright.
Taking work away from a team
Giving a challenge to a team required running that team; taking one away required
nothing beyond being able to edit it, so a project admin could reassign a team's
challenge, or hand it back to nobody, with the team having no say. A change of
owning team now needs the mover to run the team it is leaving. Clearing counts as
a change — otherwise the rule is a formality, since anyone could clear and then
set.
One consequence worth knowing: if an owning team loses all its managers, only a
superuser can move that challenge again.
Testing
353 passing. New
TeamAccessSpeccovers the matrix — each team role against anattached team and an owning team, a stranger, a challenge grant that must not
leak to its project, inheritance from the project, and an invitation that confers
nothing until accepted.
ProjectSpeccovers the derived image url. Evolution 128has been applied to a virgin database and exercised over HTTP.
Projects still have the looser transfer rule (their source side is unguarded) —
deliberately left, since the ask named challenges.