From fc3c811335b2db14f53590904a2a1c0cfe7deb45 Mon Sep 17 00:00:00 2001 From: Jonathon Leight Date: Tue, 7 Jul 2026 21:32:45 -0400 Subject: [PATCH] Fix wizard's org access management --- internal/core/endpoints_repeater_test.go | 103 +++++++++++------- internal/core/org_participation.go | 40 +++---- internal/core/repeaters.go | 7 +- internal/core/templates/repeater_added.html | 19 +--- internal/core/templates/share.html | 43 +------- internal/core/templates/share_org_limits.html | 18 ++- internal/core/web.go | 1 - internal/e2e/share_limits_test.go | 4 +- internal/e2e/wizard_access_test.go | 59 ++++++++++ internal/web/env.go | 4 +- internal/web/templates/org_access.html | 36 ++++++ 11 files changed, 202 insertions(+), 132 deletions(-) create mode 100644 internal/e2e/wizard_access_test.go create mode 100644 internal/web/templates/org_access.html diff --git a/internal/core/endpoints_repeater_test.go b/internal/core/endpoints_repeater_test.go index c989386..f3f4b06 100644 --- a/internal/core/endpoints_repeater_test.go +++ b/internal/core/endpoints_repeater_test.go @@ -193,39 +193,8 @@ func TestRepeaterSharePosts(t *testing.T) { un.Body.Close() assertRedirect(t, un, share, "unshare") - // #87 participation: the per-org "Shared" switch. Unchecked submits no - // "include" field (opt out); checked submits include=1 (participate). - org, err := st.CreateOrg(ctx, "Participation Org", owner.ID) - if err != nil { - t.Fatal(err) - } - excluded := func() bool { - orgs, err := st.ListRepeaterOrgMemberships(ctx, rep.ID) - if err != nil { - t.Fatalf("list memberships: %v", err) - } - for _, o := range orgs { - if o.OrgID == org.ID { - return o.Excluded - } - } - t.Fatalf("org %d not in memberships", org.ID) - return false - } - // The {orgID} route param is the org slug, not the numeric id. - partURL := "/repeaters/" + pid + "/orgs/" + org.Slug + "/participation" - out := post(t, ts, h.app, partURL, url.Values{}, sess) // switch off → opt out - out.Body.Close() - assertRedirect(t, out, share, "org opt out") - if !excluded() { - t.Fatal("switch off did not opt the repeater out") - } - in := post(t, ts, h.app, partURL, url.Values{"include": {"1"}}, sess) // switch on - in.Body.Close() - assertRedirect(t, in, share, "org opt in") - if excluded() { - t.Fatal("switch on did not re-include the repeater") - } + // Org participation is exercised via the "manage access" limits save in + // TestRepeaterOrgLimitsPosts (there is no standalone participation endpoint). } // TestRepeaterOrgLimitsPosts covers the per-(repeater, org) command-limits modal: @@ -258,10 +227,10 @@ func TestRepeaterOrgLimitsPosts(t *testing.T) { t.Fatalf("need >=2 ceiling commands, got %d", len(ceiling)) } - // GET renders the modal fragment (no page chrome), with the org name and cmd boxes. + // GET renders the modal fragment (no page chrome): the Shared switch + cmd boxes. frag := readBody(t, do(t, ts, h.app, base, sess)) - if !strings.Contains(frag, "Command limits") || !strings.Contains(frag, `name="cmd"`) { - t.Fatalf("limits fragment missing expected content:\n%s", frag) + if !strings.Contains(frag, "Manage access") || !strings.Contains(frag, `name="cmd"`) || !strings.Contains(frag, `name="include"`) { + t.Fatalf("manage-access fragment missing expected content:\n%s", frag) } if strings.Contains(frag, "back-link") { t.Fatal("limits fragment should be modal chrome, not a full page") @@ -290,17 +259,27 @@ func TestRepeaterOrgLimitsPosts(t *testing.T) { } return ids } + excluded := func() bool { + ex, err := st.IsRepeaterOrgExcluded(ctx, org.ID, rep.ID) + if err != nil { + t.Fatalf("excluded: %v", err) + } + return ex + } - // Restrict to exactly the first ceiling command. - save := post(t, ts, h.app, base, url.Values{"cmd": {strconv.FormatInt(ceiling[0], 10)}}, sess) + // Save restricts to exactly the first ceiling command, with the Shared switch on. + save := post(t, ts, h.app, base, url.Values{"include": {"1"}, "cmd": {strconv.FormatInt(ceiling[0], 10)}}, sess) save.Body.Close() assertRedirect(t, save, "/repeaters/"+rep.PublicID+"/share", "save limits") if got := optIn(); len(got) != 1 || got[0] != ceiling[0] { t.Fatalf("opt-in = %v, want [%d]", got, ceiling[0]) } + if excluded() { + t.Fatal("saving with the Shared switch on opted the repeater out") + } // Selecting the full ceiling collapses back to permissive (no rows stored). - full := url.Values{} + full := url.Values{"include": {"1"}} for _, id := range ceiling { full.Add("cmd", strconv.FormatInt(id, 10)) } @@ -311,14 +290,54 @@ func TestRepeaterOrgLimitsPosts(t *testing.T) { t.Fatalf("full selection should store nothing (permissive), got %v", got) } - // Restrict again, then "Remove restriction" clears it. - post(t, ts, h.app, base, url.Values{"cmd": {strconv.FormatInt(ceiling[0], 10)}}, sess).Body.Close() - clr := post(t, ts, h.app, base, url.Values{"clear": {"1"}}, sess) + // Restrict again, then "Remove restriction" clears it (switch still on). + post(t, ts, h.app, base, url.Values{"include": {"1"}, "cmd": {strconv.FormatInt(ceiling[0], 10)}}, sess).Body.Close() + clr := post(t, ts, h.app, base, url.Values{"include": {"1"}, "clear": {"1"}}, sess) clr.Body.Close() assertRedirect(t, clr, "/repeaters/"+rep.PublicID+"/share", "clear limits") if got := optIn(); len(got) != 0 { t.Fatalf("clear should remove all rows, got %v", got) } + + // The Shared switch drives participation: saving with it off opts out; on opts in. + off := post(t, ts, h.app, base, url.Values{}, sess) // switch off (no include field) + off.Body.Close() + assertRedirect(t, off, "/repeaters/"+rep.PublicID+"/share", "save opted out") + if !excluded() { + t.Fatal("saving with the Shared switch off did not opt the repeater out") + } + on := post(t, ts, h.app, base, url.Values{"include": {"1"}}, sess) + on.Body.Close() + assertRedirect(t, on, "/repeaters/"+rep.PublicID+"/share", "save opted in") + if excluded() { + t.Fatal("saving with the Shared switch on did not re-include the repeater") + } +} + +// TestRepeaterAddedPageOrgAccess: the add-repeater wizard's final step lists the +// owner's orgs with the shared "Manage access" control (same as the share page), +// not the removed one-click /participation opt-out. +func TestRepeaterAddedPageOrgAccess(t *testing.T) { + t.Parallel() + st, ctx, ts, h := splitServer(t) + owner, sess := appLogin(t, ts, st, ctx, h.app, "addedowner") + org, err := st.CreateOrg(ctx, "Added Org", owner.ID) // owner is a member + if err != nil { + t.Fatal(err) + } + rep := newOwnedRepeater(t, st, ctx, owner.ID, "Added Rep") + + body := readBody(t, do(t, ts, h.app, "/repeaters/"+rep.PublicID+"/added", sess)) + if !strings.Contains(body, org.Name) { + t.Fatalf("added page missing the owner's org %q:\n%s", org.Name, body) + } + if !strings.Contains(body, `data-testid="manage-access"`) || !strings.Contains(body, `id="limits-modal"`) { + t.Fatal("added page missing the Manage access control / modal") + } + // The old dead route must be gone. + if strings.Contains(body, "/participation") { + t.Fatal("added page still references the removed /participation endpoint") + } } // #88 POST /invite/{token}/accept — a second user redeems a share link. diff --git a/internal/core/org_participation.go b/internal/core/org_participation.go index 8e6df10..08056df 100644 --- a/internal/core/org_participation.go +++ b/internal/core/org_participation.go @@ -32,28 +32,11 @@ func (s *Handlers) repeaterOrgContext(w http.ResponseWriter, r *http.Request) (* return rep, orgID, true } -// handleSetRepeaterOrg opts a repeater into or out of an org (owner only). A -// repeater participates in every org its owner belongs to by default; this writes -// or clears the opt-out. Driven by the share page's per-org "Shared" switch: a -// checked switch submits include=1 (participate), unchecked submits nothing (opt -// out). -func (s *Handlers) handleSetRepeaterOrg(w http.ResponseWriter, r *http.Request) { - rep, orgID, ok := s.repeaterOrgContext(w, r) - if !ok { - return - } - exclude := r.FormValue("include") != "1" - if err := s.Store.SetRepeaterOrgExcluded(r.Context(), orgID, rep.ID, exclude); err != nil { - s.ServerError(w, r, "could not update participation", err) - return - } - http.Redirect(w, r, sharePath(rep.PublicID), http.StatusSeeOther) -} - -// pageRepeaterOrgLimits renders the per-org command-limits modal fragment for one -// repeater: which of the commands the org may run are allowed to run on this box. -// No opt-in rows = permissive (every ceiling command checked). Editable regardless -// of participation, so an owner can pre-set limits before opting an org back in. +// pageRepeaterOrgLimits renders the per-org "manage access" modal fragment for one +// repeater: the Shared (participation) switch plus which of the commands the org +// may run are allowed to run on this box. No opt-in rows = permissive (every +// ceiling command checked). Editable regardless of participation, so an owner can +// pre-set limits before opting an org in. func (s *Handlers) pageRepeaterOrgLimits(w http.ResponseWriter, r *http.Request) { rep, orgID, ok := s.repeaterOrgContext(w, r) if !ok { @@ -106,9 +89,10 @@ func (s *Handlers) pageRepeaterOrgLimits(w http.ResponseWriter, r *http.Request) }) } -// handleSaveRepeaterOrgLimits saves the per-(repeater, org) command opt-in list. A -// full selection (or the "Remove restriction" button) clears it back to permissive -// so we don't persist a redundant full allowlist. +// handleSaveRepeaterOrgLimits applies the "manage access" modal: it sets the org's +// participation (the Shared switch) and the per-(repeater, org) command opt-in list +// together. A full selection (or the "Remove restriction" button) clears the list +// back to permissive so we don't persist a redundant full allowlist. func (s *Handlers) handleSaveRepeaterOrgLimits(w http.ResponseWriter, r *http.Request) { rep, orgID, ok := s.repeaterOrgContext(w, r) if !ok { @@ -118,6 +102,12 @@ func (s *Handlers) handleSaveRepeaterOrgLimits(w http.ResponseWriter, r *http.Re http.Error(w, "bad form", http.StatusBadRequest) return } + // Participation: the Shared switch submits include=1 when on, nothing when off. + exclude := r.FormValue("include") != "1" + if err := s.Store.SetRepeaterOrgExcluded(r.Context(), orgID, rep.ID, exclude); err != nil { + s.ServerError(w, r, "could not update participation", err) + return + } var chosen []int64 // "Remove restriction" clears the list regardless of checkboxes. if r.FormValue("clear") == "" { diff --git a/internal/core/repeaters.go b/internal/core/repeaters.go index da8f4d5..52d8df2 100644 --- a/internal/core/repeaters.go +++ b/internal/core/repeaters.go @@ -145,12 +145,13 @@ func addErr(w http.ResponseWriter, r *http.Request, msg string) { // repeater: optionally confirm it with a modem now, and manage which organizations // it's shared with. People-sharing lives on the repeater's own sharing page. func (s *Handlers) pageRepeaterAdded(w http.ResponseWriter, r *http.Request) { - uid := s.Auth.CurrentUserID(r.Context()) - rep, _, ok := s.requireRepeaterOwned(w, r) + rep, id, ok := s.requireRepeaterOwned(w, r) if !ok { return } - orgs, err := s.Store.ListOrgsForUser(r.Context(), uid) + // Same per-org data (with participation/limit state) the share page uses, so + // the wizard's "Manage access" buttons and status badges match it exactly. + orgs, err := s.Store.ListRepeaterOrgMemberships(r.Context(), id) if err != nil { s.ServerError(w, r, "could not load organizations", err) return diff --git a/internal/core/templates/repeater_added.html b/internal/core/templates/repeater_added.html index 42cf9e5..0552b65 100644 --- a/internal/core/templates/repeater_added.html +++ b/internal/core/templates/repeater_added.html @@ -46,21 +46,11 @@ {{if .Orgs}}

