Skip to content

Commit 1320111

Browse files
authored
Add an endpoint saying who a project's work may be assigned to (#373)
* Add an endpoint saying who a project's work may be assigned to 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. * Record the new route in the OpenAPI snapshot 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.
1 parent 49af568 commit 1320111

9 files changed

Lines changed: 737 additions & 12 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,35 @@ and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.
99

1010
### Added
1111

12+
- **An endpoint that says who a project's work may be assigned to.** `GET
13+
/v1/projects/{project_id}/assignable-members` returns the people the
14+
assignment save accepts: members of the team that owns the project, active,
15+
and not service accounts. Until now nothing below super admin listed users at
16+
all, so a screen could only offer the signed-in person to themselves.
17+
18+
It returns an id and a display name, and nothing else. Not the email, which
19+
the admin team view carries because an administrator auditing who can reach a
20+
team needs one, and which would make every project page somewhere addresses
21+
can be collected from. Not the role, which does not decide who may be named.
22+
Service accounts are absent rather than labelled, because they cannot be
23+
assigned, so what comes back is the set the write accepts rather than a
24+
superset the caller has to filter.
25+
26+
Addressed by project rather than by team, so a caller never needs a team id
27+
to look work up by, and gated at `developer` to match the assignment it
28+
feeds: a caller who may perform an assignment has to be able to compose one.
29+
A caller outside the owning team is refused by the project's own rule.
30+
31+
The list and the write are computed from one query. Two copies of three
32+
conditions drift in a way that shows only from opposite ends: offering
33+
somebody the save refuses is met immediately, and failing to offer somebody
34+
it would accept is invisible, because the work goes to whoever is on the
35+
list instead. A query rather than a shared condition, because a condition
36+
leaves the join to the caller and a caller who forgets it gets a cross join
37+
that returns every active person in the deployment, with no error and no
38+
warning.
39+
40+
1241
- **The Helm chart can mount a file, so a private certificate authority works
1342
on Kubernetes too.** `env.extraVolumes` and `env.extraVolumeMounts` are raw
1443
lists appended to the backend, the scheduler and both workers, the same four

‎apps/backend/api/v1/projects.py‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@
7373
SeveritySummary,
7474
SourceArchiveUploadResponse,
7575
)
76+
from schemas.vulnerability_detail import AssignableMember, AssignableMemberList
7677
from services.batch_service import create_projects_batch
7778
from services.dependency_graph_service import get_dependency_graph
7879
from services.project_detail_service import (
@@ -1247,6 +1248,86 @@ async def upload_source_archive_endpoint(
12471248
)
12481249

12491250

1251+
1252+
# ---------------------------------------------------------------------------
1253+
# GET /v1/projects/{project_id}/assignable-members
1254+
# ---------------------------------------------------------------------------
1255+
1256+
1257+
@router.get(
1258+
"/{project_id}/assignable-members",
1259+
response_model=AssignableMemberList,
1260+
summary="People this project's work may be assigned to",
1261+
responses={
1262+
403: {
1263+
"description": (
1264+
"Caller is not on the team that owns the project. Matches the "
1265+
"project read above rather than the finding PATCH's 404: "
1266+
"reaching this needs a project id the caller already has."
1267+
)
1268+
},
1269+
404: {"description": "No such project."},
1270+
},
1271+
)
1272+
async def list_assignable_members_endpoint(
1273+
request: Request,
1274+
project_id: uuid.UUID,
1275+
session: AsyncSession = Depends(get_db),
1276+
actor: CurrentUser = Depends(require_role("developer")),
1277+
) -> Response:
1278+
"""Who may be named as the owner of a finding or an obligation here.
1279+
1280+
Addressed by project rather than by team because that is what a caller
1281+
holds: the screen is a project's finding list, and asking it for a team id
1282+
would mean handing out team ids to look work up by. The team is derived
1283+
here, and access is the project's own rule.
1284+
1285+
``developer`` for the same reason the assignment PATCH is: a caller who may
1286+
perform an assignment has to be able to compose one, and a list gated
1287+
higher would leave the write reachable only by somebody who already knew
1288+
the id.
1289+
1290+
The set is exactly what ``services.assignee`` will accept, because both go
1291+
through one predicate rather than two copies of three conditions.
1292+
1293+
A premise that holds today and will not always
1294+
---------------------------------------------
1295+
Deriving the team from the project is safe because reaching a project means
1296+
being on its team. Only ``visibility='team'`` is honoured
1297+
(``services.project_service``), so there is no other way in.
1298+
1299+
Organization-wide visibility would end that. Somebody on another team could
1300+
then read the project, and this route would hand them its members, which is
1301+
the enumeration the tests here refuse. Whoever enables it has to decide what
1302+
this endpoint does: most likely keep it on team membership rather than on
1303+
project access, since being allowed to read a project's findings is not the
1304+
same as being allowed to list the people on it.
1305+
1306+
``core.authz.team_scope_filter`` carries a pointer back here, because that
1307+
is the file the change lands in.
1308+
"""
1309+
from services.assignee import list_assignable_members
1310+
1311+
try:
1312+
project = await get_project(session, project_id=project_id, actor=actor)
1313+
except ProjectError as exc:
1314+
return _problem_for_project_error(request, exc)
1315+
1316+
rows = await list_assignable_members(session, project.team_id)
1317+
body = AssignableMemberList(
1318+
members=[
1319+
AssignableMember(user_id=user_id, full_name=full_name)
1320+
for user_id, full_name in rows
1321+
],
1322+
total=len(rows),
1323+
)
1324+
return Response(
1325+
content=body.model_dump_json(),
1326+
status_code=status.HTTP_200_OK,
1327+
media_type="application/json",
1328+
)
1329+
1330+
12501331
# slowapi's `@limiter.limit` wraps the endpoint with functools.wraps. The
12511332
# wrapper inherits slowapi's module as its `__globals__`, so under
12521333
# `from __future__ import annotations` FastAPI's `get_type_hints()` call on

‎apps/backend/core/authz.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,13 @@ def team_scope_filter(actor: CurrentUser) -> ColumnElement[bool]:
114114
lives in exactly one place and a future tweak (org-wide viewer role, etc.)
115115
lands here only.
116116
117+
One caller does not go through this and would be widened by such a tweak
118+
without touching it: ``GET /v1/projects/{project_id}/assignable-members``
119+
derives a team from a project and lists its members, which is safe only
120+
while reaching a project means being on its team. Enabling organization-wide
121+
visibility makes project access a weaker statement than team membership, so
122+
read that route before turning it on.
123+
117124
Contract:
118125
119126
- super-admin (``actor.is_superuser`` OR ``actor.role == "super_admin"``)

‎apps/backend/schemas/vulnerability_detail.py‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -880,6 +880,48 @@ class VulnerabilityDetailResponse(BaseModel):
880880
# ---------------------------------------------------------------------------
881881

882882

883+
class AssignableMember(BaseModel):
884+
"""One person who may be named as the owner of work in this project (ER65).
885+
886+
Two fields, and the omissions are the design. No email: the admin team view
887+
carries one because an administrator auditing who can reach a team needs
888+
it, and a picker does not, so a screen that only has to name colleagues
889+
does not become a place addresses are collected from. No role, which does
890+
not change who may be named. Service accounts are absent from the list
891+
entirely rather than flagged, because they are not assignable.
892+
"""
893+
894+
user_id: uuid.UUID = Field(description="Pass this as `assignee_user_id`.")
895+
full_name: str | None = Field(
896+
default=None,
897+
description=(
898+
"Display name, or null. A name is optional at registration, so "
899+
"some people have none. They are listed anyway: they are "
900+
"assignable, and hiding them would make the write reachable only "
901+
"by somebody who already knows the id. Render null as a "
902+
"placeholder, and distinguish two of them by the id rather than "
903+
"showing the same label twice."
904+
),
905+
)
906+
907+
908+
class AssignableMemberList(BaseModel):
909+
"""The people this project's work may be assigned to."""
910+
911+
members: list[AssignableMember]
912+
total: int = Field(
913+
description=(
914+
"Length of `members`. There is no paging and no cap: a team's "
915+
"whole assignable membership comes back on every call. Stated "
916+
"because a consumer would otherwise have to discover it. The "
917+
"largest team in the reference deployment holds four people, so "
918+
"this is a shape to know about rather than one to work around; a "
919+
"deployment with very large teams should ask for a bound before "
920+
"building on it."
921+
)
922+
)
923+
924+
883925
class VulnerabilityAssignmentUpdate(BaseModel):
884926
"""PATCH body for /vulnerability_findings/{id}/assignment (ER28a).
885927

‎apps/backend/services/assignee.py‎

Lines changed: 70 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,44 @@
1414

1515
import uuid
1616

17-
from sqlalchemy import select
17+
from sqlalchemy import Select, select
1818
from sqlalchemy.ext.asyncio import AsyncSession
1919

2020
from models import Membership, User
2121

2222

23+
def assignable_members_select(team_id: uuid.UUID) -> Select[tuple[uuid.UUID, str | None]]:
24+
"""Everyone who may be named as an owner of this team's work, as a query.
25+
26+
A statement rather than a bare predicate, and that is the point. The three
27+
conditions live here once so the question "may this person be named?" and
28+
the question "who may be named?" cannot drift; ER65 added the second caller,
29+
and writing the conditions again there would have made a list that offers
30+
people the write refuses, or hides people it would accept.
31+
32+
Returning the query carries the join with them. An earlier version exported
33+
the ``and_(...)`` on its own, which compiles without complaint when a caller
34+
forgets to join ``Membership``: the security review compiled
35+
``select(User.id).where(assignable_predicate(team))`` and got
36+
``FROM users, memberships``, a cross join that returns every active person
37+
in the deployment rather than the team's. No error, no warning, and every
38+
existing test still green, because the two call sites at the time happened
39+
to write the join by hand. A helper whose reason to exist is future reuse
40+
cannot rely on future callers remembering a rule its docstring states.
41+
42+
Callers narrow this rather than rebuild it: see the two below.
43+
"""
44+
return (
45+
select(User.id, User.full_name)
46+
.join(Membership, Membership.user_id == User.id)
47+
.where(
48+
User.is_active.is_(True),
49+
User.is_service_account.is_(False),
50+
Membership.team_id == team_id,
51+
)
52+
)
53+
54+
2355
async def is_assignable_to_team(
2456
session: AsyncSession, user_id: uuid.UUID, team_id: uuid.UUID
2557
) -> bool:
@@ -42,18 +74,44 @@ async def is_assignable_to_team(
4274
"""
4375
member = (
4476
await session.execute(
45-
select(User.id)
46-
.join(Membership, Membership.user_id == User.id)
47-
.where(
48-
User.id == user_id,
49-
User.is_active.is_(True),
50-
User.is_service_account.is_(False),
51-
Membership.team_id == team_id,
52-
)
53-
.limit(1)
77+
assignable_members_select(team_id).where(User.id == user_id).limit(1)
5478
)
55-
).scalar_one_or_none()
79+
).first()
5680
return member is not None
5781

5882

59-
__all__ = ["is_assignable_to_team"]
83+
async def list_assignable_members(
84+
session: AsyncSession, team_id: uuid.UUID
85+
) -> list[tuple[uuid.UUID, str | None]]:
86+
"""Everyone :func:`is_assignable_to_team` would accept for this team.
87+
88+
Returns ``(user_id, full_name)`` and nothing else. Not the email: the admin
89+
team view carries one because an administrator auditing who can reach a
90+
team needs it, and a picker does not. Not the role, which does not change
91+
who may be named. Not service accounts, because they are not assignable, so
92+
what comes back is exactly the set the write accepts rather than a
93+
superset the caller has to filter.
94+
95+
``full_name`` is nullable and optional at registration, so it can be None.
96+
Dropping those people would be the same defect from the other side: they
97+
are assignable, and a list that hides them makes the write reachable only
98+
by somebody who already knows the id. The caller renders them.
99+
100+
Ordered by name so the list is stable between calls; the ones with no name
101+
sort last together rather than being scattered through it.
102+
"""
103+
rows = (
104+
await session.execute(
105+
assignable_members_select(team_id).order_by(
106+
User.full_name.asc().nullslast(), User.id.asc()
107+
)
108+
)
109+
).all()
110+
return [(row[0], row[1]) for row in rows]
111+
112+
113+
__all__ = [
114+
"assignable_members_select",
115+
"is_assignable_to_team",
116+
"list_assignable_members",
117+
]

0 commit comments

Comments
 (0)