diff --git a/plugins/organizations/api.go b/plugins/organizations/api.go index 7c1f3f71..f4c7cf36 100644 --- a/plugins/organizations/api.go +++ b/plugins/organizations/api.go @@ -45,6 +45,14 @@ func (a *API) GetUserPermissionsInOrganization(ctx context.Context, userID strin return nil, coreerrors.ErrUnauthorized } + exists, err := a.organizationService.ExistsByID(ctx, organizationID) + if err != nil { + return nil, err + } + if !exists { + return nil, coreerrors.ErrForbidden + } + member, err := a.memberRepo.GetByOrganizationIDAndUserID(ctx, organizationID, userID) if err != nil { return nil, err @@ -69,7 +77,8 @@ func (a *API) GetAllOrganizations(ctx context.Context, actor *models.Actor) ([]t } // GetAllOrganizationsUnscoped returns every organization in the system with no -// access filtering. Authorize the caller before using it. +// access filtering, excluding soft-deleted organizations. Authorize the caller +// before using it. func (a *API) GetAllOrganizationsUnscoped(ctx context.Context) ([]types.Organization, error) { return a.organizationService.GetAllOrganizationsUnscoped(ctx) } diff --git a/plugins/organizations/api_test.go b/plugins/organizations/api_test.go new file mode 100644 index 00000000..bb42572d --- /dev/null +++ b/plugins/organizations/api_test.go @@ -0,0 +1,106 @@ +package organizations + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + coreerrors "github.com/Authula/authula/core/errors" + orgtests "github.com/Authula/authula/plugins/organizations/tests" + "github.com/Authula/authula/plugins/organizations/types" +) + +func TestAPI_GetUserPermissionsInOrganization(t *testing.T) { + t.Parallel() + + repoErr := errors.New("repository error") + + tests := []struct { + name string + userID string + organizationID string + setup func(*orgtests.MockOrganizationService, *orgtests.MockOrganizationMemberRepository) + expectErr error + expectPerms []string + }{ + { + name: "unauthorized without identifiers", + userID: "", + organizationID: "", + expectErr: coreerrors.ErrUnauthorized, + }, + { + name: "missing organization is forbidden", + userID: "user-1", + organizationID: "org-1", + setup: func(orgService *orgtests.MockOrganizationService, _ *orgtests.MockOrganizationMemberRepository) { + orgService.On("ExistsByID", mock.Anything, "org-1").Return(false, nil).Once() + }, + expectErr: coreerrors.ErrForbidden, + }, + { + name: "existence check error is propagated", + userID: "user-1", + organizationID: "org-1", + setup: func(orgService *orgtests.MockOrganizationService, _ *orgtests.MockOrganizationMemberRepository) { + orgService.On("ExistsByID", mock.Anything, "org-1").Return(false, repoErr).Once() + }, + expectErr: repoErr, + }, + { + name: "non-member is forbidden", + userID: "user-1", + organizationID: "org-1", + setup: func(orgService *orgtests.MockOrganizationService, memberRepo *orgtests.MockOrganizationMemberRepository) { + orgService.On("ExistsByID", mock.Anything, "org-1").Return(true, nil).Once() + memberRepo.On("GetByOrganizationIDAndUserID", mock.Anything, "org-1", "user-1").Return(nil, nil).Once() + }, + expectErr: coreerrors.ErrForbidden, + }, + { + name: "member receives role permissions", + userID: "user-1", + organizationID: "org-1", + setup: func(orgService *orgtests.MockOrganizationService, memberRepo *orgtests.MockOrganizationMemberRepository) { + orgService.On("ExistsByID", mock.Anything, "org-1").Return(true, nil).Once() + memberRepo.On("GetByOrganizationIDAndUserID", mock.Anything, "org-1", "user-1").Return(&types.OrganizationMember{ID: "mem-1", OrganizationID: "org-1", UserID: "user-1", Role: "member"}, nil).Once() + }, + expectPerms: []string{"organizations:read"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + orgService := &orgtests.MockOrganizationService{} + memberRepo := &orgtests.MockOrganizationMemberRepository{} + if tt.setup != nil { + tt.setup(orgService, memberRepo) + } + + accessControl := orgtests.NewAccessControlServiceStub() + accessControl.RolePermissions["member"] = []string{"organizations:read"} + + api := &API{ + organizationService: orgService, + memberRepo: memberRepo, + accessControlService: accessControl, + } + + perms, err := api.GetUserPermissionsInOrganization(context.Background(), tt.userID, tt.organizationID) + if tt.expectErr != nil { + require.Error(t, err) + require.ErrorIs(t, err, tt.expectErr) + return + } + require.NoError(t, err) + require.Equal(t, tt.expectPerms, perms) + orgService.AssertExpectations(t) + memberRepo.AssertExpectations(t) + }) + } +} diff --git a/plugins/organizations/migrationset/migrations.go b/plugins/organizations/migrationset/migrations.go index 915d267f..1bfc678f 100644 --- a/plugins/organizations/migrationset/migrations.go +++ b/plugins/organizations/migrationset/migrations.go @@ -32,11 +32,13 @@ func organizationsSQLiteInitial() migrations.Migration { slug VARCHAR(255) NOT NULL UNIQUE, logo TEXT, metadata TEXT, + deleted_at TIMESTAMP, created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, - FOREIGN KEY (owner_id) REFERENCES users(id) ON DELETE CASCADE + FOREIGN KEY (owner_id) REFERENCES users(id) ON DELETE RESTRICT );`, `CREATE INDEX IF NOT EXISTS idx_organizations_owner_id ON organizations(owner_id);`, + `CREATE INDEX IF NOT EXISTS idx_organizations_deleted_at ON organizations(deleted_at);`, `CREATE INDEX IF NOT EXISTS idx_organizations_owner_created_id ON organizations(owner_id, created_at, id);`, `DROP TRIGGER IF EXISTS update_organizations_updated_at_trigger;`, `CREATE TRIGGER update_organizations_updated_at_trigger @@ -56,7 +58,7 @@ func organizationsSQLiteInitial() migrations.Migration { status VARCHAR(32) NOT NULL DEFAULT 'pending', expires_at TIMESTAMP NOT NULL, created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, - FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE, + FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE RESTRICT, FOREIGN KEY (inviter_id) REFERENCES users(id) ON DELETE CASCADE );`, `CREATE INDEX IF NOT EXISTS idx_organization_invitations_email ON organization_invitations(email);`, @@ -73,7 +75,7 @@ func organizationsSQLiteInitial() migrations.Migration { role VARCHAR(255) NOT NULL, created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, - FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE, + FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE RESTRICT, FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE, UNIQUE (organization_id, user_id) );`, @@ -97,7 +99,7 @@ func organizationsSQLiteInitial() migrations.Migration { metadata TEXT, created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, - FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE, + FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE RESTRICT, UNIQUE (organization_id, slug) );`, `CREATE INDEX IF NOT EXISTS idx_organization_teams_organization_id ON organization_teams(organization_id);`, @@ -161,9 +163,10 @@ func organizationsPostgresInitial() migrations.Migration { slug VARCHAR(255) NOT NULL UNIQUE, logo TEXT, metadata JSONB, + deleted_at TIMESTAMP WITH TIME ZONE, created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(), updated_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(), - CONSTRAINT fk_organizations_owner FOREIGN KEY (owner_id) REFERENCES users(id) ON DELETE CASCADE + CONSTRAINT fk_organizations_owner FOREIGN KEY (owner_id) REFERENCES users(id) ON DELETE RESTRICT );`, `DROP TRIGGER IF EXISTS update_organizations_updated_at_trigger ON organizations;`, `CREATE TRIGGER update_organizations_updated_at_trigger @@ -172,6 +175,7 @@ func organizationsPostgresInitial() migrations.Migration { EXECUTE FUNCTION organizations_set_updated_at_fn();`, `CREATE INDEX IF NOT EXISTS idx_organizations_owner_id ON organizations(owner_id);`, `CREATE INDEX IF NOT EXISTS idx_organizations_owner_created_id ON organizations(owner_id, created_at, id);`, + `CREATE INDEX IF NOT EXISTS idx_organizations_deleted_at ON organizations(deleted_at) WHERE deleted_at IS NULL;`, // ----------------------------------- `CREATE TABLE IF NOT EXISTS organization_invitations ( id UUID PRIMARY KEY DEFAULT gen_random_uuid(), @@ -183,7 +187,7 @@ func organizationsPostgresInitial() migrations.Migration { expires_at TIMESTAMP WITH TIME ZONE NOT NULL, created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(), updated_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(), - CONSTRAINT fk_organization_invitations_organization FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE, + CONSTRAINT fk_organization_invitations_organization FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE RESTRICT, CONSTRAINT fk_organization_invitations_inviter FOREIGN KEY (inviter_id) REFERENCES users(id) ON DELETE CASCADE, CONSTRAINT chk_organization_invitations_status CHECK (status IN ('pending', 'accepted', 'rejected', 'revoked', 'expired')) );`, @@ -206,7 +210,7 @@ func organizationsPostgresInitial() migrations.Migration { role VARCHAR(255) NOT NULL, created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(), updated_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(), - CONSTRAINT fk_organization_members_organization FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE, + CONSTRAINT fk_organization_members_organization FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE RESTRICT, CONSTRAINT fk_organization_members_user FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE, CONSTRAINT uq_organization_members_organization_user UNIQUE (organization_id, user_id) );`, @@ -229,7 +233,7 @@ func organizationsPostgresInitial() migrations.Migration { metadata JSONB, created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(), updated_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(), - CONSTRAINT fk_organization_teams_organization FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE, + CONSTRAINT fk_organization_teams_organization FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE RESTRICT, CONSTRAINT uq_organization_teams_organization_slug UNIQUE (organization_id, slug) );`, `DROP TRIGGER IF EXISTS update_organization_teams_updated_at_trigger ON organization_teams;`, @@ -290,11 +294,13 @@ func organizationsMySQLInitial() migrations.Migration { slug VARCHAR(255) NOT NULL UNIQUE, logo TEXT NULL, metadata JSON NULL, + deleted_at TIMESTAMP NULL DEFAULT NULL, created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, - CONSTRAINT fk_organizations_owner FOREIGN KEY (owner_id) REFERENCES users(id) ON DELETE CASCADE, + CONSTRAINT fk_organizations_owner FOREIGN KEY (owner_id) REFERENCES users(id) ON DELETE RESTRICT, INDEX idx_organizations_owner_id (owner_id), - INDEX idx_organizations_owner_created_id (owner_id, created_at, id) + INDEX idx_organizations_deleted_at (deleted_at) + INDEX idx_organizations_owner_created_id (owner_id, created_at, id), ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;`, // ----------------------------------- `CREATE TABLE IF NOT EXISTS organization_invitations ( @@ -307,7 +313,7 @@ func organizationsMySQLInitial() migrations.Migration { expires_at TIMESTAMP NOT NULL, created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, - CONSTRAINT fk_organization_invitations_organization FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE, + CONSTRAINT fk_organization_invitations_organization FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE RESTRICT, CONSTRAINT fk_organization_invitations_inviter FOREIGN KEY (inviter_id) REFERENCES users(id) ON DELETE CASCADE, CONSTRAINT chk_organization_invitations_status CHECK (status IN ('pending', 'accepted', 'rejected', 'revoked', 'expired')), INDEX idx_organization_invitations_organization_id (organization_id), @@ -325,7 +331,7 @@ func organizationsMySQLInitial() migrations.Migration { role VARCHAR(255) NOT NULL, created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, - CONSTRAINT fk_organization_members_organization FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE, + CONSTRAINT fk_organization_members_organization FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE RESTRICT, CONSTRAINT fk_organization_members_user FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE, CONSTRAINT uq_organization_members_organization_user UNIQUE (organization_id, user_id), INDEX idx_organization_members_organization_id (organization_id), @@ -343,7 +349,7 @@ func organizationsMySQLInitial() migrations.Migration { metadata JSON NULL, created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, - CONSTRAINT fk_organization_teams_organization FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE CASCADE, + CONSTRAINT fk_organization_teams_organization FOREIGN KEY (organization_id) REFERENCES organizations(id) ON DELETE RESTRICT, CONSTRAINT uq_organization_teams_organization_slug UNIQUE (organization_id, slug), INDEX idx_organization_teams_organization_id (organization_id), INDEX idx_organization_teams_slug (slug), diff --git a/plugins/organizations/openapi/openapi_docs.go b/plugins/organizations/openapi/openapi_docs.go index c01fca3b..8193e810 100644 --- a/plugins/organizations/openapi/openapi_docs.go +++ b/plugins/organizations/openapi/openapi_docs.go @@ -26,7 +26,7 @@ func RegisterOpenAPIDocs(svc openapi.OpenAPIService) error { "/organizations", openapi.WithOperationID("listOrganizations"), openapi.WithSummary("List organizations"), - openapi.WithDescription("Lists every organization the authenticated user can access, both the ones they own and the ones they are a member of, newest first. Results are paginated: `page` defaults to 1 and `limit` defaults to 10. `limit` is capped by the server's configured maximum page size, which defaults to 100; a larger value is silently reduced to the cap rather than rejected, and values below the minimum fall back to the defaults. The `pagination` object in the response always reports the values actually applied."), + openapi.WithDescription("Lists every organization the authenticated user can access."), openapi.WithTags("Organizations"), openapi.WithRequest(&types.ListOrganizationsRequest{}), openapi.WithResponseStatus(http.StatusOK, &types.ListOrganizationsResponse{}), @@ -57,7 +57,7 @@ func RegisterOpenAPIDocs(svc openapi.OpenAPIService) error { "/organizations/{organization_id}", openapi.WithOperationID("deleteOrganization"), openapi.WithSummary("Delete organization"), - openapi.WithDescription("Deletes an organization."), + openapi.WithDescription("Soft-deletes an organization."), openapi.WithTags("Organizations"), openapi.WithRequest(&types.OrganizationID{}), openapi.WithResponseStatus(http.StatusOK, &types.DeleteOrganizationResponse{}), diff --git a/plugins/organizations/repositories/bun_organization_invitation_repository.go b/plugins/organizations/repositories/bun_organization_invitation_repository.go index 40a3e31b..74450056 100644 --- a/plugins/organizations/repositories/bun_organization_invitation_repository.go +++ b/plugins/organizations/repositories/bun_organization_invitation_repository.go @@ -69,10 +69,8 @@ func (r *BunOrganizationInvitationRepository) GetByOrganizationIDAndEmail(ctx co return invitation, err } -// pendingByEmailWhere matches invitations that are still awaiting a decision and -// have not expired. Both pending lookups order oldest first, because acceptance -// resolves role conflicts in favour of the earliest invitation. -const pendingByEmailWhere = "email = ? AND status = ? AND expires_at > ?" +const pendingByEmailWhere = "email = ? AND status = ? AND expires_at > ?" + + " AND organization_id IN (SELECT id FROM organizations WHERE deleted_at IS NULL)" func (r *BunOrganizationInvitationRepository) ListAllPendingByEmail(ctx context.Context, email string, page int, limit int) ([]types.OrganizationInvitation, int, error) { invites := make([]types.OrganizationInvitation, 0) @@ -152,7 +150,7 @@ const invitationWithOrgColumns = `i.id, i.email, i.inviter_id, i.organization_id const invitationWithOrgByOrganizationFrom = ` FROM organization_invitations i` + ` INNER JOIN organizations o ON o.id = i.organization_id` + - ` WHERE i.organization_id = ?` + ` WHERE i.organization_id = ? AND o.deleted_at IS NULL` func mapToInvitationWithOrgResponse(row invitationOrgRow) types.GetOrganizationInvitationResponse { return types.GetOrganizationInvitationResponse{ @@ -183,7 +181,7 @@ func (r *BunOrganizationInvitationRepository) GetByIDWithOrg(ctx context.Context SELECT `+invitationWithOrgColumns+` FROM organization_invitations i INNER JOIN organizations o ON o.id = i.organization_id - WHERE i.id = ? + WHERE i.id = ? AND o.deleted_at IS NULL `, invitationID).Scan(ctx, &row) if err == sql.ErrNoRows { return nil, nil diff --git a/plugins/organizations/repositories/bun_organization_invitation_repository_test.go b/plugins/organizations/repositories/bun_organization_invitation_repository_test.go index 17de1fa8..b5032643 100644 --- a/plugins/organizations/repositories/bun_organization_invitation_repository_test.go +++ b/plugins/organizations/repositories/bun_organization_invitation_repository_test.go @@ -756,6 +756,111 @@ func TestBunOrganizationInvitationRepository_GetAllByOrganizationIDWithOrg(t *te } } +func TestBunOrganizationInvitationRepository_GetByIDWithOrg(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + invitationID string + softDeleteOrg bool + expectFound bool + }{ + {name: "found", invitationID: "inv-1", expectFound: true}, + {name: "not found", invitationID: "missing"}, + {name: "soft-deleted organization is not found", invitationID: "inv-1", softDeleteOrg: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + db := plugintests.SetupRepoDB(t) + plugintests.SeedOrganization(t, db, "org-1", "user-1", "Acme Inc", "acme-inc") + repo := repositories.NewBunOrganizationInvitationRepository(db) + ctx := context.Background() + + _, err := repo.Create(ctx, &types.OrganizationInvitation{ID: "inv-1", Email: "user@example.com", InviterID: "user-1", OrganizationID: "org-1", Role: "member", Status: types.OrganizationInvitationStatusPending, ExpiresAt: time.Now().UTC().Add(time.Hour)}) + require.NoError(t, err) + if tt.softDeleteOrg { + softDeleteOrganization(t, db, "org-1") + } + + found, err := repo.GetByIDWithOrg(ctx, tt.invitationID) + require.NoError(t, err) + if tt.expectFound { + require.NotNil(t, found) + require.Equal(t, "inv-1", found.Invitation.ID) + require.Equal(t, "org-1", found.Organization.ID) + return + } + require.Nil(t, found) + }) + } +} + +func TestBunOrganizationInvitationRepository_ListAllByOrganizationIDWithOrgExcludesSoftDeletedOrganization(t *testing.T) { + t.Parallel() + + db := plugintests.SetupRepoDB(t) + plugintests.SeedOrganization(t, db, "org-1", "user-1", "Acme Inc", "acme-inc") + repo := repositories.NewBunOrganizationInvitationRepository(db) + ctx := context.Background() + + for i := 1; i <= 3; i++ { + _, err := repo.Create(ctx, &types.OrganizationInvitation{ + ID: fmt.Sprintf("inv-%d", i), + Email: fmt.Sprintf("user%d@example.com", i), + InviterID: "user-1", + OrganizationID: "org-1", + Role: "member", + Status: types.OrganizationInvitationStatusPending, + ExpiresAt: time.Now().UTC().Add(time.Hour), + }) + require.NoError(t, err) + } + + softDeleteOrganization(t, db, "org-1") + + invitations, total, err := repo.ListAllByOrganizationIDWithOrg(ctx, "org-1", 1, 10) + require.NoError(t, err) + require.Empty(t, invitations) + require.Equal(t, 0, total) +} + +func TestBunOrganizationInvitationRepository_PendingByEmailExcludesSoftDeletedOrganization(t *testing.T) { + t.Parallel() + + db := plugintests.SetupRepoDB(t) + plugintests.SeedOrganization(t, db, "org-1", "user-1", "Acme Inc", "acme-inc") + plugintests.SeedOrganization(t, db, "org-2", "user-2", "Beta Inc", "beta-inc") + repo := repositories.NewBunOrganizationInvitationRepository(db) + ctx := context.Background() + + for _, invitation := range []*types.OrganizationInvitation{ + {ID: "inv-1", OrganizationID: "org-1", Status: types.OrganizationInvitationStatusPending, ExpiresAt: time.Now().UTC().Add(time.Hour)}, + {ID: "inv-2", OrganizationID: "org-2", Status: types.OrganizationInvitationStatusPending, ExpiresAt: time.Now().UTC().Add(time.Hour)}, + } { + invitation.Email = "user@example.com" + invitation.InviterID = "user-1" + invitation.Role = "member" + _, err := repo.Create(ctx, invitation) + require.NoError(t, err) + } + + softDeleteOrganization(t, db, "org-1") + + all, err := repo.GetAllPendingByEmail(ctx, "user@example.com") + require.NoError(t, err) + require.Len(t, all, 1) + require.Equal(t, "inv-2", all[0].ID) + + paged, total, err := repo.ListAllPendingByEmail(ctx, "user@example.com", 1, 10) + require.NoError(t, err) + require.Len(t, paged, 1) + require.Equal(t, 1, total) + require.Equal(t, "inv-2", paged[0].ID) +} + func TestBunOrganizationInvitationRepository_GetAllByOrganizationIDWithOrgIgnoresTheDefaultLimit(t *testing.T) { t.Parallel() diff --git a/plugins/organizations/repositories/bun_organization_repository.go b/plugins/organizations/repositories/bun_organization_repository.go index c7ee6af6..15db47b1 100644 --- a/plugins/organizations/repositories/bun_organization_repository.go +++ b/plugins/organizations/repositories/bun_organization_repository.go @@ -3,6 +3,7 @@ package repositories import ( "context" "database/sql" + "time" "github.com/uptrace/bun" @@ -39,7 +40,7 @@ func (r *BunOrganizationRepository) Create(ctx context.Context, organization *ty func (r *BunOrganizationRepository) GetByID(ctx context.Context, organizationID string) (*types.Organization, error) { organization := new(types.Organization) - err := r.db.NewSelect().Model(organization).Where("id = ?", organizationID).Scan(ctx) + err := r.db.NewSelect().Model(organization).Where("id = ? AND deleted_at IS NULL", organizationID).Scan(ctx) if err == sql.ErrNoRows { return nil, nil } @@ -48,15 +49,16 @@ func (r *BunOrganizationRepository) GetByID(ctx context.Context, organizationID func (r *BunOrganizationRepository) GetBySlug(ctx context.Context, slug string) (*types.Organization, error) { organization := new(types.Organization) - err := r.db.NewSelect().Model(organization).Where("slug = ?", slug).Scan(ctx) + err := r.db.NewSelect().Model(organization).Where("slug = ? AND deleted_at IS NULL", slug).Scan(ctx) if err == sql.ErrNoRows { return nil, nil } return organization, err } -const organizationAccessibleWhere = `o.owner_id = ? OR EXISTS (` + - `SELECT 1 FROM organization_members m WHERE m.organization_id = o.id AND m.user_id = ?)` +const organizationAccessibleWhere = `(o.owner_id = ? OR EXISTS (` + + `SELECT 1 FROM organization_members m WHERE m.organization_id = o.id AND m.user_id = ?))` + + ` AND o.deleted_at IS NULL` func (r *BunOrganizationRepository) ListAllAccessibleByUserID(ctx context.Context, userID string, page int, limit int) ([]types.Organization, int, error) { organizations := make([]types.Organization, 0) @@ -91,6 +93,7 @@ func (r *BunOrganizationRepository) GetAllAccessibleByUserID(ctx context.Context func (r *BunOrganizationRepository) GetAll(ctx context.Context) ([]types.Organization, error) { organizations := make([]types.Organization, 0) err := r.db.NewSelect().Model(&organizations). + Where("deleted_at IS NULL"). OrderExpr("created_at DESC, id DESC"). Scan(ctx) if err == sql.ErrNoRows { @@ -129,11 +132,11 @@ func (r *BunOrganizationRepository) Update(ctx context.Context, organization *ty func (r *BunOrganizationRepository) Delete(ctx context.Context, organizationID string) error { return r.db.RunInTx(ctx, nil, func(ctx context.Context, tx bun.Tx) error { - if _, err := tx.NewDelete().Model(&types.Organization{}).Where("id = ?", organizationID).Exec(ctx); err != nil { - return err - } - - return nil + _, err := tx.NewUpdate().Model(&types.Organization{}). + Set("deleted_at = ?", time.Now().UTC()). + Where("id = ? AND deleted_at IS NULL", organizationID). + Exec(ctx) + return err }) } diff --git a/plugins/organizations/repositories/bun_organization_repository_test.go b/plugins/organizations/repositories/bun_organization_repository_test.go index da30d836..b5db70db 100644 --- a/plugins/organizations/repositories/bun_organization_repository_test.go +++ b/plugins/organizations/repositories/bun_organization_repository_test.go @@ -71,6 +71,22 @@ func TestBunOrganizationRepository_GetByID(t *testing.T) { return repo, ctx }, }, + { + name: "soft deleted is not found", + organizationID: "org-1", + setup: func(t *testing.T) (repositories.OrganizationRepository, context.Context) { + t.Helper() + db := plugintests.SetupRepoDB(t) + repo := repositories.NewBunOrganizationRepository(db) + ctx := context.Background() + + _, err := repo.Create(ctx, &types.Organization{ID: "org-1", OwnerID: "user-1", Name: "Acme Inc", Slug: "acme-inc"}) + require.NoError(t, err) + require.NoError(t, repo.Delete(ctx, "org-1")) + + return repo, ctx + }, + }, { name: "not found", organizationID: "missing", @@ -125,6 +141,22 @@ func TestBunOrganizationRepository_GetBySlug(t *testing.T) { return repo, ctx }, }, + { + name: "soft deleted is not found", + slug: "acme-inc", + setup: func(t *testing.T) (repositories.OrganizationRepository, context.Context) { + t.Helper() + db := plugintests.SetupRepoDB(t) + repo := repositories.NewBunOrganizationRepository(db) + ctx := context.Background() + + _, err := repo.Create(ctx, &types.Organization{ID: "org-1", OwnerID: "user-1", Name: "Acme Inc", Slug: "acme-inc"}) + require.NoError(t, err) + require.NoError(t, repo.Delete(ctx, "org-1")) + + return repo, ctx + }, + }, { name: "not found", slug: "missing", @@ -153,7 +185,7 @@ func TestBunOrganizationRepository_GetBySlug(t *testing.T) { } } -func seedAccessibleOrganizations(t *testing.T) (repositories.OrganizationRepository, context.Context) { +func seedAccessibleOrganizationsDB(t *testing.T) (*bun.DB, repositories.OrganizationRepository, context.Context) { t.Helper() db := plugintests.SetupRepoDB(t) @@ -168,7 +200,21 @@ func seedAccessibleOrganizations(t *testing.T) (repositories.OrganizationReposit plugintests.SeedOrganizationMember(t, db, "mem-c", "org-c", "user-1", "owner") plugintests.SeedOrganizationMember(t, db, "mem-d", "org-d", "user-2", "owner") - return repositories.NewBunOrganizationRepository(db), ctx + return db, repositories.NewBunOrganizationRepository(db), ctx +} + +func seedAccessibleOrganizations(t *testing.T) (repositories.OrganizationRepository, context.Context) { + t.Helper() + + _, repo, ctx := seedAccessibleOrganizationsDB(t) + return repo, ctx +} + +func softDeleteOrganization(t *testing.T, db *bun.DB, organizationID string) { + t.Helper() + + _, err := db.ExecContext(context.Background(), `UPDATE organizations SET deleted_at = CURRENT_TIMESTAMP WHERE id = ?`, organizationID) + require.NoError(t, err) } func organizationIDs(organizations []types.Organization) []string { @@ -328,40 +374,73 @@ func TestBunOrganizationRepository_Update(t *testing.T) { func TestBunOrganizationRepository_Delete(t *testing.T) { t.Parallel() - tests := []struct { - name string - organizationID string - setup func(*testing.T) (repositories.OrganizationRepository, context.Context) - }{ - { - name: "delete existing", - organizationID: "org-1", - setup: func(t *testing.T) (repositories.OrganizationRepository, context.Context) { - t.Helper() - db := plugintests.SetupRepoDB(t) - repo := repositories.NewBunOrganizationRepository(db) - ctx := context.Background() + db := plugintests.SetupRepoDB(t) + repo := repositories.NewBunOrganizationRepository(db) + ctx := context.Background() - _, err := repo.Create(ctx, &types.Organization{ID: "org-1", OwnerID: "user-1", Name: "Acme Inc", Slug: "acme-inc"}) - require.NoError(t, err) + _, err := repo.Create(ctx, &types.Organization{ID: "org-1", OwnerID: "user-1", Name: "Acme Inc", Slug: "acme-inc"}) + require.NoError(t, err) - return repo, ctx - }, - }, - } + require.NoError(t, repo.Delete(ctx, "org-1")) - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() + found, err := repo.GetByID(ctx, "org-1") + require.NoError(t, err) + require.Nil(t, found, "a soft-deleted organization must not be readable") + + var retained types.Organization + err = db.NewSelect().Model(&retained). + Where("id = ?", "org-1"). + Where("deleted_at IS NOT NULL"). + Scan(ctx) + require.NoError(t, err, "the row must still exist with deleted_at set") + require.NotNil(t, retained.DeletedAt) + require.Equal(t, "acme-inc", retained.Slug, "the slug stays reserved after a soft delete") +} - repo, ctx := tt.setup(t) +func TestBunOrganizationRepository_SoftDeletedOrganizationsAreHidden(t *testing.T) { + t.Parallel() - require.NoError(t, repo.Delete(ctx, tt.organizationID)) - found, err := repo.GetByID(ctx, tt.organizationID) - require.NoError(t, err) - require.Nil(t, found) - }) - } + db, repo, ctx := seedAccessibleOrganizationsDB(t) + softDeleteOrganization(t, db, "org-a") + softDeleteOrganization(t, db, "org-c") + + found, err := repo.GetByID(ctx, "org-a") + require.NoError(t, err) + require.Nil(t, found) + + bySlug, err := repo.GetBySlug(ctx, "owned") + require.NoError(t, err) + require.Nil(t, bySlug, "a soft-deleted organization must not be found by its reserved slug") + + accessible, _, err := repo.ListAllAccessibleByUserID(ctx, "user-1", 1, 10) + require.NoError(t, err) + require.ElementsMatch(t, []string{"org-b"}, organizationIDs(accessible)) + + allAccessible, err := repo.GetAllAccessibleByUserID(ctx, "user-1") + require.NoError(t, err) + require.ElementsMatch(t, []string{"org-b"}, organizationIDs(allAccessible)) + + all, err := repo.GetAll(ctx) + require.NoError(t, err) + require.ElementsMatch(t, []string{"org-b", "org-d"}, organizationIDs(all)) + + count, err := repo.CountAccessibleByUserID(ctx, "user-1") + require.NoError(t, err) + require.Equal(t, 1, count, "soft-deleted organizations must not count toward the quota") +} + +func TestBunOrganizationRepository_HardDeleteBlockedByChildren(t *testing.T) { + t.Parallel() + + db, _, ctx := seedAccessibleOrganizationsDB(t) + + // SQLite disables foreign key enforcement by default and the PRAGMA is a + // no-op inside the migrator's transaction, so enable it on this connection. + _, err := db.ExecContext(ctx, `PRAGMA foreign_keys = ON;`) + require.NoError(t, err) + + _, err = db.ExecContext(ctx, `DELETE FROM organizations WHERE id = ?`, "org-b") + require.Error(t, err, "FK RESTRICT must block hard-deleting an organization with members") } func TestBunOrganizationRepository_WithTx(t *testing.T) { diff --git a/plugins/organizations/services/organization_service.go b/plugins/organizations/services/organization_service.go index a4fea78f..e398b737 100644 --- a/plugins/organizations/services/organization_service.go +++ b/plugins/organizations/services/organization_service.go @@ -216,8 +216,6 @@ func (s *organizationService) GetAllOrganizations(ctx context.Context, actor *mo return organizations, nil } -// GetAllOrganizationsUnscoped bypasses actor scoping entirely. See the interface -// documentation before adding a caller. func (s *organizationService) GetAllOrganizationsUnscoped(ctx context.Context) ([]types.Organization, error) { organizations, err := s.orgRepo.GetAll(ctx) if err != nil { diff --git a/plugins/organizations/services/organization_service_test.go b/plugins/organizations/services/organization_service_test.go index 63793beb..b05f1cde 100644 --- a/plugins/organizations/services/organization_service_test.go +++ b/plugins/organizations/services/organization_service_test.go @@ -11,6 +11,7 @@ import ( coreerrors "github.com/Authula/authula/core/errors" "github.com/Authula/authula/core/pagination" internaltests "github.com/Authula/authula/internal/tests" + "github.com/Authula/authula/models" "github.com/Authula/authula/plugins/organizations/constants" orgtests "github.com/Authula/authula/plugins/organizations/tests" "github.com/Authula/authula/plugins/organizations/types" @@ -564,14 +565,14 @@ func TestOrganizationService_DeleteOrganization(t *testing.T) { name string actorUserID string organizationID string - setup func(*orgtests.MockOrganizationRepository, *orgtests.MockOrganizationHooks, *ServiceUtils) + setup func(*orgtests.MockOrganizationRepository) expectErr error }{ { name: "success", actorUserID: "user-1", organizationID: "org-1", - setup: func(repo *orgtests.MockOrganizationRepository, hooks *orgtests.MockOrganizationHooks, serviceUtils *ServiceUtils) { + setup: func(repo *orgtests.MockOrganizationRepository) { repo.On("GetByID", mock.Anything, "org-1").Return(&types.Organization{ID: "org-1", OwnerID: "user-1"}, nil).Once() repo.On("Delete", mock.Anything, "org-1").Return(nil).Once() }, @@ -583,13 +584,25 @@ func TestOrganizationService_DeleteOrganization(t *testing.T) { t.Parallel() repo := &orgtests.MockOrganizationRepository{} - hooks := &orgtests.MockOrganizationHooks{} serviceUtils := &ServiceUtils{orgRepo: repo} if tt.setup != nil { - tt.setup(repo, hooks, serviceUtils) + tt.setup(repo) } - svc := NewOrganizationService(repo, nil, serviceUtils, nil, nil, nil) + beforeCalled := false + afterCalled := false + orgHooks := &types.OrganizationServiceHooks{} + orgHooks.RegisterBeforeDelete(func(ctx context.Context, actor *models.Actor, organization *types.Organization) error { + beforeCalled = true + return nil + }) + orgHooks.RegisterAfterDelete(func(ctx context.Context, actor *models.Actor, organization *types.Organization) error { + afterCalled = true + return nil + }) + hooksExecutor := NewServiceHookExecutor(&types.OrganizationsServiceHooksConfig{Organizations: orgHooks}, nil, nil) + + svc := NewOrganizationService(repo, nil, serviceUtils, nil, nil, nil, hooksExecutor) err := svc.DeleteOrganization(context.Background(), orgtests.Actor(tt.actorUserID), tt.organizationID) if tt.expectErr != nil { require.Error(t, err) @@ -597,6 +610,66 @@ func TestOrganizationService_DeleteOrganization(t *testing.T) { return } require.NoError(t, err) + require.True(t, beforeCalled, "BeforeDeleteOrganization must fire") + require.True(t, afterCalled, "AfterDeleteOrganization must fire") + repo.AssertExpectations(t) + }) + } +} + +func TestOrganizationService_ExistsByID(t *testing.T) { + t.Parallel() + + repoErr := errors.New("repository error") + + tests := []struct { + name string + setup func(*orgtests.MockOrganizationRepository) + expectExist bool + expectErr error + }{ + { + name: "found live organization", + setup: func(repo *orgtests.MockOrganizationRepository) { + repo.On("GetByID", mock.Anything, "org-1").Return(&types.Organization{ID: "org-1", OwnerID: "user-1"}, nil).Once() + }, + expectExist: true, + }, + { + name: "soft-deleted organization reads as absent", + setup: func(repo *orgtests.MockOrganizationRepository) { + repo.On("GetByID", mock.Anything, "org-1").Return(nil, nil).Once() + }, + expectExist: false, + }, + { + name: "repository error is propagated", + setup: func(repo *orgtests.MockOrganizationRepository) { + repo.On("GetByID", mock.Anything, "org-1").Return(nil, repoErr).Once() + }, + expectErr: repoErr, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + repo := &orgtests.MockOrganizationRepository{} + if tt.setup != nil { + tt.setup(repo) + } + + svc := NewOrganizationService(repo, nil, &ServiceUtils{orgRepo: repo}, nil, nil, nil) + exists, err := svc.ExistsByID(context.Background(), "org-1") + if tt.expectErr != nil { + require.Error(t, err) + require.ErrorIs(t, err, tt.expectErr) + return + } + require.NoError(t, err) + require.Equal(t, tt.expectExist, exists) + repo.AssertExpectations(t) }) } } diff --git a/plugins/organizations/types/models.go b/plugins/organizations/types/models.go index f23c921c..4906fb4e 100644 --- a/plugins/organizations/types/models.go +++ b/plugins/organizations/types/models.go @@ -16,6 +16,7 @@ type Organization struct { Slug string `json:"slug" required:"true" nullable:"false" bun:"column:slug"` Logo *string `json:"logo" nullable:"true" bun:"column:logo"` Metadata map[string]any `json:"metadata" nullable:"true" bun:"column:metadata"` + DeletedAt *time.Time `json:"deleted_at,omitempty" nullable:"true" bun:"column:deleted_at,nullzero"` CreatedAt time.Time `json:"created_at" required:"true" nullable:"false" bun:"column:created_at,default:current_timestamp"` UpdatedAt time.Time `json:"updated_at" required:"true" nullable:"false" bun:"column:updated_at,default:current_timestamp"` }