This repeater is now shared with the organizations you belong to, so their admins and members can run - the commands those organizations are permitted to run on it. Opt out of any of them here — you can - change this anytime, and limit which commands an organization may run, from the sharing page. + the commands those organizations are permitted to run on it. Use Manage access to opt + out of any of them or limit which commands it may run — you can also change this anytime from the + sharing page.

-
- {{range .Orgs}} -
- {{.Org.Name}} - {{if eq .Role "admin"}}admin{{else}}member{{end}} -
- - -
-
- {{end}} -
+ {{template "org-access-rows" .}} {{else}}

You're not in any organizations yet. Joining one lets its admins help keep your repeater in spec. @@ -72,6 +62,7 @@ +{{template "limits-modal-shell" .}} {{end}} diff --git a/internal/core/templates/share.html b/internal/core/templates/share.html index 722b275..bfd056f 100644 --- a/internal/core/templates/share.html +++ b/internal/core/templates/share.html @@ -96,39 +96,12 @@

Organizations

This repeater is shared with every organization you belong to, so their admins and members can run the - commands those organizations are permitted to run on it. Turn an organization off to opt out, or edit - its command limits for this repeater. + commands those organizations are permitted to run on it. Use Manage access to opt it + out of an organization or limit which commands that organization may run on this repeater.

