Add an endpoint saying who a project's work may be assigned to - #373
Merged
Conversation
Nothing below super admin listed users, so a screen could only offer the signed-in person to themselves. This returns the people the assignment save accepts: on the team, active, not a service account. An id and a display name, and nothing else. The admin team view carries an email because an administrator auditing who can reach a team needs one; a picker does not, and shipping it would make every project page somewhere addresses can be collected from. Addressed by project rather than by team, so a caller never needs a team id to look work up by, and gated at developer to match the assignment it feeds. The list and the write share one query. A shared condition was the first shape, and the security review showed it compiles to a cross join when a caller forgets to join memberships, returning every active person in the deployment with no error and no warning. Returning the query carries the join with it.
The drift gate is a snapshot of the wire surface, so adding an endpoint has to show as a reviewed line rather than as a passing test. One line added and nothing else moved.
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.
The assignment work shipped self-assignment only, because no route below super admin listed users, so a screen could offer the signed-in person to themselves and nobody else. A team lead could not hand work to a team member. This is the missing half.
GET /v1/projects/{project_id}/assignable-membersreturns the people the assignment save accepts: on the team that owns the project, active, and not a service account. Addressed by project rather than by team, because that is what the caller holds and asking for a team id would mean handing out team ids to look work up by. Gated atdeveloperto match the assignment PATCH it feeds: a caller who may perform an assignment has to be able to compose one, and a list gated higher would leave the write reachable only by somebody who already knew the id.An id and a display name, and nothing else. The admin team view carries an email because an administrator auditing who can reach a team needs one; a picker does not, and shipping it would make every project page a place addresses can be collected from. Service accounts are absent rather than labelled, so the list is the set the write accepts rather than a superset the caller has to filter. Somebody who never set a display name comes back with
full_name: nulland is still listed, because they are assignable and hiding them would leave the write reachable only by id.The list and the write share one query, and that is the part the security review changed. The first shape exported the three conditions as a predicate for both callers to use. The reviewer compiled
select(User.id).where(assignable_predicate(team))and gotFROM users, memberships: a cross join returning every active person in the deployment rather than the team's, with no error, no warning, and every test still green, because the two call sites at the time happened to write the join by hand. Returning the query carries the join with it, so forgetting it is not available. A test asserts the statement has one final FROM, and builds the two-FROM form alongside so that assertion is known to distinguish something.Three other review findings are recorded rather than fixed, each for a stated reason.
The 403 body names the owning team's UUID. The enumeration test checked for names and user ids and so certified "no member data in the refusal" while the refusal carried the team id; adding it to that tuple fails. It is pre-existing on all fifteen routes built on
get_project, includingGET /v1/projects/{project_id}at a lower grade, so this route discloses nothing new, and changing the message is not this pull request's to make. A separate test asserts the id is present, so the next person meets it as a decision instead of reading the first test as a guarantee it does not give.The viewer denial is conditional. The effective grade is one number across every membership, so a viewer here who is a developer elsewhere clears the floor, and
get_projectthen asks only for membership. The gate is unchanged on purpose: the same person reaches the assignment PATCH by the same mechanism, so narrowing only the list would create the divergence between offered and accepted that the shared query exists to prevent. The case is asserted at its actual result, so changing the role model to per-team grading fails here and asks whether the write moved too.The premise that project access implies team membership is now a test rather than a comment. It switches a project to
visibility=organization, asserts the switch took, and requires an outsider to still be refused. Onlyvisibility=teamis honoured today, so whoever wires organization-wide visibility gets a member-enumeration route for free; a comment needs reading and a test does not.Left for separate work: the response has no cap, which is stated in the schema so a consumer does not have to discover it. Measured rather than guessed: the largest team in the reference deployment holds four people, so this is a shape to know about rather than one to work around, and the right bound depends on a picker screen that does not exist yet.
Verification: mypy over 916 files, ruff, and 470 tests across this endpoint, the assignment PATCH, obligations and the permission matrices. Reversing each design choice fails a different test: dropping the active, service-account or team condition, lowering the gate, removing the cross-team check, and taking the join back off the query.