From b6ea87ac197bd3d4c1bd3fca4db1fcbf8a9a11cc Mon Sep 17 00:00:00 2001 From: Kristian Wind Date: Mon, 17 Aug 2026 22:14:33 +0200 Subject: [PATCH] feat(projects): change a member's role from the member list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An invite sent with the wrong role could not be corrected. The only way back was to remove the person and invite them again — and removing a member unassigns every task they were responsible for, so fixing a dropdown cost somebody their work. This is the other side of "an external user cannot create tasks": the rule was right, the role was wrong, and there was no way to say so. PATCH against a membership that already exists, not the upsert AddMember does. Somebody who is not a member is not silently made one: that is an invite, it belongs to the invite flow, and doing it here would let a typo in a user id add a stranger to a project without anybody being asked. The owner is refused, with the real reason rather than the true but useless one. Ownership lives on the project rather than in project_members, so the plain answer would have been "not a member" — which is correct and tells nobody anything. Ownership is transferred, not granted, and two owners is a state with no way to settle a disagreement about who may remove whom. In the list, the owner's standing is text and everybody else's is a dropdown. A disabled control would be a thing that exists to say no. And the roles are finally in Danish. The API's words are `owner`, `editor` and `viewer`, and they were being printed straight into a Danish page; the list now uses the same three phrases the invite form beside it already did. The smoke test drives both browsers, because the interesting part is that the other one's permissions really change: a viewer is told the project is read-only and has no quick-add box, the owner promotes them from the list, and after a reload the box is there and the notice is gone. Co-Authored-By: Claude Opus 5 --- docs/openapi.yaml | 37 ++++++++++ internal/httpapi/projects_handlers.go | 47 +++++++++++++ internal/httpapi/projects_test.go | 89 ++++++++++++++++++++++++ internal/httpapi/server.go | 1 + internal/store/projects.go | 27 +++++++ web/e2e/smoke.spec.js | 57 +++++++++++++++ web/src/lib/api.js | 2 + web/src/routes/projekt/[id]/+page.svelte | 62 ++++++++++++++++- 8 files changed, 321 insertions(+), 1 deletion(-) diff --git a/docs/openapi.yaml b/docs/openapi.yaml index 935de8c..23ecaa1 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -1211,9 +1211,46 @@ paths: parameters: - { name: projectID, in: path, required: true, schema: { type: string } } - { name: userID, in: path, required: true, schema: { type: string } } + patch: + tags: [projects] + summary: Change what an existing member may do. The project's owner only. + description: | + Editor or viewer. An UPDATE against a membership that already exists — + somebody who is not a member is not silently made one, because that is an + invite and belongs to the invite flow. + + Not the owner: ownership is transferred, not granted, and two owners is a + state with no way to settle a disagreement about who may remove whom. The + owner also has no membership row at all — ownership lives on the project — + so this answers 409 with the real reason rather than "not a member". + + Correcting a role used to mean removing the person and inviting them + again, which also unassigns every task they were responsible for. + requestBody: + required: true + content: + application/json: + schema: + type: object + required: [role] + properties: + role: { type: string, enum: [editor, viewer] } + responses: + "204": { description: Changed. } + "404": { $ref: "#/components/responses/NotFound" } + "409": + description: The owner's standing cannot be changed. + content: + application/json: + schema: { $ref: "#/components/schemas/Error" } + "422": { $ref: "#/components/responses/Validation" } delete: tags: [projects] summary: Remove somebody from a project. Its owner only. + description: | + Their tasks are unassigned rather than left pointing at somebody who can + no longer see the project — which is why changing a role is a PATCH and + not a remove-and-reinvite. responses: "204": { description: Removed. } "404": { $ref: "#/components/responses/NotFound" } diff --git a/internal/httpapi/projects_handlers.go b/internal/httpapi/projects_handlers.go index aeac10c..cf89c9f 100644 --- a/internal/httpapi/projects_handlers.go +++ b/internal/httpapi/projects_handlers.go @@ -502,6 +502,53 @@ func (s *Server) handleInvite(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusCreated, inviteResponse{Link: link, Emailed: emailed}) } +type memberRoleRequest struct { + Role string `json:"role"` +} + +// handleSetMemberRole changes an existing member between editor and viewer. +// +// Until now the only way to correct an invite sent with the wrong role was to +// remove the person and invite them again — which also unassigns everything they +// were responsible for, because removing a member clears their tasks' assignee. +// Fixing a dropdown should not cost somebody their work. +func (s *Server) handleSetMemberRole(w http.ResponseWriter, r *http.Request) { + var req memberRoleRequest + if err := decodeJSON(w, r, &req); err != nil { + return + } + projectID := chi.URLParam(r, "projectID") + userID := chi.URLParam(r, "userID") + + role := store.Role(req.Role) + if !role.Valid() || role == store.RoleOwner { + writeFieldErrors(w, map[string]string{"role": "must be editor or viewer"}) + return + } + + // The owner has no project_members row at all — ownership lives on the project + // — so this would report "not a member", which is true and completely + // unhelpful. Say the real reason instead. + owner, err := s.db.ProjectOwner(r.Context(), projectID) + if err != nil { + s.storeError(w, "project owner", err) + return + } + if userID == owner { + writeError(w, http.StatusConflict, CodeConflict, + "the owner's standing cannot be changed; ownership is transferred, not granted") + return + } + + if err := s.db.SetMemberRole(r.Context(), projectID, userID, role); err != nil { + s.storeError(w, "set member role", err) + return + } + s.activity(r, projectID, "", "member.role_changed", + map[string]any{"user_id": userID, "role": string(role)}) + w.WriteHeader(http.StatusNoContent) +} + func (s *Server) handleRemoveMember(w http.ResponseWriter, r *http.Request) { projectID := chi.URLParam(r, "projectID") userID := chi.URLParam(r, "userID") diff --git a/internal/httpapi/projects_test.go b/internal/httpapi/projects_test.go index 84b8143..fc0c408 100644 --- a/internal/httpapi/projects_test.go +++ b/internal/httpapi/projects_test.go @@ -194,3 +194,92 @@ func TestListMembersWorksForTheInbox(t *testing.T) { t.Fatalf("status %d, body %v", resp.StatusCode, body) } } + +// A role sent by mistake used to be uncorrectable: the only way back was to remove +// the person and invite them again, which unassigns every task they were +// responsible for. Fixing a dropdown should not cost somebody their work. +func TestAMembersRoleCanBeChangedWithoutRemovingThem(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + member := ts.newUser(t, "anden@example.dk", "Anden") + + memberUser, err := ts.db.UserByEmail(t.Context(), "anden@example.dk") + if err != nil { + t.Fatal(err) + } + + _, project := ts.do(t, "POST", "/api/v1/projects", map[string]any{"name": "Delt"}) + projectID := project["id"].(string) + ts.do(t, "POST", "/api/v1/projects/"+projectID+"/invites", map[string]any{ + "email": "anden@example.dk", "role": "viewer", + }) + + // A viewer cannot write. That is the rule, not the bug. + if resp, _ := member.do(t, "POST", "/api/v1/tasks", + map[string]any{"content": "prøv", "project_id": projectID}); resp.StatusCode != http.StatusNotFound { + t.Fatalf("a viewer created a task: status %d", resp.StatusCode) + } + + resp, _ := ts.do(t, "PATCH", "/api/v1/projects/"+projectID+"/members/"+memberUser.ID, + map[string]any{"role": "editor"}) + if resp.StatusCode != http.StatusNoContent { + t.Fatalf("promote to editor: status %d", resp.StatusCode) + } + + if resp, _ := member.do(t, "POST", "/api/v1/tasks", + map[string]any{"content": "nu kan jeg", "project_id": projectID}); resp.StatusCode != http.StatusCreated { + t.Errorf("an editor could not create a task: status %d", resp.StatusCode) + } + _, members := ts.do(t, "GET", "/api/v1/projects/"+projectID+"/members", nil) + for _, raw := range members["members"].([]any) { + if m := raw.(map[string]any); m["user_id"] == memberUser.ID && m["role"] != "editor" { + t.Errorf("role = %v after the change, want editor", m["role"]) + } + } +} + +// The owner is not a member row, and ownership is not a role you can hand out. +func TestTheOwnersStandingCannotBeChanged(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + member := ts.newUser(t, "anden@example.dk", "Anden") + + owner, err := ts.db.UserByEmail(t.Context(), "kristian@example.dk") + if err != nil { + t.Fatal(err) + } + memberUser, err := ts.db.UserByEmail(t.Context(), "anden@example.dk") + if err != nil { + t.Fatal(err) + } + + _, project := ts.do(t, "POST", "/api/v1/projects", map[string]any{"name": "Delt"}) + projectID := project["id"].(string) + ts.do(t, "POST", "/api/v1/projects/"+projectID+"/invites", map[string]any{ + "email": "anden@example.dk", "role": "editor", + }) + + // Demoting the owner would leave a project nobody can administer. + if resp, _ := ts.do(t, "PATCH", "/api/v1/projects/"+projectID+"/members/"+owner.ID, + map[string]any{"role": "viewer"}); resp.StatusCode != http.StatusConflict { + t.Errorf("changing the owner's role: status %d, want 409", resp.StatusCode) + } + // And no member may promote themselves to owner. + if resp, _ := ts.do(t, "PATCH", "/api/v1/projects/"+projectID+"/members/"+memberUser.ID, + map[string]any{"role": "owner"}); resp.StatusCode != http.StatusUnprocessableEntity { + t.Errorf("granting ownership: status %d, want 422", resp.StatusCode) + } + // Nor may an editor change anybody's role: this is the owner's to do. + if resp, _ := member.do(t, "PATCH", "/api/v1/projects/"+projectID+"/members/"+memberUser.ID, + map[string]any{"role": "viewer"}); resp.StatusCode != http.StatusNotFound { + t.Errorf("an editor changed a role: status %d, want 404", resp.StatusCode) + } + // Somebody with no membership at all is not quietly given one. + stranger := ts.newUser(t, "tredje@example.dk", "Tredje") + _ = stranger + strangerUser, _ := ts.db.UserByEmail(t.Context(), "tredje@example.dk") + if resp, _ := ts.do(t, "PATCH", "/api/v1/projects/"+projectID+"/members/"+strangerUser.ID, + map[string]any{"role": "editor"}); resp.StatusCode != http.StatusNotFound { + t.Errorf("a non-member was given a role: status %d, want 404", resp.StatusCode) + } +} diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index f8d1bf2..850ee56 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -292,6 +292,7 @@ func New(cfg *config.Config, db *store.DB, log *slog.Logger, web fs.FS) *Server r.Patch("/", s.handleUpdateProject) r.Delete("/", s.handleDeleteProject) r.Post("/invites", s.handleInvite) + r.Patch("/members/{userID}", s.handleSetMemberRole) r.Delete("/members/{userID}", s.handleRemoveMember) }) }) diff --git a/internal/store/projects.go b/internal/store/projects.go index fbbd1e8..095a950 100644 --- a/internal/store/projects.go +++ b/internal/store/projects.go @@ -389,6 +389,33 @@ func (db *DB) AddMember(ctx context.Context, projectID, userID string, role Role return err } +// SetMemberRole changes what an existing member may do. +// +// An UPDATE rather than the upsert AddMember does, and the difference is the +// point: this is a PATCH against a membership that exists. Somebody who is not a +// member is not silently made one — that is an invite, it belongs to the invite +// flow, and doing it here would let a typo in a user id add a stranger to a +// project without anybody being asked. +// +// Owner is refused for the same reason AddMember refuses it: ownership is +// transferred, not granted, and two owners is a state with no way to settle a +// disagreement about who may remove whom. +func (db *DB) SetMemberRole(ctx context.Context, projectID, userID string, role Role) error { + if !role.Valid() || role == RoleOwner { + return errors.New("store: members may be editor or viewer") + } + res, err := db.ExecContext(ctx, + `UPDATE project_members SET role = ? WHERE project_id = ? AND user_id = ?`, + string(role), projectID, userID) + if err != nil { + return err + } + if n, _ := res.RowsAffected(); n == 0 { + return ErrNotFound + } + return nil +} + func (db *DB) RemoveMember(ctx context.Context, projectID, userID string) error { return db.Tx(ctx, func(tx *sql.Tx) error { // Anything assigned to them is unassigned rather than left pointing at diff --git a/web/e2e/smoke.spec.js b/web/e2e/smoke.spec.js index e57994c..73ac68d 100644 --- a/web/e2e/smoke.spec.js +++ b/web/e2e/smoke.spec.js @@ -705,6 +705,63 @@ test('den sidste administrator kan hverken fjernes eller slette sig selv', async await expect(mine.getByRole('button')).toHaveCount(0); }); +/** + * A member's role can be corrected without removing them. + * + * This is what a viewer who cannot add tasks looks like from the other side. The + * rule itself is right — a viewer may not write — but until now the only way to + * fix an invite sent with the wrong role was to remove the person and invite them + * again, which unassigns every task they were responsible for on the way past. + * + * Driven end to end because the interesting part is that the *other* browser's + * permissions actually change: the server decides, and the page has to agree. + */ +test('en rolle kan rettes uden at fjerne personen', async ({ browser, page }) => { + const trouble = watchForTrouble(page); + await page.goto('/'); + + const sidebar = page.getByRole('navigation', { name: 'Hovedmenu' }); + await sidebar.getByRole('button', { name: 'Nyt projekt' }).click(); + await sidebar.getByLabel('Projektnavn').fill('Fælles'); + await sidebar.getByLabel('Projektnavn').press('Enter'); + + await page.getByRole('button', { name: 'Del' }).click(); + await page.getByLabel('Inviter via e-mail').fill('andreas@example.dk'); + await page.getByLabel('Rolle').selectOption('viewer'); + await page.getByRole('button', { name: 'Inviter' }).click(); + const link = await page.locator('.link-out code').textContent(); + + const context = await browser.newContext({ storageState: { cookies: [], origins: [] } }); + const andreas = await context.newPage(); + await andreas.goto(link); + await andreas.getByLabel('Navn').fill('andreas'); + await andreas.getByLabel(/Adgangskode/).fill('et langt kodeord til test'); + await andreas.getByRole('button', { name: 'Opret konto' }).click(); + await expect(andreas.getByRole('navigation', { name: 'Hovedmenu' })).toBeVisible(); + + await andreas + .getByRole('navigation', { name: 'Hovedmenu' }) + .getByRole('link', { name: /Fælles/ }) + .click(); + // A viewer sees the project and is told, plainly, that it is read-only. + await expect(andreas.getByText('Du kan se dette projekt, men ikke ændre det')).toBeVisible(); + await expect(andreas.getByLabel('Ny opgave')).toHaveCount(0); + + // The owner promotes them from the member list. The owner's own row is text, + // not a dropdown: ownership is transferred, not granted. + await page.reload(); + await page.getByRole('button', { name: /Delt/ }).click(); + await expect(page.getByText('Ejer')).toBeVisible(); + await page.getByLabel('Rolle for andreas').selectOption('editor'); + + await andreas.reload(); + await expect(andreas.getByLabel('Ny opgave')).toBeVisible(); + await expect(andreas.getByText('Du kan se dette projekt')).toHaveCount(0); + await context.close(); + + expect(trouble).toEqual([]); +}); + /** * The MCP connector address must not fall through to the app shell. * diff --git a/web/src/lib/api.js b/web/src/lib/api.js index 4c0f1d2..5f8ca70 100644 --- a/web/src/lib/api.js +++ b/web/src/lib/api.js @@ -205,6 +205,8 @@ export const api = { listMembers: (projectId) => get(`/projects/${projectId}/members`), invite: (projectId, email, role) => post(`/projects/${projectId}/invites`, { email, role }), + setMemberRole: (projectId, userId, role) => + patch(`/projects/${projectId}/members/${userId}`, { role }), removeMember: (projectId, userId) => del(`/projects/${projectId}/members/${userId}`), activity: (projectId) => get(`/projects/${projectId}/activity`), diff --git a/web/src/routes/projekt/[id]/+page.svelte b/web/src/routes/projekt/[id]/+page.svelte index 91234e3..c2b9a53 100644 --- a/web/src/routes/projekt/[id]/+page.svelte +++ b/web/src/routes/projekt/[id]/+page.svelte @@ -245,6 +245,33 @@ } } + /** + * What each standing is called in the interface. + * + * The API's words are `owner`, `editor` and `viewer`, and they were being + * printed straight into a Danish page. These are the same three words the + * invite form already uses, so the list and the form agree. + */ + const ROLES = { owner: 'Ejer', editor: 'Kan redigere', viewer: 'Kan kun se' }; + + /** + * Changes what somebody may do, without removing them. + * + * Removing and re-inviting was the only way to correct a role, and it + * unassigns every task they were responsible for on the way past — so fixing a + * dropdown cost somebody their work. + */ + async function setMemberRole(member, role) { + const previous = members; + members = members.map((m) => (m.user_id === member.user_id ? { ...m, role } : m)); + try { + await api.setMemberRole(project.id, member.user_id, role); + } catch (e) { + members = previous; + app.toast(humanMessage(e)); + } + } + async function invite(event) { event.preventDefault(); inviteError = ''; @@ -360,7 +387,22 @@ {member.name[0]?.toUpperCase()} {member.name} - {member.role} + + {#if isOwner && member.role !== 'owner'} + + {:else} + {ROLES[member.role] ?? member.role} + {/if} {#if isOwner && member.role !== 'owner'}