{{if .Orgs}} -
- {{range .Orgs}} -
-
- {{.OrgName}} - {{if .Excluded}} - Opted out - {{else if .Restricted}} - Limited commands - {{else}} - All commands - {{end}} -
- -
- -
-
-
-
- {{end}} -
+ {{template "org-access-rows" .}} {{else}}

Join an organization to share this repeater with it.

{{end}} @@ -137,18 +110,14 @@ - + - +{{template "limits-modal-shell" .}} {{template "icon-arrow-left" "me-1"}}Back to dashboard {{end}} diff --git a/internal/core/templates/share_org_limits.html b/internal/core/templates/share_org_limits.html index 095f549..f73cf9d 100644 --- a/internal/core/templates/share_org_limits.html +++ b/internal/core/templates/share_org_limits.html @@ -1,13 +1,19 @@ {{/* org-limits-modal is the htmx fragment swapped into the share page's shared - #limits-modal. The form posts normally (full navigation back to the share - page), which also closes the modal — no client JS needed. */}} + #limits-modal. It controls both this org's participation (the "Shared" switch) + and its per-repeater command limits; one Save applies both. The form posts + normally (full navigation back to the share page), which also closes the + modal — no client JS needed. */}} {{define "org-limits-modal"}}
-
Command limits — {{.Org.Name}}
+
Manage access — {{.Org.Name}}
+ {{if .Excluded}} @@ -38,7 +44,7 @@ {{end}} - +
{{end}} diff --git a/internal/core/web.go b/internal/core/web.go index e89346d..9a9b949 100644 --- a/internal/core/web.go +++ b/internal/core/web.go @@ -207,7 +207,6 @@ func (s *Handlers) appRouter() chi.Router { r.Get("/repeaters/{id}/share/{userID}/commands", s.pageShareCommands) r.Post("/repeaters/{id}/share/{userID}/commands", s.handleSetShareCommands) r.Post("/repeaters/{id}/share/{userID}/steward", s.handleSetShareSteward) - r.Post("/repeaters/{id}/orgs/{orgID}/participation", s.handleSetRepeaterOrg) r.Get("/repeaters/{id}/orgs/{orgID}/limits", s.pageRepeaterOrgLimits) r.Post("/repeaters/{id}/orgs/{orgID}/limits", s.handleSaveRepeaterOrgLimits) r.Post("/invite/{token}/accept", s.handleAcceptInvite) diff --git a/internal/e2e/share_limits_test.go b/internal/e2e/share_limits_test.go index 99b2fbc..62e3811 100644 --- a/internal/e2e/share_limits_test.go +++ b/internal/e2e/share_limits_test.go @@ -59,8 +59,8 @@ func TestE2EShareOrgLimitsModal(t *testing.T) { setSessionCookie(cookie), chromedp.Navigate(shareURL), // Open the per-org limits modal; htmx loads the command grid into it. - chromedp.WaitVisible(`[data-testid="edit-limits"]`, chromedp.ByQuery), - chromedp.Click(`[data-testid="edit-limits"]`, chromedp.ByQuery), + chromedp.WaitVisible(`[data-testid="manage-access"]`, chromedp.ByQuery), + chromedp.Click(`[data-testid="manage-access"]`, chromedp.ByQuery), chromedp.WaitVisible(`#limits-modal-content [data-check-scope]`, chromedp.ByQuery), chromedp.Evaluate(countChecked, &initial), // Default is permissive: everything checked. Uncheck only the first section. diff --git a/internal/e2e/wizard_access_test.go b/internal/e2e/wizard_access_test.go new file mode 100644 index 0000000..82d4b08 --- /dev/null +++ b/internal/e2e/wizard_access_test.go @@ -0,0 +1,59 @@ +//go:build browser + +package e2e + +import ( + "strings" + "testing" + + cdplog "github.com/chromedp/cdproto/log" + "github.com/chromedp/cdproto/network" + "github.com/chromedp/chromedp" + + "github.com/jleight/meshtender/internal/store" +) + +// TestE2EWizardManageAccessModal verifies the add-repeater wizard's final step +// hosts the same "Manage access" modal as the share page: the button opens the +// shared modal (Bootstrap under the strict CSP) and htmx loads the org-limits +// fragment into it. Guards the fix for the wizard's old dead-route opt-out button. +func TestE2EWizardManageAccessModal(t *testing.T) { + srv := newE2EServer(t) + user, cookie := srv.login(t, "e2ewizard") + + // CreateOrg makes the creator a member, so the new repeater participates and + // the wizard lists it with a Manage access button. + if _, err := srv.store.CreateOrg(srv.ctx, "Wizard Org", user.ID); err != nil { + t.Fatalf("create org: %v", err) + } + rep, err := srv.store.CreateRepeater(srv.ctx, &store.Repeater{ + OwnerID: user.ID, Name: "Rep", PublicKeyHex: strings.Repeat("a", 64), + RadioFreqHz: 1, RadioBwHz: 1, RadioSF: 11, RadioCR: 5, + }) + if err != nil { + t.Fatalf("create repeater: %v", err) + } + + bctx, cancel, watch := startBrowser(t) + defer cancel() + + addedURL := srv.appURL + "/repeaters/" + rep.PublicID + "/added" + var hasSwitch bool + if err := chromedp.Run(bctx, + network.Enable(), + cdplog.Enable(), + setSessionCookie(cookie), + chromedp.Navigate(addedURL), + chromedp.WaitVisible(`[data-testid="manage-access"]`, chromedp.ByQuery), + chromedp.Click(`[data-testid="manage-access"]`, chromedp.ByQuery), + // htmx loads the org-limits fragment: the Shared switch + command grid. + chromedp.WaitVisible(`#limits-modal-content [data-check-scope]`, chromedp.ByQuery), + chromedp.Evaluate(`!!document.querySelector('#limits-modal-content input[name=include]')`, &hasSwitch), + ); err != nil { + t.Fatalf("browser run against %s: %v", addedURL, err) + } + if !hasSwitch { + t.Fatal("manage-access modal missing the Shared switch") + } + watch.assertClean(t) +} diff --git a/internal/web/env.go b/internal/web/env.go index 9c400a1..45d11f9 100644 --- a/internal/web/env.go +++ b/internal/web/env.go @@ -26,7 +26,7 @@ import ( "github.com/jleight/meshtender/internal/store" ) -//go:embed templates/base.html templates/icons.html templates/org_tabs.html templates/repeater_tabs.html templates/command_grid.html templates/org_public.html templates/org_config.html templates/org_repeaters.html templates/error.html +//go:embed templates/base.html templates/icons.html templates/org_tabs.html templates/repeater_tabs.html templates/command_grid.html templates/org_access.html templates/org_public.html templates/org_config.html templates/org_repeaters.html templates/error.html var sharedTemplatesFS embed.FS // sharedPages are full content pages (not just layout partials) that more than @@ -229,7 +229,7 @@ func TimeElement(t time.Time, kind string) template.HTML { } func NewRenderer(cfg *config.Config, surfaceTemplates fs.FS) (*Renderer, error) { - base, err := template.New("").Funcs(templateFuncs).ParseFS(sharedTemplatesFS, "templates/base.html", "templates/icons.html", "templates/org_tabs.html", "templates/repeater_tabs.html", "templates/command_grid.html") + base, err := template.New("").Funcs(templateFuncs).ParseFS(sharedTemplatesFS, "templates/base.html", "templates/icons.html", "templates/org_tabs.html", "templates/repeater_tabs.html", "templates/command_grid.html", "templates/org_access.html") if err != nil { return nil, err } diff --git a/internal/web/templates/org_access.html b/internal/web/templates/org_access.html new file mode 100644 index 0000000..e943c23 --- /dev/null +++ b/internal/web/templates/org_access.html @@ -0,0 +1,36 @@ +{{/* org-access-rows renders the per-org access list shared by the share page and + the add-repeater wizard: each org the repeater's owner belongs to, with a + status badge and a "Manage access" button that loads the shared #limits-modal + via htmx. Data: $.Repeater (needs .PublicID) and .Orgs ([]RepeaterOrgMembership + with .OrgName / .OrgSlug / .Excluded / .Restricted). Pair it with + limits-modal-shell on the same page. */}} +{{define "org-access-rows"}} +
+ {{range .Orgs}} +
+ {{.OrgName}} + {{if .Excluded}} + Opted out + {{else if .Restricted}} + Limited commands + {{else}} + All commands + {{end}} + +
+ {{end}} +
+{{end}} + +{{/* limits-modal-shell is the empty shared modal the "Manage access" buttons load + their org-limits fragment into via htmx. */}} +{{define "limits-modal-shell"}} + +{{end}}