From a848aa17d6059ce45e8a689484c19c4e577609de Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Mon, 5 Oct 2026 15:23:23 +0200 Subject: [PATCH] test: edge cases of the Users, User Groups and Organisations admin controllers - each controller driven directly on records of another type, an application that is not booted, one without a database and one whose database fails: a guard that cannot read answers a plain error, never a pass - D-30: a superuser passes the takeover guard; an unreadable membership is the opaque 500 with nothing changed - unknown partials, undeclared filters and sorts, options of fields that have none, search and sort on the three lists - a name-only update of a privileged group, validation before the guard, a seeded group protected through the config list - known gaps pinned: boolean values in a group's permissions, organisation_role after an unlink --- admin_groups_edge_test.go | 259 +++++++++++++++++++++ admin_organisations_edge_test.go | 202 ++++++++++++++++ admin_users_edge_test.go | 382 ++++++++++++++++++++++++++++++- 3 files changed, 842 insertions(+), 1 deletion(-) create mode 100644 admin_groups_edge_test.go create mode 100644 admin_organisations_edge_test.go diff --git a/admin_groups_edge_test.go b/admin_groups_edge_test.go new file mode 100644 index 0000000..273fecb --- /dev/null +++ b/admin_groups_edge_test.go @@ -0,0 +1,259 @@ +package user + +import ( + "context" + "net/http" + "reflect" + "testing" + + "git.golem15.com/golem15/sm-user-plugin/models" + "git.golem15.com/golem15/summercms/modules/backpack" + "git.golem15.com/golem15/summercms/modules/bouncer" + "git.golem15.com/golem15/summercms/modules/cabana" + "git.golem15.com/golem15/summercms/modules/compass" + "git.golem15.com/golem15/summercms/modules/pact" +) + +// TestAdminGroupsControllerGuards drives the User Groups controller directly +// on the inputs the HTTP tests cannot produce. A guard that cannot read what +// it needs answers a plain error (the opaque 500), never a pass. +func TestAdminGroupsControllerGuards(t *testing.T) { + ctx := context.Background() + const id = "golem15.user.usergroups" + notGroup := &models.User{} + var nilGroup *models.UserGroup + admin, plain := "admin", "plain-code" + damaged := "{not json" + + broken, _ := brokenApp(t) + env := newAdminEnv(t) + sets := map[string]adminControllerSet{ + "no accessor": adminControllersOn(nil), + "not booted": adminControllersOn(func() *backpack.App { return nil }), + "no database": adminControllersOn(func() *backpack.App { return bareApp(t) }), + "a failing handle": adminControllersOn(func() *backpack.App { return broken }), + "the live database": adminControllersOn(func() *backpack.App { return env.app }), + } + for name, set := range sets { + t.Run(name, func(t *testing.T) { + ctl := set[id] + for _, record := range []any{notGroup, nilGroup, nil} { + wantPlainError(t, "FormBeforeCreate", ctl.(pact.FormBeforeCreate).FormBeforeCreate(ctx, record)) + wantPlainError(t, "FormBeforeUpdate", ctl.(pact.FormBeforeUpdate).FormBeforeUpdate(ctx, record)) + wantPlainError(t, "FormBeforeDelete", ctl.(pact.FormBeforeDelete).FormBeforeDelete(ctx, record)) + wantPlainError(t, "FormAfterDelete", ctl.(pact.FormAfterDelete).FormAfterDelete(ctx, record)) + editor := ctl.(cabana.PermissionEditorProvider) + _, err := editor.AdminPermissionValues(ctx, "permissions", record) + wantPlainError(t, "AdminPermissionValues", err) + wantPlainError(t, "AdminSetPermissionValues", editor.AdminSetPermissionValues(ctx, "permissions", record, nil)) + } + editor := ctl.(cabana.PermissionEditorProvider) + if _, err := editor.AdminPermissionOptions(ctx, "name"); err == nil { + t.Fatal("permission options for another field") + } + if _, err := editor.AdminPermissionValues(ctx, "name", &models.UserGroup{}); err == nil { + t.Fatal("permission values for another field") + } + if err := editor.AdminSetPermissionValues(ctx, "name", &models.UserGroup{}, nil); err == nil { + t.Fatal("permission write for another field") + } + // A stored value that is not a JSON object is an error, never an + // empty set an edit would then save over it. + _, err := editor.AdminPermissionValues(ctx, "permissions", &models.UserGroup{Permissions: &damaged}) + wantPlainError(t, "damaged stored permissions", err) + if values, err := editor.AdminPermissionValues(ctx, "permissions", &models.UserGroup{}); err != nil || len(values) != 0 { + t.Fatalf("no stored permissions = %v %v", values, err) + } + if ctl.(pact.ListExtendQuery).ListExtendQuery(ctx, nil) != nil { + t.Fatal("ListExtendQuery invented a handle") + } + wantPlainError(t, "FormAfterDelete without a transaction", ctl.(pact.FormAfterDelete).FormAfterDelete(ctx, &models.UserGroup{ID: 1})) + + // Without a principal the privileged code is refused on every + // path, with the screen's 403 (D-06). + for what, err := range map[string]error{ + "create": ctl.(pact.FormBeforeCreate).FormBeforeCreate(ctx, &models.UserGroup{Name: "Admins", Code: &admin}), + "delete": ctl.(pact.FormBeforeDelete).FormBeforeDelete(ctx, &models.UserGroup{ID: 1, Name: "Admins", Code: &admin}), + } { + forbidden, ok := err.(*cabana.ForbiddenError) + if !ok || forbidden.Message != "golem15.user::lang.groups.privileged_forbidden" || forbidden.Details["code"] == nil { + t.Fatalf("%s of a privileged group without a principal: %v", what, err) + } + } + // With the extra permission, and for a superuser, it passes. + for _, principal := range []*bouncer.Principal{ + {PermissionGrants: map[string]bool{permManagePrivileged: true}}, + {IsSuperuser: true}, + } { + as := bouncer.WithUser(ctx, principal) + if err := ctl.(pact.FormBeforeCreate).FormBeforeCreate(as, &models.UserGroup{Name: "Admins", Code: &admin}); err != nil { + t.Fatalf("create for %+v: %v", principal, err) + } + if err := ctl.(pact.FormBeforeDelete).FormBeforeDelete(as, &models.UserGroup{ID: 1, Code: &admin}); err != nil { + t.Fatalf("delete for %+v: %v", principal, err) + } + } + // An ordinary code and a group without a code need nothing extra. + if err := ctl.(pact.FormBeforeCreate).FormBeforeCreate(ctx, &models.UserGroup{Name: "Plain", Code: &plain}); err != nil { + t.Fatal(err) + } + if err := ctl.(pact.FormBeforeDelete).FormBeforeDelete(ctx, &models.UserGroup{ID: 1}); err != nil { + t.Fatal(err) + } + + // An update reads the stored code first: with no database, a + // failing one or an unknown group it is an error, not a pass. + wantPlainError(t, "update of an unreadable group", ctl.(pact.FormBeforeUpdate).FormBeforeUpdate(ctx, &models.UserGroup{ID: 987654321, Name: "Gone", Code: &plain})) + if name != "the live database" { + _, err := editor.AdminPermissionOptions(ctx, "permissions") + wantPlainError(t, "AdminPermissionOptions", err) + } + }) + } +} + +// TestAdminGroupsEdge covers the remaining branches of the User Groups +// screen through the admin API. +func TestAdminGroupsEdge(t *testing.T) { + stamp := adminStamp() + privilegedCode := "edgepriv-" + stamp + env := newAdminEnv(t, func(cfg *compass.Config) { + if err := cfg.Set("golem15.user.privileged_groups", []string{"admin", "guest", privilegedCode}); err != nil { + t.Fatal(err) + } + }) + token := env.admin(t, permAccessGroups) + manager := env.admin(t, permAccessGroups, permManagePrivileged) + + // A group update that keeps a privileged code needs nothing extra: the + // name and description change, the code stays. + privileged := env.seedGroup(t, "Edge privileged "+stamp, privilegedCode) + env.expect(t, http.StatusOK, http.MethodPut, groupPath(privileged.ID), token, map[string]any{"name": "Edge renamed " + stamp, "code": privilegedCode, "description": "kept"}) + if got := env.group(t, privileged.ID); got.Name != "Edge renamed "+stamp || got.Code == nil || *got.Code != privilegedCode || got.Description == nil || *got.Description != "kept" { + t.Fatalf("group after a name-only update = %+v", got) + } + // The same administrator may not move the code away from, or onto, a + // privileged one; the manager may. + ordinary := env.seedGroup(t, "Edge ordinary "+stamp, "edgeord-"+stamp) + rec := env.expect(t, http.StatusForbidden, http.MethodPut, groupPath(privileged.ID), token, map[string]any{"code": "edgefree-" + stamp}) + if message := adminDecode(t, rec).Error.Message; message != privilegedGroupCodeForbidden { + t.Fatalf("403 message = %q", message) + } + if got := env.group(t, privileged.ID); *got.Code != privilegedCode { + t.Fatalf("a refused re-code changed the code to %q", *got.Code) + } + env.expect(t, http.StatusOK, http.MethodPut, groupPath(ordinary.ID), manager, map[string]any{"name": "Edge ordinary " + stamp + " b"}) + + // Validation answers before the guard: a create that would be refused + // with 403 answers 422 when its body is also invalid, and saves nothing. + rec = env.expect(t, http.StatusUnprocessableEntity, http.MethodPost, adminAPI(adminGroupsPath), token, map[string]any{"name": "Edge duplicate " + stamp, "code": privilegedCode}) + if details := adminDecode(t, rec).Error.Details; len(details["code"]) == 0 { + t.Fatalf("422 details = %v", details) + } + var count int64 + if err := env.gdb.Raw(`SELECT COUNT(*) FROM user_groups WHERE code = ?`, privilegedCode).Scan(&count).Error; err != nil || count != 1 { + t.Fatalf("groups with the privileged code = %d err = %v", count, err) + } + + // A code with characters outside letters, digits, dash and underscore is + // a 422 on the field, in the request's language. + for _, code := range []string{"with space", "dot.ted", "sl/ash", "ąę"} { + rec := env.expect(t, http.StatusUnprocessableEntity, http.MethodPost, adminAPI(adminGroupsPath), token, map[string]any{"name": "Edge bad code " + stamp, "code": code}) + details := adminDecode(t, rec).Error.Details + if got := details["code"]; len(got) != 1 || got[0] != "The code may only contain letters, numbers, dashes and underscores." { + t.Fatalf("code %q: details = %v", code, details) + } + } + + // An application protects a seeded group by naming its code in + // golem15.user.privileged_groups: the guest group then cannot be deleted + // or re-coded with golem15.users.access_groups alone. + var guestID uint + if err := env.gdb.Raw(`SELECT id FROM user_groups WHERE code = 'guest' ORDER BY id LIMIT 1`).Scan(&guestID).Error; err != nil { + t.Fatal(err) + } + if guestID == 0 { + guestID = env.seedGroup(t, "Guest", "guest").ID + } + env.expect(t, http.StatusForbidden, http.MethodDelete, groupPath(guestID), token, nil) + env.expect(t, http.StatusForbidden, http.MethodPut, groupPath(guestID), token, map[string]any{"code": "visitor-" + stamp}) + if got := env.group(t, guestID); got == nil || got.Code == nil || *got.Code != "guest" { + t.Fatalf("the protected guest group = %+v", got) + } + + // Search and sort on the list. + for _, name := range []string{"b", "a", "c"} { + env.seedGroup(t, "Sort "+stamp+" "+name, "sort-"+stamp+"-"+name) + } + order := func(query string) []string { + var rows []map[string]any + adminInto(t, env.expect(t, http.StatusOK, http.MethodGet, adminAPI(adminGroupsPath)+"?search=Sort%20"+stamp+"&"+query, token, nil), &rows) + codes := []string{} + for _, row := range rows { + code, _ := row["code"].(string) + codes = append(codes, code[len(code)-1:]) + } + return codes + } + if got := order("sort=name&dir=asc"); !reflect.DeepEqual(got, []string{"a", "b", "c"}) { + t.Fatalf("name asc = %v", got) + } + if got := order("sort=code&dir=desc"); !reflect.DeepEqual(got, []string{"c", "b", "a"}) { + t.Fatalf("code desc = %v", got) + } + for _, query := range []string{"sort=permissions", "sort=description", "filter[code]=x"} { + env.expect(t, http.StatusUnprocessableEntity, http.MethodGet, adminAPI(adminGroupsPath)+"?"+query, token, nil) + } + + // A group whose stored permissions are not a JSON object cannot be + // opened: the answer is the opaque 500, and the row is left as it is. + broken := env.seedGroup(t, "Edge damaged "+stamp, "edgedmg-"+stamp) + if err := env.gdb.Exec(`UPDATE user_groups SET permissions = '{"a":' WHERE id = ?`, broken.ID).Error; err != nil { + t.Fatal(err) + } + rec = env.expect(t, http.StatusInternalServerError, http.MethodGet, groupPath(broken.ID), token, nil) + if message := adminDecode(t, rec).Error.Message; message != "Server error" { + t.Fatalf("500 message = %q", message) + } + if got := env.group(t, broken.ID); got.Permissions == nil || *got.Permissions != `{"a":` { + t.Fatalf("damaged permissions after the read = %v", got.Permissions) + } +} + +// TestAdminGroupsLegacyBooleanPermissions pins a known difference between +// the two readers of user_groups.permissions (12.1-SECURITY-REVIEW.md, open +// item 5): the API-token grants count a stored true as granted, the admin +// editor and the merged-permission resolver read only numbers. A group +// stored with booleans therefore shows unchecked, and an edit through the +// editor does not keep the boolean. The change is towards fewer grants, +// never more. +func TestAdminGroupsLegacyBooleanPermissions(t *testing.T) { + env := newAdminEnv(t) + token := env.admin(t, permAccessGroups) + stamp := adminStamp() + legacy, offered := "legacy.bool."+stamp, "offered.bool."+stamp + if err := env.gdb.Exec(`INSERT INTO golem15_user_frontend_permissions (code, label, tab) VALUES (?, 'Legacy', 'Edge'), (?, 'Offered', 'Edge')`, legacy, offered).Error; err != nil { + t.Fatal(err) + } + group := env.seedGroup(t, "Edge boolean "+stamp, "edgebool-"+stamp) + if err := env.gdb.Exec(`UPDATE user_groups SET permissions = ? WHERE id = ?`, `{"`+legacy+`":true}`, group.ID).Error; err != nil { + t.Fatal(err) + } + var record struct { + Permissions map[string]int `json:"permissions"` + } + adminInto(t, env.expect(t, http.StatusOK, http.MethodGet, groupPath(group.ID), token, nil), &record) + if len(record.Permissions) != 0 { + t.Fatalf("a boolean value is shown as %v", record.Permissions) + } + // A save that does not carry the field leaves the stored value alone. + env.expect(t, http.StatusOK, http.MethodPut, groupPath(group.ID), token, map[string]any{"description": "untouched"}) + if got := env.group(t, group.ID); got.Permissions == nil || *got.Permissions != `{"`+legacy+`":true}` { + t.Fatalf("permissions after a save without the field = %v", got.Permissions) + } + // A save that carries the field writes numbers only. + env.expect(t, http.StatusOK, http.MethodPut, groupPath(group.ID), token, map[string]any{"permissions": map[string]int{offered: 1}}) + if got := env.group(t, group.ID); got.Permissions == nil || *got.Permissions != `{"`+offered+`":1}` { + t.Fatalf("permissions after an edit = %v", got.Permissions) + } +} diff --git a/admin_organisations_edge_test.go b/admin_organisations_edge_test.go new file mode 100644 index 0000000..23087ca --- /dev/null +++ b/admin_organisations_edge_test.go @@ -0,0 +1,202 @@ +package user + +import ( + "context" + "net/http" + "reflect" + "testing" + + "git.golem15.com/golem15/sm-user-plugin/models" + "git.golem15.com/golem15/summercms/modules/backpack" + "git.golem15.com/golem15/summercms/modules/cabana" + "git.golem15.com/golem15/summercms/modules/pact" +) + +// TestAdminOrganisationsControllerGuards drives the Organisations controller +// directly on the inputs the HTTP tests cannot produce. +func TestAdminOrganisationsControllerGuards(t *testing.T) { + ctx := context.Background() + const id = "golem15.user.organisations" + notOrganisation := &models.User{} + var nilOrganisation *models.Organisation + + env := newAdminEnv(t) + sets := map[string]adminControllerSet{ + "no accessor": adminControllersOn(nil), + "not booted": adminControllersOn(func() *backpack.App { return nil }), + "no database": adminControllersOn(func() *backpack.App { return bareApp(t) }), + "the live database": adminControllersOn(func() *backpack.App { return env.app }), + } + for name, set := range sets { + t.Run(name, func(t *testing.T) { + ctl := set[id] + for _, record := range []any{notOrganisation, nilOrganisation, nil} { + wantPlainError(t, "FormBeforeCreate", ctl.(pact.FormBeforeCreate).FormBeforeCreate(ctx, record)) + wantPlainError(t, "FormBeforeUpdate", ctl.(pact.FormBeforeUpdate).FormBeforeUpdate(ctx, record)) + wantPlainError(t, "FormBeforeDelete", ctl.(pact.FormBeforeDelete).FormBeforeDelete(ctx, record)) + } + wantPlainError(t, "FormBeforeDelete without a transaction", ctl.(pact.FormBeforeDelete).FormBeforeDelete(ctx, &models.Organisation{ID: 1})) + + // The slug check needs no database; without a translator the + // refusal is in English. + if err := ctl.(pact.FormBeforeCreate).FormBeforeCreate(ctx, &models.Organisation{Name: "Fine", Slug: "fine_slug-1"}); err != nil { + t.Fatal(err) + } + if err := ctl.(pact.FormBeforeUpdate).FormBeforeUpdate(ctx, &models.Organisation{Name: "Empty"}); err != nil { + t.Fatalf("an empty slug is left to the required rule: %v", err) + } + err := ctl.(pact.FormBeforeCreate).FormBeforeCreate(ctx, &models.Organisation{Name: "Bad", Slug: "bad slug"}) + invalid, ok := err.(*cabana.ValidationError) + if !ok || !reflect.DeepEqual(invalid.Details["slug"], []string{"The slug may only contain letters, numbers, dashes and underscores."}) { + t.Fatalf("slug refusal = %v", err) + } + + // The members manager is the only relation: any other name has + // no candidates. + manage := ctl.(pact.RelationExtendManageQuery) + if manage.RelationExtendManageQuery(ctx, "members", nil) != nil || manage.RelationExtendManageQuery(ctx, "groups", nil) != nil { + t.Fatal("RelationExtendManageQuery invented a handle") + } + contracts := ctl.(cabana.AdminRelationContractProvider).AdminRelationContracts() + if len(contracts) != 1 || contracts[0].Name != "members" || contracts[0].Kind != cabana.RelationHasMany || contracts[0].ForeignKey != "organisation_id" { + t.Fatalf("relation contracts = %+v", contracts) + } + if _, ok := contracts[0].NewRelated().(*models.User); !ok { + t.Fatal("the members relation is not on users") + } + if name == "the live database" { + var all, none int64 + users := env.gdb.Model(&models.User{}) + if err := manage.RelationExtendManageQuery(ctx, "members", users).Count(&all).Error; err != nil { + t.Fatal(err) + } + if err := manage.RelationExtendManageQuery(ctx, "groups", env.gdb.Model(&models.User{})).Count(&none).Error; err != nil || none != 0 { + t.Fatalf("candidates of another relation = %d err = %v", none, err) + } + _ = all + } + }) + } +} + +// TestAdminOrganisationsEdge covers the remaining branches of the +// Organisations screen through the admin API. +func TestAdminOrganisationsEdge(t *testing.T) { + env := newAdminEnv(t) + token := env.admin(t, permAccessUsers) + stamp := adminStamp() + + // An organisation without members is deleted and nothing else changes. + bystander := env.seedUser(t, "bystander-"+stamp, nil) + empty := env.seedOrganisation(t, "Edge empty "+stamp) + env.expect(t, http.StatusOK, http.MethodDelete, organisationPath(empty.ID, ""), token, nil) + if env.organisation(t, empty.ID) != nil { + t.Fatal("the organisation survived its delete") + } + if env.reload(t, bystander.ID) == nil { + t.Fatal("deleting an organisation without members deleted a user") + } + env.expect(t, http.StatusNotFound, http.MethodGet, organisationPath(empty.ID, ""), token, nil) + + // A slug with other characters is a 422 on the field, on create and on + // update; a duplicate slug too. + home := env.seedOrganisation(t, "Edge home "+stamp) + for _, slug := range []string{"with space", "dot.ted", "sl/ash", "ąę"} { + rec := env.expect(t, http.StatusUnprocessableEntity, http.MethodPost, adminAPI(adminOrganisationsPath), token, map[string]any{"name": "Edge bad " + stamp, "slug": slug}) + if details := adminDecode(t, rec).Error.Details; len(details["slug"]) == 0 { + t.Fatalf("slug %q: details = %v", slug, details) + } + env.expect(t, http.StatusUnprocessableEntity, http.MethodPut, organisationPath(home.ID, ""), token, map[string]any{"slug": slug}) + } + env.expect(t, http.StatusUnprocessableEntity, http.MethodPost, adminAPI(adminOrganisationsPath), token, map[string]any{"name": "Edge twin " + stamp, "slug": home.Slug}) + if got := env.organisation(t, home.ID); got.Slug != home.Slug { + t.Fatalf("a refused update changed the slug to %q", got.Slug) + } + + // Search and sort on the list. + for _, name := range []string{"b", "a", "c"} { + env.seedOrganisation(t, "Orgsort "+stamp+" "+name) + } + order := func(query string) []string { + var rows []map[string]any + adminInto(t, env.expect(t, http.StatusOK, http.MethodGet, adminAPI(adminOrganisationsPath)+"?search=Orgsort%20"+stamp+"&"+query, token, nil), &rows) + names := []string{} + for _, row := range rows { + name, _ := row["name"].(string) + names = append(names, name[len(name)-1:]) + } + return names + } + if got := order("sort=name&dir=asc"); !reflect.DeepEqual(got, []string{"a", "b", "c"}) { + t.Fatalf("name asc = %v", got) + } + if got := order("sort=slug&dir=desc"); !reflect.DeepEqual(got, []string{"c", "b", "a"}) { + t.Fatalf("slug desc = %v", got) + } + env.expect(t, http.StatusUnprocessableEntity, http.MethodGet, adminAPI(adminOrganisationsPath)+"?filter[slug]=x", token, nil) + + // A deactivated user is not offered as a member and cannot be linked: + // the candidates leave soft-deleted users out. + gone := env.seedUser(t, "gone-"+stamp, nil) + if err := env.gdb.Exec(`UPDATE users SET deleted_at = NOW() WHERE id = ?`, gone.ID).Error; err != nil { + t.Fatal(err) + } + var candidates []map[string]any + adminInto(t, env.expect(t, http.StatusOK, http.MethodGet, organisationPath(home.ID, "/relations/members/candidates?search=gone-"+stamp), token, nil), &candidates) + if len(candidates) != 0 { + t.Fatalf("a deactivated user is a candidate: %v", candidates) + } + rec := env.call(t, http.MethodPost, organisationPath(home.ID, "/relations/members/link"), token, map[string]any{"ids": []uint{gone.ID}}) + if rec.Code != http.StatusUnprocessableEntity { + t.Fatalf("link of a deactivated user = %d %s", rec.Code, rec.Body.String()) + } + if got := env.reload(t, gone.ID); got.OrganisationID != nil { + t.Fatalf("a deactivated user was linked to %d", *got.OrganisationID) + } + // Ids that are not users, and an empty list, change nothing. + for _, body := range []map[string]any{{"ids": []uint{987654321}}, {"ids": []uint{}}, {}} { + rec := env.call(t, http.MethodPost, organisationPath(home.ID, "/relations/members/link"), token, body) + if rec.Code != http.StatusUnprocessableEntity { + t.Fatalf("link %v = %d %s", body, rec.Code, rec.Body.String()) + } + } +} + +// 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) { + env := newAdminEnv(t) + token := env.admin(t, permAccessUsers) + stamp := adminStamp() + first := env.seedOrganisation(t, "Role first "+stamp) + second := env.seedOrganisation(t, "Role second "+stamp) + doomed := env.seedOrganisation(t, "Role doomed "+stamp) + owner := "owner" + member := env.seedUser(t, "role-"+stamp, func(u *models.User) { u.OrganisationID, u.OrganisationRole = &first.ID, &owner }) + other := env.seedUser(t, "role-other-"+stamp, func(u *models.User) { u.OrganisationID, u.OrganisationRole = &doomed.ID, &owner }) + ids := map[string]any{"ids": []uint{member.ID}} + + 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) + } + 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" { + t.Fatalf("after the link elsewhere: 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 { + t.Fatalf("member of a deleted organisation = %+v", got) + } +} diff --git a/admin_users_edge_test.go b/admin_users_edge_test.go index 0c80510..43ede38 100644 --- a/admin_users_edge_test.go +++ b/admin_users_edge_test.go @@ -1,11 +1,27 @@ package user import ( + "context" + "database/sql" + "errors" "net/http" + "net/url" + "os" + "path/filepath" + "reflect" "strconv" + "strings" "testing" + "git.golem15.com/golem15/sm-user-plugin/controllers" "git.golem15.com/golem15/sm-user-plugin/models" + "git.golem15.com/golem15/summercms/modules/backpack" + "git.golem15.com/golem15/summercms/modules/bouncer" + "git.golem15.com/golem15/summercms/modules/cabana" + "git.golem15.com/golem15/summercms/modules/compass" + "git.golem15.com/golem15/summercms/modules/lagoon" + "git.golem15.com/golem15/summercms/modules/pact" + "gorm.io/gorm" ) // TestAdminUsersGroupFilterValue (D-23): the groups filter takes a group id. @@ -45,5 +61,369 @@ func TestAdminUsersGroupFilterValue(t *testing.T) { if page := env.users(t, token, "search="+marker); page.Total != 2 { t.Fatalf("unfiltered list = %s", page.Raw) } - _ = models.FilterByGroup +} + +// adminControllerSet is the plugin's three admin controllers built on one app +// accessor, by controller id. +type adminControllerSet map[string]pact.AdminController + +func adminControllersOn(app func() *backpack.App) adminControllerSet { + out := adminControllerSet{} + for _, ctl := range controllers.AdminControllers(app) { + out[ctl.ID()] = ctl + } + return out +} + +// bareApp is a booted application value without a database handle, a bucket +// or a translator. +func bareApp(t *testing.T) *backpack.App { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "app.yaml"), []byte("name: user-bare\n"), 0o644); err != nil { + t.Fatal(err) + } + cfg, err := compass.Open(compass.Options{Dir: dir, Environ: []string{"SUMMER_ENV=development"}}) + if err != nil { + t.Fatal(err) + } + return backpack.New(cfg) +} + +// brokenApp is an application whose database handle fails every query: its +// connection pool is closed. +func brokenApp(t *testing.T) (*backpack.App, *gorm.DB) { + t.Helper() + adminHarnessDB(t) + u, err := url.Parse(sessionDSN) + if err != nil { + t.Fatal(err) + } + u.Path = "/user_admin" + db, err := sql.Open("pgx", u.String()) + if err != nil { + t.Fatal(err) + } + gdb, err := lagoon.Use(t.Context(), db) + if err != nil { + t.Fatal(err) + } + app := bareApp(t) + if err := lagoon.Publish(app, db, gdb); err != nil { + t.Fatal(err) + } + if err := db.Close(); err != nil { + t.Fatal(err) + } + return app, gdb +} + +// wantPlainError asserts that err is an ordinary error: the framework answers +// it as the opaque 500. A ForbiddenError or a ValidationError would be shown +// to the administrator. +func wantPlainError(t *testing.T, what string, err error) { + t.Helper() + var forbidden *cabana.ForbiddenError + var invalid *cabana.ValidationError + if err == nil || errors.As(err, &forbidden) || errors.As(err, &invalid) { + t.Fatalf("%s: err = %v, want a plain error", what, err) + } +} + +// TestAdminUsersControllerGuards drives the Users controller directly, the +// way the framework calls it, on the inputs the HTTP tests cannot produce: a +// record of another type, an application that is not booted, one without a +// database handle and one whose database fails. Each ends in a plain error +// (the opaque 500) or the documented empty answer; none passes a guard. +func TestAdminUsersControllerGuards(t *testing.T) { + ctx := context.Background() + notUser := &models.UserGroup{} + var nilUser *models.User + user := &models.User{ID: 987654001, Email: "guards@example.test"} + + sets := map[string]adminControllerSet{ + "no accessor": adminControllersOn(nil), + "not booted": adminControllersOn(func() *backpack.App { return nil }), + "no database": adminControllersOn(func() *backpack.App { return bareApp(t) }), + "a failing handle": nil, + "the live database": nil, + } + broken, brokenDB := brokenApp(t) + sets["a failing handle"] = adminControllersOn(func() *backpack.App { return broken }) + env := newAdminEnv(t) + sets["the live database"] = adminControllersOn(func() *backpack.App { return env.app }) + + for name, set := range sets { + t.Run(name, func(t *testing.T) { + ctl := set["golem15.user.users"] + live := name == "the live database" + + // Records of another type and nil records. + for _, record := range []any{notUser, nilUser, nil} { + wantPlainError(t, "FormBeforeUpdate", ctl.(pact.FormBeforeUpdate).FormBeforeUpdate(ctx, record)) + wantPlainError(t, "FormBeforeCreate", ctl.(pact.FormBeforeCreate).FormBeforeCreate(ctx, record)) + wantPlainError(t, "FormAfterCreate", ctl.(pact.FormAfterCreate).FormAfterCreate(ctx, record)) + wantPlainError(t, "FormBeforeDelete", ctl.(pact.FormBeforeDelete).FormBeforeDelete(ctx, record)) + wantPlainError(t, "FormAfterDelete", ctl.(pact.FormAfterDelete).FormAfterDelete(ctx, record)) + _, err := ctl.(pact.ListRowStates).ListRowStates(ctx, env.gdb, []any{record}) + wantPlainError(t, "ListRowStates", err) + editor := ctl.(cabana.PermissionEditorProvider) + _, err = editor.AdminPermissionValues(ctx, "permissions", record) + wantPlainError(t, "AdminPermissionValues", err) + wantPlainError(t, "AdminSetPermissionValues", editor.AdminSetPermissionValues(ctx, "permissions", record, map[string]int{"a": 1})) + for _, action := range ctl.(pact.HasAdminBulkActions).AdminBulkActions() { + _, err := action.Run(ctx, pact.AdminBulkActionInput{Records: []any{record}}) + wantPlainError(t, "bulk "+action.Name, err) + } + for _, action := range ctl.(pact.HasAdminRecordActions).AdminRecordActions() { + _, err := action.Applies(ctx, record) + wantPlainError(t, "applies "+action.Name, err) + _, err = action.Run(ctx, pact.AdminRecordActionInput{Record: record}) + wantPlainError(t, "record "+action.Name, err) + } + } + // The hint of a record that is not a user is empty, never an error. + if data, err := ctl.(pact.AdminPartialData).PartialData(ctx, "hint", notUser); err != nil || !reflect.ValueOf(data).IsZero() { + t.Fatalf("hint of another record = %+v %v", data, err) + } + if _, err := ctl.(pact.AdminPartialData).PartialData(ctx, "other", user); err == nil || !strings.Contains(err.Error(), `unknown partial "other"`) { + t.Fatalf("unknown partial: %v", err) + } + + // Wrong field names. + editor := ctl.(cabana.PermissionEditorProvider) + if _, err := editor.AdminPermissionOptions(ctx, "groups"); err == nil { + t.Fatal("permission options for another field") + } + if _, err := editor.AdminPermissionValues(ctx, "groups", user); err == nil { + t.Fatal("permission values for another field") + } + if err := editor.AdminSetPermissionValues(ctx, "groups", user, nil); err == nil { + t.Fatal("permission write for another field") + } + if lock, err := ctl.(cabana.RelationLockProvider).AdminRelationLocks(ctx, "organisation"); err != nil || len(lock.IDs) != 0 || lock.Message != "" { + t.Fatalf("locks of another field = %+v %v", lock, err) + } + if options := ctl.(pact.FilterOptions).FilterOptions("other"); options != nil { + t.Fatalf("options of another scope = %v", options) + } + if ctl.(pact.ListExtendQuery).ListExtendQuery(ctx, nil) != nil || ctl.(pact.FormExtendQuery).FormExtendQuery(ctx, nil) != nil { + t.Fatal("an extend query invented a handle") + } + + // A bulk or record action outside a transaction never writes. + for _, action := range ctl.(pact.HasAdminBulkActions).AdminBulkActions() { + _, err := action.Run(ctx, pact.AdminBulkActionInput{Records: []any{user}}) + wantPlainError(t, "bulk "+action.Name+" without a transaction", err) + } + for _, action := range ctl.(pact.HasAdminRecordActions).AdminRecordActions() { + _, err := action.Run(ctx, pact.AdminRecordActionInput{Record: user}) + wantPlainError(t, "record "+action.Name+" without a transaction", err) + } + wantPlainError(t, "FormAfterDelete without a transaction", ctl.(pact.FormAfterDelete).FormAfterDelete(ctx, user)) + + if live { + // On a working database the same calls answer, and an update + // of a user who does not exist is an error, not a pass. + wantPlainError(t, "update of an unknown user", ctl.(pact.FormBeforeUpdate).FormBeforeUpdate(ctx, user)) + if options := ctl.(pact.FilterOptions).FilterOptions(models.FilterByGroup); options == nil { + t.Fatal("no group choices on a working database") + } + if lock, err := ctl.(cabana.RelationLockProvider).AdminRelationLocks(ctx, "groups"); err != nil || lock.Message == "" { + t.Fatalf("locks without a principal = %+v %v, want the locks", lock, err) + } + return + } + + // Without a working database every read that a guard depends on + // is an error: the guard never answers "not privileged". + wantPlainError(t, "FormBeforeUpdate", ctl.(pact.FormBeforeUpdate).FormBeforeUpdate(ctx, user)) + wantPlainError(t, "FormBeforeCreate", ctl.(pact.FormBeforeCreate).FormBeforeCreate(ctx, user)) + wantPlainError(t, "FormBeforeDelete", ctl.(pact.FormBeforeDelete).FormBeforeDelete(ctx, user)) + _, err := ctl.(cabana.RelationLockProvider).AdminRelationLocks(ctx, "groups") + wantPlainError(t, "AdminRelationLocks", err) + _, err = editor.AdminPermissionOptions(ctx, "permissions") + wantPlainError(t, "AdminPermissionOptions", err) + _, err = ctl.(pact.AdminPartialData).PartialData(ctx, "hint", user) + wantPlainError(t, "PartialData", err) + for _, action := range ctl.(pact.HasAdminRecordActions).AdminRecordActions() { + if action.Name == "activate" { + continue // reads the record only + } + _, err := action.Applies(ctx, user) + wantPlainError(t, "applies "+action.Name, err) + } + if options := ctl.(pact.FilterOptions).FilterOptions(models.FilterByGroup); options != nil { + t.Fatalf("group choices without a database = %v", options) + } + if name == "a failing handle" { + _, err := ctl.(pact.ListRowStates).ListRowStates(ctx, brokenDB, []any{user}) + wantPlainError(t, "ListRowStates on a failing handle", err) + } + + // A principal who holds the extra permission, and a superuser, + // pass the delete guard before any query is made (D-30). + for _, principal := range []*bouncer.Principal{ + {PermissionGrants: map[string]bool{permManagePrivileged: true}}, + {IsSuperuser: true}, + } { + if err := ctl.(pact.FormBeforeDelete).FormBeforeDelete(bouncer.WithUser(ctx, principal), user); err != nil { + t.Fatalf("delete guard for %+v: %v", principal, err) + } + if lock, err := ctl.(cabana.RelationLockProvider).AdminRelationLocks(bouncer.WithUser(ctx, principal), "groups"); err != nil || len(lock.IDs) != 0 { + t.Fatalf("locks for %+v = %+v %v", principal, lock, err) + } + } + // One who holds only the screen's permission does not. + limited := bouncer.WithUser(ctx, &bouncer.Principal{PermissionGrants: map[string]bool{permAccessUsers: true}}) + wantPlainError(t, "delete guard without the extra permission", ctl.(pact.FormBeforeDelete).FormBeforeDelete(limited, user)) + }) + } +} + +// TestAdminUsersEdge covers the remaining branches of the Users screen +// through the admin API: an unknown partial, an undeclared filter, options +// of a field that has none, search and sort, the superuser, and a membership +// query that fails while the takeover guard needs it (D-30). +func TestAdminUsersEdge(t *testing.T) { + env := newAdminEnv(t) + token := env.admin(t, permAccessUsers) + marker := "edge" + adminStamp() + surname := "Sur" + marker + first := env.seedUser(t, "a-"+marker, func(u *models.User) { u.Surname = &surname }) + second := env.seedUser(t, "b-"+marker, nil) + third := env.seedUser(t, "c-"+marker, nil) + id := strconv.FormatUint(uint64(first.ID), 10) + + // Partials: only the declared hint is served. + for _, name := range []string{"nope", "status", "_hint", "hint.htm", "..%2Fhint"} { + rec := env.call(t, http.MethodGet, adminAPI(adminUsersPath+"/partials/"+name+"?id="+id), token, nil) + if rec.Code != http.StatusNotFound && rec.Code != http.StatusBadRequest && rec.Code != http.StatusUnprocessableEntity { + t.Fatalf("partial %s answers %d: %s", name, rec.Code, rec.Body.String()) + } + assertNoSecret(t, rec.Body.String()) + } + env.expect(t, http.StatusOK, http.MethodGet, adminAPI(adminUsersPath+"/partials/hint?id="+id), token, nil) + env.expect(t, http.StatusNotFound, http.MethodGet, adminAPI(adminUsersPath+"/partials/hint?id=987654321"), token, nil) + + // Filters: an undeclared name and a malformed value are 422, never a + // query. + for _, query := range []string{ + "filter[nope]=1", "filter[password]=x", "filter[activated]=maybe", + "filter[created_date]=yesterday", "filter[created_date]=2020-06-30..2020-06-01x", + } { + env.expect(t, http.StatusUnprocessableEntity, http.MethodGet, adminAPI(adminUsersPath)+"?"+query, token, nil) + } + // Options exist for the groups filter and the two relation fields only. + for _, path := range []string{ + "/filters/activated/options", "/filters/created_date/options", "/filters/nope/options", + "/fields/name/options", "/fields/password/options", "/fields/permissions/options", "/fields/nope/options", + } { + rec := env.call(t, http.MethodGet, adminAPI(adminUsersPath+path), token, nil) + if rec.Code != http.StatusNotFound && rec.Code != http.StatusUnprocessableEntity { + t.Fatalf("%s answers %d: %s", path, rec.Code, rec.Body.String()) + } + } + + // Search reaches the invisible surname column; sort orders both ways and + // refuses a column that is not sortable. + if page := env.users(t, token, "search="+surname); page.Total != 1 || !page.has(first.ID) { + t.Fatalf("surname search = %s", page.Raw) + } + order := func(query string) []uint { + ids := []uint{} + for _, row := range env.users(t, token, "search="+marker+"&"+query).Rows { + ids = append(ids, uint(row["id"].(float64))) + } + return ids + } + if got, want := order("sort=email&dir=asc"), []uint{first.ID, second.ID, third.ID}; !reflect.DeepEqual(got, want) { + t.Fatalf("email asc = %v, want %v", got, want) + } + if got, want := order("sort=email&dir=desc"), []uint{third.ID, second.ID, first.ID}; !reflect.DeepEqual(got, want) { + t.Fatalf("email desc = %v, want %v", got, want) + } + if got, want := order("sort=id&dir=desc"), []uint{third.ID, second.ID, first.ID}; !reflect.DeepEqual(got, want) { + t.Fatalf("id desc = %v, want %v", got, want) + } + for _, query := range []string{"sort=password", "sort=permissions", "sort=email&dir=sideways", "sort=email%3Bdrop", "per_page=2"} { + env.expect(t, http.StatusUnprocessableEntity, http.MethodGet, adminAPI(adminUsersPath)+"?"+query, token, nil) + } + + // D-30: a superuser needs no extra permission for a privileged member. + code := "edgepriv-" + marker + privileged := func(cfg *compass.Config) { + if err := cfg.Set("golem15.user.privileged_groups", []string{code}); err != nil { + t.Fatal(err) + } + } + env = newAdminEnv(t, privileged) + limited := env.admin(t, permAccessUsers) + super := env.admin(t) + if err := env.gdb.Exec(`UPDATE backend_users SET is_superuser = TRUE WHERE id = (SELECT MAX(id) FROM backend_users)`).Error; err != nil { + t.Fatal(err) + } + group := env.seedGroup(t, "Edge privileged "+marker, code) + member := env.seedUser(t, "member-"+marker, nil) + env.join(t, member, group) + env.expect(t, http.StatusForbidden, http.MethodPut, userPath(member.ID, ""), limited, map[string]any{"email": "taken-" + marker + "@example.test"}) + renamed := "super-" + marker + "@example.test" + env.expect(t, http.StatusOK, http.MethodPut, userPath(member.ID, ""), super, map[string]any{"email": renamed}) + if got := env.reload(t, member.ID); got.Email != renamed { + t.Fatalf("superuser update: email = %s", got.Email) + } + + // D-30: when the membership cannot be read, the guard does not let the + // request through. The answer is the opaque 500 and nothing changes. + before := env.reload(t, member.ID) + victim := env.seedUser(t, "victim-"+marker, nil) + rename := func(from, to string) { + t.Helper() + if err := env.gdb.Exec(`ALTER TABLE ` + from + ` RENAME TO ` + to).Error; err != nil { + t.Fatal(err) + } + } + rename("users_groups", "users_groups_away") + restored := false + restore := func() { + if !restored { + restored = true + rename("users_groups_away", "users_groups") + } + } + t.Cleanup(restore) + for what, rec := range map[string]interface { + Result() *http.Response + }{ + "email change": env.call(t, http.MethodPut, userPath(member.ID, ""), limited, map[string]any{"email": "stolen-" + marker + "@example.test"}), + "password reset": env.call(t, http.MethodPut, userPath(member.ID, ""), limited, map[string]any{"password": "stolen-secret-1", "password_confirmation": "stolen-secret-1"}), + "delete": env.call(t, http.MethodDelete, userPath(member.ID, ""), limited, nil), + "bulk delete": env.bulk(t, limited, "delete", victim, member), + "ordinary delete": env.call(t, http.MethodDelete, userPath(victim.ID, ""), limited, nil), + } { + res := rec.Result() + body := make([]byte, 4096) + n, _ := res.Body.Read(body) + text := string(body[:n]) + if res.StatusCode != http.StatusInternalServerError { + t.Fatalf("%s with an unreadable membership answers %d: %s", what, res.StatusCode, text) + } + if strings.Contains(text, "users_groups") || strings.Contains(strings.ToLower(text), "sqlstate") || strings.Contains(text, "relation") { + t.Fatalf("%s leaks the database error: %s", what, text) + } + } + restore() + after := env.reload(t, member.ID) + if after == nil || after.Email != before.Email || after.Password != before.Password || !reflect.DeepEqual(after.TokensValidAfter, before.TokensValidAfter) { + t.Fatalf("a failed guard changed the member: %+v", after) + } + if env.reload(t, victim.ID) == nil { + t.Fatal("a failed guard deleted a user") + } + if got := env.groupIDs(t, member.ID); !reflect.DeepEqual(got, []uint{group.ID}) { + t.Fatalf("memberships after the failed guard = %v", got) + } + if status, _ := env.userLogin(t, before.Email, "correct-horse"); status != http.StatusOK { + t.Fatalf("login with the old password = %d", status) + } }