Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit669e790

Browse files
authored
test: add unit test to excercise bug when idp sync hits deleted orgs (#17405)
Deleted organizations are still attempting to sync members. This causesan error on inserting the member, and would likely cause issues later inthe sync process even if that member is inserted. Deleted orgs should beskipped.
1 parent64172d3 commit669e790

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+
deletebool
2021
allUsersAllowanceint32
2122
members []uuid.UUID
2223
groupsmap[database.Group][]uuid.UUID
@@ -45,6 +46,12 @@ func (b OrganizationBuilder) EveryoneAllowance(allowance int) OrganizationBuilde
4546
returnb
4647
}
4748

49+
func (bOrganizationBuilder)Deleted(deletedbool)OrganizationBuilder {
50+
//nolint: revive // returns modified struct
51+
b.delete=deleted
52+
returnb
53+
}
54+
4855
func (bOrganizationBuilder)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+
ifb.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
returnOrganizationResponse{
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
deferq.mutex.Unlock()
23592359

2360-
deleted:=slices.DeleteFunc(q.data.organizationMembers,func(member database.OrganizationMember)bool {
2361-
returnmember.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+
returnmatch
23622365
})
2363-
iflen(deleted)==0 {
2366+
if!deleted {
23642367
returnsql.ErrNoRows
23652368
}
23662369

@@ -4156,6 +4159,9 @@ func (q *FakeQuerier) GetOrganizations(_ context.Context, args database.GetOrgan
41564159
ifargs.Name!=""&&!strings.EqualFold(org.Name,args.Name) {
41574160
continue
41584161
}
4162+
ifargs.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:=rangeq.organizations {
4175-
iforganization.ID!=organizationMember.OrganizationID||organization.Deleted!=arg.Deleted {
4181+
iforganization.ID!=organizationMember.OrganizationID {
4182+
continue
4183+
}
4184+
4185+
ifarg.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 aboutcustomizing 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= @deletedAND
58+
-- Optionally provide a filter for deleted organizations.
59+
CASE WHEN
60+
sqlc.narg('deleted') ::boolean ISNULL THEN
61+
true
62+
ELSE
63+
deleted=sqlc.narg('deleted')
64+
ENDAND
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
returnnil// 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
iferr!=nil {
9797
returnxerrors.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
iferr!=nil {
105107
returnxerrors.Errorf("failed to get user organizations: %w",err)
@@ -109,10 +111,35 @@ func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, u
109111
returnorg.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+
iflen(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+
iferr!=nil {
128+
returnxerrors.Errorf("failed to get expected organizations: %w",err)
129+
}
130+
finalExpected=db2sdk.List(expectedOrganizations,func(org database.Organization) uuid.UUID {
131+
returnorg.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+
returna==b
142+
})
116143
for_,orgID:=rangeadd {
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
iferr!=nil {
125152
ifxerrors.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+
ifdatabase.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
returnxerrors.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
iflen(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+
funcTestSyncOrganizations(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+
returnorg.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
iferrors.Is(err,sql.ErrNoRows) {
13461346
err=nil

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp