diff --git a/AGENTS.md b/AGENTS.md index 5b3d5f053..c90b932ff 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -179,7 +179,7 @@ Do not: ## Local Development -Event codec: `CODEC_PROVIDER=json` is required wherever workers are exercised (the worker command/result envelopes carry untyped bodies Avro cannot serialize); the Makefile and docker-compose set it everywhere. `tracking-events` reads the same setting: the consumer decodes both of its topics with one codec, so the Rust publisher honours `CODEC_PROVIDER` on Kafka as well as on NATS. Avro there needs a Schema Registry and is refused at boot without one; JSON needs nothing. +Event codec: `json` is the default the Makefile and docker-compose set, because it needs nothing. `avro` works too: the worker command and result envelopes carry an `any` body, and a schema is derived for each from the declared registry in `internal/models/event_variants.go` (see `event_schema.go`), so a new event type is not carried until it is added there. It is only compiled into the `-kafka` images and resolves every event against `SCHEMA_REGISTRY_URL`. `tracking-events` reads the same setting: the consumer decodes both of its topics with one codec, so the Rust publisher honours `CODEC_PROVIDER` on Kafka as well as on NATS. Avro there needs a Schema Registry and is refused at boot without one; JSON needs nothing. Infra runs in docker; the Go services and frontends run natively on the host for fast iteration — no docker image rebuilds when you change app code. Targets live in the `Makefile`. diff --git a/admin/src/app/dashboard/TestersPage.tsx b/admin/src/app/dashboard/TestersPage.tsx index 9ac688318..df3632e97 100644 --- a/admin/src/app/dashboard/TestersPage.tsx +++ b/admin/src/app/dashboard/TestersPage.tsx @@ -2,8 +2,14 @@ // // A tester is an account handed to somebody outside the team: a vendor's // reviewer during an OAuth verification, an auditor, a support engineer. It is -// an ordinary account with its own workspace, marked exempt from the emailed -// login code because the holder cannot read this instance's mail. +// an ordinary account marked exempt from the emailed login code, because the +// holder cannot read this instance's mail. +// +// It can either get a workspace of its own or join one that already exists. The +// second is for a review judged on the app doing real work, where an empty +// workspace shows none of it. Joining names a role explicitly: this is the one +// path that grants workspace access without anybody in that workspace asking +// for it, so there is no default. // // The list exists because the way this goes wrong is not creating one, it is // forgetting it. An exemption taken out for a two-week review is still there a @@ -18,8 +24,10 @@ import { Badge } from "@/components/ui/badge"; import { Skeleton } from "@/components/ui/skeleton"; import { useAdminPerm } from "@/hooks/useAdminPerm"; import { AdminPerm } from "@/lib/auth/permissions"; -import { createTester, listTesters, revokeTester } from "@/lib/api/client/admin/testers"; -import type { CreatedTester } from "@/lib/api/models/admin"; +import { createTester, listTesters, listOrganizationRoles, revokeTester } from "@/lib/api/client/admin/testers"; +import { listOrganizations } from "@/lib/api/client/admin/organizations"; +import { DASHBOARD_URL } from "@/lib/env"; +import type { AdminOrgListItem, CreatedTester } from "@/lib/api/models/admin"; function fmt(ts?: string | null) { if (!ts) return "—"; @@ -33,6 +41,10 @@ export default function TestersPage() { const [email, setEmail] = useState(""); const [orgName, setOrgName] = useState(""); const [reason, setReason] = useState(""); + const [mode, setMode] = useState<"new" | "existing">("new"); + const [orgQuery, setOrgQuery] = useState(""); + const [org, setOrg] = useState(null); + const [roleID, setRoleID] = useState(""); // Held in state, never refetched: the server returns it once and cannot // produce it again. const [created, setCreated] = useState(null); @@ -42,14 +54,40 @@ export default function TestersPage() { queryFn: () => listTesters(), }); + // Only searched once there is something to search on: the unfiltered first + // page is a list of arbitrary workspaces, which is not a picker. + const orgs = useQuery({ + queryKey: ["admin", "testers", "orgs", orgQuery], + queryFn: () => listOrganizations({ q: orgQuery.trim() }), + enabled: mode === "existing" && orgQuery.trim().length >= 2, + }); + + const roles = useQuery({ + queryKey: ["admin", "testers", "roles", org?.id], + queryFn: () => listOrganizationRoles(org!.id), + enabled: mode === "existing" && !!org, + }); + + const joining = mode === "existing"; + const ready = !!email.trim() && !!reason.trim() && (!joining || (!!org && !!roleID)); + const create = useMutation({ mutationFn: () => - createTester({ email: email.trim(), org_name: orgName.trim() || undefined, reason: reason.trim() }), + createTester({ + email: email.trim(), + reason: reason.trim(), + ...(joining + ? { organization_id: org!.id, role_id: roleID } + : { org_name: orgName.trim() || undefined }), + }), onSuccess: (t) => { setCreated(t); setEmail(""); setOrgName(""); setReason(""); + setOrgQuery(""); + setOrg(null); + setRoleID(""); qc.invalidateQueries({ queryKey: ["admin", "testers"] }); toast.success("Tester created"); }, @@ -73,6 +111,12 @@ export default function TestersPage() { } const rows = testers.data?.data ?? []; + // A wrong sign-in address that looks plausible is the failure mode here: it + // is copied straight into a vendor's verification form. The local default + // surviving onto a deployed panel means nobody configured one. + const looksUnset = + /^https?:\/\/localhost(:|\/|$)/.test(DASHBOARD_URL) && + !/^https?:\/\/localhost(:|\/|$)/.test(window.location.origin); return (
@@ -82,9 +126,10 @@ export default function TestersPage() { Testers

- Accounts for people outside the team. Each gets its own workspace and skips the emailed - login code, because the holder cannot read this instance's mail. Everything else still - applies: the password, the captcha and the sign-in risk assessment. + Accounts for people outside the team. Each skips the emailed login code, because the + holder cannot read this instance's mail. Everything else still applies: the password, + the captcha and the sign-in risk assessment. A tester either gets a workspace of its own + or joins one that already exists.

@@ -93,9 +138,14 @@ export default function TestersPage() {
Copy these now. The password is not stored anywhere readable.
+
+ {created.joined_existing + ? "This account is a member of an existing workspace and will land in it on sign-in." + : "This account owns a new, empty workspace."} +
{[ - ["Sign in at", window.location.origin.replace("admin.", "dev.")], + ["Sign in at", DASHBOARD_URL], ["Email", created.email], ["Password", created.password], ].map(([k, v]) => ( @@ -113,6 +163,13 @@ export default function TestersPage() { ))}
+ {looksUnset && ( +
+ That sign-in address is this panel's build-time default, not this + deployment's dashboard. Set VITE_DASHBOARD_URL (or{" "} + WARMBLY_DASHBOARD_URL) before sending it to anyone. +
+ )} @@ -128,12 +185,116 @@ export default function TestersPage() { value={email} onChange={(e) => setEmail(e.target.value)} /> - setOrgName(e.target.value)} - /> + +
+ {(["new", "existing"] as const).map((m) => ( + + ))} +
+ + {mode === "new" ? ( + setOrgName(e.target.value)} + /> + ) : ( +
+

+ The tester becomes a member of this workspace and gets none of its own, so + signing in lands straight in it. It can see whatever the role below allows, + including real mailboxes and real contacts. +

+ + {org ? ( +
+ {org.name} + {org.owner_email} + +
+ ) : ( + <> + setOrgQuery(e.target.value)} + /> + {orgs.isError ? ( +
+ Could not search workspaces. Retry before concluding there is no match. +
+ ) : orgs.isFetching ? ( + + ) : (orgs.data?.data.length ?? 0) > 0 ? ( +
    + {orgs.data!.data.map((o) => ( +
  • + +
    • + ))} +
+ ) : orgQuery.trim().length >= 2 ? ( +
No workspace matches that.
+ ) : null} + + )} + + {org && ( + + )} + {org && roles.isError && ( +
+ Could not load this workspace's roles, so there is nothing safe to pick. +
+ )} +
+ )} + create.mutate()} > Create diff --git a/admin/src/lib/api/client/admin/testers.ts b/admin/src/lib/api/client/admin/testers.ts index a7338701b..1c069dc66 100644 --- a/admin/src/lib/api/client/admin/testers.ts +++ b/admin/src/lib/api/client/admin/testers.ts @@ -6,7 +6,7 @@ // exemptions are the same query. import { Request } from "@/lib/api/client"; -import type { LoginCodeExemption, CreatedTester } from "@/lib/api/models/admin"; +import type { LoginCodeExemption, CreatedTester, AdminOrgRole } from "@/lib/api/models/admin"; export function listTesters(): Promise<{ data: LoginCodeExemption[] }> { return Request({ method: "GET", url: "/admin/testers", authorization: true }); @@ -14,12 +14,21 @@ export function listTesters(): Promise<{ data: LoginCodeExemption[] }> { export function createTester(body: { email: string; - org_name?: string; reason: string; + /** Names a new workspace. Ignored when organization_id is set. */ + org_name?: string; + /** Joins an existing workspace instead of minting one. Requires role_id. */ + organization_id?: string; + role_id?: string; }): Promise { return Request({ method: "POST", url: "/admin/testers", authorization: true, data: body }); } +/** The roles of one workspace, for the join-existing path. */ +export function listOrganizationRoles(orgID: string): Promise<{ data: AdminOrgRole[] }> { + return Request({ method: "GET", url: `/admin/organizations/${orgID}/roles`, authorization: true }); +} + export function revokeTester(id: string): Promise<{ revoked: boolean }> { return Request({ method: "DELETE", url: `/admin/testers/${id}`, authorization: true }); } diff --git a/admin/src/lib/api/models/admin.ts b/admin/src/lib/api/models/admin.ts index fd08714b5..2192551ba 100644 --- a/admin/src/lib/api/models/admin.ts +++ b/admin/src/lib/api/models/admin.ts @@ -969,6 +969,20 @@ export interface CreatedTester { email: string; organization_id: string; password: string; + /** True when the tester joined a workspace that already existed rather + * than one minted for it. */ + joined_existing: boolean; +} + +/** A workspace role, as the Testers page offers them. Roles are ordinary rows + * an org can rename or delete, so the list is per workspace and not fixed. */ +export interface AdminOrgRole { + id: string; + organization_id: string; + name: string; + description?: string | null; + color?: string | null; + permissions: number; } // --- Promo codes ----------------------------------------------------------- diff --git a/docs/content/docs/development/accounts-and-access.mdx b/docs/content/docs/development/accounts-and-access.mdx index d2aec7616..59811659e 100644 --- a/docs/content/docs/development/accounts-and-access.mdx +++ b/docs/content/docs/development/accounts-and-access.mdx @@ -111,6 +111,19 @@ A reason is required, and so is `--by`: this command talks to Postgres directly The admin panel has the same thing under **Testers**: create one, see the password once, and revoke an exemption when the reason no longer holds. The account survives a revoke, so anything it did stays attributable; it simply stops bypassing the code. +#### Which workspace a tester lands in + +A tester either gets a workspace of its own or joins one that already exists, and the choice is on the create form. + +Its own is the default and the safer one. The reviewer connects their own mailbox and exercises the app without reaching anything of yours, which is usually all an OAuth verification needs to see. + +Joining an existing workspace is for a review judged on the app doing real work, where an empty workspace shows none of it. The tester becomes an ordinary member of that workspace and gets none of its own, so signing in lands straight in it: the dashboard skips its workspace picker only when someone belongs to exactly one. Two things follow from that, and both are the point rather than a side effect: + +- **You pick the role, and there is no default.** This is the one path that grants workspace access without anybody in that workspace asking for it, so the form will not submit until a role is named. The role decides everything the holder can reach, and it is recorded on the audit row as well as the members table, which the role can be changed out of later. +- **The tester can see real data.** A role carrying `access_unibox` or `manage_emails` means real mail and real mailboxes, belonging to real people. Give the narrowest role the review actually needs, and revoke it when the review ends. + +The tester occupies a seat like any other member, so a workspace at its team member limit refuses the join rather than quietly exceeding it. + `warmblyctl status` lists every exempt account on each run with its reason and the date, because the way this goes wrong is not granting one, it is forgetting it. Remove one with: ```bash diff --git a/internal/api/handler/admin_organization.go b/internal/api/handler/admin_organization.go index d33d5ac03..c82918c84 100644 --- a/internal/api/handler/admin_organization.go +++ b/internal/api/handler/admin_organization.go @@ -58,6 +58,25 @@ func (h *Handler) AdminGetOrganizationMembers(c *gin.Context) { c.JSON(http.StatusOK, &models.AdminOrgMembersResult{Data: members}) } +// AdminGetOrganizationRoles returns a workspace's roles. The Testers page +// needs them: joining a tester to an existing workspace has to name a role, and +// roles are ordinary rows an org can rename, add to and delete, so the panel +// cannot offer a fixed list. +func (h *Handler) AdminGetOrganizationRoles(c *gin.Context) { + orgID, err := uuid.Parse(c.Param("id")) + if err != nil { + errx.JSON(c, errx.New(errx.BadRequest, "invalid organization ID")) + return + } + + roles, xerr := h.OrganizationService.ListRoles(c.Request.Context(), orgID) + if xerr != nil { + errx.JSON(c, xerr) + return + } + c.JSON(http.StatusOK, gin.H{"data": roles}) +} + // AdminGetOrgOverrides returns the override row for an org, or 200 with // null when no admin has touched it yet. Read access requires only // view_organizations. diff --git a/internal/api/handler/admin_tester.go b/internal/api/handler/admin_tester.go index ad59fa80d..4f9218b3c 100644 --- a/internal/api/handler/admin_tester.go +++ b/internal/api/handler/admin_tester.go @@ -25,12 +25,25 @@ type adminCreateTesterRequest struct { Email string `json:"email"` OrgName string `json:"org_name"` Reason string `json:"reason"` + // OrgID joins the tester to a workspace that already exists instead of + // minting an empty one. That is what a vendor's reviewer needs: an OAuth + // verification is judged on the app doing real work, and a workspace with + // no mailbox in it shows none of that. RoleID is then required, because + // this is the one path that grants workspace access without anybody in + // that workspace asking for it, and a default would be a permission + // nobody chose. + OrgID *uuid.UUID `json:"organization_id"` + RoleID *uuid.UUID `json:"role_id"` } type adminCreateTesterResponse struct { UserID uuid.UUID `json:"user_id"` Email string `json:"email"` OrgID uuid.UUID `json:"organization_id"` + // Joined reports whether the tester landed in an existing workspace rather + // than one made for it, so the panel can say which and the operator is not + // left guessing what they just handed out. + Joined bool `json:"joined_existing"` // Password is returned once and never stored in a readable form. Losing it // means making another tester, which is cheap. Password string `json:"password"` @@ -83,6 +96,11 @@ func (h *Handler) AdminCreateTester(c *gin.Context) { return } + if req.OrgID != nil && req.RoleID == nil { + errx.JSON(c, errx.New(errx.BadRequest, "joining an existing workspace needs a role, so the access granted is one somebody chose")) + return + } + if existing, lerr := h.UserRepo.GetUserByEmail(c.Request.Context(), parsed.Address); lerr == nil && existing != nil { errx.JSON(c, errx.New(errx.BadRequest, "an account with that address already exists")) return @@ -109,41 +127,77 @@ func (h *Handler) AdminCreateTester(c *gin.Context) { return } - orgName := strings.TrimSpace(req.OrgName) - if orgName == "" { - orgName = "Tester workspace" - } - org, oerr := h.OrganizationService.Create(c.Request.Context(), created.ID, orgName) - if oerr != nil { - // Undo the account rather than leave the address taken by something - // unusable: revoking would not free it, and a retry would fail on the - // existing-email check, so the operator would have nowhere to go. - if derr := h.UserRepo.DeleteOrphanExemptUser(c.Request.Context(), created.ID); derr != nil { - errx.JSON(c, errx.New(errx.Internal, - "the workspace could not be created and the half-made account could not be removed; it is listed under Testers")) + // A tester joined to an existing workspace gets no workspace of its own on + // purpose. Owning one would leave the reviewer a member of two, and the + // dashboard only skips its workspace picker when there is exactly one, so + // the first thing they would meet is a chooser naming an empty workspace. + var orgID uuid.UUID + joined := req.OrgID != nil + if joined { + member, merr := h.OrganizationService.AttachTester(c.Request.Context(), *req.OrgID, created.ID, *adminID, *req.RoleID) + if merr != nil { + h.undoHalfMadeTester(c, created.ID, merr) return } - errx.JSON(c, errx.New(errx.Internal, "could not create the workspace, so nothing was created. Try again.")) - return - } - if h.TrialService != nil { - // Best effort: without it the workspace has no subscription row and - // reads as unpaid, which is recoverable from the admin panel. - _ = h.TrialService.StartFreeTrialWithOrg(c.Request.Context(), created.ID, org.ID) + orgID = member.OrganizationID + } else { + orgName := strings.TrimSpace(req.OrgName) + if orgName == "" { + orgName = "Tester workspace" + } + org, oerr := h.OrganizationService.Create(c.Request.Context(), created.ID, orgName) + if oerr != nil { + h.undoHalfMadeTester(c, created.ID, oerr) + return + } + orgID = org.ID + if h.TrialService != nil { + // Best effort: without it the workspace has no subscription row and + // reads as unpaid, which is recoverable from the admin panel. + _ = h.TrialService.StartFreeTrialWithOrg(c.Request.Context(), created.ID, org.ID) + } } - h.logTesterAction(c, *adminID, created.ID, "create_tester", map[string]any{ - "email": created.Email, "reason": reason, "organization_id": org.ID.String(), - }) + entry := map[string]any{ + "email": created.Email, "reason": reason, "organization_id": orgID.String(), + "joined_existing": joined, + } + if joined { + // The role is the whole of what this tester can reach, so it belongs in + // the audit row rather than only in the members table it can be + // changed out of later. + entry["role_id"] = req.RoleID.String() + } + h.logTesterAction(c, *adminID, created.ID, "create_tester", entry) c.JSON(http.StatusOK, adminCreateTesterResponse{ UserID: created.ID, Email: created.Email, - OrgID: org.ID, + OrgID: orgID, + Joined: joined, Password: password, }) } +// undoHalfMadeTester removes an account whose workspace step failed. Leaving it +// would take the address without giving anybody anything: revoking the +// exemption does not free the address, and a retry fails the existing-email +// check, so the operator would have nowhere to go. The delete is guarded on the +// account holding no membership, so it cannot remove a tester that did join. +// +// The original error is what the operator sees, because a seat limit and a role +// that does not exist are both things they can fix and neither is a 500. Only a +// cleanup that itself fails changes the answer, since that is the one case +// where something was left behind. +func (h *Handler) undoHalfMadeTester(c *gin.Context, userID uuid.UUID, cause *errx.Error) { + if derr := h.UserRepo.DeleteOrphanExemptUser(c.Request.Context(), userID); derr != nil { + errx.JSON(c, errx.New(errx.Internal, + cause.Message+"; the half-made account could not be removed either, and it is listed under Testers")) + return + } + errx.JSON(c, cause) +} + // AdminListTesters returns every account holding a login-code exemption, which // is the set an operator needs to review and prune. func (h *Handler) AdminListTesters(c *gin.Context) { diff --git a/internal/api/routes.go b/internal/api/routes.go index 89a88346d..280c7304e 100644 --- a/internal/api/routes.go +++ b/internal/api/routes.go @@ -1470,6 +1470,7 @@ func Run( adminRoutes.GET("/organizations", middleware.RequireAdminPermission(models.AdminPermViewOrganizations), h.AdminListOrganizations) adminRoutes.GET("/organizations/:id", middleware.RequireAdminPermission(models.AdminPermViewOrganizations), h.AdminGetOrganization) adminRoutes.GET("/organizations/:id/members", middleware.RequireAdminPermission(models.AdminPermViewOrganizations), h.AdminGetOrganizationMembers) + adminRoutes.GET("/organizations/:id/roles", middleware.RequireAdminPermission(models.AdminPermViewOrganizations), h.AdminGetOrganizationRoles) adminRoutes.GET("/organizations/:id/overrides", middleware.RequireAdminPermission(models.AdminPermViewOrganizations), h.AdminGetOrgOverrides) adminRoutes.PUT("/organizations/:id/overrides", middleware.RequireAdminPermission(models.AdminPermManageOrganizations), h.AdminUpdateOrgOverrides) // A plan granted by an operator rather than Stripe. Same permission as diff --git a/internal/app/organization/attach_tester_test.go b/internal/app/organization/attach_tester_test.go new file mode 100644 index 000000000..b4eb65f79 --- /dev/null +++ b/internal/app/organization/attach_tester_test.go @@ -0,0 +1,103 @@ +package organization + +import ( + "context" + "testing" + + "github.com/google/uuid" + "github.com/warmbly/warmbly/internal/errx" + "github.com/warmbly/warmbly/internal/models" + "github.com/warmbly/warmbly/internal/repository" +) + +// Only the reads AttachTester makes before it writes are implemented; anything +// else panics, which is what keeps this honest about the path it covers. +type attachRepo struct { + repository.OrganizationRepository + org *models.Organization + member *models.OrganizationMember + role *models.OrganizationRole + added int +} + +func (r *attachRepo) GetByID(context.Context, uuid.UUID) (*models.Organization, error) { + return r.org, nil +} + +func (r *attachRepo) GetMember(context.Context, uuid.UUID, uuid.UUID) (*models.OrganizationMember, error) { + return r.member, nil +} + +func (r *attachRepo) GetRoleByID(context.Context, uuid.UUID, uuid.UUID) (*models.OrganizationRole, error) { + return r.role, nil +} + +func (r *attachRepo) AddMemberWithRoles(_ context.Context, m *models.OrganizationMember, _ []uuid.UUID) error { + r.added++ + r.member = m + return nil +} + +func attachSvc(r *attachRepo) *organizationService { + return &organizationService{orgRepo: r} +} + +// A workspace that does not exist must not silently become one, because the +// caller has already made the account by this point. +func TestAttachTesterRefusesAMissingWorkspace(t *testing.T) { + r := &attachRepo{org: nil} + _, xerr := attachSvc(r).AttachTester(context.Background(), uuid.New(), uuid.New(), uuid.New(), uuid.New()) + if xerr == nil { + t.Fatal("a missing workspace was accepted") + } + if xerr.Code != errx.NotFound { + t.Fatalf("code = %v, want NotFound", xerr.Code) + } + if r.added != 0 { + t.Fatalf("wrote %d memberships for a workspace that does not exist", r.added) + } +} + +// The role is the whole of what a tester can reach. A role that does not +// resolve must fail rather than fall back to one, and must not reach the write. +func TestAttachTesterRefusesAnUnknownRole(t *testing.T) { + r := &attachRepo{org: &models.Organization{ID: uuid.New()}, role: nil} + _, xerr := attachSvc(r).AttachTester(context.Background(), uuid.New(), uuid.New(), uuid.New(), uuid.New()) + if xerr == nil { + t.Fatal("an unknown role was accepted") + } + if xerr.Code != errx.BadRequest { + t.Fatalf("code = %v, want BadRequest", xerr.Code) + } + if r.added != 0 { + t.Fatalf("wrote %d memberships for a role that does not exist", r.added) + } +} + +// Repeating the call must not re-role an existing member. A tester left on a +// narrow role should stay on it, and a second call is how that would quietly +// widen. +func TestAttachTesterLeavesAnExistingMembershipAlone(t *testing.T) { + orgID, userID := uuid.New(), uuid.New() + existing := &models.OrganizationMember{ + OrganizationID: orgID, + UserID: userID, + Role: "Viewer", + Permissions: models.PermViewCampaigns, + } + r := &attachRepo{ + org: &models.Organization{ID: orgID}, + member: existing, + role: &models.OrganizationRole{ID: uuid.New(), Name: "Admin", Permissions: models.AllPermissions}, + } + got, xerr := attachSvc(r).AttachTester(context.Background(), orgID, userID, uuid.New(), r.role.ID) + if xerr != nil { + t.Fatalf("unexpected error: %v", xerr) + } + if r.added != 0 { + t.Fatalf("wrote %d memberships for a user who was already a member", r.added) + } + if got.Role != "Viewer" || got.Permissions != models.PermViewCampaigns { + t.Fatalf("membership was widened to %s/%d", got.Role, got.Permissions) + } +} diff --git a/internal/app/organization/service.go b/internal/app/organization/service.go index 960e2126d..601bb5265 100644 --- a/internal/app/organization/service.go +++ b/internal/app/organization/service.go @@ -69,6 +69,11 @@ type OrganizationService interface { GetMembers(ctx context.Context, orgID uuid.UUID) ([]models.OrganizationMember, *errx.Error) GetMembership(ctx context.Context, orgID, userID uuid.UUID) (*models.OrganizationMember, *errx.Error) InviteMember(ctx context.Context, orgID uuid.UUID, inviterID uuid.UUID, req *models.InviteMemberRequest) (*models.OrganizationInvitation, *errx.Error) + // AttachTester joins a tester account to an existing workspace without an + // invitation. Operator-only: the admin is not a member, so there is no + // actor permission to check against, and the authority is the + // manage_testers bit plus the admin audit row the caller writes. + AttachTester(ctx context.Context, orgID, userID, adminID, roleID uuid.UUID) (*models.OrganizationMember, *errx.Error) AcceptInvitation(ctx context.Context, token string, userID uuid.UUID, email string) (*models.OrganizationMember, *errx.Error) AcceptInvitationByID(ctx context.Context, invitationID, userID uuid.UUID, email string) (*models.OrganizationMember, *errx.Error) PreviewInvitation(ctx context.Context, token string) (*models.InvitationPreview, *errx.Error) @@ -448,6 +453,78 @@ func (s *organizationService) GetUserDefaultOrganization(ctx context.Context, us return org, nil } +// AttachTester joins a user to an existing workspace directly, bypassing the +// invitation round trip. It exists for one case: a reviewer who has to see a +// real workspace and cannot read this instance's mail, so neither the invite +// email nor the accept-as-the-invited-address check can be satisfied. +// +// It deliberately does NOT take the escalation check InviteMember applies. That +// check asks whether the actor holds every permission they are handing out, and +// an operator is not a member of the workspace at all, so there is nothing to +// compare against. What stands in for it is the manage_testers permission bit +// on the route and the admin audit row the handler writes. +// +// Idempotent: an existing membership is returned untouched rather than +// re-roled, so a repeated call cannot quietly widen what a tester can reach. +func (s *organizationService) AttachTester(ctx context.Context, orgID, userID, adminID, roleID uuid.UUID) (*models.OrganizationMember, *errx.Error) { + if _, xerr := s.Get(ctx, orgID); xerr != nil { + return nil, xerr + } + + // Before the seat check, so a repeat call cannot be refused for a seat it + // already holds. + if existing, err := s.orgRepo.GetMember(ctx, orgID, userID); err != nil { + errs.CaptureException(err) + return nil, errx.New(errx.Internal, "failed to read the membership") + } else if existing != nil { + return existing, nil + } + + role, err := s.orgRepo.GetRoleByID(ctx, orgID, roleID) + if err != nil { + errs.CaptureException(err) + return nil, errx.New(errx.Internal, "failed to load the role") + } + if role == nil { + return nil, errx.New(errx.BadRequest, "that role does not exist in this workspace") + } + + // Last gate before the write, and the workspace's own: a tester occupies a + // seat like anyone else, so exceeding it silently would bill wrong and read + // as a bug later. + canAdd, xerr := s.CanAddMember(ctx, orgID) + if xerr != nil { + return nil, xerr + } + if !canAdd { + return nil, errx.New(errx.Forbidden, "that workspace is at its team member limit") + } + + now := time.Now() + member := &models.OrganizationMember{ + ID: uuid.New(), + OrganizationID: orgID, + UserID: userID, + Role: role.Name, + RoleID: &role.ID, + Permissions: role.Permissions, + // Recorded as invited by the operator who made the tester, so the + // members list names somebody rather than showing a member nobody added. + InvitedBy: &adminID, + InvitedAt: now, + AcceptedAt: &now, + } + if err := s.orgRepo.AddMemberWithRoles(ctx, member, []uuid.UUID{role.ID}); err != nil { + errs.CaptureException(err) + return nil, errx.New(errx.Internal, "failed to add the tester to the workspace") + } + + if updated, _ := s.orgRepo.GetMember(ctx, orgID, userID); updated != nil { + return updated, nil + } + return member, nil +} + // GetMembers retrieves all members of an organization func (s *organizationService) GetMembers(ctx context.Context, orgID uuid.UUID) ([]models.OrganizationMember, *errx.Error) { members, err := s.orgRepo.GetMembers(ctx, orgID)