synced/coder
1
0
Fork 0
mirror of https://github.com/coder/coder.git synced 2025-07-13 21:36:50 +00:00
Code Issues Packages Projects Releases Wiki Activity

test: add unit test to excercise bug when idp sync hits deleted orgs (#17405)

Browse Source 
Deleted organizations are still attempting to sync members. This causes
an error on inserting the member, and would likely cause issues later in
the sync process even if that member is inserted. Deleted orgs should be
skipped.
This commit is contained in:
Steven Masley
2025-04-16 09:27:35 -05:00
committed by GitHub
parent 64172d374f
commit 669e790df6
9 changed files with 242 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
coderd/database/dbauthz/dbauthz_test.go
View File

@ -886,7 +886,7 @@ func (s *MethodTestSuite) TestOrganization() {
_ = dbgen.OrganizationMember(s.T(), db, database.OrganizationMember{UserID: u.ID, OrganizationID: a.ID})
b := dbgen.Organization(s.T(), db, database.Organization{})
_ = dbgen.OrganizationMember(s.T(), db, database.OrganizationMember{UserID: u.ID, OrganizationID: b.ID})
check.Args(database.GetOrganizationsByUserIDParams{UserID: u.ID, Deleted: false}).Asserts(a, policy.ActionRead, b, policy.ActionRead).Returns(slice.New(a, b))
check.Args(database.GetOrganizationsByUserIDParams{UserID: u.ID, Deleted: sql.NullBool{Valid: true, Bool: false}}).Asserts(a, policy.ActionRead, b, policy.ActionRead).Returns(slice.New(a, b))
}))
s.Run("InsertOrganization", s.Subtest(func(db database.Store, check *expects) {
check.Args(database.InsertOrganizationParams{
@ -994,8 +994,7 @@ func (s *MethodTestSuite) TestOrganization() {
member, policy.ActionRead,
member, policy.ActionDelete).
WithNotAuthorized("no rows").
WithCancelled(cancelledErr).
ErrorsWithInMemDB(sql.ErrNoRows)
WithCancelled(cancelledErr)
}))
s.Run("UpdateOrganization", s.Subtest(func(db database.Store, check *expects) {
o := dbgen.Organization(s.T(), db, database.Organization{

18
coderd/database/dbfake/builder.go
View File

@ -17,6 +17,7 @@ type OrganizationBuilder struct {
t *testing.T
db database.Store
seed database.Organization
delete bool
allUsersAllowance int32
members []uuid.UUID
groups map[database.Group][]uuid.UUID
@ -45,6 +46,12 @@ func (b OrganizationBuilder) EveryoneAllowance(allowance int) OrganizationBuilde
return b
}
func (b OrganizationBuilder) Deleted(deleted bool) OrganizationBuilder {
//nolint: revive // returns modified struct
b.delete = deleted
return b
}
func (b OrganizationBuilder) Seed(seed database.Organization) OrganizationBuilder {
//nolint: revive // returns modified struct
b.seed = seed
@ -119,6 +126,17 @@ func (b OrganizationBuilder) Do() OrganizationResponse {
}
}
if b.delete {
now := dbtime.Now()
err = b.db.UpdateOrganizationDeletedByID(ctx, database.UpdateOrganizationDeletedByIDParams{
UpdatedAt: now,
ID: org.ID,
})
require.NoError(b.t, err)
org.Deleted = true
org.UpdatedAt = now
}
return OrganizationResponse{
Org: org,
AllUsersGroup: everyone,

18
coderd/database/dbmem/dbmem.go
View File

@ -2357,10 +2357,13 @@ func (q *FakeQuerier) DeleteOrganizationMember(ctx context.Context, arg database
q.mutex.Lock()
defer q.mutex.Unlock()
deleted := slices.DeleteFunc(q.data.organizationMembers, func(member database.OrganizationMember) bool {
return member.OrganizationID == arg.OrganizationID && member.UserID == arg.UserID
deleted := false
q.data.organizationMembers = slices.DeleteFunc(q.data.organizationMembers, func(member database.OrganizationMember) bool {
match := member.OrganizationID == arg.OrganizationID && member.UserID == arg.UserID
deleted = deleted || match
return match
})
if len(deleted) == 0 {
if !deleted {
return sql.ErrNoRows
}
@ -4156,6 +4159,9 @@ func (q *FakeQuerier) GetOrganizations(_ context.Context, args database.GetOrgan
if args.Name != "" && !strings.EqualFold(org.Name, args.Name) {
continue
}
if args.Deleted != org.Deleted {
continue
}
tmp = append(tmp, org)
}
@ -4172,7 +4178,11 @@ func (q *FakeQuerier) GetOrganizationsByUserID(_ context.Context, arg database.G
continue
}
for _, organization := range q.organizations {
if organization.ID != organizationMember.OrganizationID || organization.Deleted != arg.Deleted {
if organization.ID != organizationMember.OrganizationID {
continue
}
if arg.Deleted.Valid && organization.Deleted != arg.Deleted.Bool {
continue
}
organizations = append(organizations, organization)

11
coderd/database/queries.sql.go
View File

@ -5680,8 +5680,13 @@ SELECT
FROM
organizations
WHERE
-- Optionally include deleted organizations
deleted = $2 AND
-- Optionally provide a filter for deleted organizations.
CASE WHEN
$2 :: boolean IS NULL THEN
true
ELSE
deleted = $2
END AND
id = ANY(
SELECT
organization_id
@ -5694,7 +5699,7 @@ WHERE
type GetOrganizationsByUserIDParams struct {
UserID uuid.UUID `db:"user_id" json:"user_id"`
Deleted bool `db:"deleted" json:"deleted"`
Deleted sql.NullBool `db:"deleted" json:"deleted"`
}
func (q *sqlQuerier) GetOrganizationsByUserID(ctx context.Context, arg GetOrganizationsByUserIDParams) ([]Organization, error) {

9
coderd/database/queries/organizations.sql
View File

@ -55,8 +55,13 @@ SELECT
FROM
organizations
WHERE
-- Optionally include deleted organizations
deleted = @deleted AND
-- Optionally provide a filter for deleted organizations.
CASE WHEN
sqlc.narg('deleted') :: boolean IS NULL THEN
true
ELSE
deleted = sqlc.narg('deleted')
END AND
id = ANY(
SELECT
organization_id

57
coderd/idpsync/organization.go
View File

@ -92,14 +92,16 @@ func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, u
return nil // No sync configured, nothing to do
}
expectedOrgs, err := orgSettings.ParseClaims(ctx, tx, params.MergedClaims)
expectedOrgIDs, err := orgSettings.ParseClaims(ctx, tx, params.MergedClaims)
if err != nil {
return xerrors.Errorf("organization claims: %w", err)
}
// Fetch all organizations, even deleted ones. This is to remove a user
// from any deleted organizations they may be in.
existingOrgs, err := tx.GetOrganizationsByUserID(ctx, database.GetOrganizationsByUserIDParams{
UserID: user.ID,
Deleted: false,
Deleted: sql.NullBool{},
})
if err != nil {
return xerrors.Errorf("failed to get user organizations: %w", err)
@ -109,10 +111,35 @@ func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, u
return org.ID
})
// finalExpected is the final set of org ids the user is expected to be in.
// Deleted orgs are omitted from this set.
finalExpected := expectedOrgIDs
if len(expectedOrgIDs) > 0 {
// If you pass in an empty slice to the db arg, you get all orgs. So the slice
// has to be non-empty to get the expected set. Logically it also does not make
// sense to fetch an empty set from the db.
expectedOrganizations, err := tx.GetOrganizations(ctx, database.GetOrganizationsParams{
IDs: expectedOrgIDs,
// Do not include deleted organizations. Omitting deleted orgs will remove the
// user from any deleted organizations they are a member of.
Deleted: false,
})
if err != nil {
return xerrors.Errorf("failed to get expected organizations: %w", err)
}
finalExpected = db2sdk.List(expectedOrganizations, func(org database.Organization) uuid.UUID {
return org.ID
})
}
// Find the difference in the expected and the existing orgs, and
// correct the set of orgs the user is a member of.
add, remove := slice.SymmetricDifference(existingOrgIDs, expectedOrgs)
notExists := make([]uuid.UUID, 0)
add, remove := slice.SymmetricDifference(existingOrgIDs, finalExpected)
// notExists is purely for debugging. It logs when the settings want
// a user in an organization, but the organization does not exist.
notExists := slice.DifferenceFunc(expectedOrgIDs, finalExpected, func(a, b uuid.UUID) bool {
return a == b
})
for _, orgID := range add {
_, err := tx.InsertOrganizationMember(ctx, database.InsertOrganizationMemberParams{
OrganizationID: orgID,
@ -123,9 +150,30 @@ func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, u
})
if err != nil {
if xerrors.Is(err, sql.ErrNoRows) {
// This should not happen because we check the org existence
// beforehand.
notExists = append(notExists, orgID)
continue
}
if database.IsUniqueViolation(err, database.UniqueOrganizationMembersPkey) {
// If we hit this error we have a bug. The user already exists in the
// organization, but was not detected to be at the start of this function.
// Instead of failing the function, an error will be logged. This is to not bring
// down the entire syncing behavior from a single failed org. Failing this can
// prevent user logins, so only fatal non-recoverable errors should be returned.
//
// Inserting a user is privilege escalation. So skipping this instead of failing
// leaves the user with fewer permissions. So this is safe from a security
// perspective to continue.
s.Logger.Error(ctx, "syncing user to organization failed as they are already a member, please report this failure to Coder",
slog.F("user_id", user.ID),
slog.F("username", user.Username),
slog.F("organization_id", orgID),
slog.Error(err),
)
continue
}
return xerrors.Errorf("add user to organization: %w", err)
}
}
@ -141,6 +189,7 @@ func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, u
}
if len(notExists) > 0 {
notExists = slice.Unique(notExists) // Remove duplicates
s.Logger.Debug(ctx, "organizations do not exist but attempted to use in org sync",
slog.F("not_found", notExists),
slog.F("user_id", user.ID),

111
coderd/idpsync/organizations_test.go
View File

@ -1,6 +1,7 @@
package idpsync_test
import (
"database/sql"
"testing"
"github.com/golang-jwt/jwt/v4"
@ -8,6 +9,11 @@ import (
"github.com/stretchr/testify/require"
"cdr.dev/slog/sloggers/slogtest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/db2sdk"
"github.com/coder/coder/v2/coderd/database/dbfake"
"github.com/coder/coder/v2/coderd/database/dbgen"
"github.com/coder/coder/v2/coderd/database/dbtestutil"
"github.com/coder/coder/v2/coderd/idpsync"
"github.com/coder/coder/v2/coderd/runtimeconfig"
"github.com/coder/coder/v2/testutil"
@ -38,3 +44,108 @@ func TestParseOrganizationClaims(t *testing.T) {
require.False(t, params.SyncEntitled)
})
}
func TestSyncOrganizations(t *testing.T) {
t.Parallel()
// This test creates some deleted organizations and checks the behavior is
// correct.
t.Run("SyncUserToDeletedOrg", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitMedium)
db, _ := dbtestutil.NewDB(t)
user := dbgen.User(t, db, database.User{})
// Create orgs for:
// - stays = User is a member, and stays
// - leaves = User is a member, and leaves
// - joins = User is not a member, and joins
// For deleted orgs, the user **should not** be a member of afterwards.
// - deletedStays = User is a member of deleted org, and wants to stay
// - deletedLeaves = User is a member of deleted org, and wants to leave
// - deletedJoins = User is not a member of deleted org, and wants to join
stays := dbfake.Organization(t, db).Members(user).Do()
leaves := dbfake.Organization(t, db).Members(user).Do()
joins := dbfake.Organization(t, db).Do()
deletedStays := dbfake.Organization(t, db).Members(user).Deleted(true).Do()
deletedLeaves := dbfake.Organization(t, db).Members(user).Deleted(true).Do()
deletedJoins := dbfake.Organization(t, db).Deleted(true).Do()
// Now sync the user to the deleted organization
s := idpsync.NewAGPLSync(
slogtest.Make(t, &slogtest.Options{}),
runtimeconfig.NewManager(),
idpsync.DeploymentSyncSettings{
OrganizationField: "orgs",
OrganizationMapping: map[string][]uuid.UUID{
"stay": {stays.Org.ID, deletedStays.Org.ID},
"leave": {leaves.Org.ID, deletedLeaves.Org.ID},
"join": {joins.Org.ID, deletedJoins.Org.ID},
},
OrganizationAssignDefault: false,
},
)
err := s.SyncOrganizations(ctx, db, user, idpsync.OrganizationParams{
SyncEntitled: true,
MergedClaims: map[string]interface{}{
"orgs": []string{"stay", "join"},
},
})
require.NoError(t, err)
orgs, err := db.GetOrganizationsByUserID(ctx, database.GetOrganizationsByUserIDParams{
UserID: user.ID,
Deleted: sql.NullBool{},
})
require.NoError(t, err)
require.Len(t, orgs, 2)
// Verify the user only exists in 2 orgs. The one they stayed, and the one they
// joined.
inIDs := db2sdk.List(orgs, func(org database.Organization) uuid.UUID {
return org.ID
})
require.ElementsMatch(t, []uuid.UUID{stays.Org.ID, joins.Org.ID}, inIDs)
})
t.Run("UserToZeroOrgs", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitMedium)
db, _ := dbtestutil.NewDB(t)
user := dbgen.User(t, db, database.User{})
deletedLeaves := dbfake.Organization(t, db).Members(user).Deleted(true).Do()
// Now sync the user to the deleted organization
s := idpsync.NewAGPLSync(
slogtest.Make(t, &slogtest.Options{}),
runtimeconfig.NewManager(),
idpsync.DeploymentSyncSettings{
OrganizationField: "orgs",
OrganizationMapping: map[string][]uuid.UUID{
"leave": {deletedLeaves.Org.ID},
},
OrganizationAssignDefault: false,
},
)
err := s.SyncOrganizations(ctx, db, user, idpsync.OrganizationParams{
SyncEntitled: true,
MergedClaims: map[string]interface{}{
"orgs": []string{},
},
})
require.NoError(t, err)
orgs, err := db.GetOrganizationsByUserID(ctx, database.GetOrganizationsByUserIDParams{
UserID: user.ID,
Deleted: sql.NullBool{},
})
require.NoError(t, err)
require.Len(t, orgs, 0)
})
}

2
coderd/users.go
View File

@ -1340,7 +1340,7 @@ func (api *API) organizationsByUser(rw http.ResponseWriter, r *http.Request) {
organizations, err := api.Database.GetOrganizationsByUserID(ctx, database.GetOrganizationsByUserIDParams{
UserID: user.ID,
Deleted: false,
Deleted: sql.NullBool{Bool: false, Valid: true},
})
if errors.Is(err, sql.ErrNoRows) {
err = nil

42
enterprise/coderd/enidpsync/organizations_test.go
View File

@ -14,6 +14,7 @@ import (
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/db2sdk"
"github.com/coder/coder/v2/coderd/database/dbauthz"
"github.com/coder/coder/v2/coderd/database/dbfake"
"github.com/coder/coder/v2/coderd/database/dbgen"
"github.com/coder/coder/v2/coderd/database/dbtestutil"
"github.com/coder/coder/v2/coderd/entitlements"
@ -89,7 +90,8 @@ func TestOrganizationSync(t *testing.T) {
Name: "SingleOrgDeployment",
Case: func(t *testing.T, db database.Store) OrganizationSyncTestCase {
def, _ := db.GetDefaultOrganization(context.Background())
other := dbgen.Organization(t, db, database.Organization{})
other := dbfake.Organization(t, db).Do()
deleted := dbfake.Organization(t, db).Deleted(true).Do()
return OrganizationSyncTestCase{
Entitlements: entitled,
Settings: idpsync.DeploymentSyncSettings{
@ -123,11 +125,19 @@ func TestOrganizationSync(t *testing.T) {
})
dbgen.OrganizationMember(t, db, database.OrganizationMember{
UserID: user.ID,
OrganizationID: other.ID,
OrganizationID: other.Org.ID,
})
dbgen.OrganizationMember(t, db, database.OrganizationMember{
UserID: user.ID,
OrganizationID: deleted.Org.ID,
})
},
Sync: ExpectedUser{
Organizations: []uuid.UUID{def.ID, other.ID},
Organizations: []uuid.UUID{
def.ID, other.Org.ID,
// The user remains in the deleted org because no idp sync happens.
deleted.Org.ID,
},
},
},
},
@ -138,17 +148,19 @@ func TestOrganizationSync(t *testing.T) {
Name: "MultiOrgWithDefault",
Case: func(t *testing.T, db database.Store) OrganizationSyncTestCase {
def, _ := db.GetDefaultOrganization(context.Background())
one := dbgen.Organization(t, db, database.Organization{})
two := dbgen.Organization(t, db, database.Organization{})
three := dbgen.Organization(t, db, database.Organization{})
one := dbfake.Organization(t, db).Do()
two := dbfake.Organization(t, db).Do()
three := dbfake.Organization(t, db).Do()
deleted := dbfake.Organization(t, db).Deleted(true).Do()
return OrganizationSyncTestCase{
Entitlements: entitled,
Settings: idpsync.DeploymentSyncSettings{
OrganizationField: "organizations",
OrganizationMapping: map[string][]uuid.UUID{
"first": {one.ID},
"second": {two.ID},
"third": {three.ID},
"first": {one.Org.ID},
"second": {two.Org.ID},
"third": {three.Org.ID},
"deleted": {deleted.Org.ID},
},
OrganizationAssignDefault: true,
},
@ -167,7 +179,7 @@ func TestOrganizationSync(t *testing.T) {
{
Name: "AlreadyInOrgs",
Claims: jwt.MapClaims{
"organizations": []string{"second", "extra"},
"organizations": []string{"second", "extra", "deleted"},
},
ExpectedParams: idpsync.OrganizationParams{
SyncEntitled: true,
@ -180,18 +192,18 @@ func TestOrganizationSync(t *testing.T) {
})
dbgen.OrganizationMember(t, db, database.OrganizationMember{
UserID: user.ID,
OrganizationID: one.ID,
OrganizationID: one.Org.ID,
})
},
Sync: ExpectedUser{
Organizations: []uuid.UUID{def.ID, two.ID},
Organizations: []uuid.UUID{def.ID, two.Org.ID},
},
},
{
Name: "ManyClaims",
Claims: jwt.MapClaims{
// Add some repeats
"organizations": []string{"second", "extra", "first", "third", "second", "second"},
"organizations": []string{"second", "extra", "first", "third", "second", "second", "deleted"},
},
ExpectedParams: idpsync.OrganizationParams{
SyncEntitled: true,
@ -204,11 +216,11 @@ func TestOrganizationSync(t *testing.T) {
})
dbgen.OrganizationMember(t, db, database.OrganizationMember{
UserID: user.ID,
OrganizationID: one.ID,
OrganizationID: one.Org.ID,
})
},
Sync: ExpectedUser{
Organizations: []uuid.UUID{def.ID, one.ID, two.ID, three.ID},
Organizations: []uuid.UUID{def.ID, one.Org.ID, two.Org.ID, three.Org.ID},
},
},
},