Scope link-sharing settings to the window's team (P6) - #15440
Scope link-sharing settings to the window's team (P6)#15440warp-agent-staging[bot] wants to merge 5 commits into
Conversation
|
This PR was generated with Warp. Comment |
There was a problem hiding this comment.
Overview
P6 scopes the two link-sharing getters to the window's team and re-reads policy immediately before acting, closing a stale-modal path where a menu opened under one team applied its policy after a switch. The review found the migration correct and every check green; the fixes are going back to the author. Three things below are notes for you rather than changes.
Concerns
app/src/drive/sharing/dialog/mod.rs:1171-1195— a direct-sharing mutation on this surface is still ungated, and it is pre-existing. Promoting an inherited guest calls the sameupdate_manager.add_object_guests(...)thatsend_invitationsdoes, but neither the guest-list render norSetGuestAccessLevelconsultscan_direct_link_share. Untouched by this PR, so not a finding against it — but it meansdirect_link_sharing_enableddoes not fully close the channel on the surface this PR just hardened, and it is worth knowing before you read the guards as complete.app/src/workspaces/user_workspaces.rs:1943— the workspace-fallback guard here ishas_teams()(current workspace only), whereteams_allow_codebase_contextguards on the user having no teams in any workspace. A user on a team in workspace A and none in B therefore reads B'scurrent_workspace().settings, which isworkspaceTeamIDs[0]server-side when B has teams they are not in. I think the narrower guard is better-targeted here, since the settings object is per-workspace — but it is the same axis as the open question on #15443, and I have told the author to leave it rather than have two getters settle it inconsistently. One decision should cover both.- Summary-level: the client guards are the only enforcement verified here.
TeamScope's contract says this is scope rather than authority and that the server still authorizes, but server-side enforcement of link-sharing policy was not checked as part of this review. Worth confirming that assumption holds for this particular policy, since if it does not, these guards are load-bearing on their own.
Verdict
Checks: build pass, tests pass (6564/6567; the 3 failures reproduce identically at the base — unauthenticated nsc, missing bundled SVG assets, no real zsh), CI green, visual proof n/a
Found: 0 critical, 3 important, 1 suggestion, 2 nits — all with the author; the items above are yours.
641501a to
682377c
Compare
752cf23 to
d955a02
Compare
There was a problem hiding this comment.
Overview
An architecture review across the six open team-scope PRs found this one to be the cleanest of the set, with one open question that only you can settle because it is a disagreement between this PR and #15443.
Concerns
- This is the only PR of the six that distinguishes an unknown team from a teamless one, and #15443 would take that distinction away.
app/src/drive/sharing/dialog/mod.rs:502-520fails closed onteam_context's outerNone(window never registered, or registered to a departed team) and reads policy on the innerSome(TeamContext { team_uid: None }), with the doc line "A window that simply has no team is a different thing." #15443 makesteam_contexttotal, which erases the outerNoneand makes this fail-closed branch unreachable. - The justification for erasing it covers render paths but not this PR's two enforcement sites. #15443's doc reasons that "a caller on a render path has no better answer to give", which is clearly right for a display affordance. The
send_invitationsandSetLinkPermissionsaction guards here are enforcement, not render, and an unknown team silently reading as "no policy" on a sharing control is the fail-open this whole stack exists to remove — so the two PRs genuinely disagree and the answer is a product judgement, not a code one. - Being fixed already, listed so the review is complete: the teamless arm returns a hardcoded
LinkSharingSettings::UNRESTRICTEDrather than falling back tocurrent_workspace().settingsper your ruling on the thread. That is a permissive constant on a sharing control; the author is replacing it, which also deletes theUNRESTRICTEDconstant added toworkspace.rs.
Verdict
Checks: build pass, tests pass, CI green, visual proof n/a
Found: 0 critical, 1 important, 0 suggestions, 0 nits
The important finding is the enforcement-versus-render question above, which needs your decision rather than a change from the author. Nothing else in this PR was asked to change: at +35 net it is the smallest of the six, both getters are replaced in place with no coexistence left behind, and it adds no team resolver.
Replace the ambient `is_anyone_with_link_sharing_enabled` / `is_direct_link_sharing_enabled` reads with getters that take a `TeamScope`, resolving each window's own team policy instead of the current workspace's arbitrarily-chosen `teams[0]` effective settings. The sharing dialog resolves a `TeamContext` from its own view handle, freshly on every read, and re-checks the policy before sending invitations and before applying link permissions, so a dialog left open while its window moves to a restricting team cannot act on the policy it opened with.
…ange - `SetLinkPermissions` only guards grants. `None` revokes link access, and blocking it left a user unable to tighten an over-shared object in exactly the race the guard exists for. - Cover the guards themselves. `UpdateManager` marks an object as having a pending permissions change synchronously, before it issues any request, so the new tests can assert whether a dispatch reached the target. - Subscribe to `UserWorkspaces`: nothing else re-rendered the dialog on a team change, so the invite form and link row stayed on screen under the old team's policy while the guards refused to act on them. - Rename the unresolvable-window test, which named the teamless case but tests a window `UserWorkspaces` never saw, and correct the two docs that said an unresolved team denies when only an unresolvable window does.
…tests A dialog targeting a Warp Drive object reads its access level and renders its owner, which `initialize_app_for_terminal_view` does not provide models for; the other dialog tests target a session or nothing and so never hit those paths.
Isaiah's ruling for this stack is that falling back to workspace settings when the scope has no team is the intended behaviour. A hardcoded `UNRESTRICTED` was not that: on a sharing control it was potentially more permissive than the workspace's own policy. Drop the `has_teams()` guard so a teamless scope reads `current_workspace().settings` directly, and delete the constant, returning `workspace.rs` to master. The no-workspace arm stays permissive, preserving the `unwrap_or(true)` these getters had before they took a scope; there is no workspace policy to read there, and denying would newly hide the share UI while workspaces load.
d955a02 to
27d0fca
Compare


