fix: clear a stale organisation_role when a user's organisation changes in the admin

The members relation manager and the organisation field of the Users form
declare cabana ClearOnChange on organisation_role: adding or removing a
member, and a form save that moves a user to another organisation or to
none, clear the role unless the same save writes it. Needs a SummerCMS
release with ClearOnChange.

README.md

@@ -24,11 +24,11 @@ It owns the `users` table extensions, `user_throttle`, `jwt_blacklist`, `user_ap
 - **Rate limiting.** Every route runs behind the `user-api` bucket: 120 requests a minute per authenticated user, or per client IP for anonymous calls.
 - **Profile.** `POST /update`, `POST /marketing-consent`, `POST /avatar` and `POST /avatar/remove` (the avatar is stored through the framework's attachment storage).
 - **Deactivation and bans.** Deactivation is self-service and reversible: a password login or a `golem15.sociallogin` sign-in restores a deactivated account and sends the reactivate mail, and an administrator can restore it too. It ends every earlier session JWT and personal API token of the user (`RevokeUserAccess` stamps `tokens_valid_after` and revokes the `user_api_tokens` rows), so the user signs in again and gets a fresh token, while the old API tokens stay revoked. A ban is the lock: password login, social sign-in and `POST /refresh` refuse a banned user, and banning revokes the user's sessions and API tokens. Neither unban nor restore revives a token. Upgrade note: accounts deactivated or banned before this version keep their API tokens until they are deactivated or banned again (both actions are idempotent) or `RevokeUserAccess` runs for them.
-- **Organisations.** The `Organisation` model and the `organisation_id` and `organisation_role` user columns, as a foreign-key target for other plugins. An organisation has a name, a slug (filled from the name by `Slugify` when it is left empty), a description, an avatar and its members: the users whose `organisation_id` points at it.
+- **Organisations.** The `Organisation` model and the `organisation_id` and `organisation_role` user columns, as a foreign-key target for other plugins. An organisation has a name, a slug (filled from the name by `Slugify` when it is left empty), a description, an avatar and its members: the users whose `organisation_id` points at it. `organisation_role` belongs to that membership: the admin clears it whenever a user's organisation changes (see the Organisations screen), and an application that sets `organisation_id` itself sets or clears the role with it.
 - **User groups.** The `user_groups` table (seeded with the `guest` and `registered` groups, as in PHP) and the `users_groups` membership pivot, the `UserGroup` model and the `User.Groups` relation, which is never serialized. `UserGroupCodes` and `HasGroupCode` let other plugins check membership server-side, for example a site-admin check on the group code `admin`. No user API route reads or writes groups, and the user payload still answers `"groups":[]`. Membership is written in one place only: the `groups` field of the admin user form. Apart from that, rows leave `users_groups` only when a user or a group is deleted for good. The admin Users list can filter by group.
 - **Admin screens.** The plugin registers the Users screen with the framework's admin (`pact.HasAdminControllers`, controller ID `golem15.user.users`), the port of the WinterCMS `Users` controller: a list with search, the group, registration date and activated filters and row states (deactivated, banned, not activated); a read-only preview with a status hint; and a form with name, email, a password with confirmation, an optional invitation mail, the user's groups, the organisation, the avatar and the user's frontend permissions. Bulk actions activate, deactivate, restore, ban and unban users; the preview offers activate, unban and unsuspend while they apply. Deactivating is the soft delete, and deactivate and ban both end the user's earlier sessions and personal API tokens (see Deactivation and bans); the delete button and the bulk delete remove a user for good, together with the throttle rows, group memberships and attachments. A user created in the admin starts not activated. There is no impersonation and no guest account.
 - **User Groups screen.** Controller ID `golem15.user.usergroups`, the port of the WinterCMS `UserGroups` controller, behind `golem15.users.access_groups`: a list with the name, code, creation date and number of members, and a form with the name, the code (letters, digits, underscore and hyphen; unique), a description and the group's frontend permissions as checkboxes, stored in `user_groups.permissions` as a JSON object of code to `1`. A group can be deleted from its form; its memberships go with it and its members stay. The screen never changes who is in a group.
-- **Organisations screen.** Controller ID `golem15.user.organisations`, the port of the WinterCMS `Organisations` controller, behind `golem15.users.access_users`: a list and a form with the name, slug, description and avatar, and on an existing organisation the Members tab. Adding a member sets that user's `organisation_id` and removing one clears it; a user is never deleted. Only users without an organisation can be added, so a user moves by being removed from one organisation and added to the other (or through the organisation field of the user form). Deleting an organisation clears `organisation_id` and `organisation_role` of its members first.
+- **Organisations screen.** Controller ID `golem15.user.organisations`, the port of the WinterCMS `Organisations` controller, behind `golem15.users.access_users`: a list and a form with the name, slug, description and avatar, and on an existing organisation the Members tab. Adding a member sets that user's `organisation_id` and removing one clears it; both also clear the user's `organisation_role`, so a role never moves to another organisation or outlives the membership. A user is never deleted. Only users without an organisation can be added, so a user moves by being removed from one organisation and added to the other (or through the organisation field of the user form, where a save that changes the organisation, to another one or to none, clears `organisation_role` unless the same save writes it). Deleting an organisation clears `organisation_id` and `organisation_role` of its members first.
 - **Privileged groups.** The groups whose code is listed in `golem15.user.privileged_groups` (default `admin`) are privileged: an application may treat their members as site administrators. Three things therefore need the backend permission `golem15.users.manage_privileged_groups` on top of the screen's own permission, and answer 403 without it, saving nothing. First, membership: adding a privileged group to a user or removing it, on create and on update of the user form. An administrator without the permission sees those groups locked in the field and can still change the user's other groups. Second, the code: creating a group with a privileged code, changing a group's code to or from a privileged one, and deleting a privileged group. Its name, description and frontend permissions need only `golem15.users.access_groups`. Codes compare case-sensitively, and a group without a code is never privileged. Third, the member's account, which is protected against takeover: changing that user's email or password, and deleting them for good, needs the backend permission `golem15.users.manage_privileged_groups` on top of `golem15.users.access_users`. Without it the request answers 403 and nothing is saved; a bulk delete that contains such a user deletes none of the selected users. Everything else on such a user (name, organisation, avatar, frontend permissions, activate, deactivate, restore, ban, unban, unsuspend) needs only `golem15.users.access_users`. `PrivilegedGroupCodes`, `IsPrivilegedCode`, `PrivilegedGroupIDs` and `IsPrivilegedMember` expose the rule to other code.
 - **Frontend permissions.** `golem15_user_frontend_permissions` is the catalogue of permissions a site defines for its users (rows come from the application's migrations or seeds). A user's own choices are stored in `users.permissions` and a group's in `user_groups.permissions`, as a JSON object of code to `1` (allow) or `-1` (deny). `MergedPermissions` combines them as PHP `getMergedPermissions` does: among a user's groups the most positive value wins, then the user's own value replaces the groups'. `HasPermission` and `UserHasPermission` answer whether a code is held: only at exactly `1`, with Winter's `*` wildcards. Neither value appears in the user payload, whose `permissions` key stays `null`.
 - **Last seen.** `users.last_seen` is stamped by `POST /login` and a successful `POST /refresh` through `TouchLastSeen`, at most once per five minutes per user. A failed write is logged and never fails authentication. It is shown in the admin list and is not part of any user payload.
@@ -236,6 +236,7 @@ Each command uses the database the serve command published or, run from the cons
 ## Dependencies
 
 - The SummerCMS framework v0.4.0, `git.golem15.com/golem15/summercms` (modules `backpack`, `bonfire`, `bouncer`, `diary`, `pact`, `party`, `surf` and others).
+- The organisation role cleanup of the admin (members manager and user form) uses cabana's `ClearOnChange`, which needs SummerCMS v0.4.3 or later (the `go.mod` require moves to that tag once it is released).
 - gormigrate, GORM, golang-jwt and gocloud.dev, as pinned in `go.mod`.
 - testcontainers-go for the Postgres-backed tests.
 
@@ -254,4 +255,6 @@ go test ./...           # full run, starts Postgres with testcontainers (needs D
 
 `TestOffboarding` runs the admin deactivate and ban bulk actions and checks that the user's older session JWT and personal API token stop working, that a password login restores a deactivated account with a working token while the old credentials stay dead, and that unban revives nothing; `TestRefreshSubjectChecks` covers the `/refresh` subject checks; `TestRevokeUserAccess` (in `classes`) covers the revocation writes.
 
+`TestAdminOrganisationMembershipClearsRole` checks that the members manager's unlink and link and a Users form save that changes the organisation clear `organisation_role`, that a save keeping the organisation keeps it, and that deleting an organisation clears both columns.
+
 The admin tests (`TestAdmin*`) boot the plugin with the admin API mounted on a database of their own and sign in backend administrators with a chosen permission set; the helpers are in `admin_harness_test.go`.

admin_organisations_edge_test.go

@@ -162,13 +162,13 @@ func TestAdminOrganisationsEdge(t *testing.T) {
 	}
 }
 
-// TestAdminOrganisationRoleOutlivesMembership pins a known gap
-// (12.1-SECURITY-REVIEW.md, open item 6): removing a member through the
-// members manager clears organisation_id and nothing else, so the user keeps
-// organisation_role and carries it into the next organisation they are
-// added to. Deleting the organisation clears both. When the unlink learns to
-// clear the role, this test changes with it.
-func TestAdminOrganisationRoleOutlivesMembership(t *testing.T) {
+// TestAdminOrganisationMembershipClearsRole: a role belongs to one
+// organisation (12.1-SECURITY-REVIEW.md open item 6, closed). Removing a
+// member through the members manager clears organisation_id and
+// organisation_role, adding one clears a leftover role, and changing the
+// organisation in the Users form clears the role while a save that keeps the
+// organisation keeps it. Deleting the organisation clears both.
+func TestAdminOrganisationMembershipClearsRole(t *testing.T) {
 	env := newAdminEnv(t)
 	token := env.admin(t, permAccessUsers)
 	stamp := adminStamp()
@@ -182,18 +182,49 @@ func TestAdminOrganisationRoleOutlivesMembership(t *testing.T) {
 
 	env.expect(t, http.StatusOK, http.MethodPost, organisationPath(first.ID, "/relations/members/unlink"), token, ids)
 	got := env.reload(t, member.ID)
-	if got.OrganisationID != nil {
-		t.Fatalf("organisation_id after the unlink = %d", *got.OrganisationID)
-	}
-	if got.OrganisationRole == nil || *got.OrganisationRole != "owner" {
-		t.Fatalf("organisation_role after the unlink = %v; the known gap is closed, update this test and the review", got.OrganisationRole)
+	if got.OrganisationID != nil || got.OrganisationRole != nil {
+		t.Fatalf("after the unlink: organisation %v role %v, want both NULL", got.OrganisationID, got.OrganisationRole)
 	}
 	env.expect(t, http.StatusOK, http.MethodPost, organisationPath(second.ID, "/relations/members/link"), token, ids)
 	got = env.reload(t, member.ID)
-	if got.OrganisationID == nil || *got.OrganisationID != second.ID || got.OrganisationRole == nil || *got.OrganisationRole != "owner" {
+	if got.OrganisationID == nil || *got.OrganisationID != second.ID || got.OrganisationRole != nil {
 		t.Fatalf("after the link elsewhere: organisation %v role %v", got.OrganisationID, got.OrganisationRole)
 	}
 
+	// An organisation-less user with a leftover role loses it when added.
+	leftover := env.seedUser(t, "role-leftover-"+stamp, func(u *models.User) { u.OrganisationRole = &owner })
+	env.expect(t, http.StatusOK, http.MethodPost, organisationPath(first.ID, "/relations/members/link"), token, map[string]any{"ids": []uint{leftover.ID}})
+	if got := env.reload(t, leftover.ID); got.OrganisationID == nil || *got.OrganisationID != first.ID || got.OrganisationRole != nil {
+		t.Fatalf("linked leftover: organisation %v role %v", got.OrganisationID, got.OrganisationRole)
+	}
+
+	// The Users form: keeping the organisation keeps the role, changing it
+	// (to another organisation or to none) clears it.
+	formUser := env.seedUser(t, "role-form-"+stamp, func(u *models.User) { u.OrganisationID, u.OrganisationRole = &first.ID, &owner })
+	setRole := func(t *testing.T) {
+		t.Helper()
+		if err := env.gdb.Model(&models.User{}).Where("id = ?", formUser.ID).UpdateColumn("organisation_role", owner).Error; err != nil {
+			t.Fatal(err)
+		}
+	}
+	saveForm := func(t *testing.T, organisation any) *models.User {
+		t.Helper()
+		env.expect(t, http.StatusOK, http.MethodPut, userPath(formUser.ID, ""), token, map[string]any{
+			"name": "role-form", "email": formUser.Email, "organisation": organisation,
+		})
+		return env.reload(t, formUser.ID)
+	}
+	if got := saveForm(t, first.ID); got.OrganisationRole == nil || *got.OrganisationRole != owner {
+		t.Fatalf("a save that keeps the organisation dropped the role: %v", got.OrganisationRole)
+	}
+	if got := saveForm(t, second.ID); got.OrganisationID == nil || *got.OrganisationID != second.ID || got.OrganisationRole != nil {
+		t.Fatalf("after moving to another organisation: organisation %v role %v", got.OrganisationID, got.OrganisationRole)
+	}
+	setRole(t)
+	if got := saveForm(t, nil); got.OrganisationID != nil || got.OrganisationRole != nil {
+		t.Fatalf("after clearing the organisation: organisation %v role %v", got.OrganisationID, got.OrganisationRole)
+	}
+
 	// The delete of an organisation releases its members completely.
 	env.expect(t, http.StatusOK, http.MethodDelete, organisationPath(doomed.ID, ""), token, nil)
 	if got := env.reload(t, other.ID); got == nil || got.OrganisationID != nil || got.OrganisationRole != nil {

admin_organisations_test.go

@@ -475,6 +475,11 @@ func TestAdminOrganisationMembers(t *testing.T) {
 	if rec := mutate(there, "unlink", foreign); rec.Code != http.StatusOK {
 		t.Fatalf("unlink there = %d %s", rec.Code, rec.Body.String())
 	}
+	// The role belonged to the organisation left: the unlink cleared it.
+	if got := env.reload(t, foreign.ID); got.OrganisationRole != nil {
+		t.Fatalf("organisation_role after leaving = %q", *got.OrganisationRole)
+	}
+	stored[foreign.ID].OrganisationRole = nil
 	if rec := mutate(here, "link", foreign); rec.Code != http.StatusOK || organisationOf(foreign) != here.ID {
 		t.Fatalf("link after the release = %d %s", rec.Code, rec.Body.String())
 	}

controllers/organisations_admin_controller.go

@@ -23,8 +23,9 @@ const membersRelation = "members"
 // manager. The screen and every write need golem15.users.access_users, as in
 // PHP.
 //
-// The members manager sets and clears users.organisation_id and nothing
-// else. It never writes users_groups.
+// The members manager sets and clears users.organisation_id, and clears
+// users.organisation_role with it (a role belongs to one organisation). It
+// never writes users_groups.
 type organisationsAdminController struct {
 	app appAccess
 }
@@ -73,12 +74,15 @@ func (c organisationsAdminController) requireSlugFormat(ctx context.Context, mod
 // AdminRelationContracts declares the members relation manager: the users
 // whose organisation_id is the organisation. Linking sets the column and
 // unlinking clears it; the column is nullable, so a user is never deleted.
+// Both also clear organisation_role (ClearOnChange), so a role never moves
+// to another organisation or outlives the membership.
 func (organisationsAdminController) AdminRelationContracts() []cabana.RelationContract {
 	return []cabana.RelationContract{{
-		Name:       membersRelation,
-		Kind:       cabana.RelationHasMany,
-		NewRelated: func() any { return &models.User{} },
-		ForeignKey: "organisation_id",
+		Name:          membersRelation,
+		Kind:          cabana.RelationHasMany,
+		NewRelated:    func() any { return &models.User{} },
+		ForeignKey:    "organisation_id",
+		ClearOnChange: []string{"organisation_role"},
 		Columns: map[string]string{
 			"name":  "name",
 			"email": "email",

controllers/users_admin_controller.go

@@ -364,7 +364,9 @@ const groupsField = "groups"
 // AdminFieldRelations binds the organisation picker and the groups field.
 // organisation_id is a protected column, so the contract opts in to writing
 // it through this one field; the submitted id is still checked against the
-// options. groups is the users_groups pivot: a save that carries the field
+// options. A save that changes the organisation (to another one or to none)
+// clears organisation_role (ClearOnChange) unless the same save writes it.
+// groups is the users_groups pivot: a save that carries the field
 // replaces the user's memberships, and AdminRelationLocks decides which of
 // them the administrator may change.
 func (usersAdminController) AdminFieldRelations() []cabana.FieldRelationContract {
@@ -376,6 +378,7 @@ func (usersAdminController) AdminFieldRelations() []cabana.FieldRelationContract
 			ForeignKey:         "organisation_id",
 			LabelColumn:        "name",
 			WritableForeignKey: true,
+			ClearOnChange:      []string{"organisation_role"},
 		},
 		{
 			Field:             groupsField,