mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 14:00:40 +00:00
5749671dd9
* fix(oauth): include_future_workspaces is no longer a control; limit to current is an action (BUG-3338)
The wildcard (all_current_workspaces) is live and covers workspaces
joined later, and nothing ever auto-added on include_future_workspaces,
so the Connected Apps checkbox promised something false either way:
unticked under the wildcard it still granted workspaces joined later,
and ticked on a specific list it added nothing.
- Connected Apps drops the checkbox and labels the wildcard "All my
workspaces, including ones I join later".
- PATCH /connected-apps/{id}/flags ignores include_future_workspaces
(an older page may still send it); the stored value follows
all_current_workspaces, as the consent screen always wrote it.
- POST /connected-apps/{id}/limit-to-current copies the user's live
memberships into the list and turns the wildcard off in one
transaction: the explicit form of "only my current workspaces".
- The consent screen is unchanged.
Rows already stored with the wildcard on and include_future off are
narrowed by a separate migration, held for a production count.
Claude-Session: https://claude.ai/code/session_01H63zaWhkb5pAafx2WGjrVC
* fix(oauth): limit acts only on a wildcard, keeps guest workspaces, and the empty-list rule holds under the connection lock (BUG-3338)
Codex review round 1:
- A repeated limit (a stale tab) on an already specific list added the
workspaces joined since; it now changes nothing unless the wildcard
is on.
- The snapshot left out workspaces the user reaches only as a guest
through a collection or item grant, which the wildcard reaches; it
now includes them.
- Removing a workspace and turning the wildcard off checked the
empty-list rule outside any lock, so a concurrent limit or flags
change could leave a specific list empty. Both now check and write
in one transaction holding the connection row's lock.
- Turning the wildcard off and limiting now re-check live connections,
as a removal does (TASK-3365).
Claude-Session: https://claude.ai/code/session_01H63zaWhkb5pAafx2WGjrVC
* fix(oauth): the limit snapshot counts only live guest grants (BUG-3338)
Codex review round 2: a grant on a soft-deleted item or collection
confers no access, but put its workspace in the snapshot, where it
counted toward the not-empty rule and would come back to life if the
content were restored. The grant subqueries now use the live-grant
filter UserHasGrantsInWorkspace uses.
Claude-Session: https://claude.ai/code/session_01H63zaWhkb5pAafx2WGjrVC
256 lines
10 KiB
Go
256 lines
10 KiB
Go
package server
|
|
|
|
import (
|
|
"encoding/json"
|
|
"errors"
|
|
"net/http"
|
|
"reflect"
|
|
"sort"
|
|
"testing"
|
|
|
|
"github.com/PerpetualSoftware/pad/internal/store"
|
|
)
|
|
|
|
// BUG-3338: include_future_workspaces never narrowed anything. The
|
|
// wildcard (all_current_workspaces) is live and covers workspaces joined
|
|
// later, and nothing auto-added on the flag, so the Connected Apps
|
|
// checkbox promised something false either way. It is no longer a
|
|
// control, and "only my current workspaces" is an explicit action that
|
|
// copies the user's memberships into the list.
|
|
|
|
func storedIncludeFuture(t *testing.T, srv *Server, requestID string) bool {
|
|
t.Helper()
|
|
c, err := srv.store.GetOAuthConnection(requestID)
|
|
if err != nil || c == nil {
|
|
t.Fatalf("GetOAuthConnection(%s): %v", requestID, err)
|
|
}
|
|
return c.IncludeFutureWorkspaces
|
|
}
|
|
|
|
func TestBUG3338_FlagsPatchIgnoresIncludeFuture(t *testing.T) {
|
|
srv, _ := connectedAppsTestServer(t)
|
|
_, tok, _ := seedConnectedAppForMutations(t, srv, "bug3338-flags")
|
|
|
|
cases := []struct {
|
|
name string
|
|
allCurrent, sentFuture bool
|
|
}{
|
|
// The combination the checkbox used to store: wildcard on,
|
|
// "future" off. It now follows the wildcard.
|
|
{"wildcard on, future sent off", true, false},
|
|
{"wildcard off, future sent on", false, true},
|
|
}
|
|
for _, tc := range cases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
rr := doAuthedJSON(srv, "PATCH", "/api/v1/connected-apps/bug3338-flags/flags",
|
|
map[string]bool{
|
|
"may_create_workspaces": true,
|
|
"all_current_workspaces": tc.allCurrent,
|
|
"include_future_workspaces": tc.sentFuture,
|
|
}, tok)
|
|
if rr.Code != http.StatusOK {
|
|
t.Fatalf("status = %d (body=%s)", rr.Code, rr.Body.String())
|
|
}
|
|
if got := storedIncludeFuture(t, srv, "bug3338-flags"); got != tc.allCurrent {
|
|
t.Errorf("stored include_future = %v, want it to follow all_current (%v)", got, tc.allCurrent)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestBUG3338_LimitToCurrentSnapshotsMemberships(t *testing.T) {
|
|
srv, _ := connectedAppsTestServer(t)
|
|
user, tok := loginTestUser(t, srv)
|
|
if err := srv.store.CreateOAuthConnection(store.OAuthConnection{
|
|
RequestID: "bug3338-limit",
|
|
UserID: user.ID,
|
|
MayCreateWorkspaces: true,
|
|
AllCurrentWorkspaces: true,
|
|
IncludeFutureWorkspaces: false,
|
|
}); err != nil {
|
|
t.Fatalf("CreateOAuthConnection: %v", err)
|
|
}
|
|
_, slugA := mustSeedWorkspaceForMutation(t, srv, user.ID, "bug3338-a", "owner")
|
|
_, slugB := mustSeedWorkspaceForMutation(t, srv, user.ID, "bug3338-b", "editor")
|
|
_, gone := mustSeedWorkspaceForMutation(t, srv, user.ID, "bug3338-gone", "owner")
|
|
if err := srv.store.DeleteWorkspace(gone); err != nil {
|
|
t.Fatalf("DeleteWorkspace: %v", err)
|
|
}
|
|
|
|
// Before: the wildcard grants a workspace joined later, whatever the
|
|
// stored "future" flag says. That is the defect the checkbox hid.
|
|
if access, err := srv.store.GetOAuthConnectionAccess("bug3338-limit"); err != nil || !access.AllCurrentWorkspaces {
|
|
t.Fatalf("control: wildcard connection: %+v %v", access, err)
|
|
}
|
|
|
|
rr := doAuthedJSON(srv, "POST", "/api/v1/connected-apps/bug3338-limit/limit-to-current", nil, tok)
|
|
if rr.Code != http.StatusOK {
|
|
t.Fatalf("limit-to-current: %d %s", rr.Code, rr.Body.String())
|
|
}
|
|
var dto connectedAppDTO
|
|
_ = json.Unmarshal(rr.Body.Bytes(), &dto)
|
|
if dto.AllCurrentWorkspaces {
|
|
t.Error("response still reads all_current_workspaces=true")
|
|
}
|
|
|
|
want := []string{slugA, slugB}
|
|
sort.Strings(want)
|
|
access, err := srv.store.GetOAuthConnectionAccess("bug3338-limit")
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if access.AllCurrentWorkspaces || !reflect.DeepEqual(access.WorkspaceSlugs, want) {
|
|
t.Fatalf("after limit: %+v, want specific %v (the soft-deleted workspace left out)", access, want)
|
|
}
|
|
if storedIncludeFuture(t, srv, "bug3338-limit") {
|
|
t.Error("include_future left on after limiting")
|
|
}
|
|
|
|
// A workspace joined afterwards is not covered.
|
|
mustSeedWorkspaceForMutation(t, srv, user.ID, "bug3338-later", "owner")
|
|
access, err = srv.store.GetOAuthConnectionAccess("bug3338-limit")
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if !reflect.DeepEqual(access.WorkspaceSlugs, want) {
|
|
t.Errorf("a workspace joined after the limit is covered: %v", access.WorkspaceSlugs)
|
|
}
|
|
|
|
// A second press (a stale tab) on the now-specific list must not add
|
|
// the workspace joined since the first one.
|
|
if rr := doAuthedJSON(srv, "POST", "/api/v1/connected-apps/bug3338-limit/limit-to-current", nil, tok); rr.Code != http.StatusOK {
|
|
t.Fatalf("second limit: %d %s", rr.Code, rr.Body.String())
|
|
}
|
|
access, err = srv.store.GetOAuthConnectionAccess("bug3338-limit")
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if access.AllCurrentWorkspaces || !reflect.DeepEqual(access.WorkspaceSlugs, want) {
|
|
t.Errorf("a repeated limit widened the list: %+v, want %v", access, want)
|
|
}
|
|
}
|
|
|
|
// The snapshot is what the wildcard reaches today, which includes a
|
|
// workspace the user reaches only as a guest through a grant.
|
|
func TestBUG3338_LimitToCurrentKeepsGuestWorkspaces(t *testing.T) {
|
|
srv, _ := connectedAppsTestServer(t)
|
|
user, tok := loginTestUser(t, srv)
|
|
owner, _ := loginTestUserAs(t, srv, "owner3338@example.test", "Owner", "pw-3338-abcdefgh")
|
|
if err := srv.store.CreateOAuthConnection(store.OAuthConnection{
|
|
RequestID: "bug3338-guest",
|
|
UserID: user.ID,
|
|
AllCurrentWorkspaces: true,
|
|
}); err != nil {
|
|
t.Fatalf("CreateOAuthConnection: %v", err)
|
|
}
|
|
_, mine := mustSeedWorkspaceForMutation(t, srv, user.ID, "bug3338-mine", "owner")
|
|
itemWSID, itemWS := mustSeedWorkspaceForMutation(t, srv, owner.ID, "bug3338-item-guest", "owner")
|
|
collWSID, collWS := mustSeedWorkspaceForMutation(t, srv, owner.ID, "bug3338-coll-guest", "owner")
|
|
it := mustItem(t, srv, itemWSID, mustCollection(t, srv, itemWSID, "Tasks").ID, "shared")
|
|
if _, err := srv.store.CreateItemGrant(itemWSID, it.ID, user.ID, "view", owner.ID); err != nil {
|
|
t.Fatalf("CreateItemGrant: %v", err)
|
|
}
|
|
if _, err := srv.store.CreateCollectionGrant(collWSID, mustCollection(t, srv, collWSID, "Docs").ID, user.ID, "view", owner.ID); err != nil {
|
|
t.Fatalf("CreateCollectionGrant: %v", err)
|
|
}
|
|
mustSeedWorkspaceForMutation(t, srv, owner.ID, "bug3338-unrelated", "owner")
|
|
// A grant on deleted content confers nothing, so its workspace stays out.
|
|
deadWSID, _ := mustSeedWorkspaceForMutation(t, srv, owner.ID, "bug3338-dead-grant", "owner")
|
|
deadColl := mustCollection(t, srv, deadWSID, "Gone")
|
|
deadItem := mustItem(t, srv, deadWSID, mustCollection(t, srv, deadWSID, "Tasks").ID, "deleted")
|
|
if _, err := srv.store.CreateItemGrant(deadWSID, deadItem.ID, user.ID, "view", owner.ID); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if _, err := srv.store.CreateCollectionGrant(deadWSID, deadColl.ID, user.ID, "view", owner.ID); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if err := srv.store.DeleteItem(deadItem.ID); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if err := srv.store.DeleteCollection(deadColl.ID, ""); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
|
|
if rr := doAuthedJSON(srv, "POST", "/api/v1/connected-apps/bug3338-guest/limit-to-current", nil, tok); rr.Code != http.StatusOK {
|
|
t.Fatalf("limit-to-current: %d %s", rr.Code, rr.Body.String())
|
|
}
|
|
access, err := srv.store.GetOAuthConnectionAccess("bug3338-guest")
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
want := []string{mine, itemWS, collWS}
|
|
sort.Strings(want)
|
|
if !reflect.DeepEqual(access.WorkspaceSlugs, want) {
|
|
t.Errorf("limited to %v, want %v (member + both live guest workspaces; not the unrelated one, nor one reached only through grants on deleted content)", access.WorkspaceSlugs, want)
|
|
}
|
|
}
|
|
|
|
// The empty-list rule on removal and on turning the wildcard off is
|
|
// enforced by the store under the connection's lock.
|
|
func TestBUG3338_GuardedWritesRefuseAnEmptyList(t *testing.T) {
|
|
srv, _ := connectedAppsTestServer(t)
|
|
user, _ := loginTestUser(t, srv)
|
|
wsID, _ := mustSeedWorkspaceForMutation(t, srv, user.ID, "bug3338-only", "owner")
|
|
if err := srv.store.CreateOAuthConnection(store.OAuthConnection{RequestID: "bug3338-guard", UserID: user.ID}); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if err := srv.store.AddConnectionWorkspace("bug3338-guard", wsID, store.AddedByUser); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if err := srv.store.RemoveConnectionWorkspaceUnlessLast("bug3338-guard", wsID); !errors.Is(err, store.ErrLastConnectionWorkspace) {
|
|
t.Fatalf("removing the last workspace: err = %v", err)
|
|
}
|
|
if n, _ := srv.store.ConnectionWorkspaceCount("bug3338-guard"); n != 1 {
|
|
t.Errorf("refused removal deleted the row: count %d", n)
|
|
}
|
|
// Under the wildcard the last row may go, and then the wildcard may
|
|
// not be turned off.
|
|
if err := srv.store.SetScopeFlagsGuarded("bug3338-guard", true, true, true); err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
if err := srv.store.RemoveConnectionWorkspaceUnlessLast("bug3338-guard", wsID); err != nil {
|
|
t.Fatalf("removal under the wildcard: %v", err)
|
|
}
|
|
if err := srv.store.SetScopeFlagsGuarded("bug3338-guard", true, false, false); !errors.Is(err, store.ErrLastConnectionWorkspace) {
|
|
t.Fatalf("turning the wildcard off with an empty list: err = %v", err)
|
|
}
|
|
if a, _ := srv.store.GetOAuthConnectionAccess("bug3338-guard"); !a.AllCurrentWorkspaces {
|
|
t.Error("refused flag change still turned the wildcard off")
|
|
}
|
|
}
|
|
|
|
func TestBUG3338_LimitToCurrentRefusals(t *testing.T) {
|
|
srv, _ := connectedAppsTestServer(t)
|
|
user, tok := loginTestUser(t, srv)
|
|
if err := srv.store.CreateOAuthConnection(store.OAuthConnection{
|
|
RequestID: "bug3338-empty",
|
|
UserID: user.ID,
|
|
AllCurrentWorkspaces: true,
|
|
}); err != nil {
|
|
t.Fatalf("CreateOAuthConnection: %v", err)
|
|
}
|
|
|
|
t.Run("no memberships: refused, still a wildcard", func(t *testing.T) {
|
|
rr := doAuthedJSON(srv, "POST", "/api/v1/connected-apps/bug3338-empty/limit-to-current", nil, tok)
|
|
if rr.Code != http.StatusBadRequest {
|
|
t.Fatalf("status = %d, want 400 (body=%s)", rr.Code, rr.Body.String())
|
|
}
|
|
access, err := srv.store.GetOAuthConnectionAccess("bug3338-empty")
|
|
if err != nil || !access.AllCurrentWorkspaces {
|
|
t.Errorf("a refused limit changed the connection: %+v %v", access, err)
|
|
}
|
|
})
|
|
|
|
t.Run("another user's connection: 404, untouched", func(t *testing.T) {
|
|
_, otherTok := loginTestUserAs(t, srv, "other3338@example.test", "Other", "pw-3338-abcdefgh")
|
|
rr := doAuthedJSON(srv, "POST", "/api/v1/connected-apps/bug3338-empty/limit-to-current", nil, otherTok)
|
|
if rr.Code != http.StatusNotFound {
|
|
t.Fatalf("status = %d, want 404 (body=%s)", rr.Code, rr.Body.String())
|
|
}
|
|
access, err := srv.store.GetOAuthConnectionAccess("bug3338-empty")
|
|
if err != nil || !access.AllCurrentWorkspaces {
|
|
t.Errorf("another user's call changed the connection: %+v %v", access, err)
|
|
}
|
|
})
|
|
}
|