Skip to content

Commit 7670820

Browse files
gcp-cherry-pick-bot[bot]Emyrk
gcp-cherry-pick-bot[bot]
and
Emyrk
authored
test: add unit test to excercise bug when idp sync hits deleted orgs (cherry-pick #17405) (#17423)
Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
1 parent b56ffe0 commit 7670820

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
@@ -839,7 +839,7 @@ func (s *MethodTestSuite) TestOrganization() {
839839
_ = dbgen.OrganizationMember(s.T(), db, database.OrganizationMember{UserID: u.ID, OrganizationID: a.ID})
840840
b := dbgen.Organization(s.T(), db, database.Organization{})
841841
_ = dbgen.OrganizationMember(s.T(), db, database.OrganizationMember{UserID: u.ID, OrganizationID: b.ID})
842-
check.Args(database.GetOrganizationsByUserIDParams{UserID: u.ID, Deleted: false}).Asserts(a, policy.ActionRead, b, policy.ActionRead).Returns(slice.New(a, b))
842+
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))
843843
}))
844844
s.Run("InsertOrganization", s.Subtest(func(db database.Store, check *expects) {
845845
check.Args(database.InsertOrganizationParams{
@@ -948,8 +948,7 @@ func (s *MethodTestSuite) TestOrganization() {
948948
member, policy.ActionRead,
949949
member, policy.ActionDelete).
950950
WithNotAuthorized("no rows").
951-
WithCancelled(cancelledErr).
952-
ErrorsWithInMemDB(sql.ErrNoRows)
951+
WithCancelled(cancelledErr)
953952
}))
954953
s.Run("UpdateOrganization", s.Subtest(func(db database.Store, check *expects) {
955954
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
@@ -44,6 +45,12 @@ func (b OrganizationBuilder) EveryoneAllowance(allowance int) OrganizationBuilde
4445
return b
4546
}
4647

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

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

‎coderd/database/dbmem/dbmem.go

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

2170-
deleted := slices.DeleteFunc(q.data.organizationMembers, func(member database.OrganizationMember) bool {
2171-
return member.OrganizationID == arg.OrganizationID && member.UserID == arg.UserID
2170+
deleted := false
2171+
q.data.organizationMembers = slices.DeleteFunc(q.data.organizationMembers, func(member database.OrganizationMember) bool {
2172+
match := member.OrganizationID == arg.OrganizationID && member.UserID == arg.UserID
2173+
deleted = deleted || match
2174+
return match
21722175
})
2173-
if len(deleted) == 0 {
2176+
if !deleted {
21742177
return sql.ErrNoRows
21752178
}
21762179

@@ -3758,6 +3761,9 @@ func (q *FakeQuerier) GetOrganizations(_ context.Context, args database.GetOrgan
37583761
if args.Name != "" && !strings.EqualFold(org.Name, args.Name) {
37593762
continue
37603763
}
3764+
if args.Deleted != org.Deleted {
3765+
continue
3766+
}
37613767
tmp = append(tmp, org)
37623768
}
37633769

@@ -3774,7 +3780,11 @@ func (q *FakeQuerier) GetOrganizationsByUserID(_ context.Context, arg database.G
37743780
continue
37753781
}
37763782
for _, organization := range q.organizations {
3777-
if organization.ID != organizationMember.OrganizationID || organization.Deleted != arg.Deleted {
3783+
if organization.ID != organizationMember.OrganizationID {
3784+
continue
3785+
}
3786+
3787+
if arg.Deleted.Valid && organization.Deleted != arg.Deleted.Bool {
37783788
continue
37793789
}
37803790
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
@@ -1274,7 +1274,7 @@ func (api *API) organizationsByUser(rw http.ResponseWriter, r *http.Request) {
12741274

12751275
organizations, err := api.Database.GetOrganizationsByUserID(ctx, database.GetOrganizationsByUserIDParams{
12761276
UserID: user.ID,
1277-
Deleted: false,
1277+
Deleted: sql.NullBool{Bool: false, Valid: true},
12781278
})
12791279
if errors.Is(err, sql.ErrNoRows) {
12801280
err = nil

0 commit comments

Comments
 (0)