refac: DELETE /v1/user/{id}?expected_role_id=N, user.remove audit event, 409 target_role_not_found, GetAll returns its error, Own-scope visibility filter moved into repos, JSON tags removed from use-case types
This commit is contained in:
Florian Sylvain committed
1 parent
5d6f378fce
commit
b39a41be88
124 files changed
+8816
-523
No files matched your search
@@ -0,0 +1,288 @@
|
||||
package test
|
||||
|
||||
import (
|
||||
"RenewCMS/api/pkg/apperr"
|
||||
"RenewCMS/internal/domain/article"
|
||||
"RenewCMS/internal/domain/audit"
|
||||
"RenewCMS/internal/domain/role"
|
||||
"RenewCMS/internal/domain/user"
|
||||
persistence "RenewCMS/internal/infrastructure/persistence/repositories"
|
||||
"RenewCMS/internal/infrastructure/useCases"
|
||||
"errors"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strconv"
|
||||
"testing"
|
||||
)
|
||||
|
||||
const removeManagerID uint32 = 60
|
||||
|
||||
func newRemoveWorld(t *testing.T) *usersAPIWorld {
|
||||
t.Helper()
|
||||
w := newUsersAPIWorld(t)
|
||||
manager, _ := w.roles.Create(role.Role{Name: "Remover", Version: 1, Grants: grantRows("users.view=all", "users.remove=all", "content.read=all", "content.create=all", "content.update=own", "content.publish=own", "schema.view=all")})
|
||||
w.ids["Remover"] = manager.ID
|
||||
w.addMember(removeManagerID, "remover", "remover@renew.example", manager.ID, true)
|
||||
return w
|
||||
}
|
||||
|
||||
func (w *usersAPIWorld) remove(t *testing.T, caller uint32, target string, expected string) *httptest.ResponseRecorder {
|
||||
t.Helper()
|
||||
path := "/v1/user/" + target
|
||||
if expected != "" {
|
||||
path += "?expected_role_id=" + expected
|
||||
}
|
||||
return w.do(t, caller, http.MethodDelete, path, "")
|
||||
}
|
||||
|
||||
func TestRemoveUserAnswers(t *testing.T) {
|
||||
rows := []struct {
|
||||
name string
|
||||
caller uint32
|
||||
target string
|
||||
expected string
|
||||
status int
|
||||
code string
|
||||
key string
|
||||
gone bool
|
||||
}{
|
||||
{"Owner removes Ines", uaOwner, "9", "Writer", 204, "", "", true},
|
||||
{"Admin removes Ines", uaAdmin, "9", "Writer", 204, "", "", true},
|
||||
{"People manager removes an Intern", removeManagerID, "46", "Intern", 204, "", "", true},
|
||||
{"Owner with a stale role", uaOwner, "9", "Admin", 409, apperr.UserRoleChangedErrorMessage, "", false},
|
||||
{"absent expected_role_id", uaOwner, "9", "", 400, apperr.InvalidRoleErrorMessage, "", false},
|
||||
{"zero expected_role_id", uaOwner, "9", "0", 400, apperr.InvalidRoleErrorMessage, "", false},
|
||||
{"negative expected_role_id", uaOwner, "9", "-1", 400, apperr.InvalidRoleErrorMessage, "", false},
|
||||
{"text expected_role_id", uaOwner, "9", "abc", 400, apperr.InvalidRoleErrorMessage, "", false},
|
||||
{"bad id", uaOwner, "abc", "Writer", 400, apperr.IdUint32ErrorMessage, "", false},
|
||||
{"unknown user", uaOwner, "9999", "Writer", 404, apperr.UserNotFoundErrorMessage, "", false},
|
||||
{"Owner removes self", uaOwner, "3", "Owner", 403, apperr.OwnRoleErrorMessage, "", false},
|
||||
{"Admin removes self", uaAdmin, "5", "Admin", 403, apperr.OwnRoleErrorMessage, "", false},
|
||||
{"Admin removes the Owner", uaAdmin, "3", "Owner", 403, apperr.RoleLockedErrorMessage, "", false},
|
||||
{"People manager removes a Content lead", removeManagerID, "45", "Content lead", 403, "escalation", "content.update", false},
|
||||
{"People manager removes an Admin", removeManagerID, "5", "Admin", 403, "escalation", "", false},
|
||||
{"Writer lacks users.remove", uaWriter, "46", "Intern", 403, "missing_permission", "", false},
|
||||
}
|
||||
for _, row := range rows {
|
||||
t.Run(row.name, func(t *testing.T) {
|
||||
w := newRemoveWorld(t)
|
||||
expected := row.expected
|
||||
if _, err := strconv.Atoi(expected); err != nil && expected != "" {
|
||||
if id, ok := w.ids[expected]; ok {
|
||||
expected = strconv.FormatUint(uint64(id), 10)
|
||||
}
|
||||
}
|
||||
targetID, _ := strconv.ParseUint(row.target, 10, 32)
|
||||
rec := w.remove(t, row.caller, row.target, expected)
|
||||
switch {
|
||||
case row.status == 204:
|
||||
if rec.Code != 204 || rec.Body.Len() != 0 {
|
||||
t.Fatalf("unexpected %d %q", rec.Code, rec.Body.String())
|
||||
}
|
||||
case row.code == "missing_permission":
|
||||
usersAPIAssertMissing(t, rec, "users.remove")
|
||||
case row.code == "escalation":
|
||||
assertEscalation(t, rec, row.key)
|
||||
default:
|
||||
assertRefusal(t, rec, row.status, row.code)
|
||||
}
|
||||
_, present := w.users.users[uint32(targetID)]
|
||||
if _, existed := newRemoveWorld(t).users.users[uint32(targetID)]; existed && present == row.gone {
|
||||
t.Errorf("target presence %v, gone expected %v", present, row.gone)
|
||||
}
|
||||
wantEvents := 0
|
||||
if row.gone {
|
||||
wantEvents = 1
|
||||
}
|
||||
if len(w.recorder.events) != wantEvents {
|
||||
t.Errorf("expected %d events, got %v", wantEvents, w.recorder.actions())
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
t.Run("no cookie", func(t *testing.T) {
|
||||
w := newRemoveWorld(t)
|
||||
rec := w.anonymous(t, http.MethodDelete, "/v1/user/9?expected_role_id="+w.idText("Writer"), "")
|
||||
usersAPIAssertPlain(t, rec, http.StatusUnauthorized, apperr.UnauthorizedErrorMessage)
|
||||
if _, present := w.users.users[9]; !present {
|
||||
t.Error("nothing may be removed")
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
func TestRemoveUserCheckOrder(t *testing.T) {
|
||||
t.Run("a stale role beats role_locked and escalation", func(t *testing.T) {
|
||||
w := newRemoveWorld(t)
|
||||
assertRefusal(t, w.remove(t, uaAdmin, "3", w.idText("Writer")), 409, apperr.UserRoleChangedErrorMessage)
|
||||
rec := w.remove(t, removeManagerID, "45", w.idText("Admin"))
|
||||
assertRefusal(t, rec, 409, apperr.UserRoleChangedErrorMessage)
|
||||
})
|
||||
|
||||
t.Run("own_role beats a stale role", func(t *testing.T) {
|
||||
w := newRemoveWorld(t)
|
||||
assertRefusal(t, w.remove(t, uaAdmin, "5", w.idText("Writer")), 403, apperr.OwnRoleErrorMessage)
|
||||
})
|
||||
|
||||
t.Run("missing_permission is answered before the target lookup", func(t *testing.T) {
|
||||
w := newRemoveWorld(t)
|
||||
before := w.users.getCalls
|
||||
usersAPIAssertMissing(t, w.remove(t, uaWriter, "9", w.idText("Writer")), "users.remove")
|
||||
if w.users.getCalls != before+1 {
|
||||
t.Errorf("only the caller may be looked up, got %d extra Get calls", w.users.getCalls-before-1)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("the id is read before expected_role_id", func(t *testing.T) {
|
||||
w := newRemoveWorld(t)
|
||||
usersAPIAssertPlain(t, w.remove(t, uaOwner, "abc", ""), 400, apperr.IdUint32ErrorMessage)
|
||||
})
|
||||
|
||||
t.Run("expected_role_id is read before the target lookup", func(t *testing.T) {
|
||||
w := newRemoveWorld(t)
|
||||
assertRefusal(t, w.remove(t, uaOwner, "9999", ""), 400, apperr.InvalidRoleErrorMessage)
|
||||
})
|
||||
}
|
||||
|
||||
func TestRemoveUserRaceAnswers409(t *testing.T) {
|
||||
w := newRemoveWorld(t)
|
||||
w.users.beforeDeleteIfRole = func() {
|
||||
stored := w.users.users[9]
|
||||
stored.RoleID = w.id("Admin")
|
||||
w.users.users[9] = stored
|
||||
}
|
||||
assertRefusal(t, w.remove(t, uaOwner, "9", w.idText("Writer")), 409, apperr.UserRoleChangedErrorMessage)
|
||||
if _, present := w.users.users[9]; !present || len(w.recorder.events) != 0 {
|
||||
t.Error("the user must remain and nothing be recorded")
|
||||
}
|
||||
}
|
||||
|
||||
func TestRemoveUserWriteFailure(t *testing.T) {
|
||||
w := newRemoveWorld(t)
|
||||
w.users.fail.failNextWith("DeleteIfRole", errors.New("disk full"))
|
||||
usersAPIAssertPlain(t, w.remove(t, uaOwner, "9", w.idText("Writer")), 500, "Internal server error")
|
||||
if _, present := w.users.users[9]; !present {
|
||||
t.Error("the user must remain")
|
||||
}
|
||||
if len(w.recorder.events) != 1 || w.recorder.events[0].Action != audit.ActionUserRemove || w.recorder.events[0].Outcome != audit.OutcomeFailed {
|
||||
t.Errorf("expected one failed user.remove event, got %+v", w.recorder.events)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRemoveUserAudit(t *testing.T) {
|
||||
w := newRemoveWorld(t)
|
||||
if rec := w.remove(t, uaAdmin, "9", w.idText("Writer")); rec.Code != 204 {
|
||||
t.Fatalf("unexpected %d", rec.Code)
|
||||
}
|
||||
if len(w.recorder.events) != 1 {
|
||||
t.Fatalf("expected one event, got %d", len(w.recorder.events))
|
||||
}
|
||||
event := w.recorder.events[0]
|
||||
if event.Action != "user.remove" || event.Category != audit.CategoryUsers || event.Outcome != audit.OutcomeSuccess {
|
||||
t.Errorf("unexpected event %+v", event)
|
||||
}
|
||||
if event.Target.Type != audit.TargetUser || event.Target.ID != "9" || event.Target.Name != "ines" {
|
||||
t.Errorf("unexpected target %+v", event.Target)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRemoveUserRevokesAccess(t *testing.T) {
|
||||
w := newRemoveWorld(t)
|
||||
w.container.LookupUserNamesUseCase = useCases.NewLookupUserNamesUseCase(w.users)
|
||||
w.rebuildRouter()
|
||||
removedKey, _ := w.issueKey(uaWriter, "ines")
|
||||
otherKey, _ := w.issueKey(uaContributor, "other")
|
||||
if rec := w.do(t, uaWriter, http.MethodGet, "/v1/user/lookup?ids=9", ""); rec.Code == http.StatusUnauthorized {
|
||||
t.Fatalf("the session must work before the removal, got %d", rec.Code)
|
||||
}
|
||||
if rec := w.remove(t, uaOwner, "9", w.idText("Writer")); rec.Code != 204 {
|
||||
t.Fatalf("unexpected %d", rec.Code)
|
||||
}
|
||||
usersAPIAssertPlain(t, w.do(t, uaWriter, http.MethodGet, "/v1/user/lookup?ids=9", ""), http.StatusUnauthorized, apperr.UnauthorizedErrorMessage)
|
||||
if rec := w.withKey(t, http.MethodGet, "/v1/user/lookup?ids=9", "", removedKey); rec.Code != http.StatusUnauthorized {
|
||||
t.Errorf("the removed user's key must answer 401, got %d", rec.Code)
|
||||
}
|
||||
if rec := w.withKey(t, http.MethodGet, "/v1/user/lookup?ids=9", "", otherKey); rec.Code == http.StatusUnauthorized {
|
||||
t.Errorf("another user's key must keep working")
|
||||
}
|
||||
if rec := w.do(t, uaContributor, http.MethodGet, "/v1/user/lookup?ids=9", ""); rec.Code == http.StatusUnauthorized {
|
||||
t.Errorf("another user's session must keep working")
|
||||
}
|
||||
}
|
||||
|
||||
func TestRemoveUserLeavesContentAndInvites(t *testing.T) {
|
||||
w := newRemoveWorld(t)
|
||||
w.articles.articles[1] = article.Article{ID: 1, Title: "Kept", CreatedBy: uaWriter}
|
||||
invitesBefore, _ := w.invites.GetAll()
|
||||
if rec := w.remove(t, uaOwner, "9", w.idText("Writer")); rec.Code != 204 {
|
||||
t.Fatalf("unexpected %d", rec.Code)
|
||||
}
|
||||
if w.articles.articles[1].CreatedBy != uaWriter {
|
||||
t.Errorf("the article must keep its created_by")
|
||||
}
|
||||
invitesAfter, _ := w.invites.GetAll()
|
||||
if len(invitesAfter) != len(invitesBefore) {
|
||||
t.Errorf("invites must stay")
|
||||
}
|
||||
}
|
||||
|
||||
func TestRemoveUserOnlyAcceptedUsers(t *testing.T) {
|
||||
w := newRemoveWorld(t)
|
||||
invites, _ := w.invites.GetAll()
|
||||
inviteID := invites[0].ID + 1000
|
||||
assertRefusal(t, w.remove(t, uaOwner, strconv.FormatUint(uint64(inviteID), 10), w.idText("Writer")), 404, apperr.UserNotFoundErrorMessage)
|
||||
if rec := w.do(t, uaOwner, http.MethodDelete, "/v1/user/invite/"+strconv.FormatUint(uint64(invites[0].ID), 10), ""); rec.Code != 204 {
|
||||
t.Errorf("invites stay cancellable, got %d", rec.Code)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRemoveUserDeleteIfRolePersistence(t *testing.T) {
|
||||
db := openMigratedRolesDB(t)
|
||||
repo := persistence.NewUserRepository(db)
|
||||
admin, writer := storedRole(t, db, "Admin").ID, storedRole(t, db, "Writer").ID
|
||||
uaPersistedUser(t, db, 5, &admin, true)
|
||||
uaPersistedUser(t, db, 6, &writer, true)
|
||||
if err := repo.DeleteIfRole(5, writer); !errors.Is(err, user.ErrRoleChanged) {
|
||||
t.Fatalf("expected ErrRoleChanged, got %v", err)
|
||||
}
|
||||
if err := repo.DeleteIfRole(99, writer); !errors.Is(err, user.ErrUserNotFound) {
|
||||
t.Fatalf("expected ErrUserNotFound, got %v", err)
|
||||
}
|
||||
if err := repo.DeleteIfRole(5, admin); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if _, err := repo.Get(5); !errors.Is(err, user.ErrUserNotFound) {
|
||||
t.Errorf("user 5 must be gone, got %v", err)
|
||||
}
|
||||
if _, err := repo.Get(6); err != nil {
|
||||
t.Errorf("user 6 must remain, got %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestReassignTargetNotFoundAndToName(t *testing.T) {
|
||||
w := newUsersAPIWorld(t)
|
||||
contributor := w.idText("Contributor")
|
||||
path := "/v1/role/" + contributor + "/reassign"
|
||||
rows := []struct {
|
||||
name string
|
||||
caller uint32
|
||||
target string
|
||||
status int
|
||||
code string
|
||||
}{
|
||||
{"missing target", uaOwner, "9999", 409, apperr.ReassignTargetNotFoundErrorMessage},
|
||||
{"itself", uaOwner, contributor, 400, apperr.InvalidReassignErrorMessage},
|
||||
{"locked", uaOwner, strconv.FormatUint(uint64(ownerRoleID), 10), 400, apperr.InvalidReassignErrorMessage},
|
||||
}
|
||||
for _, row := range rows {
|
||||
t.Run(row.name, func(t *testing.T) {
|
||||
w := newUsersAPIWorld(t)
|
||||
assertRefusal(t, w.do(t, row.caller, http.MethodPost, path, `{"target_role_id": `+row.target+`}`), row.status, row.code)
|
||||
if len(w.recorder.events) != 0 || w.storedRoleID(41) != w.id("Contributor") {
|
||||
t.Error("a refusal moves nobody and records nothing")
|
||||
}
|
||||
})
|
||||
}
|
||||
t.Run("missing source", func(t *testing.T) {
|
||||
assertRefusal(t, w.do(t, uaOwner, http.MethodPost, "/v1/role/9999/reassign", `{"target_role_id": `+w.idText("Writer")+`}`), 404, apperr.RoleNotFoundErrorMessage)
|
||||
})
|
||||
}
|
||||
Reference in new issue
Block a user