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

Commit7670820

Browse files
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 parentb56ffe0 commit7670820

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

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

2170-
deleted:=slices.DeleteFunc(q.data.organizationMembers,func(member database.OrganizationMember)bool {
2171-
returnmember.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+
returnmatch
21722175
})
2173-
iflen(deleted)==0 {
2176+
if!deleted {
21742177
returnsql.ErrNoRows
21752178
}
21762179

@@ -3758,6 +3761,9 @@ func (q *FakeQuerier) GetOrganizations(_ context.Context, args database.GetOrgan
37583761
ifargs.Name!=""&&!strings.EqualFold(org.Name,args.Name) {
37593762
continue
37603763
}
3764+
ifargs.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:=rangeq.organizations {
3777-
iforganization.ID!=organizationMember.OrganizationID||organization.Deleted!=arg.Deleted {
3783+
iforganization.ID!=organizationMember.OrganizationID {
3784+
continue
3785+
}
3786+
3787+
ifarg.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 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
@@ -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
iferrors.Is(err,sql.ErrNoRows) {
12801280
err=nil

0 commit comments

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