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

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
Show file tree
Hide file tree
Changes fromall commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletionscoderd/database/dbauthz/dbauthz_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -878,7 +878,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{
Expand DownExpand Up@@ -987,8 +987,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{
Expand Down
18 changes: 18 additions & 0 deletionscoderd/database/dbfake/builder.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -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
Expand DownExpand Up@@ -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,
Expand Down
18 changes: 14 additions & 4 deletionscoderd/database/dbmem/dbmem.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2350,10 +2350,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
})
iflen(deleted) == 0 {
if!deleted {
return sql.ErrNoRows
}

Expand DownExpand Up@@ -4143,6 +4146,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)
}

Expand All@@ -4159,7 +4165,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)
Expand Down
13 changes: 9 additions & 4 deletionscoderd/database/queries.sql.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.

9 changes: 7 additions & 2 deletionscoderd/database/queries/organizations.sql
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand Down
57 changes: 53 additions & 4 deletionscoderd/idpsync/organization.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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)
Expand All@@ -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,
Expand All@@ -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)
}
}
Expand All@@ -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),
Expand Down
111 changes: 111 additions & 0 deletionscoderd/idpsync/organizations_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,19 @@
package idpsync_test

import (
"database/sql"
"testing"

"github.com/golang-jwt/jwt/v4"
"github.com/google/uuid"
"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"
Expand DownExpand Up@@ -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 changes: 1 addition & 1 deletioncoderd/users.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1302,7 +1302,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
Expand Down
Loading
Loading
[8]ページ先頭
©2009-2025 Movatter.jp