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'}