From de95ba3590f1bc8efcfc64e9deeaeec49dd31b8a Mon Sep 17 00:00:00 2001 From: Jonathon Leight Date: Tue, 7 Jul 2026 20:33:21 -0400 Subject: [PATCH] Switch permissions back to per-repeater --- internal/core/endpoints_org_test.go | 13 +- internal/core/ignored_errors_test.go | 25 ---- internal/core/org_participation.go | 125 ------------------ internal/core/templates/org.html | 1 - internal/core/templates/org_commands.html | 93 ------------- internal/core/web.go | 2 - internal/e2e/limit_commands_test.go | 103 --------------- internal/e2e/org_actions_test.go | 56 -------- .../0037_repeater_org_command_optin.sql | 47 +++++++ internal/store/org_command_optin.go | 43 ------ internal/store/org_repeater_command_optin.go | 45 +++++++ internal/store/orgs_resolution_test.go | 91 ++++++++++++- internal/store/share_commands.go | 12 +- 13 files changed, 188 insertions(+), 468 deletions(-) delete mode 100644 internal/core/templates/org_commands.html delete mode 100644 internal/e2e/limit_commands_test.go delete mode 100644 internal/e2e/org_actions_test.go create mode 100644 internal/store/migrations/0037_repeater_org_command_optin.sql delete mode 100644 internal/store/org_command_optin.go create mode 100644 internal/store/org_repeater_command_optin.go diff --git a/internal/core/endpoints_org_test.go b/internal/core/endpoints_org_test.go index d4bcac7..a171146 100644 --- a/internal/core/endpoints_org_test.go +++ b/internal/core/endpoints_org_test.go @@ -9,10 +9,10 @@ import ( ) // Black-box coverage for the org-management POST endpoints (create/edit/links/ -// members/my-commands and join/leave). Each asserts the 303 redirect target and a -// cheap store side-effect; none render anything. +// members and join/leave). Each asserts the 303 redirect target and a cheap store +// side-effect; none render anything. -// #43 create, #44 edit, #104 links, #48 member role, #50 my-commands. +// #43 create, #44 edit, #104 links, #48 member role. func TestOrgManagementPosts(t *testing.T) { t.Parallel() st, ctx, ts, h := splitServer(t) @@ -65,13 +65,6 @@ func TestOrgManagementPosts(t *testing.T) { if admin, _ := st.IsOrgAdmin(ctx, orgID, other.ID); !admin { t.Fatal("promote did not make the member an admin") } - - // #50 my-commands — "clear" removes any restriction, redirecting to the editor. - cmds := post(t, ts, h.app, "/orgs/"+slug+"/my-commands", url.Values{"clear": {"1"}}, sess) - cmds.Body.Close() - if loc, _ := url.Parse(cmds.Header.Get("Location")); cmds.StatusCode != http.StatusSeeOther || loc.Path != "/orgs/"+slug+"/my-commands" { - t.Fatalf("save my-commands = %d %q, want 303 → my-commands", cmds.StatusCode, cmds.Header.Get("Location")) - } } // #56 update config profile, #57 delete config profile. (Create #54 and regions diff --git a/internal/core/ignored_errors_test.go b/internal/core/ignored_errors_test.go index 0a0331a..ca04ac2 100644 --- a/internal/core/ignored_errors_test.go +++ b/internal/core/ignored_errors_test.go @@ -42,28 +42,3 @@ func TestShareCommandsPageReadErrorFailsClosed(t *testing.T) { t.Fatalf("status = %d, want 500 (a swallowed error would render 200 with an empty, data-wiping form)", resp.StatusCode) } } - -// TestOrgCommandsPageReadErrorFailsClosed: if loading a member's per-org opt-in -// list fails, the page must 500 rather than render as "permissive" (all checked), -// which a Save would persist as clearing the member's real restriction. -func TestOrgCommandsPageReadErrorFailsClosed(t *testing.T) { - t.Parallel() - st, ctx, ts, h := splitServer(t) - member, sess := appLogin(t, ts, st, ctx, h.app, "orgmember") - org, err := st.CreateOrg(ctx, "Org", member.ID) // creator is an admin member - if err != nil { - t.Fatal(err) - } - - // Make OrgOptInCommandIDs (SELECT … FROM org_command_optin) fail; the handler's - // earlier queries (org, role, catalog ceiling) use other tables. - if _, err := st.Pool().Exec(ctx, `DROP TABLE org_command_optin`); err != nil { - t.Fatalf("drop org_command_optin: %v", err) - } - - resp := do(t, ts, h.app, "/orgs/"+org.Slug+"/my-commands", sess) - resp.Body.Close() - if resp.StatusCode != http.StatusInternalServerError { - t.Fatalf("status = %d, want 500 (a swallowed error would render 200 as permissive, wiping the restriction on save)", resp.StatusCode) - } -} diff --git a/internal/core/org_participation.go b/internal/core/org_participation.go index 0ab4654..a6e29b2 100644 --- a/internal/core/org_participation.go +++ b/internal/core/org_participation.go @@ -2,7 +2,6 @@ package core import ( "net/http" - "strconv" "github.com/go-chi/chi/v5" @@ -47,127 +46,3 @@ func (s *Handlers) handleSetRepeaterOrg(w http.ResponseWriter, r *http.Request) } http.Redirect(w, r, sharePath(rep.PublicID), http.StatusSeeOther) } - -// pageOrgCommands lets a member restrict, for one org, which of the commands that -// org is permitted to run actually run on the member's repeaters. No restriction -// (the default) means every command in the org's ceiling can run. -func (s *Handlers) pageOrgCommands(w http.ResponseWriter, r *http.Request) { - uid := s.Auth.CurrentUserID(r.Context()) - id, ok := s.orgID(r) - if !ok { - s.NotFound(w, r) - return - } - org, err := s.Store.GetOrg(r.Context(), id) - if err != nil { - s.NotFound(w, r) - return - } - role, isMember, err := s.Store.OrgRole(r.Context(), id, uid) - if err != nil || !isMember { - s.NotFound(w, r) - return - } - ceiling, err := s.orgCeilingCommands(r) - if err != nil { - s.ServerError(w, r, "could not load commands", err) - return - } - // Must not swallow this error: an empty list reads as "permissive" (everything - // checked), and saving that would clear the member's real restriction. - optIn, err := s.Store.OrgOptInCommandIDs(r.Context(), id, uid) - if err != nil { - s.ServerError(w, r, "could not load commands", err) - return - } - restricted := len(optIn) > 0 - // Permissive (no list) shows everything checked: all ceiling commands may run. - checked := make(map[int64]bool, len(ceiling)) - if restricted { - for _, cid := range optIn { - checked[cid] = true - } - } else { - for _, c := range ceiling { - checked[c.ID] = true - } - } - s.Render(w, r, "org_commands.html", map[string]any{ - "Org": org, - "Nav": s.OrgNavFor(r.Context(), org.ID, org.Slug, "", true, role == "admin"), - "Groups": groupCommands(ceiling, checked), - "Restricted": restricted, - }) -} - -// handleSaveOrgCommands saves the member's per-org opt-in list. If every ceiling -// command is selected (or none of the modes restrict), the list is cleared back to -// permissive so we don't persist a redundant full allowlist. -func (s *Handlers) handleSaveOrgCommands(w http.ResponseWriter, r *http.Request) { - uid := s.Auth.CurrentUserID(r.Context()) - id, ok := s.orgID(r) - if !ok { - s.NotFound(w, r) - return - } - if _, isMember, err := s.Store.OrgRole(r.Context(), id, uid); err != nil || !isMember { - s.NotFound(w, r) - return - } - if err := r.ParseForm(); err != nil { - http.Error(w, "bad form", http.StatusBadRequest) - return - } - // "Remove restriction" clears the list regardless of checkboxes. - if r.FormValue("clear") != "" { - if err := s.Store.SetOrgOptIn(r.Context(), id, uid, nil); err != nil { - s.ServerError(w, r, "could not save", err) - return - } - http.Redirect(w, r, "/orgs/"+orgParam(r)+"/my-commands", http.StatusSeeOther) //nolint:gosec // G710: local path or config-pinned origin - return - } - ceiling, err := s.orgCeilingCommands(r) - if err != nil { - s.ServerError(w, r, "could not load commands", err) - return - } - chosen := parseCommandIDs(r.Form["cmd"]) - // Selecting the full ceiling is equivalent to permissive — store nothing. - if len(chosen) >= len(ceiling) { - chosen = nil - } - if err := s.Store.SetOrgOptIn(r.Context(), id, uid, chosen); err != nil { - s.ServerError(w, r, "could not save", err) - return - } - http.Redirect(w, r, "/orgs/"+orgParam(r)+"/my-commands", http.StatusSeeOther) //nolint:gosec // G710: local path or config-pinned origin -} - -// orgCeilingCommands returns the catalog commands an org is ever permitted to run -// (member or admin tier) — the universe the opt-in editor restricts within. -func (s *Handlers) orgCeilingCommands(r *http.Request) ([]*store.Command, error) { - catalog, err := s.Store.ListCommands(r.Context()) - if err != nil { - return nil, err - } - var out []*store.Command - for _, c := range catalog { - if c.OrgMemberAllowed || c.OrgAdminAllowed { - out = append(out, c) - } - } - return out, nil -} - -// parseCommandIDs parses a slice of form values into catalog command ids, skipping -// any that aren't valid integers. -func parseCommandIDs(values []string) []int64 { - var ids []int64 - for _, v := range values { - if cid, err := strconv.ParseInt(v, 10, 64); err == nil { - ids = append(ids, cid) - } - } - return ids -} diff --git a/internal/core/templates/org.html b/internal/core/templates/org.html index 6dffa24..a733878 100644 --- a/internal/core/templates/org.html +++ b/internal/core/templates/org.html @@ -10,7 +10,6 @@
{{template "icon-world" "dropdown-item-icon"}}View public page - {{template "icon-list" "dropdown-item-icon"}}Limit commands
diff --git a/internal/core/templates/org_commands.html b/internal/core/templates/org_commands.html deleted file mode 100644 index 2a148be..0000000 --- a/internal/core/templates/org_commands.html +++ /dev/null @@ -1,93 +0,0 @@ -{{define "title"}}Limit commands · {{.Org.Name}} · MeshTender{{end}} -{{define "header"}} -
-
-
{{.Org.Name}}
-

