ποΈ PUT-1704 + PUT-1707: TeamService β workspace lifecycle, disable and re-enable - #3712
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
4dfb179 to
d4e8d74
Compare
d4e8d74 to
05dade4
Compare
05dade4 to
be53b88
Compare
be53b88 to
246ecda
Compare
246ecda to
3fffcce
Compare
3fffcce to
008a236
Compare
008a236 to
c769ca2
Compare
c769ca2 to
0330cf3
Compare
0330cf3 to
bf907a0
Compare
bf907a0 to
d304b15
Compare
Salazareo
left a comment
There was a problem hiding this comment.
I'm a bit unsure about whether or not we should use suspended column for this. We've primarily used it for abuse prevention, so might cause some unwanted effects in gui/ prod metrics.
that said, the reason field might be enough to have that not be a problem, but well need to start checking and exposing that reason (only if its disabled by workspace).
i leave it to you to decide, but just make sure that this suspended reason is handled properly everywhre
d304b15 to
f8794e6
Compare
f8794e6 to
0af13b3
Compare
0af13b3 to
ba83e5b
Compare
ba83e5b to
1a4c526
Compare
1a4c526 to
765ae53
Compare
765ae53 to
f9d9585
Compare
f9d9585 to
aad4994
Compare
Local validationEngine: mysql 8 Β· Disable suspends the account and kills the live session Re-enable clears suspension, and deliberately does not resurrect the old The old token staying dead is the right behaviour β re-enabling an account should Both transitions are audited, with actor attribution: Lifecycle behaves as described, and the disable β re-enable β fresh-login round |
Covers PUT-1704 and PUT-1707: creating a workspace, admitting the master account, and the whole of offboarding. `createWorkspace` admits the creator with org_owned = 0, which is what makes the master pay for itself and stay an invalid target of every member route. `checkOwnerInvariant` asserts the rule no dialect can express -- the owner is a member with org_owned = 0 and the only such member -- and a test breaks it deliberately, since the schema cannot refuse a second one. Three authority checks: 404 to a stranger so the endpoint is not an existence oracle, 403 to a member who is not the master, and the master refused as a target of member routes. Handle problems surface as 400 (unusable) or 409 (taken), including the unique-index race. `TeamStore` throws a bare Error, which the server would turn into a 500 and a deduped critical alarm -- an uppercase handle should not page on-call. Disable writes `user.suspended` as well as suspended_at and suspended_reason. PUT-1707 named only the latter two, but those are siblings added by 0061 and 0063 -- `userProtected` rejects on `if (user.suspended)` and reads neither. Setting only the timestamp and reason would have recorded a disable that never took effect, and disable is the whole of offboarding here. Sessions are dropped through SessionStore.removeByUuid rather than a raw DELETE. The store invalidates every composite cache key with its double-delete pattern; without that a disabled member keeps authenticating from cache for the session TTL, which is exactly the "next request, not after a cache TTL" property disable is supposed to have. Revoking also preserves last_ip and last_user_agent, which the member-facing audit view reads. Files are untouched and re-enable restores the account. Adds team_not_found, not_the_master_account and not_an_org_account to the HttpError legacy codes, which the controller also needs. Billing events, invalidateActorSubscription, audit rows and the GUI push are deliberately not here -- they belong to phase 3 and PUT-1708.
aad4994 to
da2b42d
Compare
Covers PUT-1704 (create a workspace, admit the workspace owner) and PUT-1707 (disable and re-enable) together β both are
TeamServicemethods and neither is large enough to review alone.β A spec bug worth reading before the code
PUT-1707 said to set
user.suspended_at/suspended_reason(0061,0063). Those are real columns β but neither of them enforces anything:The enforced column is the older boolean
suspendedfrom0001.suspended_atandsuspended_reasonare its siblings;0061's own comment calls it "the timestamp sibling of the existing booleansuspendedflag".Implemented literally, disable would have written a perfect record of an offboarding that never happened β and since this design has no remove-from-workspace, no file transfer and no retention clock, disable is the entire offboarding control. All three columns are now written together, and the test is named for it:
sets the column the request gate actually reads. Ticket and design doc corrected.The invariant no dialect can express
createWorkspaceadmits the creator withorg_owned = 0. That single value is what makes the workspace owner pay for itself and stay an invalid target of every member route.checkOwnerInvariantasserts what the schema cannot: the owner is a member withorg_owned = 0, and is the only such member. There's a test that deliberately breaks it by admitting a secondorg_owned = 0account, because nothing in any dialect refuses that β which is the reason the check exists at all.Authority
Three checks, and between them the whole authorization model:
team_not_foundβ so the endpoint is not an existence oraclenot_the_workspace_ownernot_an_org_accountβorg_ownedis tested explicitly, never inferred from NULLAdds those three values to the
HttpErrorlegacy-code union, which is a closed type. PUT-1708's routes need the same three.What is deliberately not here
Stated rather than buried, since PUT-1707 lists them:
invalidateActorSubscriptionouter.gui.*push + member notificationVerification
The disable tests assert the observable consequences rather than the write: sessions are gone,
fsentriesare untouched, re-enable clears all three columns, the workspace owner cannot be disabled even by itself, a non-master's disable order is refused, and a member of another workspace cannot be reached.A detail that cost six red tests
HttpErrorexposesstatusCode, notstatus. My first assertions usedtoMatchObject({ status: 404 }), which fails against everyHttpErrorregardless of what it actually contains β so all six rejection tests failed while the seven behavioural ones passed. Worth knowing before writing the controller tests in PUT-1708, where nearly every assertion is a rejection.Closes PUT-1704 and PUT-1707.