🏗️ PUT-1736: a team directory apps can read, once the team opens it - #3745
🏗️ PUT-1736: a team directory apps can read, once the team opens it#3745jfcastro92 wants to merge 1 commit into
Conversation
c43e43d to
c1c1fc7
Compare
Coverage Report
File Coverage |
c1c1fc7 to
4bca032
Compare
4bca032 to
65bfba9
Compare
65bfba9 to
abf5895
Compare
abf5895 to
c6e2cd7
Compare
Coverage Report for puter.js SDK
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
c6e2cd7 to
c634771
Compare
Local validation — the directory, API and GUIEngine mysql 8, live instance. The migration applied and the column is real: Every gate, against the running serverThe payload line is the one worth reading twice: the response carries The activation distinction, exercised liveThe bug this PR nearly shipped with was testing "never activated" as The live run walks a seat through all three states and checks the directory at each:
Confirmed on real data rather than fixtures, and the API run separately shows a provisioned seat has The toggle in the consolePlaywright, real browser: Screenshot
One fix came out of looking at it. The confirm button was styled VerificationTypecheck clean · backend 7897 passed | 26 skipped · GUI+SDK 275 passed · all 13 commits typecheck individually. Falsified earlier and still standing: the opt-in check (2 tests), the membership requirement (1), the exclusion filter (1). |
Members can already enumerate each other: `/teams/:uid/members` needs a user actor and nothing more. The only thing this adds is admitting an app actor to the same names, so an app can offer colleagues without the member driving it. That is the whole risk, so it is off until the team owner turns it on. `group.directory_enabled` defaults to 0, and a team that has not opted in answers 404 rather than 403 -- whether a team has this on is not something an app should be able to probe for either. Three things bound what an app sees. The membership tested is always the person's, never the app's, so an app installed by a member of one team can never read another's. The page carries username and uuid and nothing else -- no email, activation state, usage or role. And suspended accounts and ones that never took up their credential are left out, since offering someone who cannot sign in is noise and their existence is not this list's to disclose. Activation is the forced-change flag clearing, not the password existing: a provisioned seat holds its temporary password from birth, so testing `password IS NOT NULL` would have leaked exactly the accounts meant to be excluded. A test covers that distinction. Turning the directory on or off writes an audit row, because it changes who can read the member list and that is not something a team should be able to alter silently. Setting it to the value it already has records nothing. The toggle lives in TabTeams, and turning it on asks for confirmation while turning it off does not -- one grants access, the other only takes it away. Closes PUT-1736.
c634771 to
24f11d5
Compare
1 participant
Closes PUT-1736. Stacked on #3742.
The delta is one flag, not a new disclosure
The ticket reads as "should colleagues be visible to apps," but
/teams/:uid/membersalready hasrequireUserActor: trueand nothing more — any member can already enumerate their colleagues today. The gate is the actor type, not the data.So the only thing this adds is admitting an app-under-user actor to the same names. That is the whole risk, and it is what the setting governs.
The open question, resolved
The ticket said resolve before building. Chosen: a per-team setting, default off.
Rejected alternatives, and why:
⚠ Worth a reviewer's eye: the names disclosed belong to the members, and the owner flips the switch. What makes that defensible is that the payload is strictly what colleagues already see, it is off unless deliberately turned on, and turning it on is audited. If that is not enough, the escalation is a member-level opt-out on top — deliberately not built, because a setting nobody finds is worse than no setting.
What bounds an app
directory_enableddefaults to 0. A team that has not opted in answers 404, not 403 — whether a team has this on is not something an app should be able to probe for either.usernameanduuidand nothing else. No email, activation state, usage or role — those stay on the admin routes.The bug this nearly shipped with
The obvious test for "never activated" is
password IS NOT NULL. That is wrong: a provisioned seat holds its temporary password from birth, so the filter would have admitted exactly the accounts meant to be excluded. Activation isrequires_password_changeclearing.I only caught it because the HTTP test asserted the seat was absent and it wasn't. There is now a test that walks a seat through provisioned → activated → suspended and asserts directory membership at each step.
The toggle
In
TabTeams, next to the team card. Turning it on asks for confirmation naming what apps will be able to read; turning it off does not — one grants access, the other only takes it away. A refused confirmation repaints so the checkbox does not sit checked against the truth.Both transitions write an audit row (
directory_enabled/directory_disabled), and setting it to the value it already has records nothing.Verification
Typecheck clean,
check:puterjs:typesclean. Backend 7892 passed | 26 skipped, GUI and SDK 79 passed. All 13 commits in the stack typecheck individually.Falsified — each gate broken, the failing test confirmed, then restored:
Migration
0082_team-directory.sql/mysql_mig_37/postgres_mig_26, addinggroup.directory_enabledwith a default of 0. Idempotent on all three, since mysql has no per-file applied-state tracking.