Skip to content

Commit 669e790

Browse files
EmyrkEmyrk
authored
test: add unit test to excercise bug when idp sync hits deleted orgs (#17405)
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.
1 parent 64172d3 commit 669e790

File tree

9 files changed

+242
-33
lines changed

9 files changed

+242
-33
lines changed

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -886,7 +886,7 @@ func (s *MethodTestSuite) TestOrganization() {
886886
_ = dbgen.OrganizationMember(s.T(), db, database.OrganizationMember{UserID: u.ID, OrganizationID: a.ID})
887887
b := dbgen.Organization(s.T(), db, database.Organization{})
888888
_ = dbgen.OrganizationMember(s.T(), db, database.OrganizationMember{UserID: u.ID, OrganizationID: b.ID})
889-
check.Args(database.GetOrganizationsByUserIDParams{UserID: u.ID, Deleted: false}).Asserts(a, policy.ActionRead, b, policy.ActionRead).Returns(slice.New(a, b))
889+
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))
890890
}))
891891
s.Run("InsertOrganization", s.Subtest(func(db database.Store, check *expects) {
892892
check.Args(database.InsertOrganizationParams{
@@ -994,8 +994,7 @@ func (s *MethodTestSuite) TestOrganization() {
994994
member, policy.ActionRead,
995995
member, policy.ActionDelete).
996996
WithNotAuthorized("no rows").
997-
WithCancelled(cancelledErr).
998-
ErrorsWithInMemDB(sql.ErrNoRows)
997+
WithCancelled(cancelledErr)
999998
}))
1000999
s.Run("UpdateOrganization", s.Subtest(func(db database.Store, check *expects) {
10011000
o := dbgen.Organization(s.T(), db, database.Organization{

‎coderd/database/dbfake/builder.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ type OrganizationBuilder struct {
1717
t *testing.T
1818
db database.Store
1919
seed database.Organization
20+
delete bool
2021
allUsersAllowance int32
2122
members []uuid.UUID
2223
groups map[database.Group][]uuid.UUID
@@ -45,6 +46,12 @@ func (b OrganizationBuilder) EveryoneAllowance(allowance int) OrganizationBuilde
4546
return b
4647
}
4748

49+
func (b OrganizationBuilder) Deleted(deleted bool) OrganizationBuilder {
50+
//nolint: revive // returns modified struct
51+
b.delete = deleted
52+
return b
53+
}
54+
4855
func (b OrganizationBuilder) Seed(seed database.Organization) OrganizationBuilder {
4956
//nolint: revive // returns modified struct
5057
b.seed = seed
@@ -119,6 +126,17 @@ func (b OrganizationBuilder) Do() OrganizationResponse {
119126
}
120127
}
121128

129+
if b.delete {
130+
now := dbtime.Now()
131+
err = b.db.UpdateOrganizationDeletedByID(ctx, database.UpdateOrganizationDeletedByIDParams{
132+
UpdatedAt: now,
133+
ID: org.ID,
134+
})
135+
require.NoError(b.t, err)
136+
org.Deleted = true
137+
org.UpdatedAt = now
138+
}
139+
122140
return OrganizationResponse{
123141
Org: org,
124142
AllUsersGroup: everyone,

‎coderd/database/dbmem/dbmem.go

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2357,10 +2357,13 @@ func (q *FakeQuerier) DeleteOrganizationMember(ctx context.Context, arg database
23572357
q.mutex.Lock()
23582358
defer q.mutex.Unlock()
23592359

2360-
deleted := slices.DeleteFunc(q.data.organizationMembers, func(member database.OrganizationMember) bool {
2361-
return member.OrganizationID == arg.OrganizationID && member.UserID == arg.UserID
2360+
deleted := false
2361+
q.data.organizationMembers = slices.DeleteFunc(q.data.organizationMembers, func(member database.OrganizationMember) bool {
2362+
match := member.OrganizationID == arg.OrganizationID && member.UserID == arg.UserID
2363+
deleted = deleted || match
2364+
return match
23622365
})
2363-
if len(deleted) == 0 {
2366+
if !deleted {
23642367
return sql.ErrNoRows
23652368
}
23662369

@@ -4156,6 +4159,9 @@ func (q *FakeQuerier) GetOrganizations(_ context.Context, args database.GetOrgan
41564159
if args.Name != "" && !strings.EqualFold(org.Name, args.Name) {
41574160
continue
41584161
}
4162+
if args.Deleted != org.Deleted {
4163+
continue
4164+
}
41594165
tmp = append(tmp, org)
41604166
}
41614167

@@ -4172,7 +4178,11 @@ func (q *FakeQuerier) GetOrganizationsByUserID(_ context.Context, arg database.G
41724178
continue
41734179
}
41744180
for _, organization := range q.organizations {
4175-
if organization.ID != organizationMember.OrganizationID || organization.Deleted != arg.Deleted {
4181+
if organization.ID != organizationMember.OrganizationID {
4182+
continue
4183+
}
4184+
4185+
if arg.Deleted.Valid && organization.Deleted != arg.Deleted.Bool {
41764186
continue
41774187
}
41784188
organizations = append(organizations, organization)

‎coderd/database/queries.sql.go

Lines changed: 9 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/organizations.sql

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,13 @@ SELECT
5555
FROM
5656
organizations
5757
WHERE
58-
-- Optionally include deleted organizations
59-
deleted = @deleted AND
58+
-- Optionally provide a filter for deleted organizations.
59+
CASE WHEN
60+
sqlc.narg('deleted') :: boolean IS NULL THEN
61+
true
62+
ELSE
63+
deleted = sqlc.narg('deleted')
64+
END AND
6065
id = ANY(
6166
SELECT
6267
organization_id

‎coderd/idpsync/organization.go

Lines changed: 53 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -92,14 +92,16 @@ func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, u
9292
return nil // No sync configured, nothing to do
9393
}
9494

95-
expectedOrgs, err := orgSettings.ParseClaims(ctx, tx, params.MergedClaims)
95+
expectedOrgIDs, err := orgSettings.ParseClaims(ctx, tx, params.MergedClaims)
9696
if err != nil {
9797
return xerrors.Errorf("organization claims: %w", err)
9898
}
9999

100+
// Fetch all organizations, even deleted ones. This is to remove a user
101+
// from any deleted organizations they may be in.
100102
existingOrgs, err := tx.GetOrganizationsByUserID(ctx, database.GetOrganizationsByUserIDParams{
101103
UserID: user.ID,
102-
Deleted: false,
104+
Deleted: sql.NullBool{},
103105
})
104106
if err != nil {
105107
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
109111
return org.ID
110112
})
111113

114+
// finalExpected is the final set of org ids the user is expected to be in.
115+
// Deleted orgs are omitted from this set.
116+
finalExpected := expectedOrgIDs
117+
if len(expectedOrgIDs) > 0 {
118+
// If you pass in an empty slice to the db arg, you get all orgs. So the slice
119+
// has to be non-empty to get the expected set. Logically it also does not make
120+
// sense to fetch an empty set from the db.
121+
expectedOrganizations, err := tx.GetOrganizations(ctx, database.GetOrganizationsParams{
122+
IDs: expectedOrgIDs,
123+
// Do not include deleted organizations. Omitting deleted orgs will remove the
124+
// user from any deleted organizations they are a member of.
125+
Deleted: false,
126+
})
127+
if err != nil {
128+
return xerrors.Errorf("failed to get expected organizations: %w", err)
129+
}
130+
finalExpected = db2sdk.List(expectedOrganizations, func(org database.Organization) uuid.UUID {
131+
return org.ID
132+
})
133+
}
134+
112135
// Find the difference in the expected and the existing orgs, and
113136
// correct the set of orgs the user is a member of.
114-
add, remove := slice.SymmetricDifference(existingOrgIDs, expectedOrgs)
115-
notExists := make([]uuid.UUID, 0)
137+
add, remove := slice.SymmetricDifference(existingOrgIDs, finalExpected)
138+
// notExists is purely for debugging. It logs when the settings want
139+
// a user in an organization, but the organization does not exist.
140+
notExists := slice.DifferenceFunc(expectedOrgIDs, finalExpected, func(a, b uuid.UUID) bool {
141+
return a == b
142+
})
116143
for _, orgID := range add {
117144
_, err := tx.InsertOrganizationMember(ctx, database.InsertOrganizationMemberParams{
118145
OrganizationID: orgID,
@@ -123,9 +150,30 @@ func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, u
123150
})
124151
if err != nil {
125152
if xerrors.Is(err, sql.ErrNoRows) {
153+
// This should not happen because we check the org existence
154+
// beforehand.
126155
notExists = append(notExists, orgID)
127156
continue
128157
}
158+
159+
if database.IsUniqueViolation(err, database.UniqueOrganizationMembersPkey) {
160+
// If we hit this error we have a bug. The user already exists in the
161+
// organization, but was not detected to be at the start of this function.
162+
// Instead of failing the function, an error will be logged. This is to not bring
163+
// down the entire syncing behavior from a single failed org. Failing this can
164+
// prevent user logins, so only fatal non-recoverable errors should be returned.
165+
//
166+
// Inserting a user is privilege escalation. So skipping this instead of failing
167+
// leaves the user with fewer permissions. So this is safe from a security
168+
// perspective to continue.
169+
s.Logger.Error(ctx, "syncing user to organization failed as they are already a member, please report this failure to Coder",
170+
slog.F("user_id", user.ID),
171+
slog.F("username", user.Username),
172+
slog.F("organization_id", orgID),
173+
slog.Error(err),
174+
)
175+
continue
176+
}
129177
return xerrors.Errorf("add user to organization: %w", err)
130178
}
131179
}
@@ -141,6 +189,7 @@ func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, u
141189
}
142190

143191
if len(notExists) > 0 {
192+
notExists = slice.Unique(notExists) // Remove duplicates
144193
s.Logger.Debug(ctx, "organizations do not exist but attempted to use in org sync",
145194
slog.F("not_found", notExists),
146195
slog.F("user_id", user.ID),

‎coderd/idpsync/organizations_test.go

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,19 @@
11
package idpsync_test
22

33
import (
4+
"database/sql"
45
"testing"
56

67
"github.com/golang-jwt/jwt/v4"
78
"github.com/google/uuid"
89
"github.com/stretchr/testify/require"
910

1011
"cdr.dev/slog/sloggers/slogtest"
12+
"github.com/coder/coder/v2/coderd/database"
13+
"github.com/coder/coder/v2/coderd/database/db2sdk"
14+
"github.com/coder/coder/v2/coderd/database/dbfake"
15+
"github.com/coder/coder/v2/coderd/database/dbgen"
16+
"github.com/coder/coder/v2/coderd/database/dbtestutil"
1117
"github.com/coder/coder/v2/coderd/idpsync"
1218
"github.com/coder/coder/v2/coderd/runtimeconfig"
1319
"github.com/coder/coder/v2/testutil"
@@ -38,3 +44,108 @@ func TestParseOrganizationClaims(t *testing.T) {
3844
require.False(t, params.SyncEntitled)
3945
})
4046
}
47+
48+
func TestSyncOrganizations(t *testing.T) {
49+
t.Parallel()
50+
51+
// This test creates some deleted organizations and checks the behavior is
52+
// correct.
53+
t.Run("SyncUserToDeletedOrg", func(t *testing.T) {
54+
t.Parallel()
55+
56+
ctx := testutil.Context(t, testutil.WaitMedium)
57+
db, _ := dbtestutil.NewDB(t)
58+
user := dbgen.User(t, db, database.User{})
59+
60+
// Create orgs for:
61+
// - stays = User is a member, and stays
62+
// - leaves = User is a member, and leaves
63+
// - joins = User is not a member, and joins
64+
// For deleted orgs, the user **should not** be a member of afterwards.
65+
// - deletedStays = User is a member of deleted org, and wants to stay
66+
// - deletedLeaves = User is a member of deleted org, and wants to leave
67+
// - deletedJoins = User is not a member of deleted org, and wants to join
68+
stays := dbfake.Organization(t, db).Members(user).Do()
69+
leaves := dbfake.Organization(t, db).Members(user).Do()
70+
joins := dbfake.Organization(t, db).Do()
71+
72+
deletedStays := dbfake.Organization(t, db).Members(user).Deleted(true).Do()
73+
deletedLeaves := dbfake.Organization(t, db).Members(user).Deleted(true).Do()
74+
deletedJoins := dbfake.Organization(t, db).Deleted(true).Do()
75+
76+
// Now sync the user to the deleted organization
77+
s := idpsync.NewAGPLSync(
78+
slogtest.Make(t, &slogtest.Options{}),
79+
runtimeconfig.NewManager(),
80+
idpsync.DeploymentSyncSettings{
81+
OrganizationField: "orgs",
82+
OrganizationMapping: map[string][]uuid.UUID{
83+
"stay": {stays.Org.ID, deletedStays.Org.ID},
84+
"leave": {leaves.Org.ID, deletedLeaves.Org.ID},
85+
"join": {joins.Org.ID, deletedJoins.Org.ID},
86+
},
87+
OrganizationAssignDefault: false,
88+
},
89+
)
90+
91+
err := s.SyncOrganizations(ctx, db, user, idpsync.OrganizationParams{
92+
SyncEntitled: true,
93+
MergedClaims: map[string]interface{}{
94+
"orgs": []string{"stay", "join"},
95+
},
96+
})
97+
require.NoError(t, err)
98+
99+
orgs, err := db.GetOrganizationsByUserID(ctx, database.GetOrganizationsByUserIDParams{
100+
UserID: user.ID,
101+
Deleted: sql.NullBool{},
102+
})
103+
require.NoError(t, err)
104+
require.Len(t, orgs, 2)
105+
106+
// Verify the user only exists in 2 orgs. The one they stayed, and the one they
107+
// joined.
108+
inIDs := db2sdk.List(orgs, func(org database.Organization) uuid.UUID {
109+
return org.ID
110+
})
111+
require.ElementsMatch(t, []uuid.UUID{stays.Org.ID, joins.Org.ID}, inIDs)
112+
})
113+
114+
t.Run("UserToZeroOrgs", func(t *testing.T) {
115+
t.Parallel()
116+
117+
ctx := testutil.Context(t, testutil.WaitMedium)
118+
db, _ := dbtestutil.NewDB(t)
119+
user := dbgen.User(t, db, database.User{})
120+
121+
deletedLeaves := dbfake.Organization(t, db).Members(user).Deleted(true).Do()
122+
123+
// Now sync the user to the deleted organization
124+
s := idpsync.NewAGPLSync(
125+
slogtest.Make(t, &slogtest.Options{}),
126+
runtimeconfig.NewManager(),
127+
idpsync.DeploymentSyncSettings{
128+
OrganizationField: "orgs",
129+
OrganizationMapping: map[string][]uuid.UUID{
130+
"leave": {deletedLeaves.Org.ID},
131+
},
132+
OrganizationAssignDefault: false,
133+
},
134+
)
135+
136+
err := s.SyncOrganizations(ctx, db, user, idpsync.OrganizationParams{
137+
SyncEntitled: true,
138+
MergedClaims: map[string]interface{}{
139+
"orgs": []string{},
140+
},
141+
})
142+
require.NoError(t, err)
143+
144+
orgs, err := db.GetOrganizationsByUserID(ctx, database.GetOrganizationsByUserIDParams{
145+
UserID: user.ID,
146+
Deleted: sql.NullBool{},
147+
})
148+
require.NoError(t, err)
149+
require.Len(t, orgs, 0)
150+
})
151+
}

‎coderd/users.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1340,7 +1340,7 @@ func (api *API) organizationsByUser(rw http.ResponseWriter, r *http.Request) {
13401340

13411341
organizations, err := api.Database.GetOrganizationsByUserID(ctx, database.GetOrganizationsByUserIDParams{
13421342
UserID: user.ID,
1343-
Deleted: false,
1343+
Deleted: sql.NullBool{Bool: false, Valid: true},
13441344
})
13451345
if errors.Is(err, sql.ErrNoRows) {
13461346
err = nil

0 commit comments

Comments
 (0)