Limit commands

-
-
-{{end}} -{{define "content"}} -{{template "org-tabs" .Nav}} - -
-
-

- Restrict which of the commands {{.Org.Name}} is permitted to run actually run on - your repeaters shared with it. This applies to every repeater you share with this organization. - The Access column shows whether a command is available to all members - (Members) or to organization admins only - (Admins). - {{if .Restricted}} - You're currently allowing only the checked commands. - {{else}} - No restriction is set — every command the organization is permitted to run can run. Uncheck commands to - restrict them. - {{end}} -

-
-
- - - {{range .Groups}} -
-
-

{{.Name}}

-
- - -
-
-
- - - - - - - - - - - {{range .Commands}} - - - - - - - {{end}} - -
CommandDescriptionAccess
- {{.Template}} - {{if .Args}}{{.Args}}{{end}} - {{if .Risky}}risky{{end}} - {{.Description}} - {{if .MemberAllowed}}Members{{else}}Admins{{end}} -
-
-
- {{end}} -
- - {{if .Restricted}} - - {{end}} - Cancel -
-
-{{end}} diff --git a/internal/core/web.go b/internal/core/web.go index 46249ed..9c206f7 100644 --- a/internal/core/web.go +++ b/internal/core/web.go @@ -217,8 +217,6 @@ func (s *Handlers) appRouter() chi.Router { r.Post("/orgs/{id}/join", s.handleJoinOrg) r.Post("/orgs/{id}/leave", s.handleLeaveOrg) r.Post("/orgs/{id}/members/{userID}", s.handleSetOrgMember) - r.Get("/orgs/{id}/my-commands", s.pageOrgCommands) - r.Post("/orgs/{id}/my-commands", s.handleSaveOrgCommands) r.Get("/orgs/{id}/config", s.pageOrgConfig) r.Get("/orgs/{id}/config/edit", s.pageConfigHub) r.Get("/orgs/{id}/config/profiles/new", s.pageProfileEdit) diff --git a/internal/e2e/limit_commands_test.go b/internal/e2e/limit_commands_test.go deleted file mode 100644 index 3935285..0000000 --- a/internal/e2e/limit_commands_test.go +++ /dev/null @@ -1,103 +0,0 @@ -//go:build browser - -package e2e - -import ( - "testing" - - cdplog "github.com/chromedp/cdproto/log" - "github.com/chromedp/cdproto/network" - "github.com/chromedp/chromedp" -) - -// TestE2ELimitCommandsSelectAll exercises the per-section select-all / select-none -// controls on the "Limit commands" page. It asserts the toggles are scoped to -// their own card: unchecking one section's boxes must leave other sections -// untouched, and re-checking restores them. Also asserts the page runs clean -// under the strict CSP (the toggle is CSP-safe delegated JS, no inline handlers). -func TestE2ELimitCommandsSelectAll(t *testing.T) { - srv := newE2EServer(t) - user, cookie := srv.login(t, "e2euser") - - // The page needs an org the user belongs to; CreateOrg makes the creator an - // admin member. Command groups come from the migrated catalog ceiling. - org, err := srv.store.CreateOrg(srv.ctx, "Toggle Org", user.ID) - if err != nil { - t.Fatalf("create org: %v", err) - } - - bctx, cancel, watch := startBrowser(t) - defer cancel() - - url := srv.appURL + "/orgs/" + org.Slug + "/my-commands" - - // Count of checkboxes in each section, and how many are checked. Sections are - // [data-check-scope] cards; each has its own Select all / Select none buttons. - const countChecked = `(function () { - var scopes = document.querySelectorAll('[data-check-scope]'); - return Array.prototype.map.call(scopes, function (s) { - var boxes = s.querySelectorAll('input[type=checkbox]'); - var checked = 0; - Array.prototype.forEach.call(boxes, function (b) { if (b.checked) checked++; }); - return [boxes.length, checked]; - }); - })()` - - // Every command row carries an Access badge (Members or Admins), the tier - // column the table was added for. Count how many the table renders. - const countAccessBadges = `document.querySelectorAll('[data-check-scope] tbody tr td .badge.bg-azure-lt, [data-check-scope] tbody tr td .badge.bg-success-lt').length` - - var initial, afterNone, afterAll [][]int - var accessBadges int - if err := chromedp.Run(bctx, - network.Enable(), - cdplog.Enable(), - setSessionCookie(cookie), - chromedp.Navigate(url), - chromedp.WaitVisible(`[data-check-scope]`, chromedp.ByQuery), - chromedp.Evaluate(countChecked, &initial), - chromedp.Evaluate(countAccessBadges, &accessBadges), - // Uncheck only the first section. - chromedp.Click(`[data-check-scope]:first-of-type [data-check-none]`, chromedp.ByQuery), - chromedp.Evaluate(countChecked, &afterNone), - // Re-check the first section. - chromedp.Click(`[data-check-scope]:first-of-type [data-check-all]`, chromedp.ByQuery), - chromedp.Evaluate(countChecked, &afterAll), - ); err != nil { - t.Fatalf("browser run against %s: %v", url, err) - } - - // One Access badge per command row: total should match the total checkbox count. - totalBoxes := 0 - for _, s := range initial { - totalBoxes += s[0] - } - if accessBadges != totalBoxes { - t.Fatalf("Access column rendered %d tier badges, want one per command row (%d)", accessBadges, totalBoxes) - } - - if len(initial) < 2 { - t.Fatalf("expected at least 2 command sections, got %d", len(initial)) - } - // Default is permissive: everything checked. - for i, s := range initial { - if s[0] == 0 || s[1] != s[0] { - t.Fatalf("section %d: expected all %d boxes checked initially, got %d checked", i, s[0], s[1]) - } - } - // Select none scoped to section 0: only section 0 clears. - if afterNone[0][1] != 0 { - t.Fatalf("section 0: Select none left %d boxes checked, want 0", afterNone[0][1]) - } - for i := 1; i < len(afterNone); i++ { - if afterNone[i][1] != afterNone[i][0] { - t.Fatalf("section %d: unscoped Select none changed it (%d/%d checked)", i, afterNone[i][1], afterNone[i][0]) - } - } - // Select all restores section 0 to fully checked. - if afterAll[0][1] != afterAll[0][0] { - t.Fatalf("section 0: Select all left %d/%d boxes checked, want all", afterAll[0][1], afterAll[0][0]) - } - - watch.assertClean(t) -} diff --git a/internal/e2e/org_actions_test.go b/internal/e2e/org_actions_test.go deleted file mode 100644 index 0978ec3..0000000 --- a/internal/e2e/org_actions_test.go +++ /dev/null @@ -1,56 +0,0 @@ -//go:build browser - -package e2e - -import ( - "strings" - "testing" - - cdplog "github.com/chromedp/cdproto/log" - "github.com/chromedp/cdproto/network" - "github.com/chromedp/chromedp" -) - -// TestE2EOrgActionsMenuLimitCommands verifies the org page's "Actions" dropdown -// opens under the strict CSP (Bootstrap's data-bs-toggle, no inline JS) and that -// its "Limit commands" item navigates to the org-scoped my-commands page — the -// new home for that org-wide, per-member setting after it moved off the -// per-repeater details page. -func TestE2EOrgActionsMenuLimitCommands(t *testing.T) { - srv := newE2EServer(t) - user, cookie := srv.login(t, "e2euser") - - // CreateOrg makes the creator an admin member, so the member view (with the - // Actions dropdown) renders. - org, err := srv.store.CreateOrg(srv.ctx, "Actions Org", user.ID) - if err != nil { - t.Fatalf("create org: %v", err) - } - - bctx, cancel, watch := startBrowser(t) - defer cancel() - - orgURL := srv.appURL + "/orgs/" + org.Slug - var landedURL string - if err := chromedp.Run(bctx, - network.Enable(), - cdplog.Enable(), - setSessionCookie(cookie), - chromedp.Navigate(orgURL), - // Open the dropdown (exercises Bootstrap JS under the CSP), then click the - // Limit commands item once the menu is shown. - chromedp.Click(`.dropdown-toggle`, chromedp.ByQuery), - chromedp.WaitVisible(`.dropdown-menu.show a[href$="/my-commands"]`, chromedp.ByQuery), - chromedp.Click(`.dropdown-menu.show a[href$="/my-commands"]`, chromedp.ByQuery), - // The my-commands page renders the command sections behind its form. - chromedp.WaitVisible(`#cmdform-perms`, chromedp.ByQuery), - chromedp.Location(&landedURL), - ); err != nil { - t.Fatalf("browser run against %s: %v", orgURL, err) - } - - if !strings.HasSuffix(landedURL, "/orgs/"+org.Slug+"/my-commands") { - t.Fatalf("Actions → Limit commands landed on %q, want the org my-commands page", landedURL) - } - watch.assertClean(t) -} diff --git a/internal/store/migrations/0037_repeater_org_command_optin.sql b/internal/store/migrations/0037_repeater_org_command_optin.sql new file mode 100644 index 0000000..73dd003 --- /dev/null +++ b/internal/store/migrations/0037_repeater_org_command_optin.sql @@ -0,0 +1,47 @@ +-- +goose Up +-- Move the owner's optional per-org command allowlist from per-(owner, org) to +-- per-(repeater, org). The old model forced one restriction across ALL of an +-- owner's repeaters in an org; keying per repeater lets a single box diverge — +-- e.g. a tower repeater under strict control that allows only `advert` while the +-- owner's other repeaters in the same org stay permissive. Semantics are otherwise +-- unchanged: no rows for a (org, repeater) pair = permissive (the site ceiling +-- applies unchanged); >=1 row = restricted to exactly those commands (still +-- intersected with the ceiling and the caller's tier). This is the opposite +-- default from share_commands, where no rows means deny-all. +CREATE TABLE org_repeater_command_optin ( + org_id BIGINT NOT NULL REFERENCES organizations(id) ON DELETE CASCADE, + repeater_id BIGINT NOT NULL REFERENCES repeaters(id) ON DELETE CASCADE, + command_id BIGINT NOT NULL REFERENCES command_catalog(id) ON DELETE CASCADE, + PRIMARY KEY (org_id, repeater_id, command_id) +); +CREATE INDEX org_repeater_command_optin_repeater_id_idx ON org_repeater_command_optin(repeater_id); + +-- Preserve current behavior exactly: replicate each owner's per-org list onto +-- every repeater that owner owns. Repeaters that don't participate in the org +-- (excluded, or owner no longer a member) carry harmless rows the auth query +-- never reaches. +INSERT INTO org_repeater_command_optin (org_id, repeater_id, command_id) +SELECT o.org_id, r.id, o.command_id +FROM org_command_optin o +JOIN repeaters r ON r.owner_id = o.owner_id; + +DROP TABLE org_command_optin; + +-- +goose Down +-- Best-effort, lossy reverse: collapse per-repeater lists back to per-owner by +-- union. Per-repeater divergence introduced under the new model can't be +-- represented per-owner, so a repeater restricted differently from its siblings +-- widens to the union of the owner's lists for that org on the way down. +CREATE TABLE org_command_optin ( + org_id BIGINT NOT NULL REFERENCES organizations(id) ON DELETE CASCADE, + owner_id BIGINT NOT NULL REFERENCES users(id) ON DELETE CASCADE, + command_id BIGINT NOT NULL REFERENCES command_catalog(id) ON DELETE CASCADE, + PRIMARY KEY (org_id, owner_id, command_id) +); + +INSERT INTO org_command_optin (org_id, owner_id, command_id) +SELECT DISTINCT o.org_id, r.owner_id, o.command_id +FROM org_repeater_command_optin o +JOIN repeaters r ON r.id = o.repeater_id; + +DROP TABLE org_repeater_command_optin; diff --git a/internal/store/org_command_optin.go b/internal/store/org_command_optin.go deleted file mode 100644 index 1218b2b..0000000 --- a/internal/store/org_command_optin.go +++ /dev/null @@ -1,43 +0,0 @@ -package store - -import ( - "context" - "fmt" - - "github.com/jackc/pgx/v5" -) - -// An owner's optional per-org command allowlist. No rows for an (org, owner) pair -// means permissive — the site ceiling (org_member_allowed / org_admin_allowed) -// applies unchanged. One or more rows restrict that org to exactly the listed -// commands on the owner's repeaters (still intersected with the ceiling and tier). - -// OrgOptInCommandIDs returns the command ids an owner has opted into for an org. -// An empty result means no restriction (permissive). -func (s *Store) OrgOptInCommandIDs(ctx context.Context, orgID, ownerID int64) ([]int64, error) { - rows, err := s.pool.Query(ctx, - `SELECT command_id FROM org_command_optin WHERE org_id = $1 AND owner_id = $2`, orgID, ownerID) - if err != nil { - return nil, fmt.Errorf("org opt-in commands: %w", err) - } - return collectRows(rows, scanID) -} - -// SetOrgOptIn replaces an owner's opt-in command list for an org. Passing no ids -// clears the restriction (reverts to permissive). -func (s *Store) SetOrgOptIn(ctx context.Context, orgID, ownerID int64, commandIDs []int64) error { - return s.inTx(ctx, func(tx pgx.Tx) error { - if _, err := tx.Exec(ctx, - `DELETE FROM org_command_optin WHERE org_id = $1 AND owner_id = $2`, orgID, ownerID); err != nil { - return fmt.Errorf("clear org opt-in: %w", err) - } - for _, id := range commandIDs { - if _, err := tx.Exec(ctx, - `INSERT INTO org_command_optin (org_id, owner_id, command_id) VALUES ($1, $2, $3) - ON CONFLICT DO NOTHING`, orgID, ownerID, id); err != nil { - return fmt.Errorf("insert org opt-in: %w", err) - } - } - return nil - }) -} diff --git a/internal/store/org_repeater_command_optin.go b/internal/store/org_repeater_command_optin.go new file mode 100644 index 0000000..6d5d3f5 --- /dev/null +++ b/internal/store/org_repeater_command_optin.go @@ -0,0 +1,45 @@ +package store + +import ( + "context" + "fmt" + + "github.com/jackc/pgx/v5" +) + +// A repeater's optional per-org command allowlist. No rows for a (org, repeater) +// pair means permissive — the site ceiling (org_member_allowed / org_admin_allowed) +// applies unchanged. One or more rows restrict that org to exactly the listed +// commands on that repeater (still intersected with the ceiling and the caller's +// tier). Keyed per repeater so one box (e.g. a tower under strict control) can +// diverge from the owner's other repeaters in the same org. + +// RepeaterOrgOptInCommandIDs returns the command ids opted into for a repeater in +// an org. An empty result means no restriction (permissive). +func (s *Store) RepeaterOrgOptInCommandIDs(ctx context.Context, orgID, repeaterID int64) ([]int64, error) { + rows, err := s.pool.Query(ctx, + `SELECT command_id FROM org_repeater_command_optin WHERE org_id = $1 AND repeater_id = $2`, orgID, repeaterID) + if err != nil { + return nil, fmt.Errorf("repeater org opt-in commands: %w", err) + } + return collectRows(rows, scanID) +} + +// SetRepeaterOrgOptIn replaces a repeater's opt-in command list for an org. +// Passing no ids clears the restriction (reverts to permissive). +func (s *Store) SetRepeaterOrgOptIn(ctx context.Context, orgID, repeaterID int64, commandIDs []int64) error { + return s.inTx(ctx, func(tx pgx.Tx) error { + if _, err := tx.Exec(ctx, + `DELETE FROM org_repeater_command_optin WHERE org_id = $1 AND repeater_id = $2`, orgID, repeaterID); err != nil { + return fmt.Errorf("clear repeater org opt-in: %w", err) + } + for _, id := range commandIDs { + if _, err := tx.Exec(ctx, + `INSERT INTO org_repeater_command_optin (org_id, repeater_id, command_id) VALUES ($1, $2, $3) + ON CONFLICT DO NOTHING`, orgID, repeaterID, id); err != nil { + return fmt.Errorf("insert repeater org opt-in: %w", err) + } + } + return nil + }) +} diff --git a/internal/store/orgs_resolution_test.go b/internal/store/orgs_resolution_test.go index 52e4e3f..64dc10b 100644 --- a/internal/store/orgs_resolution_test.go +++ b/internal/store/orgs_resolution_test.go @@ -123,15 +123,15 @@ func TestOrgCommandResolution(t *testing.T) { // Outsider: nothing. check("outsider/advert", can(outsider, advert), false) - // Owner opts the org into only {advert}: the admin loses the admin-tier - // commands not in the list, but keeps advert. - if err := st.SetOrgOptIn(ctx, org.ID, owner, []int64{advert}); err != nil { + // Owner restricts this repeater in the org to only {advert}: the admin loses + // the admin-tier commands not in the list, but keeps advert. + if err := st.SetRepeaterOrgOptIn(ctx, org.ID, rep.ID, []int64{advert}); err != nil { t.Fatal(err) } check("admin/set.radio with opt-in", can(adminM, setRadio), false) check("admin/advert with opt-in", can(adminM, advert), true) // Clearing the opt-in restores the full ceiling. - if err := st.SetOrgOptIn(ctx, org.ID, owner, nil); err != nil { + if err := st.SetRepeaterOrgOptIn(ctx, org.ID, rep.ID, nil); err != nil { t.Fatal(err) } check("admin/set.radio after clear", can(adminM, setRadio), true) @@ -288,3 +288,86 @@ func TestOrgRepeaterAccess(t *testing.T) { t.Errorf("outsider GetRepeaterForUser = %v, want ErrNotFound", err) } } + +// TestRepeaterOrgOptInPerRepeaterDiverges is the reason the opt-in was reshaped +// from per-(owner, org) to per-(repeater, org): an owner with two repeaters in the +// same org must be able to lock one down (e.g. a tower repeater under strict +// control, allowing only advert) while the other stays permissive. Under the old +// per-owner model this was impossible — one list governed all their repeaters. +func TestRepeaterOrgOptInPerRepeaterDiverges(t *testing.T) { + t.Parallel() + st, ctx := orgTestStore(t) + + cmdID := func(key string) int64 { + var id int64 + if err := st.pool.QueryRow(ctx, `SELECT id FROM command_catalog WHERE key=$1`, key).Scan(&id); err != nil { + t.Fatalf("command %q: %v", key, err) + } + return id + } + setRadio, advert := cmdID("set.radio"), cmdID("advert") + setCeiling := func(id int64, member, admin bool) { + if err := st.UpdateCommandFlags(ctx, id, false, false, member, admin); err != nil { + t.Fatalf("set ceiling %d: %v", id, err) + } + } + setCeiling(setRadio, false, true) + setCeiling(advert, true, false) + + owner, err := st.CreateUser(ctx, "diverge-owner", "") + if err != nil { + t.Fatal(err) + } + adminM, err := st.CreateUser(ctx, "diverge-admin", "") + if err != nil { + t.Fatal(err) + } + + mkRep := func(pkByte string) *Repeater { + rep, err := st.CreateRepeater(ctx, &Repeater{ + OwnerID: owner.ID, Name: "R", PublicKeyHex: strings.Repeat(pkByte, 64), + RadioFreqHz: 1, RadioBwHz: 1, RadioSF: 11, RadioCR: 5, + }) + if err != nil { + t.Fatalf("create repeater: %v", err) + } + return rep + } + tower := mkRep("d") // locked down + spare := mkRep("e") // permissive + + org, err := st.CreateOrg(ctx, "Region", owner.ID) // owner is org-admin + if err != nil { + t.Fatalf("create org: %v", err) + } + if err := st.AddOrgMember(ctx, org.ID, adminM.ID, "admin"); err != nil { + t.Fatal(err) + } + + // Restrict only the tower to {advert}; leave the spare untouched (permissive). + if err := st.SetRepeaterOrgOptIn(ctx, org.ID, tower.ID, []int64{advert}); err != nil { + t.Fatal(err) + } + + can := func(repID, c int64) bool { + ok, err := st.CanSendCommand(ctx, adminM.ID, repID, c) + if err != nil { + t.Fatalf("CanSendCommand: %v", err) + } + return ok + } + // Tower: advert only, set.radio denied by the restriction. + if !can(tower.ID, advert) { + t.Error("tower/advert = false, want true") + } + if can(tower.ID, setRadio) { + t.Error("tower/set.radio = true, want false (restricted to advert)") + } + // Spare: full admin-tier ceiling, unaffected by the tower's restriction. + if !can(spare.ID, setRadio) { + t.Error("spare/set.radio = false, want true (permissive) — restriction leaked across repeaters") + } + if !can(spare.ID, advert) { + t.Error("spare/advert = false, want true") + } +} diff --git a/internal/store/share_commands.go b/internal/store/share_commands.go index 10e7b3e..31ab67b 100644 --- a/internal/store/share_commands.go +++ b/internal/store/share_commands.go @@ -21,8 +21,8 @@ import ( // - the repeater participates in an org they and the owner both belong to (the // owner is a member and hasn't excluded the repeater), the command is within // the site ceiling for their tier (member → org_member_allowed; admin → -// org_member_allowed OR org_admin_allowed), AND the owner either set no opt-in -// list for that org (permissive) or listed this command. +// org_member_allowed OR org_admin_allowed), AND that repeater has no opt-in +// list for that org (permissive) or the list includes this command. func (s *Store) ListSendableCommandIDs(ctx context.Context, userID, repeaterID int64) ([]int64, error) { rows, err := s.pool.Query(ctx, ` SELECT c.id @@ -41,10 +41,10 @@ func (s *Store) ListSendableCommandIDs(ctx context.Context, userID, repeaterID i WHERE e.org_id = ownm.org_id AND e.repeater_id = r.id) AND (c.org_member_allowed OR (usrm.role = 'admin' AND c.org_admin_allowed)) AND ( - NOT EXISTS (SELECT 1 FROM org_command_optin o - WHERE o.org_id = ownm.org_id AND o.owner_id = r.owner_id) - OR EXISTS (SELECT 1 FROM org_command_optin o - WHERE o.org_id = ownm.org_id AND o.owner_id = r.owner_id AND o.command_id = c.id) + NOT EXISTS (SELECT 1 FROM org_repeater_command_optin o + WHERE o.org_id = ownm.org_id AND o.repeater_id = r.id) + OR EXISTS (SELECT 1 FROM org_repeater_command_optin o + WHERE o.org_id = ownm.org_id AND o.repeater_id = r.id AND o.command_id = c.id) ) )`, userID, repeaterID)