Description
P6 of the stack that migrates client settings getters onto per-window team scope (tech spec: #15347, tracking: REV-2205). Rebased onto
masternow that P0 (#15439) has landed; this diff is only P6.Workspace-settings fallback retired:
WorkspaceSettings::link_sharing_settingsfor both link-sharing reads.UserWorkspaces::is_anyone_with_link_sharing_enabledandis_direct_link_sharing_enabledno longer readcurrent_workspace().settingsfor a user who is on a team; they take aTeamScopeand read that scope's team's effectiveTeamSettings::link_sharing. A scope with no team falls back tocurrent_workspace().settings, which is the intended behaviour for this stack; what is gone is any path where a window on a team reads another team's effective settings.Per the contract in
TeamScope's doc:&impl TeamScope), neverOption<&dyn TeamScope>.team_uid()isNoneis not on a team, and reads the workspace's own settings rather than any team's. With no workspace either, both channels are permitted — preserving theunwrap_or(true)these getters had before they took a scope, since there is no policy to read and denying would newly hide the share UI while workspaces load.UserWorkspacescan turn a scope into a&Team: the lookup is a privateteam_from_scope, and callers only ever get the setting.app/src/workspaces/workspace.rsis byte-identical tomaster— this PR adds nothing to it.Stale-policy fix on the share surfaces
The shape found on #15352 (a modal resolving a policy at open, then acting on the stale value) was present here, so this PR resolves late rather than pinning, following the
TeamContextdefault:SharingDialogholds its ownWeakViewHandleand mints aTeamContextfrom it on every read, so the gates reflect the window's team as of that frame.send_invitationsbails when the window's team no longer permits direct link sharing.SetLinkPermissionshandler bails when the window's team no longer permits anyone-with-link sharing. Its menu items are built once inreset_link_sharing_menuwhen the menu opens, which was the stale surface. Only granting is guarded —access_level == Nonerevokes link access, and a user tightening an over-shared object must not be turned away by the very policy that wants it tightened.SharingDialognow subscribes toUserWorkspaces. Nothing else re-rendered it on a team change, so the invite form and link-sharing row would have stayed on screen under the old team's policy while the guards refused to act on them — turning Invite into a dead control rather than removing it.UserWorkspacesnever saw; every real window registers inRootView::new. A window that merely has no team is a different case and is permitted.is_invite_link_enabledis untouched. It is declared immediately below the two I changed, on the sameWorkspaceSettings, and was tempting, but it is genuinely workspace-only and stays that way, as doesis_discoverable.Behaviour: no change for a single-team workspace, which is every workspace today — the team's effective link-sharing settings are the same values the workspace-level read returned. One case is strictly better than before: on cold start, cached workspaces restore real team settings from sqlite while the workspace's settings come back as
Default, so a cold-start window on a cached team now reads that team's real policy where the old code read all-falsedummy data.Linked Issue
REV-2205
ready-to-specorready-to-implement.Testing
Unit tests, in the repo's
${filename}_tests.rsconvention.app/src/workspaces/user_workspaces_tests.rs— the getters:test_link_sharing_resolves_each_windows_own_team— two windows on different teams, one permitting and one forbidding, with a workspace-level policy that disagrees with both so a fallback read would be visible.test_link_sharing_for_a_teamless_window_reads_the_workspace_not_another_team— a registered window with no team, where the workspace's policy is the mirror image of the team's, so reading the wrong source is visible in both channels.test_link_sharing_falls_back_to_the_workspace_for_a_user_on_no_team— a workspace with no teams still governs through its own settings.app/src/drive/sharing/dialog/mod_tests.rs— the consumers. The twoset_link_permissions_*tests are the ones that pin the bug fix: they assert whether the mutation reached the target, reading the pending-permissions-change flagUpdateManagersets synchronously before it issues any request. Deleting the guard fails them.set_link_permissions_refuses_to_grant_under_a_forbidding_team— a grant does not reach the object, and a revocation still does.set_link_permissions_grants_under_a_permitting_team— the guard is not blanket-blocking.link_sharing_gates_resolve_each_windows_own_team— two dialogs in two windows on different teams give opposite answers.link_sharing_gates_follow_a_window_onto_its_new_team— a dialog opened under a permitting team loses both gates once its window reconciles onto a forbidding team.link_sharing_gates_deny_when_the_dialogs_window_cannot_be_resolved— pins the deny-on-unresolvable-scope choice, as distinct from the teamless-window case above it.These eight tests are confirmed to have executed, not merely to have been present in a green run — a green tick alone says the suite passed, not that new tests ran. From the
Run Linux testsjob log at27d0fca6:Summary [ 56.957s] 11073 tests run: 11073 passed, 34 skipped, with each of the eight appearing as an individualPASSline by name and zeroFAILlines in the job. Re-checked after the rebase onto #15441, since a conflict resolution can silently drop a test hunk../script/runNot manually tested: this agent's sandbox has ~3 GB of RAM and
rustcis OOM-killed part-way through thewarpcrate, socargo check,cargo clippyand the test run could not be completed locally.cargo fmtwas run; CI is doing the rest.Known adjacent gaps, deliberately not fixed here. Promoting an inherited guest (
mod.rs) calls the sameadd_object_guestswithout consultingcan_direct_link_share, so the channel this PR hardens is not fully closed — pre-existing, and out of this slice.SetGuestAccessLevelandRemoveGuestact on a guest index captured when the menu opened, which can target the wrong guest if the list changes underneath; same family of bug, different axis, also pre-existing.Agent Mode
CHANGELOG-NONE