From a8d0495fefd1b6bea95b3f103c8bda8e6f1fb50b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 28 Aug 2024 09:35:36 -0500 Subject: [PATCH 01/23] chore: implement filters for the organizations query From d788be77e2d09a01fd2b36c772b1db1326091d60 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 26 Aug 2024 09:34:03 -0500 Subject: [PATCH 02/23] chore: implement organization sync and create idpsync package IDP sync code should be refactored to be contained in it's own package. This will make it easier to maintain and test moving forward. --- coderd/database/models.go | 2 +- coderd/database/querier.go | 2 +- coderd/idpsync/idpsync.go | 125 +++++++++++++++++ .../idpsync_test.go} | 6 +- coderd/idpsync/organization.go | 127 ++++++++++++++++++ coderd/userauth.go | 70 +++------- 6 files changed, 277 insertions(+), 55 deletions(-) create mode 100644 coderd/idpsync/idpsync.go rename coderd/{userauth_internal_test.go => idpsync/idpsync_test.go} (95%) create mode 100644 coderd/idpsync/organization.go diff --git a/coderd/database/models.go b/coderd/database/models.go index 959609d82eb79..18571981a4c92 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -1,6 +1,6 @@ // Code generated by sqlc. DO NOT EDIT. // versions: -// sqlc v1.25.0 +// sqlc v1.26.0 package database diff --git a/coderd/database/querier.go b/coderd/database/querier.go index a159735cbbf69..974478a767579 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -1,6 +1,6 @@ // Code generated by sqlc. DO NOT EDIT. // versions: -// sqlc v1.25.0 +// sqlc v1.26.0 package database diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go new file mode 100644 index 0000000000000..18bce4488762f --- /dev/null +++ b/coderd/idpsync/idpsync.go @@ -0,0 +1,125 @@ +package idpsync + +import ( + "net/http" + "strings" + + "github.com/google/uuid" + "golang.org/x/xerrors" + + "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/entitlements" + "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/site" +) + +// IDPSync is the configuration for syncing user information from an external +// IDP. All related code to syncing user information should be in this package. +type IDPSync struct { + logger slog.Logger + entitlements *entitlements.Set + + // OrganizationField selects the claim field to be used as the created user's + // organizations. If the field is the empty string, then no organization updates + // will ever come from the OIDC provider. + OrganizationField string + // OrganizationMapping controls how organizations returned by the OIDC provider get mapped + OrganizationMapping map[string][]uuid.UUID + // OrganizationAlwaysAssign will ensure all users that authenticate will be + // placed into the specified organizations. + OrganizationAlwaysAssign []uuid.UUID +} + +func NewSync(logger slog.Logger, set *entitlements.Set) *IDPSync { + return &IDPSync{ + logger: logger.Named("idp-sync"), + entitlements: set, + } +} + +// ParseStringSliceClaim parses the claim for groups and roles, expected []string. +// +// Some providers like ADFS return a single string instead of an array if there +// is only 1 element. So this function handles the edge cases. +func ParseStringSliceClaim(claim interface{}) ([]string, error) { + groups := make([]string, 0) + if claim == nil { + return groups, nil + } + + // The simple case is the type is exactly what we expected + asStringArray, ok := claim.([]string) + if ok { + return asStringArray, nil + } + + asArray, ok := claim.([]interface{}) + if ok { + for i, item := range asArray { + asString, ok := item.(string) + if !ok { + return nil, xerrors.Errorf("invalid claim type. Element %d expected a string, got: %T", i, item) + } + groups = append(groups, asString) + } + return groups, nil + } + + asString, ok := claim.(string) + if ok { + if asString == "" { + // Empty string should be 0 groups. + return []string{}, nil + } + // If it is a single string, first check if it is a csv. + // If a user hits this, it is likely a misconfiguration and they need + // to reconfigure their IDP to send an array instead. + if strings.Contains(asString, ",") { + return nil, xerrors.Errorf("invalid claim type. Got a csv string (%q), change this claim to return an array of strings instead.", asString) + } + return []string{asString}, nil + } + + // Not sure what the user gave us. + return nil, xerrors.Errorf("invalid claim type. Expected an array of strings, got: %T", claim) +} + +// HttpError is a helper struct for returning errors from the IDP sync process. +// A regular error is not sufficient because many of these errors are surfaced +// to a user logging in, and the errors should be descriptive. +type HttpError struct { + Code int + Msg string + Detail string + RenderStaticPage bool + RenderDetailMarkdown bool +} + +func (e HttpError) Write(rw http.ResponseWriter, r *http.Request) { + if e.RenderStaticPage { + site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ + Status: e.Code, + HideStatus: true, + Title: e.Msg, + Description: e.Detail, + RetryEnabled: false, + DashboardURL: "/login", + + RenderDescriptionMarkdown: e.RenderDetailMarkdown, + }) + return + } + httpapi.Write(r.Context(), rw, e.Code, codersdk.Response{ + Message: e.Msg, + Detail: e.Detail, + }) +} + +func (e HttpError) Error() string { + if e.Detail != "" { + return e.Detail + } + + return e.Msg +} diff --git a/coderd/userauth_internal_test.go b/coderd/idpsync/idpsync_test.go similarity index 95% rename from coderd/userauth_internal_test.go rename to coderd/idpsync/idpsync_test.go index 421e654995fdf..673d6131da6ef 100644 --- a/coderd/userauth_internal_test.go +++ b/coderd/idpsync/idpsync_test.go @@ -1,10 +1,12 @@ -package coderd +package idpsync_test import ( "encoding/json" "testing" "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/idpsync" ) func TestParseStringSliceClaim(t *testing.T) { @@ -123,7 +125,7 @@ func TestParseStringSliceClaim(t *testing.T) { require.NoError(t, err, "unmarshal json claim") } - found, err := parseStringSliceClaim(c.GoClaim) + found, err := idpsync.ParseStringSliceClaim(c.GoClaim) if c.ErrorExpected { require.Error(t, err) } else { diff --git a/coderd/idpsync/organization.go b/coderd/idpsync/organization.go new file mode 100644 index 0000000000000..902172aaa6922 --- /dev/null +++ b/coderd/idpsync/organization.go @@ -0,0 +1,127 @@ +package idpsync + +import ( + "context" + "database/sql" + "net/http" + + "github.com/google/uuid" + "golang.org/x/xerrors" + + "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/db2sdk" + "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/database/dbtime" + "github.com/coder/coder/v2/coderd/util/slice" +) + +func (s IDPSync) ParseOrganizationClaims(ctx context.Context, mergedClaims map[string]interface{}) (OrganizationParams, *HttpError) { + // nolint:gocritic // all syncing is done as a system user + ctx = dbauthz.AsSystemRestricted(ctx) + + // Copy in the always included static set of organizations. + userOrganizations := make([]uuid.UUID, len(s.OrganizationAlwaysAssign)) + copy(userOrganizations, s.OrganizationAlwaysAssign) + + // Pull extra organizations from the claims. + if s.OrganizationField != "" { + organizationRaw, ok := mergedClaims[s.OrganizationField] + if ok { + parsedOrganizations, err := ParseStringSliceClaim(organizationRaw) + if err != nil { + return OrganizationParams{}, &HttpError{ + Code: http.StatusBadRequest, + Msg: "Failed to sync organizations from the OIDC claims", + Detail: err.Error(), + RenderStaticPage: false, + RenderDetailMarkdown: false, + } + } + + // Keep track of which claims are not mapped for debugging purposes. + var ignored []string + for _, parsedOrg := range parsedOrganizations { + if mappedOrganization, ok := s.OrganizationMapping[parsedOrg]; ok { + // parsedOrg is in the mapping, so add the mapped organizations to the + // user's organizations. + userOrganizations = append(userOrganizations, mappedOrganization...) + } else { + ignored = append(ignored, parsedOrg) + } + } + + s.logger.Debug(ctx, "parsed organizations from claim", + slog.F("len", len(parsedOrganizations)), + slog.F("ignored", ignored), + slog.F("organizations", parsedOrganizations), + ) + } + } + + return OrganizationParams{ + Organizations: userOrganizations, + }, nil +} + +type OrganizationParams struct { + // Organizations is the list of organizations the user should be a member of + // assuming syncing is turned on. + Organizations []uuid.UUID +} + +func (s IDPSync) SyncOrganizations(ctx context.Context, tx database.Store, user database.User, params OrganizationParams) error { + // nolint:gocritic // all syncing is done as a system user + ctx = dbauthz.AsSystemRestricted(ctx) + + existingOrgs, err := tx.GetOrganizationsByUserID(ctx, user.ID) + if err != nil { + return xerrors.Errorf("failed to get user organizations: %w", err) + } + + existingOrgIDs := db2sdk.List(existingOrgs, 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, params.Organizations) + notExists := make([]uuid.UUID, 0) + for _, orgID := range add { + //nolint:gocritic // System actor being used to assign orgs + _, err := tx.InsertOrganizationMember(dbauthz.AsSystemRestricted(ctx), database.InsertOrganizationMemberParams{ + OrganizationID: orgID, + UserID: user.ID, + CreatedAt: dbtime.Now(), + UpdatedAt: dbtime.Now(), + Roles: []string{}, + }) + if err != nil { + if xerrors.Is(err, sql.ErrNoRows) { + notExists = append(notExists, orgID) + continue + } + return xerrors.Errorf("add user to organization: %w", err) + } + } + + for _, orgID := range remove { + //nolint:gocritic // System actor being used to assign orgs + err := tx.DeleteOrganizationMember(dbauthz.AsSystemRestricted(ctx), database.DeleteOrganizationMemberParams{ + OrganizationID: orgID, + UserID: user.ID, + }) + if err != nil { + return xerrors.Errorf("remove user from organization: %w", err) + } + } + + if len(notExists) > 0 { + 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), + slog.F("username", user.Username), + ) + } + return nil +} diff --git a/coderd/userauth.go b/coderd/userauth.go index 1a5488d2d6ded..589b70a178116 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -25,6 +25,7 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/idpsync" "github.com/coder/coder/v2/coderd/apikey" "github.com/coder/coder/v2/coderd/audit" @@ -767,6 +768,16 @@ type OIDCConfig struct { // UserRolesDefault is the default set of roles to assign to a user if role sync // is enabled. UserRolesDefault []string + // OrganizationField selects the claim field to be used as the created user's + // organizations. If the field is the empty string, then no organization updates + // will ever come from the OIDC provider. + OrganizationField string + // OrganizationMapping controls how organizations returned by the OIDC provider get mapped + OrganizationMapping map[string][]string + // OrganizationAlwaysAssign will ensure all users that authenticate will be + // placed into the specified organizations. 'default' is a special keyword + // that will use the `IsDefault` organization. + OrganizationAlwaysAssign []string // SignInText is the text to display on the OIDC login button SignInText string // IconURL points to the URL of an icon to display on the OIDC login button @@ -1095,7 +1106,7 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac usingGroups = true groupsRaw, ok := mergedClaims[api.OIDCConfig.GroupField] if ok { - parsedGroups, err := parseStringSliceClaim(groupsRaw) + parsedGroups, err := idpsync.ParseStringSliceClaim(groupsRaw) if err != nil { api.Logger.Debug(ctx, "groups field was an unknown type in oidc claims", slog.F("type", fmt.Sprintf("%T", groupsRaw)), @@ -1174,7 +1185,7 @@ func (api *API) oidcRoles(ctx context.Context, mergedClaims map[string]interface rolesRow = []interface{}{} } - parsedRoles, err := parseStringSliceClaim(rolesRow) + parsedRoles, err := idpsync.ParseStringSliceClaim(rolesRow) if err != nil { api.Logger.Error(ctx, "oidc claims user roles field was an unknown type", slog.F("type", fmt.Sprintf("%T", rolesRow)), @@ -1264,6 +1275,8 @@ type oauthLoginParams struct { Username string Name string AvatarURL string + // OrganizationSync has the organizations that the user will be assigned to. + OrganizationSync idpsync.OrganizationParams // Is UsingGroups is true, then the user will be assigned // to the Groups provided. UsingGroups bool @@ -1438,8 +1451,10 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C //nolint:gocritic user, err = api.CreateUser(dbauthz.AsSystemRestricted(ctx), tx, CreateUserRequest{ CreateUserRequestWithOrgs: codersdk.CreateUserRequestWithOrgs{ - Email: params.Email, - Username: params.Username, + Email: params.Email, + Username: params.Username, + // TODO: Remove this, and only use organization sync from + // params OrganizationIDs: []uuid.UUID{defaultOrganization.ID}, }, LoginType: params.LoginType, @@ -1868,50 +1883,3 @@ func wrongLoginTypeHTTPError(user database.LoginType, params database.LoginType) params, user, addedMsg), } } - -// parseStringSliceClaim parses the claim for groups and roles, expected []string. -// -// Some providers like ADFS return a single string instead of an array if there -// is only 1 element. So this function handles the edge cases. -func parseStringSliceClaim(claim interface{}) ([]string, error) { - groups := make([]string, 0) - if claim == nil { - return groups, nil - } - - // The simple case is the type is exactly what we expected - asStringArray, ok := claim.([]string) - if ok { - return asStringArray, nil - } - - asArray, ok := claim.([]interface{}) - if ok { - for i, item := range asArray { - asString, ok := item.(string) - if !ok { - return nil, xerrors.Errorf("invalid claim type. Element %d expected a string, got: %T", i, item) - } - groups = append(groups, asString) - } - return groups, nil - } - - asString, ok := claim.(string) - if ok { - if asString == "" { - // Empty string should be 0 groups. - return []string{}, nil - } - // If it is a single string, first check if it is a csv. - // If a user hits this, it is likely a misconfiguration and they need - // to reconfigure their IDP to send an array instead. - if strings.Contains(asString, ",") { - return nil, xerrors.Errorf("invalid claim type. Got a csv string (%q), change this claim to return an array of strings instead.", asString) - } - return []string{asString}, nil - } - - // Not sure what the user gave us. - return nil, xerrors.Errorf("invalid claim type. Expected an array of strings, got: %T", claim) -} From f596da131ccc1dd93c113b0c5d147c343ccf0685 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 26 Aug 2024 10:10:43 -0500 Subject: [PATCH 03/23] chore: refactor into agpl and enterprise --- cli/server.go | 6 ++ coderd/idpsync/idpsync.go | 38 +++++++---- coderd/idpsync/organization.go | 69 +++++++------------- coderd/userauth.go | 15 ++--- codersdk/deployment.go | 50 +++++++------- enterprise/coderd/enidpsync/enidpsync.go | 20 ++++++ enterprise/coderd/enidpsync/organizations.go | 63 ++++++++++++++++++ 7 files changed, 172 insertions(+), 89 deletions(-) create mode 100644 enterprise/coderd/enidpsync/enidpsync.go create mode 100644 enterprise/coderd/enidpsync/organizations.go diff --git a/cli/server.go b/cli/server.go index 53c974f373cd4..a5e140138a3b9 100644 --- a/cli/server.go +++ b/cli/server.go @@ -55,6 +55,7 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/sloghuman" + "github.com/coder/coder/v2/coderd/idpsync" "github.com/coder/pretty" "github.com/coder/quartz" "github.com/coder/retry" @@ -197,6 +198,11 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.De SignupsDisabledText: vals.OIDC.SignupsDisabledText.String(), IconURL: vals.OIDC.IconURL.String(), IgnoreEmailVerified: vals.OIDC.IgnoreEmailVerified.Value(), + IDPSync: idpsync.NewSync(logger, idpsync.SyncSettings{ + OrganizationField: vals.OIDC.OrganizationField.Value(), + OrganizationMapping: vals.OIDC.OrganizationMapping.Value, + OrganizationAssignDefault: vals.OIDC.OrganizationAssignDefault.Value, + }), }, nil } diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go index 18bce4488762f..f459a573585fd 100644 --- a/coderd/idpsync/idpsync.go +++ b/coderd/idpsync/idpsync.go @@ -1,6 +1,7 @@ package idpsync import ( + "context" "net/http" "strings" @@ -8,33 +9,46 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog" - "github.com/coder/coder/v2/coderd/entitlements" + "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/site" ) -// IDPSync is the configuration for syncing user information from an external +type IDPSync interface { + // ParseOrganizationClaims takes claims from an OIDC provider, and returns the + // organization sync params for assigning users into organizations. + ParseOrganizationClaims(ctx context.Context, _ map[string]interface{}) (OrganizationParams, *HttpError) + // SyncOrganizations assigns and removed users from organizations based on the + // provided params. + SyncOrganizations(ctx context.Context, tx database.Store, user database.User, params OrganizationParams) error +} + +// AGPLIDPSync is the configuration for syncing user information from an external // IDP. All related code to syncing user information should be in this package. -type IDPSync struct { - logger slog.Logger - entitlements *entitlements.Set +type AGPLIDPSync struct { + Logger slog.Logger + + SyncSettings +} +type SyncSettings struct { // OrganizationField selects the claim field to be used as the created user's // organizations. If the field is the empty string, then no organization updates // will ever come from the OIDC provider. OrganizationField string // OrganizationMapping controls how organizations returned by the OIDC provider get mapped OrganizationMapping map[string][]uuid.UUID - // OrganizationAlwaysAssign will ensure all users that authenticate will be - // placed into the specified organizations. - OrganizationAlwaysAssign []uuid.UUID + // OrganizationAssignDefault will ensure all users that authenticate will be + // placed into the default organization. This is mostly a hack to support + // legacy deployments. + OrganizationAssignDefault bool } -func NewSync(logger slog.Logger, set *entitlements.Set) *IDPSync { - return &IDPSync{ - logger: logger.Named("idp-sync"), - entitlements: set, +func NewSync(logger slog.Logger, settings SyncSettings) *AGPLIDPSync { + return &AGPLIDPSync{ + Logger: logger.Named("idp-sync"), + SyncSettings: settings, } } diff --git a/coderd/idpsync/organization.go b/coderd/idpsync/organization.go index 902172aaa6922..4cbbe8748afb3 100644 --- a/coderd/idpsync/organization.go +++ b/coderd/idpsync/organization.go @@ -3,7 +3,6 @@ package idpsync import ( "context" "database/sql" - "net/http" "github.com/google/uuid" "golang.org/x/xerrors" @@ -16,64 +15,46 @@ import ( "github.com/coder/coder/v2/coderd/util/slice" ) -func (s IDPSync) ParseOrganizationClaims(ctx context.Context, mergedClaims map[string]interface{}) (OrganizationParams, *HttpError) { +func (s AGPLIDPSync) ParseOrganizationClaims(ctx context.Context, _ map[string]interface{}) (OrganizationParams, *HttpError) { // nolint:gocritic // all syncing is done as a system user ctx = dbauthz.AsSystemRestricted(ctx) - // Copy in the always included static set of organizations. - userOrganizations := make([]uuid.UUID, len(s.OrganizationAlwaysAssign)) - copy(userOrganizations, s.OrganizationAlwaysAssign) - - // Pull extra organizations from the claims. - if s.OrganizationField != "" { - organizationRaw, ok := mergedClaims[s.OrganizationField] - if ok { - parsedOrganizations, err := ParseStringSliceClaim(organizationRaw) - if err != nil { - return OrganizationParams{}, &HttpError{ - Code: http.StatusBadRequest, - Msg: "Failed to sync organizations from the OIDC claims", - Detail: err.Error(), - RenderStaticPage: false, - RenderDetailMarkdown: false, - } - } - - // Keep track of which claims are not mapped for debugging purposes. - var ignored []string - for _, parsedOrg := range parsedOrganizations { - if mappedOrganization, ok := s.OrganizationMapping[parsedOrg]; ok { - // parsedOrg is in the mapping, so add the mapped organizations to the - // user's organizations. - userOrganizations = append(userOrganizations, mappedOrganization...) - } else { - ignored = append(ignored, parsedOrg) - } - } - - s.logger.Debug(ctx, "parsed organizations from claim", - slog.F("len", len(parsedOrganizations)), - slog.F("ignored", ignored), - slog.F("organizations", parsedOrganizations), - ) - } - } - + // For AGPL we only rely on 'OrganizationAlwaysAssign' return OrganizationParams{ - Organizations: userOrganizations, + SyncEnabled: false, + IncludeDefault: s.OrganizationAssignDefault, + Organizations: []uuid.UUID{}, }, nil } type OrganizationParams struct { + // SyncEnabled if false will skip syncing the user's organizations. + SyncEnabled bool + IncludeDefault bool // Organizations is the list of organizations the user should be a member of // assuming syncing is turned on. Organizations []uuid.UUID } -func (s IDPSync) SyncOrganizations(ctx context.Context, tx database.Store, user database.User, params OrganizationParams) error { +func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, user database.User, params OrganizationParams) error { + // Nothing happens if sync is not enabled + if !params.SyncEnabled { + return nil + } + // nolint:gocritic // all syncing is done as a system user ctx = dbauthz.AsSystemRestricted(ctx) + // This is a bit hacky, but if AssignDefault is included, then always + // make sure to include the default org in the list of expected. + if s.OrganizationAssignDefault { + defaultOrg, err := tx.GetDefaultOrganization(ctx) + if err != nil { + return xerrors.Errorf("failed to get default organization: %w", err) + } + params.Organizations = append(params.Organizations, defaultOrg.ID) + } + existingOrgs, err := tx.GetOrganizationsByUserID(ctx, user.ID) if err != nil { return xerrors.Errorf("failed to get user organizations: %w", err) @@ -117,7 +98,7 @@ func (s IDPSync) SyncOrganizations(ctx context.Context, tx database.Store, user } if len(notExists) > 0 { - s.logger.Debug(ctx, "organizations do not exist but attempted to use in org sync", + 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), slog.F("username", user.Username), diff --git a/coderd/userauth.go b/coderd/userauth.go index 589b70a178116..6d621e3847faf 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -738,6 +738,11 @@ type OIDCConfig struct { // support the userinfo endpoint, or if the userinfo endpoint causes // undesirable behavior. IgnoreUserInfo bool + // IDPSync contains all the configuration for syncing user information + // from the external IDP. + IDPSync idpsync.IDPSync + + // TODO: Move all idp fields into the IDPSync struct // GroupField selects the claim field to be used as the created user's // groups. If the group field is the empty string, then no group updates // will ever come from the OIDC provider. @@ -768,16 +773,6 @@ type OIDCConfig struct { // UserRolesDefault is the default set of roles to assign to a user if role sync // is enabled. UserRolesDefault []string - // OrganizationField selects the claim field to be used as the created user's - // organizations. If the field is the empty string, then no organization updates - // will ever come from the OIDC provider. - OrganizationField string - // OrganizationMapping controls how organizations returned by the OIDC provider get mapped - OrganizationMapping map[string][]string - // OrganizationAlwaysAssign will ensure all users that authenticate will be - // placed into the specified organizations. 'default' is a special keyword - // that will use the `IsDefault` organization. - OrganizationAlwaysAssign []string // SignInText is the text to display on the OIDC login button SignInText string // IconURL points to the URL of an icon to display on the OIDC login button diff --git a/codersdk/deployment.go b/codersdk/deployment.go index d7c1ddce4fec9..6efa9b73e74df 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -14,6 +14,7 @@ import ( "strings" "time" + "github.com/google/uuid" "golang.org/x/mod/semver" "golang.org/x/xerrors" @@ -512,29 +513,32 @@ type OIDCConfig struct { ClientID serpent.String `json:"client_id" typescript:",notnull"` ClientSecret serpent.String `json:"client_secret" typescript:",notnull"` // ClientKeyFile & ClientCertFile are used in place of ClientSecret for PKI auth. - ClientKeyFile serpent.String `json:"client_key_file" typescript:",notnull"` - ClientCertFile serpent.String `json:"client_cert_file" typescript:",notnull"` - EmailDomain serpent.StringArray `json:"email_domain" typescript:",notnull"` - IssuerURL serpent.String `json:"issuer_url" typescript:",notnull"` - Scopes serpent.StringArray `json:"scopes" typescript:",notnull"` - IgnoreEmailVerified serpent.Bool `json:"ignore_email_verified" typescript:",notnull"` - UsernameField serpent.String `json:"username_field" typescript:",notnull"` - NameField serpent.String `json:"name_field" typescript:",notnull"` - EmailField serpent.String `json:"email_field" typescript:",notnull"` - AuthURLParams serpent.Struct[map[string]string] `json:"auth_url_params" typescript:",notnull"` - IgnoreUserInfo serpent.Bool `json:"ignore_user_info" typescript:",notnull"` - GroupAutoCreate serpent.Bool `json:"group_auto_create" typescript:",notnull"` - GroupRegexFilter serpent.Regexp `json:"group_regex_filter" typescript:",notnull"` - GroupAllowList serpent.StringArray `json:"group_allow_list" typescript:",notnull"` - GroupField serpent.String `json:"groups_field" typescript:",notnull"` - GroupMapping serpent.Struct[map[string]string] `json:"group_mapping" typescript:",notnull"` - UserRoleField serpent.String `json:"user_role_field" typescript:",notnull"` - UserRoleMapping serpent.Struct[map[string][]string] `json:"user_role_mapping" typescript:",notnull"` - UserRolesDefault serpent.StringArray `json:"user_roles_default" typescript:",notnull"` - SignInText serpent.String `json:"sign_in_text" typescript:",notnull"` - IconURL serpent.URL `json:"icon_url" typescript:",notnull"` - SignupsDisabledText serpent.String `json:"signups_disabled_text" typescript:",notnull"` - SkipIssuerChecks serpent.Bool `json:"skip_issuer_checks" typescript:",notnull"` + ClientKeyFile serpent.String `json:"client_key_file" typescript:",notnull"` + ClientCertFile serpent.String `json:"client_cert_file" typescript:",notnull"` + EmailDomain serpent.StringArray `json:"email_domain" typescript:",notnull"` + IssuerURL serpent.String `json:"issuer_url" typescript:",notnull"` + Scopes serpent.StringArray `json:"scopes" typescript:",notnull"` + IgnoreEmailVerified serpent.Bool `json:"ignore_email_verified" typescript:",notnull"` + UsernameField serpent.String `json:"username_field" typescript:",notnull"` + NameField serpent.String `json:"name_field" typescript:",notnull"` + EmailField serpent.String `json:"email_field" typescript:",notnull"` + AuthURLParams serpent.Struct[map[string]string] `json:"auth_url_params" typescript:",notnull"` + IgnoreUserInfo serpent.Bool `json:"ignore_user_info" typescript:",notnull"` + OrganizationField serpent.String `json:"organization_field" typescript:",notnull"` + OrganizationMapping serpent.Struct[map[string][]uuid.UUID] `json:"organization_mapping" typescript:",notnull"` + OrganizationAssignDefault serpent.Bool `json:"organization_assign_default" typescript:",notnull"` + GroupAutoCreate serpent.Bool `json:"group_auto_create" typescript:",notnull"` + GroupRegexFilter serpent.Regexp `json:"group_regex_filter" typescript:",notnull"` + GroupAllowList serpent.StringArray `json:"group_allow_list" typescript:",notnull"` + GroupField serpent.String `json:"groups_field" typescript:",notnull"` + GroupMapping serpent.Struct[map[string]string] `json:"group_mapping" typescript:",notnull"` + UserRoleField serpent.String `json:"user_role_field" typescript:",notnull"` + UserRoleMapping serpent.Struct[map[string][]string] `json:"user_role_mapping" typescript:",notnull"` + UserRolesDefault serpent.StringArray `json:"user_roles_default" typescript:",notnull"` + SignInText serpent.String `json:"sign_in_text" typescript:",notnull"` + IconURL serpent.URL `json:"icon_url" typescript:",notnull"` + SignupsDisabledText serpent.String `json:"signups_disabled_text" typescript:",notnull"` + SkipIssuerChecks serpent.Bool `json:"skip_issuer_checks" typescript:",notnull"` } type TelemetryConfig struct { diff --git a/enterprise/coderd/enidpsync/enidpsync.go b/enterprise/coderd/enidpsync/enidpsync.go new file mode 100644 index 0000000000000..3dbfd94e38ed4 --- /dev/null +++ b/enterprise/coderd/enidpsync/enidpsync.go @@ -0,0 +1,20 @@ +package enidpsync + +import ( + "cdr.dev/slog" + + "github.com/coder/coder/v2/coderd/entitlements" + "github.com/coder/coder/v2/coderd/idpsync" +) + +type EnterpriseIDPSync struct { + entitlements *entitlements.Set + agpl *idpsync.AGPLIDPSync +} + +func NewSync(logger slog.Logger, entitlements *entitlements.Set, settings idpsync.SyncSettings) *EnterpriseIDPSync { + return &EnterpriseIDPSync{ + entitlements: entitlements, + agpl: idpsync.NewSync(logger, settings), + } +} diff --git a/enterprise/coderd/enidpsync/organizations.go b/enterprise/coderd/enidpsync/organizations.go new file mode 100644 index 0000000000000..86ca6aeb28836 --- /dev/null +++ b/enterprise/coderd/enidpsync/organizations.go @@ -0,0 +1,63 @@ +package enidpsync + +import ( + "context" + "net/http" + + "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/idpsync" + "github.com/coder/coder/v2/codersdk" +) + +func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedClaims map[string]interface{}) (idpsync.OrganizationParams, *HttpError) { + s := e.agpl + if !e.entitlements.Enabled(codersdk.FeatureMultipleOrganizations) { + // Default to agpl if multi-org is not enabled + return e.agpl.ParseOrganizationClaims(ctx, mergedClaims) + } + + // nolint:gocritic // all syncing is done as a system user + ctx = dbauthz.AsSystemRestricted(ctx) + + // Pull extra organizations from the claims. + if s.OrganizationField != "" { + organizationRaw, ok := mergedClaims[s.OrganizationField] + if ok { + parsedOrganizations, err := idpsync.ParseStringSliceClaim(organizationRaw) + if err != nil { + return idpsync.OrganizationParams{}, &idpsync.HttpError{ + Code: http.StatusBadRequest, + Msg: "Failed to sync organizations from the OIDC claims", + Detail: err.Error(), + RenderStaticPage: false, + RenderDetailMarkdown: false, + } + } + + // Keep track of which claims are not mapped for debugging purposes. + var ignored []string + for _, parsedOrg := range parsedOrganizations { + if mappedOrganization, ok := s.OrganizationMapping[parsedOrg]; ok { + // parsedOrg is in the mapping, so add the mapped organizations to the + // user's organizations. + userOrganizations = append(userOrganizations, mappedOrganization...) + } else { + ignored = append(ignored, parsedOrg) + } + } + + s.Logger.Debug(ctx, "parsed organizations from claim", + slog.F("len", len(parsedOrganizations)), + slog.F("ignored", ignored), + slog.F("organizations", parsedOrganizations), + ) + } + } + + return idpsync.OrganizationParams{ + SyncEnabled: true, + IncludeDefault: s.OrganizationAssignDefault, + Organizations: userOrganizations, + }, nil +} From d42772c2b52642cf6638d5d604a6fd14d93750d9 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 26 Aug 2024 10:19:58 -0500 Subject: [PATCH 04/23] compilation fixes --- enterprise/coderd/enidpsync/organizations.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/enterprise/coderd/enidpsync/organizations.go b/enterprise/coderd/enidpsync/organizations.go index 86ca6aeb28836..248ba2f388846 100644 --- a/enterprise/coderd/enidpsync/organizations.go +++ b/enterprise/coderd/enidpsync/organizations.go @@ -4,13 +4,15 @@ import ( "context" "net/http" + "github.com/google/uuid" + "cdr.dev/slog" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/idpsync" "github.com/coder/coder/v2/codersdk" ) -func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedClaims map[string]interface{}) (idpsync.OrganizationParams, *HttpError) { +func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedClaims map[string]interface{}) (idpsync.OrganizationParams, *idpsync.HttpError) { s := e.agpl if !e.entitlements.Enabled(codersdk.FeatureMultipleOrganizations) { // Default to agpl if multi-org is not enabled @@ -19,6 +21,7 @@ func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedCl // nolint:gocritic // all syncing is done as a system user ctx = dbauthz.AsSystemRestricted(ctx) + userOrganizations := make([]uuid.UUID, 0) // Pull extra organizations from the claims. if s.OrganizationField != "" { From 7d3ec1466c99078fe0d52fa5b005f23757966b9d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 26 Aug 2024 13:08:36 -0500 Subject: [PATCH 05/23] fix compile issues --- cli/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/server.go b/cli/server.go index a5e140138a3b9..6be6e1c4ebd60 100644 --- a/cli/server.go +++ b/cli/server.go @@ -201,7 +201,7 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.De IDPSync: idpsync.NewSync(logger, idpsync.SyncSettings{ OrganizationField: vals.OIDC.OrganizationField.Value(), OrganizationMapping: vals.OIDC.OrganizationMapping.Value, - OrganizationAssignDefault: vals.OIDC.OrganizationAssignDefault.Value, + OrganizationAssignDefault: vals.OIDC.OrganizationAssignDefault.Value(), }), }, nil } From 9287fb975d2fc5761f0ff4c48c07b7a0163ff5a5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 26 Aug 2024 15:41:44 -0500 Subject: [PATCH 06/23] trying to figure out how to initialize both AGPL and enterprise --- cli/server.go | 13 ++++++++----- coderd/idpsync/idpsync.go | 18 +++++++++++++++--- coderd/idpsync/organization.go | 2 +- coderd/userauth.go | 7 +++++++ enterprise/coderd/enidpsync/enidpsync.go | 4 ++-- 5 files changed, 33 insertions(+), 11 deletions(-) diff --git a/cli/server.go b/cli/server.go index 6be6e1c4ebd60..189ca3604944c 100644 --- a/cli/server.go +++ b/cli/server.go @@ -170,6 +170,13 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.De groupAllowList[group] = true } + idpSyncSetting := idpsync.SyncSettings{ + OrganizationField: vals.OIDC.OrganizationField.Value(), + OrganizationMapping: vals.OIDC.OrganizationMapping.Value, + OrganizationAssignDefault: vals.OIDC.OrganizationAssignDefault.Value(), + } + syncer.Configure(idpSyncSetting) + return &coderd.OIDCConfig{ OAuth2Config: useCfg, Provider: oidcProvider, @@ -198,11 +205,7 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.De SignupsDisabledText: vals.OIDC.SignupsDisabledText.String(), IconURL: vals.OIDC.IconURL.String(), IgnoreEmailVerified: vals.OIDC.IgnoreEmailVerified.Value(), - IDPSync: idpsync.NewSync(logger, idpsync.SyncSettings{ - OrganizationField: vals.OIDC.OrganizationField.Value(), - OrganizationMapping: vals.OIDC.OrganizationMapping.Value, - OrganizationAssignDefault: vals.OIDC.OrganizationAssignDefault.Value(), - }), + IDPSync: syncer, }, nil } diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go index f459a573585fd..acd01582dfc6e 100644 --- a/coderd/idpsync/idpsync.go +++ b/coderd/idpsync/idpsync.go @@ -16,6 +16,11 @@ import ( ) type IDPSync interface { + // Configure is a method on the struct only because it is easier to configure + // from the AGPL initialization. For the enterprise code to get these settings, + // it makes sense to have the AGPL call 'Configure' rather than duplicate + // the code to create these settings. + Configure(settings SyncSettings) // ParseOrganizationClaims takes claims from an OIDC provider, and returns the // organization sync params for assigning users into organizations. ParseOrganizationClaims(ctx context.Context, _ map[string]interface{}) (OrganizationParams, *HttpError) @@ -45,13 +50,20 @@ type SyncSettings struct { OrganizationAssignDefault bool } -func NewSync(logger slog.Logger, settings SyncSettings) *AGPLIDPSync { +func NewSync(logger slog.Logger) *AGPLIDPSync { return &AGPLIDPSync{ - Logger: logger.Named("idp-sync"), - SyncSettings: settings, + Logger: logger.Named("idp-sync"), + SyncSettings: SyncSettings{ + // A sane default + OrganizationAssignDefault: true, + }, } } +func (s *AGPLIDPSync) Configure(settings SyncSettings) { + s.SyncSettings = settings +} + // ParseStringSliceClaim parses the claim for groups and roles, expected []string. // // Some providers like ADFS return a single string instead of an array if there diff --git a/coderd/idpsync/organization.go b/coderd/idpsync/organization.go index 4cbbe8748afb3..0c02f08d62ce3 100644 --- a/coderd/idpsync/organization.go +++ b/coderd/idpsync/organization.go @@ -19,7 +19,7 @@ func (s AGPLIDPSync) ParseOrganizationClaims(ctx context.Context, _ map[string]i // nolint:gocritic // all syncing is done as a system user ctx = dbauthz.AsSystemRestricted(ctx) - // For AGPL we only rely on 'OrganizationAlwaysAssign' + // For AGPL we only sync the default organization. return OrganizationParams{ SyncEnabled: false, IncludeDefault: s.OrganizationAssignDefault, diff --git a/coderd/userauth.go b/coderd/userauth.go index 6d621e3847faf..b00e64bb7f321 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -1026,6 +1026,12 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { return } + orgSync, orgSyncErr := api.OIDCConfig.IDPSync.ParseOrganizationClaims(ctx, mergedClaims) + if orgSyncErr != nil { + orgSyncErr.Write(rw, r) + return + } + // If a new user is authenticating for the first time // the audit action is 'register', not 'login' if user.ID == uuid.Nil { @@ -1047,6 +1053,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { Roles: roles, UsingGroups: usingGroups, Groups: groups, + OrganizationSync: orgSync, CreateMissingGroups: api.OIDCConfig.CreateMissingGroups, GroupFilter: api.OIDCConfig.GroupFilter, DebugContext: OauthDebugContext{ diff --git a/enterprise/coderd/enidpsync/enidpsync.go b/enterprise/coderd/enidpsync/enidpsync.go index 3dbfd94e38ed4..ea437c7ad84d7 100644 --- a/enterprise/coderd/enidpsync/enidpsync.go +++ b/enterprise/coderd/enidpsync/enidpsync.go @@ -12,9 +12,9 @@ type EnterpriseIDPSync struct { agpl *idpsync.AGPLIDPSync } -func NewSync(logger slog.Logger, entitlements *entitlements.Set, settings idpsync.SyncSettings) *EnterpriseIDPSync { +func NewSync(logger slog.Logger, entitlements *entitlements.Set) *EnterpriseIDPSync { return &EnterpriseIDPSync{ entitlements: entitlements, - agpl: idpsync.NewSync(logger, settings), + agpl: idpsync.NewSync(logger), } } From 9c12b18ab37de207b26dd4ee156bee69be2d5356 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 26 Aug 2024 15:53:26 -0500 Subject: [PATCH 07/23] ended up with a global factory funciton --- cli/server.go | 16 +++++------- coderd/idpsync/idpsync.go | 27 +++++++++----------- enterprise/coderd/enidpsync/enidpsync.go | 10 +++++--- enterprise/coderd/enidpsync/organizations.go | 13 +++++----- 4 files changed, 32 insertions(+), 34 deletions(-) diff --git a/cli/server.go b/cli/server.go index 189ca3604944c..3433692e238c9 100644 --- a/cli/server.go +++ b/cli/server.go @@ -55,6 +55,7 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/sloghuman" + "github.com/coder/coder/v2/coderd/entitlements" "github.com/coder/coder/v2/coderd/idpsync" "github.com/coder/pretty" "github.com/coder/quartz" @@ -108,7 +109,7 @@ import ( "github.com/coder/coder/v2/tailnet" ) -func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.DeploymentValues) (*coderd.OIDCConfig, error) { +func createOIDCConfig(ctx context.Context, logger slog.Logger, entitlements *entitlements.Set, vals *codersdk.DeploymentValues) (*coderd.OIDCConfig, error) { if vals.OIDC.ClientID == "" { return nil, xerrors.Errorf("OIDC client ID must be set!") } @@ -170,13 +171,6 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.De groupAllowList[group] = true } - idpSyncSetting := idpsync.SyncSettings{ - OrganizationField: vals.OIDC.OrganizationField.Value(), - OrganizationMapping: vals.OIDC.OrganizationMapping.Value, - OrganizationAssignDefault: vals.OIDC.OrganizationAssignDefault.Value(), - } - syncer.Configure(idpSyncSetting) - return &coderd.OIDCConfig{ OAuth2Config: useCfg, Provider: oidcProvider, @@ -205,7 +199,11 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.De SignupsDisabledText: vals.OIDC.SignupsDisabledText.String(), IconURL: vals.OIDC.IconURL.String(), IgnoreEmailVerified: vals.OIDC.IgnoreEmailVerified.Value(), - IDPSync: syncer, + IDPSync: idpsync.NewSync(logger, entitlements, idpsync.SyncSettings{ + OrganizationField: vals.OIDC.OrganizationField.Value(), + OrganizationMapping: vals.OIDC.OrganizationMapping.Value, + OrganizationAssignDefault: vals.OIDC.OrganizationAssignDefault.Value(), + }), }, nil } diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go index acd01582dfc6e..78d030bd10b83 100644 --- a/coderd/idpsync/idpsync.go +++ b/coderd/idpsync/idpsync.go @@ -10,17 +10,21 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/entitlements" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/site" ) +// NewSync is a factory function for creating an IDP sync object. +// Due to the way we instantiate Coder, there is no way for the enterprise +// cli wrapper to pass in the enterprise IDP sync object. +// So instead, if the code is compiled with the enterprise logic, it will +// override this function to return the enterprise IDP sync object. +// For unit testing, the callers can specifically choose which "NewSync" to use. +var NewSync = NewAGPLSync + type IDPSync interface { - // Configure is a method on the struct only because it is easier to configure - // from the AGPL initialization. For the enterprise code to get these settings, - // it makes sense to have the AGPL call 'Configure' rather than duplicate - // the code to create these settings. - Configure(settings SyncSettings) // ParseOrganizationClaims takes claims from an OIDC provider, and returns the // organization sync params for assigning users into organizations. ParseOrganizationClaims(ctx context.Context, _ map[string]interface{}) (OrganizationParams, *HttpError) @@ -50,20 +54,13 @@ type SyncSettings struct { OrganizationAssignDefault bool } -func NewSync(logger slog.Logger) *AGPLIDPSync { +func NewAGPLSync(logger slog.Logger, _ *entitlements.Set, settings SyncSettings) IDPSync { return &AGPLIDPSync{ - Logger: logger.Named("idp-sync"), - SyncSettings: SyncSettings{ - // A sane default - OrganizationAssignDefault: true, - }, + Logger: logger.Named("idp-sync"), + SyncSettings: settings, } } -func (s *AGPLIDPSync) Configure(settings SyncSettings) { - s.SyncSettings = settings -} - // ParseStringSliceClaim parses the claim for groups and roles, expected []string. // // Some providers like ADFS return a single string instead of an array if there diff --git a/enterprise/coderd/enidpsync/enidpsync.go b/enterprise/coderd/enidpsync/enidpsync.go index ea437c7ad84d7..5c301fa271803 100644 --- a/enterprise/coderd/enidpsync/enidpsync.go +++ b/enterprise/coderd/enidpsync/enidpsync.go @@ -7,14 +7,18 @@ import ( "github.com/coder/coder/v2/coderd/idpsync" ) +func init() { + idpsync.NewSync = NewSync +} + type EnterpriseIDPSync struct { entitlements *entitlements.Set - agpl *idpsync.AGPLIDPSync + *idpsync.AGPLIDPSync } -func NewSync(logger slog.Logger, entitlements *entitlements.Set) *EnterpriseIDPSync { +func NewSync(logger slog.Logger, entitlements *entitlements.Set, settings idpsync.SyncSettings) idpsync.IDPSync { return &EnterpriseIDPSync{ entitlements: entitlements, - agpl: idpsync.NewSync(logger), + AGPLIDPSync: idpsync.NewAGPLSync(logger, entitlements, settings), } } diff --git a/enterprise/coderd/enidpsync/organizations.go b/enterprise/coderd/enidpsync/organizations.go index 248ba2f388846..f51c938f44462 100644 --- a/enterprise/coderd/enidpsync/organizations.go +++ b/enterprise/coderd/enidpsync/organizations.go @@ -13,10 +13,9 @@ import ( ) func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedClaims map[string]interface{}) (idpsync.OrganizationParams, *idpsync.HttpError) { - s := e.agpl if !e.entitlements.Enabled(codersdk.FeatureMultipleOrganizations) { // Default to agpl if multi-org is not enabled - return e.agpl.ParseOrganizationClaims(ctx, mergedClaims) + return e.AGPLIDPSync.ParseOrganizationClaims(ctx, mergedClaims) } // nolint:gocritic // all syncing is done as a system user @@ -24,8 +23,8 @@ func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedCl userOrganizations := make([]uuid.UUID, 0) // Pull extra organizations from the claims. - if s.OrganizationField != "" { - organizationRaw, ok := mergedClaims[s.OrganizationField] + if e.OrganizationField != "" { + organizationRaw, ok := mergedClaims[e.OrganizationField] if ok { parsedOrganizations, err := idpsync.ParseStringSliceClaim(organizationRaw) if err != nil { @@ -41,7 +40,7 @@ func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedCl // Keep track of which claims are not mapped for debugging purposes. var ignored []string for _, parsedOrg := range parsedOrganizations { - if mappedOrganization, ok := s.OrganizationMapping[parsedOrg]; ok { + if mappedOrganization, ok := e.OrganizationMapping[parsedOrg]; ok { // parsedOrg is in the mapping, so add the mapped organizations to the // user's organizations. userOrganizations = append(userOrganizations, mappedOrganization...) @@ -50,7 +49,7 @@ func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedCl } } - s.Logger.Debug(ctx, "parsed organizations from claim", + e.Logger.Debug(ctx, "parsed organizations from claim", slog.F("len", len(parsedOrganizations)), slog.F("ignored", ignored), slog.F("organizations", parsedOrganizations), @@ -60,7 +59,7 @@ func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedCl return idpsync.OrganizationParams{ SyncEnabled: true, - IncludeDefault: s.OrganizationAssignDefault, + IncludeDefault: e.OrganizationAssignDefault, Organizations: userOrganizations, }, nil } From 210239f1f41079cd8bc43edf0945f75d8ff38f71 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 26 Aug 2024 16:02:52 -0500 Subject: [PATCH 08/23] fixup compile issues --- cli/server.go | 3 ++- coderd/idpsync/idpsync.go | 6 ++++-- enterprise/coderd/enidpsync/enidpsync.go | 8 +++++--- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/cli/server.go b/cli/server.go index 3433692e238c9..f6c7ff94792eb 100644 --- a/cli/server.go +++ b/cli/server.go @@ -612,6 +612,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. SSHConfigOptions: configSSHOptions, }, AllowWorkspaceRenames: vals.AllowWorkspaceRenames.Value(), + Entitlements: entitlements.New(), NotificationsEnqueuer: notifications.NewNoopEnqueuer(), // Changed further down if notifications enabled. } if httpServers.TLSConfig != nil { @@ -674,7 +675,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. // Missing: // - Userinfo // - Verify - oc, err := createOIDCConfig(ctx, options.Logger, vals) + oc, err := createOIDCConfig(ctx, options.Logger, options.Entitlements, vals) if err != nil { return xerrors.Errorf("create oidc config: %w", err) } diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go index 78d030bd10b83..2aeeb9e75a31e 100644 --- a/coderd/idpsync/idpsync.go +++ b/coderd/idpsync/idpsync.go @@ -22,7 +22,9 @@ import ( // So instead, if the code is compiled with the enterprise logic, it will // override this function to return the enterprise IDP sync object. // For unit testing, the callers can specifically choose which "NewSync" to use. -var NewSync = NewAGPLSync +var NewSync = func(logger slog.Logger, entitlements *entitlements.Set, settings SyncSettings) IDPSync { + return NewAGPLSync(logger, entitlements, settings) +} type IDPSync interface { // ParseOrganizationClaims takes claims from an OIDC provider, and returns the @@ -54,7 +56,7 @@ type SyncSettings struct { OrganizationAssignDefault bool } -func NewAGPLSync(logger slog.Logger, _ *entitlements.Set, settings SyncSettings) IDPSync { +func NewAGPLSync(logger slog.Logger, _ *entitlements.Set, settings SyncSettings) *AGPLIDPSync { return &AGPLIDPSync{ Logger: logger.Named("idp-sync"), SyncSettings: settings, diff --git a/enterprise/coderd/enidpsync/enidpsync.go b/enterprise/coderd/enidpsync/enidpsync.go index 5c301fa271803..412cb647e53f5 100644 --- a/enterprise/coderd/enidpsync/enidpsync.go +++ b/enterprise/coderd/enidpsync/enidpsync.go @@ -8,7 +8,9 @@ import ( ) func init() { - idpsync.NewSync = NewSync + idpsync.NewSync = func(logger slog.Logger, entitlements *entitlements.Set, settings idpsync.SyncSettings) idpsync.IDPSync { + return NewSync(logger, entitlements, settings) + } } type EnterpriseIDPSync struct { @@ -16,9 +18,9 @@ type EnterpriseIDPSync struct { *idpsync.AGPLIDPSync } -func NewSync(logger slog.Logger, entitlements *entitlements.Set, settings idpsync.SyncSettings) idpsync.IDPSync { +func NewSync(logger slog.Logger, entitlements *entitlements.Set, settings idpsync.SyncSettings) *EnterpriseIDPSync { return &EnterpriseIDPSync{ entitlements: entitlements, - AGPLIDPSync: idpsync.NewAGPLSync(logger, entitlements, settings), + AGPLIDPSync: idpsync.NewAGPLSync(logger.With(slog.F("enterprise_capable", "true")), entitlements, settings), } } From 94e05e703157352df9756e4bf1a1c58f0b9d637d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 27 Aug 2024 12:04:09 -0500 Subject: [PATCH 09/23] fixup errors --- coderd/userauth.go | 165 +++++++++++++++++---------------------------- 1 file changed, 63 insertions(+), 102 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index b00e64bb7f321..80baf0bb197e7 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -41,7 +41,6 @@ import ( "github.com/coder/coder/v2/coderd/userpassword" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/cryptorand" - "github.com/coder/coder/v2/site" ) const ( @@ -665,7 +664,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { }) cookies, user, key, err := api.oauthLogin(r, params) defer params.CommitAuditLogs() - var httpErr httpError + var httpErr idpsync.HttpError if xerrors.As(err, &httpErr) { httpErr.Write(rw, r) return @@ -1065,7 +1064,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { }) cookies, user, key, err := api.oauthLogin(r, params) defer params.CommitAuditLogs() - var httpErr httpError + var httpErr idpsync.HttpError if xerrors.As(err, &httpErr) { httpErr.Write(rw, r) return @@ -1093,7 +1092,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { } // oidcGroups returns the groups for the user from the OIDC claims. -func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interface{}) (bool, []string, *httpError) { +func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interface{}) (bool, []string, *idpsync.HttpError) { logger := api.Logger.Named(userAuthLoggerName) usingGroups := false var groups []string @@ -1114,11 +1113,11 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac slog.F("type", fmt.Sprintf("%T", groupsRaw)), slog.Error(err), ) - return false, nil, &httpError{ - code: http.StatusBadRequest, - msg: "Failed to sync groups from OIDC claims", - detail: err.Error(), - renderStaticPage: false, + return false, nil, &idpsync.HttpError{ + Code: http.StatusBadRequest, + Msg: "Failed to sync groups from OIDC claims", + Detail: err.Error(), + RenderStaticPage: false, } } @@ -1147,11 +1146,11 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac if len(groups) == 0 { detail = "You are currently not a member of any groups! Ask an administrator to add you to an authorized group to login." } - return usingGroups, groups, &httpError{ - code: http.StatusForbidden, - msg: "Not a member of an allowed group", - detail: detail, - renderStaticPage: true, + return usingGroups, groups, &idpsync.HttpError{ + Code: http.StatusForbidden, + Msg: "Not a member of an allowed group", + Detail: detail, + RenderStaticPage: true, } } } @@ -1171,7 +1170,7 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac // It would be preferred to just return an error, however this function // decorates returned errors with the appropriate HTTP status codes and details // that are hard to carry in a standard `error` without more work. -func (api *API) oidcRoles(ctx context.Context, mergedClaims map[string]interface{}) ([]string, *httpError) { +func (api *API) oidcRoles(ctx context.Context, mergedClaims map[string]interface{}) ([]string, *idpsync.HttpError) { roles := api.OIDCConfig.UserRolesDefault if !api.OIDCConfig.RoleSyncEnabled() { return roles, nil @@ -1193,11 +1192,11 @@ func (api *API) oidcRoles(ctx context.Context, mergedClaims map[string]interface slog.F("type", fmt.Sprintf("%T", rolesRow)), slog.Error(err), ) - return nil, &httpError{ - code: http.StatusInternalServerError, - msg: "Login disabled until OIDC config is fixed", - detail: fmt.Sprintf("Roles claim must be an array of strings, type found: %T. Disabling role sync will allow login to proceed.", rolesRow), - renderStaticPage: false, + return nil, &idpsync.HttpError{ + Code: http.StatusInternalServerError, + Msg: "Login disabled until OIDC config is fixed", + Detail: fmt.Sprintf("Roles claim must be an array of strings, type found: %T. Disabling role sync will allow login to proceed.", rolesRow), + RenderStaticPage: false, } } @@ -1318,43 +1317,6 @@ func (p *oauthLoginParams) CommitAuditLogs() { } } -type httpError struct { - code int - msg string - detail string - renderStaticPage bool - - renderDetailMarkdown bool -} - -func (e httpError) Write(rw http.ResponseWriter, r *http.Request) { - if e.renderStaticPage { - site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ - Status: e.code, - HideStatus: true, - Title: e.msg, - Description: e.detail, - RetryEnabled: false, - DashboardURL: "/login", - - RenderDescriptionMarkdown: e.renderDetailMarkdown, - }) - return - } - httpapi.Write(r.Context(), rw, e.code, codersdk.Response{ - Message: e.msg, - Detail: e.detail, - }) -} - -func (e httpError) Error() string { - if e.detail != "" { - return e.detail - } - - return e.msg -} - func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.Cookie, database.User, database.APIKey, error) { var ( ctx = r.Context() @@ -1391,13 +1353,12 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C if api.OIDCConfig != nil && api.OIDCConfig.SignupsDisabledText != "" { signupsDisabledText = render.HTMLFromMarkdown(api.OIDCConfig.SignupsDisabledText) } - return httpError{ - code: http.StatusForbidden, - msg: "Signups are disabled", - detail: signupsDisabledText, - renderStaticPage: true, - - renderDetailMarkdown: true, + return &idpsync.HttpError{ + Code: http.StatusForbidden, + Msg: "Signups are disabled", + Detail: signupsDisabledText, + RenderStaticPage: true, + RenderDetailMarkdown: true, } } @@ -1443,9 +1404,9 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C } } if !validUsername { - return httpError{ - code: http.StatusConflict, - msg: fmt.Sprintf("exhausted alternatives for taken username %q", original), + return &idpsync.HttpError{ + Code: http.StatusConflict, + Msg: fmt.Sprintf("exhausted alternatives for taken username %q", original), } } } @@ -1586,11 +1547,11 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C //nolint:gocritic err := api.Options.SetUserSiteRoles(dbauthz.AsSystemRestricted(ctx), logger, tx, user.ID, filtered) if err != nil { - return httpError{ - code: http.StatusBadRequest, - msg: "Invalid roles through OIDC claims", - detail: fmt.Sprintf("Error from role assignment attempt: %s", err.Error()), - renderStaticPage: true, + return &idpsync.HttpError{ + Code: http.StatusBadRequest, + Msg: "Invalid roles through OIDC claims", + Detail: fmt.Sprintf("Error from role assignment attempt: %s", err.Error()), + RenderStaticPage: true, } } if len(ignored) > 0 { @@ -1701,17 +1662,17 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data // Trying to convert to OIDC, but the email does not match. // So do not make a new user, just block the request. if user.ID == uuid.Nil { - return database.User{}, httpError{ - code: http.StatusBadRequest, - msg: fmt.Sprintf("The oidc account with the email %q does not match the email of the account you are trying to convert. Contact your administrator to resolve this issue.", params.Email), + return database.User{}, idpsync.HttpError{ + Code: http.StatusBadRequest, + Msg: fmt.Sprintf("The oidc account with the email %q does not match the email of the account you are trying to convert. Contact your administrator to resolve this issue.", params.Email), } } jwtCookie, err := r.Cookie(OAuthConvertCookieValue) if err != nil { - return database.User{}, httpError{ - code: http.StatusBadRequest, - msg: fmt.Sprintf("Convert to oauth cookie not found. Missing signed jwt to authorize this action. " + + return database.User{}, idpsync.HttpError{ + Code: http.StatusBadRequest, + Msg: fmt.Sprintf("Convert to oauth cookie not found. Missing signed jwt to authorize this action. " + "Please try again."), } } @@ -1721,15 +1682,15 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data }) if xerrors.Is(err, jwt.ErrSignatureInvalid) || !token.Valid { // These errors are probably because the user is mixing 2 coder deployments. - return database.User{}, httpError{ - code: http.StatusBadRequest, - msg: "Using an invalid jwt to authorize this action. Ensure there is only 1 coder deployment and try again.", + return database.User{}, idpsync.HttpError{ + Code: http.StatusBadRequest, + Msg: "Using an invalid jwt to authorize this action. Ensure there is only 1 coder deployment and try again.", } } if err != nil { - return database.User{}, httpError{ - code: http.StatusInternalServerError, - msg: fmt.Sprintf("Error parsing jwt: %v", err), + return database.User{}, idpsync.HttpError{ + Code: http.StatusInternalServerError, + Msg: fmt.Sprintf("Error parsing jwt: %v", err), } } @@ -1749,16 +1710,16 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data oauthConvertAudit.Old = user if claims.RegisteredClaims.Issuer != api.DeploymentID { - return database.User{}, httpError{ - code: http.StatusForbidden, - msg: "Request to convert login type failed. Issuer mismatch. Found a cookie from another coder deployment, please try again.", + return database.User{}, idpsync.HttpError{ + Code: http.StatusForbidden, + Msg: "Request to convert login type failed. Issuer mismatch. Found a cookie from another coder deployment, please try again.", } } if params.State.StateString != claims.State { - return database.User{}, httpError{ - code: http.StatusForbidden, - msg: "Request to convert login type failed. State mismatch.", + return database.User{}, idpsync.HttpError{ + Code: http.StatusForbidden, + Msg: "Request to convert login type failed. State mismatch.", } } @@ -1768,9 +1729,9 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data if user.ID != claims.UserID || codersdk.LoginType(user.LoginType) != claims.FromLoginType || codersdk.LoginType(params.LoginType) != claims.ToLoginType { - return database.User{}, httpError{ - code: http.StatusForbidden, - msg: fmt.Sprintf("Request to convert login type from %s to %s failed", user.LoginType, params.LoginType), + return database.User{}, idpsync.HttpError{ + Code: http.StatusForbidden, + Msg: fmt.Sprintf("Request to convert login type from %s to %s failed", user.LoginType, params.LoginType), } } @@ -1784,9 +1745,9 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data UserID: user.ID, }) if err != nil { - return database.User{}, httpError{ - code: http.StatusInternalServerError, - msg: "Failed to convert user to new login type", + return database.User{}, idpsync.HttpError{ + Code: http.StatusInternalServerError, + Msg: "Failed to convert user to new login type", } } oauthConvertAudit.New = user @@ -1872,16 +1833,16 @@ func clearOAuthConvertCookie() *http.Cookie { } } -func wrongLoginTypeHTTPError(user database.LoginType, params database.LoginType) httpError { +func wrongLoginTypeHTTPError(user database.LoginType, params database.LoginType) idpsync.HttpError { addedMsg := "" if user == database.LoginTypePassword { addedMsg = " You can convert your account to use this login type by visiting your account settings." } - return httpError{ - code: http.StatusForbidden, - renderStaticPage: true, - msg: "Incorrect login type", - detail: fmt.Sprintf("Attempting to use login type %q, but the user has the login type %q.%s", + return idpsync.HttpError{ + Code: http.StatusForbidden, + RenderStaticPage: true, + Msg: "Incorrect login type", + Detail: fmt.Sprintf("Attempting to use login type %q, but the user has the login type %q.%s", params, user, addedMsg), } } From e2badf410a589a05dea36f69b2950269beb7aece Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 27 Aug 2024 12:07:58 -0500 Subject: [PATCH 10/23] fixup some comments --- coderd/idpsync/organization.go | 2 ++ enterprise/coderd/enidpsync/organizations.go | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/coderd/idpsync/organization.go b/coderd/idpsync/organization.go index 0c02f08d62ce3..20eae1c8e81f5 100644 --- a/coderd/idpsync/organization.go +++ b/coderd/idpsync/organization.go @@ -36,6 +36,8 @@ type OrganizationParams struct { Organizations []uuid.UUID } +// SyncOrganizations if enabled will ensure the user is a member of the provided +// organizations. It will add and remove their membership to match the expected set. func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, user database.User, params OrganizationParams) error { // Nothing happens if sync is not enabled if !params.SyncEnabled { diff --git a/enterprise/coderd/enidpsync/organizations.go b/enterprise/coderd/enidpsync/organizations.go index f51c938f44462..5f215810235c5 100644 --- a/enterprise/coderd/enidpsync/organizations.go +++ b/enterprise/coderd/enidpsync/organizations.go @@ -58,7 +58,8 @@ func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedCl } return idpsync.OrganizationParams{ - SyncEnabled: true, + // If the field is not set, then sync is not enabled. + SyncEnabled: e.OrganizationField != "", IncludeDefault: e.OrganizationAssignDefault, Organizations: userOrganizations, }, nil From eb7e2c5bfc6e4069e28d2510324f13c2fc51600d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 27 Aug 2024 12:14:24 -0500 Subject: [PATCH 11/23] Actually enable org sync in the oidc flow --- coderd/idpsync/organization.go | 4 +++- coderd/userauth.go | 27 ++++++++++++++++++++++----- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/coderd/idpsync/organization.go b/coderd/idpsync/organization.go index 20eae1c8e81f5..46028ea9601b9 100644 --- a/coderd/idpsync/organization.go +++ b/coderd/idpsync/organization.go @@ -29,7 +29,9 @@ func (s AGPLIDPSync) ParseOrganizationClaims(ctx context.Context, _ map[string]i type OrganizationParams struct { // SyncEnabled if false will skip syncing the user's organizations. - SyncEnabled bool + SyncEnabled bool + // IncludeDefault is primarily for single org deployments. It will ensure + // a user is always inserted into the default org. IncludeDefault bool // Organizations is the list of organizations the user should be a member of // assuming syncing is turned on. diff --git a/coderd/userauth.go b/coderd/userauth.go index 80baf0bb197e7..a434aaac5871f 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -659,6 +659,11 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { AvatarURL: ghUser.GetAvatarURL(), Name: normName, DebugContext: OauthDebugContext{}, + OrganizationSync: idpsync.OrganizationParams{ + SyncEnabled: false, + IncludeDefault: true, + Organizations: []uuid.UUID{}, + }, }).SetInitAuditRequest(func(params *audit.RequestParams) (*audit.Request[database.User], func()) { return audit.InitRequest[database.User](rw, params) }) @@ -1411,14 +1416,19 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C } } + // Even if org sync is disabled, single org deployments will always + // have this set to true. + orgIDs := []uuid.UUID{} + if params.OrganizationSync.IncludeDefault { + orgIDs = append(orgIDs, defaultOrganization.ID) + } + //nolint:gocritic user, err = api.CreateUser(dbauthz.AsSystemRestricted(ctx), tx, CreateUserRequest{ CreateUserRequestWithOrgs: codersdk.CreateUserRequestWithOrgs{ - Email: params.Email, - Username: params.Username, - // TODO: Remove this, and only use organization sync from - // params - OrganizationIDs: []uuid.UUID{defaultOrganization.ID}, + Email: params.Email, + Username: params.Username, + OrganizationIDs: orgIDs, }, LoginType: params.LoginType, }) @@ -1481,6 +1491,13 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C } } + // Only OIDC really supports syncing like this. At some point, we might + // want to move this configuration and allow github to allow do org syncing. + err = api.OIDCConfig.IDPSync.SyncOrganizations(ctx, tx, user, params.OrganizationSync) + if err != nil { + return xerrors.Errorf("sync organizations: %w", err) + } + // Ensure groups are correct. // This places all groups into the default organization. // To go multi-org, we need to add a mapping feature here to know which From 951a7240622b4791be013bfc86a6588d3138cb44 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 27 Aug 2024 13:11:23 -0500 Subject: [PATCH 12/23] test: start implementing sync tests --- coderd/idpsync/idpsync.go | 3 +- coderd/idpsync/organization.go | 3 +- coderd/idpsync/organizations_test.go | 58 ++++++ enterprise/coderd/enidpsync/organizations.go | 3 +- .../coderd/enidpsync/organizations_test.go | 183 ++++++++++++++++++ 5 files changed, 247 insertions(+), 3 deletions(-) create mode 100644 coderd/idpsync/organizations_test.go create mode 100644 enterprise/coderd/enidpsync/organizations_test.go diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go index 2aeeb9e75a31e..f65e60bad089f 100644 --- a/coderd/idpsync/idpsync.go +++ b/coderd/idpsync/idpsync.go @@ -5,6 +5,7 @@ import ( "net/http" "strings" + "github.com/golang-jwt/jwt/v4" "github.com/google/uuid" "golang.org/x/xerrors" @@ -29,7 +30,7 @@ var NewSync = func(logger slog.Logger, entitlements *entitlements.Set, settings type IDPSync interface { // ParseOrganizationClaims takes claims from an OIDC provider, and returns the // organization sync params for assigning users into organizations. - ParseOrganizationClaims(ctx context.Context, _ map[string]interface{}) (OrganizationParams, *HttpError) + ParseOrganizationClaims(ctx context.Context, _ jwt.MapClaims) (OrganizationParams, *HttpError) // SyncOrganizations assigns and removed users from organizations based on the // provided params. SyncOrganizations(ctx context.Context, tx database.Store, user database.User, params OrganizationParams) error diff --git a/coderd/idpsync/organization.go b/coderd/idpsync/organization.go index 46028ea9601b9..d0ed1556fa59e 100644 --- a/coderd/idpsync/organization.go +++ b/coderd/idpsync/organization.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" + "github.com/golang-jwt/jwt/v4" "github.com/google/uuid" "golang.org/x/xerrors" @@ -15,7 +16,7 @@ import ( "github.com/coder/coder/v2/coderd/util/slice" ) -func (s AGPLIDPSync) ParseOrganizationClaims(ctx context.Context, _ map[string]interface{}) (OrganizationParams, *HttpError) { +func (s AGPLIDPSync) ParseOrganizationClaims(ctx context.Context, _ jwt.MapClaims) (OrganizationParams, *HttpError) { // nolint:gocritic // all syncing is done as a system user ctx = dbauthz.AsSystemRestricted(ctx) diff --git a/coderd/idpsync/organizations_test.go b/coderd/idpsync/organizations_test.go new file mode 100644 index 0000000000000..070e5591ed512 --- /dev/null +++ b/coderd/idpsync/organizations_test.go @@ -0,0 +1,58 @@ +package idpsync + +import ( + "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/entitlements" + "github.com/coder/coder/v2/testutil" +) + +func TestParseOrganizationClaims(t *testing.T) { + t.Parallel() + + t.Run("SingleOrgDeployment", func(t *testing.T) { + t.Parallel() + + s := NewAGPLSync(slogtest.Make(t, &slogtest.Options{}), entitlements.New(), SyncSettings{ + OrganizationField: "", + OrganizationMapping: nil, + OrganizationAssignDefault: true, + }) + + ctx := testutil.Context(t, testutil.WaitMedium) + + params, err := s.ParseOrganizationClaims(ctx, jwt.MapClaims{}) + require.Nil(t, err) + + require.Empty(t, params.Organizations) + require.True(t, params.IncludeDefault) + require.False(t, params.SyncEnabled) + }) + + t.Run("AGPL", func(t *testing.T) { + t.Parallel() + + // AGPL has limited behavior + s := NewAGPLSync(slogtest.Make(t, &slogtest.Options{}), entitlements.New(), SyncSettings{ + OrganizationField: "orgs", + OrganizationMapping: map[string][]uuid.UUID{ + "random": {uuid.New()}, + }, + OrganizationAssignDefault: false, + }) + + ctx := testutil.Context(t, testutil.WaitMedium) + + params, err := s.ParseOrganizationClaims(ctx, jwt.MapClaims{}) + require.Nil(t, err) + + require.Empty(t, params.Organizations) + require.False(t, params.IncludeDefault) + require.False(t, params.SyncEnabled) + }) +} diff --git a/enterprise/coderd/enidpsync/organizations.go b/enterprise/coderd/enidpsync/organizations.go index 5f215810235c5..989d150b0f4ff 100644 --- a/enterprise/coderd/enidpsync/organizations.go +++ b/enterprise/coderd/enidpsync/organizations.go @@ -4,6 +4,7 @@ import ( "context" "net/http" + "github.com/golang-jwt/jwt/v4" "github.com/google/uuid" "cdr.dev/slog" @@ -12,7 +13,7 @@ import ( "github.com/coder/coder/v2/codersdk" ) -func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedClaims map[string]interface{}) (idpsync.OrganizationParams, *idpsync.HttpError) { +func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedClaims jwt.MapClaims) (idpsync.OrganizationParams, *idpsync.HttpError) { if !e.entitlements.Enabled(codersdk.FeatureMultipleOrganizations) { // Default to agpl if multi-org is not enabled return e.AGPLIDPSync.ParseOrganizationClaims(ctx, mergedClaims) diff --git a/enterprise/coderd/enidpsync/organizations_test.go b/enterprise/coderd/enidpsync/organizations_test.go new file mode 100644 index 0000000000000..5564f9b79a4cf --- /dev/null +++ b/enterprise/coderd/enidpsync/organizations_test.go @@ -0,0 +1,183 @@ +package enidpsync + +import ( + "context" + "testing" + + "github.com/golang-jwt/jwt/v4" + "github.com/google/uuid" + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/require" + + "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/db2sdk" + "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/database/dbgen" + "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/coderd/entitlements" + "github.com/coder/coder/v2/coderd/idpsync" + "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/testutil" +) + +type ExpectedUser struct { + SyncError bool + Organizations []uuid.UUID +} + +type Expectations struct { + Name string + Claims jwt.MapClaims + // Parse + ParseError func(t *testing.T, httpErr *idpsync.HttpError) + ExpectedParams idpsync.OrganizationParams + // Mutate allows mutating the user before syncing + Mutate func(t *testing.T, db database.Store, user database.User) + Sync ExpectedUser +} + +type OrganizationSyncTestCase struct { + Settings idpsync.SyncSettings + Entitlements *entitlements.Set + Exps []Expectations +} + +func TestOrganizationSync(t *testing.T) { + t.Parallel() + + if dbtestutil.WillUsePostgres() { + t.Skip("Skipping test because it populates a lot of db entries, which is slow on postgres") + } + + requireUserOrgs := func(t *testing.T, db database.Store, user database.User, expected []uuid.UUID) { + t.Helper() + + // nolint:gocritic // in testing + members, err := db.OrganizationMembers(dbauthz.AsSystemRestricted(context.Background()), database.OrganizationMembersParams{ + UserID: user.ID, + }) + require.NoError(t, err) + + foundIDs := db2sdk.List(members, func(m database.OrganizationMembersRow) uuid.UUID { + return m.OrganizationMember.OrganizationID + }) + require.ElementsMatch(t, expected, foundIDs, "match user organizations") + } + + entitled := entitlements.New() + entitled.Update(func(entitlements *codersdk.Entitlements) { + entitlements.Features[codersdk.FeatureMultipleOrganizations] = codersdk.Feature{ + Entitlement: codersdk.EntitlementEntitled, + Enabled: true, + Limit: nil, + Actual: nil, + } + }) + + testCases := []struct { + Name string + Case func(t *testing.T, db database.Store) OrganizationSyncTestCase + }{ + { + Name: "SingleOrgDeployment", + Case: func(t *testing.T, db database.Store) OrganizationSyncTestCase { + def, _ := db.GetDefaultOrganization(context.Background()) + other := dbgen.Organization(t, db, database.Organization{}) + return OrganizationSyncTestCase{ + Entitlements: entitled, + Settings: idpsync.SyncSettings{ + OrganizationField: "", + OrganizationMapping: nil, + OrganizationAssignDefault: true, + }, + Exps: []Expectations{ + { + Name: "NoOrganizations", + Claims: jwt.MapClaims{}, + ExpectedParams: idpsync.OrganizationParams{ + SyncEnabled: false, + IncludeDefault: true, + Organizations: []uuid.UUID{}, + }, + Sync: ExpectedUser{ + Organizations: []uuid.UUID{}, + }, + }, + { + Name: "AlreadyInOrgs", + Claims: jwt.MapClaims{}, + ExpectedParams: idpsync.OrganizationParams{ + SyncEnabled: false, + IncludeDefault: true, + Organizations: []uuid.UUID{}, + }, + Mutate: func(t *testing.T, db database.Store, user database.User) { + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + UserID: user.ID, + OrganizationID: def.ID, + }) + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + UserID: user.ID, + OrganizationID: other.ID, + }) + }, + Sync: ExpectedUser{ + Organizations: []uuid.UUID{def.ID, other.ID}, + }, + }, + }, + } + }, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.Name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitMedium) + logger := slogtest.Make(t, &slogtest.Options{}) + + rdb, _ := dbtestutil.NewDB(t) + db := dbauthz.New(rdb, rbac.NewAuthorizer(prometheus.NewRegistry()), logger, coderdtest.AccessControlStorePointer()) + caseData := tc.Case(t, rdb) + if caseData.Entitlements == nil { + caseData.Entitlements = entitlements.New() + } + + // Create a new sync object + sync := NewSync(logger, caseData.Entitlements, caseData.Settings) + for _, exp := range caseData.Exps { + t.Run(exp.Name, func(t *testing.T) { + params, httpErr := sync.ParseOrganizationClaims(ctx, exp.Claims) + if exp.ParseError != nil { + exp.ParseError(t, httpErr) + return + } + + require.Equal(t, exp.ExpectedParams.SyncEnabled, params.SyncEnabled, "match enabled") + require.Equal(t, exp.ExpectedParams.IncludeDefault, params.IncludeDefault, "match include default") + if exp.ExpectedParams.Organizations == nil { + exp.ExpectedParams.Organizations = []uuid.UUID{} + } + require.ElementsMatch(t, exp.ExpectedParams.Organizations, params.Organizations, "match organizations") + + user := dbgen.User(t, db, database.User{}) + if exp.Mutate != nil { + exp.Mutate(t, db, user) + } + + err := sync.SyncOrganizations(ctx, db, user, params) + if exp.Sync.SyncError { + require.Error(t, err) + return + } + requireUserOrgs(t, db, user, exp.Sync.Organizations) + }) + } + }) + } +} From 8350ccafb6544e8fa5911c98887657d5dd3226e8 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 27 Aug 2024 15:11:12 -0500 Subject: [PATCH 13/23] fixup duplicate assignments --- enterprise/coderd/enidpsync/organizations.go | 4 +- .../coderd/enidpsync/organizations_test.go | 92 ++++++++++++++++++- 2 files changed, 93 insertions(+), 3 deletions(-) diff --git a/enterprise/coderd/enidpsync/organizations.go b/enterprise/coderd/enidpsync/organizations.go index 989d150b0f4ff..473e08955b8af 100644 --- a/enterprise/coderd/enidpsync/organizations.go +++ b/enterprise/coderd/enidpsync/organizations.go @@ -10,6 +10,7 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/idpsync" + "github.com/coder/coder/v2/coderd/util/slice" "github.com/coder/coder/v2/codersdk" ) @@ -62,6 +63,7 @@ func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedCl // If the field is not set, then sync is not enabled. SyncEnabled: e.OrganizationField != "", IncludeDefault: e.OrganizationAssignDefault, - Organizations: userOrganizations, + // Do not return duplicates + Organizations: slice.Unique(userOrganizations), }, nil } diff --git a/enterprise/coderd/enidpsync/organizations_test.go b/enterprise/coderd/enidpsync/organizations_test.go index 5564f9b79a4cf..9b9ee7ebda2de 100644 --- a/enterprise/coderd/enidpsync/organizations_test.go +++ b/enterprise/coderd/enidpsync/organizations_test.go @@ -132,6 +132,92 @@ func TestOrganizationSync(t *testing.T) { } }, }, + { + Name: "MultiOrgWithDefault", + Case: func(t *testing.T, db database.Store) OrganizationSyncTestCase { + def, _ := db.GetDefaultOrganization(context.Background()) + one := dbgen.Organization(t, db, database.Organization{}) + two := dbgen.Organization(t, db, database.Organization{}) + three := dbgen.Organization(t, db, database.Organization{}) + return OrganizationSyncTestCase{ + Entitlements: entitled, + Settings: idpsync.SyncSettings{ + OrganizationField: "organizations", + OrganizationMapping: map[string][]uuid.UUID{ + "first": {one.ID}, + "second": {two.ID}, + "third": {three.ID}, + }, + OrganizationAssignDefault: true, + }, + Exps: []Expectations{ + { + Name: "NoOrganizations", + Claims: jwt.MapClaims{}, + ExpectedParams: idpsync.OrganizationParams{ + SyncEnabled: true, + IncludeDefault: true, + Organizations: []uuid.UUID{}, + }, + Sync: ExpectedUser{ + Organizations: []uuid.UUID{def.ID}, + }, + }, + { + Name: "AlreadyInOrgs", + Claims: jwt.MapClaims{ + "organizations": []string{"second", "extra"}, + }, + ExpectedParams: idpsync.OrganizationParams{ + SyncEnabled: true, + IncludeDefault: true, + Organizations: []uuid.UUID{two.ID}, + }, + Mutate: func(t *testing.T, db database.Store, user database.User) { + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + UserID: user.ID, + OrganizationID: def.ID, + }) + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + UserID: user.ID, + OrganizationID: one.ID, + }) + }, + Sync: ExpectedUser{ + Organizations: []uuid.UUID{def.ID, two.ID}, + }, + }, + { + Name: "ManyClaims", + Claims: jwt.MapClaims{ + // Add some repeats + "organizations": []string{"second", "extra", "first", "third", "second", "second"}, + }, + ExpectedParams: idpsync.OrganizationParams{ + SyncEnabled: true, + IncludeDefault: true, + Organizations: []uuid.UUID{ + two.ID, one.ID, three.ID, + }, + }, + Mutate: func(t *testing.T, db database.Store, user database.User) { + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + UserID: user.ID, + OrganizationID: def.ID, + }) + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + UserID: user.ID, + OrganizationID: one.ID, + }) + }, + Sync: ExpectedUser{ + Organizations: []uuid.UUID{def.ID, one.ID, two.ID, three.ID}, + }, + }, + }, + } + }, + }, } for _, tc := range testCases { @@ -157,6 +243,7 @@ func TestOrganizationSync(t *testing.T) { exp.ParseError(t, httpErr) return } + require.Nil(t, httpErr, "no parse error") require.Equal(t, exp.ExpectedParams.SyncEnabled, params.SyncEnabled, "match enabled") require.Equal(t, exp.ExpectedParams.IncludeDefault, params.IncludeDefault, "match include default") @@ -167,14 +254,15 @@ func TestOrganizationSync(t *testing.T) { user := dbgen.User(t, db, database.User{}) if exp.Mutate != nil { - exp.Mutate(t, db, user) + exp.Mutate(t, rdb, user) } - err := sync.SyncOrganizations(ctx, db, user, params) + err := sync.SyncOrganizations(ctx, rdb, user, params) if exp.Sync.SyncError { require.Error(t, err) return } + require.NoError(t, err) requireUserOrgs(t, db, user, exp.Sync.Organizations) }) } From d5bf63aa412bf25c941ad00670e4e0e5873ca942 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 27 Aug 2024 15:22:35 -0500 Subject: [PATCH 14/23] linting --- cli/server.go | 4 +- coderd/idpsync/idpsync.go | 14 +++---- coderd/idpsync/organization.go | 5 +-- coderd/idpsync/organizations_test.go | 7 ++-- coderd/userauth.go | 40 +++++++++---------- enterprise/coderd/enidpsync/enidpsync.go | 6 +-- enterprise/coderd/enidpsync/organizations.go | 4 +- .../coderd/enidpsync/organizations_test.go | 6 +-- 8 files changed, 42 insertions(+), 44 deletions(-) diff --git a/cli/server.go b/cli/server.go index f6c7ff94792eb..79979fea7e392 100644 --- a/cli/server.go +++ b/cli/server.go @@ -109,7 +109,7 @@ import ( "github.com/coder/coder/v2/tailnet" ) -func createOIDCConfig(ctx context.Context, logger slog.Logger, entitlements *entitlements.Set, vals *codersdk.DeploymentValues) (*coderd.OIDCConfig, error) { +func createOIDCConfig(ctx context.Context, logger slog.Logger, set *entitlements.Set, vals *codersdk.DeploymentValues) (*coderd.OIDCConfig, error) { if vals.OIDC.ClientID == "" { return nil, xerrors.Errorf("OIDC client ID must be set!") } @@ -199,7 +199,7 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, entitlements *ent SignupsDisabledText: vals.OIDC.SignupsDisabledText.String(), IconURL: vals.OIDC.IconURL.String(), IgnoreEmailVerified: vals.OIDC.IgnoreEmailVerified.Value(), - IDPSync: idpsync.NewSync(logger, entitlements, idpsync.SyncSettings{ + IDPSync: idpsync.NewSync(logger, set, idpsync.SyncSettings{ OrganizationField: vals.OIDC.OrganizationField.Value(), OrganizationMapping: vals.OIDC.OrganizationMapping.Value, OrganizationAssignDefault: vals.OIDC.OrganizationAssignDefault.Value(), diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go index f65e60bad089f..7c578e56dc2b3 100644 --- a/coderd/idpsync/idpsync.go +++ b/coderd/idpsync/idpsync.go @@ -23,14 +23,14 @@ import ( // So instead, if the code is compiled with the enterprise logic, it will // override this function to return the enterprise IDP sync object. // For unit testing, the callers can specifically choose which "NewSync" to use. -var NewSync = func(logger slog.Logger, entitlements *entitlements.Set, settings SyncSettings) IDPSync { - return NewAGPLSync(logger, entitlements, settings) +var NewSync = func(logger slog.Logger, set *entitlements.Set, settings SyncSettings) IDPSync { + return NewAGPLSync(logger, set, settings) } type IDPSync interface { // ParseOrganizationClaims takes claims from an OIDC provider, and returns the // organization sync params for assigning users into organizations. - ParseOrganizationClaims(ctx context.Context, _ jwt.MapClaims) (OrganizationParams, *HttpError) + ParseOrganizationClaims(ctx context.Context, _ jwt.MapClaims) (OrganizationParams, *HTTPError) // SyncOrganizations assigns and removed users from organizations based on the // provided params. SyncOrganizations(ctx context.Context, tx database.Store, user database.User, params OrganizationParams) error @@ -111,10 +111,10 @@ func ParseStringSliceClaim(claim interface{}) ([]string, error) { return nil, xerrors.Errorf("invalid claim type. Expected an array of strings, got: %T", claim) } -// HttpError is a helper struct for returning errors from the IDP sync process. +// HTTPError is a helper struct for returning errors from the IDP sync process. // A regular error is not sufficient because many of these errors are surfaced // to a user logging in, and the errors should be descriptive. -type HttpError struct { +type HTTPError struct { Code int Msg string Detail string @@ -122,7 +122,7 @@ type HttpError struct { RenderDetailMarkdown bool } -func (e HttpError) Write(rw http.ResponseWriter, r *http.Request) { +func (e HTTPError) Write(rw http.ResponseWriter, r *http.Request) { if e.RenderStaticPage { site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ Status: e.Code, @@ -142,7 +142,7 @@ func (e HttpError) Write(rw http.ResponseWriter, r *http.Request) { }) } -func (e HttpError) Error() string { +func (e HTTPError) Error() string { if e.Detail != "" { return e.Detail } diff --git a/coderd/idpsync/organization.go b/coderd/idpsync/organization.go index d0ed1556fa59e..1a5d1904df599 100644 --- a/coderd/idpsync/organization.go +++ b/coderd/idpsync/organization.go @@ -16,10 +16,7 @@ import ( "github.com/coder/coder/v2/coderd/util/slice" ) -func (s AGPLIDPSync) ParseOrganizationClaims(ctx context.Context, _ jwt.MapClaims) (OrganizationParams, *HttpError) { - // nolint:gocritic // all syncing is done as a system user - ctx = dbauthz.AsSystemRestricted(ctx) - +func (s AGPLIDPSync) ParseOrganizationClaims(ctx context.Context, _ jwt.MapClaims) (OrganizationParams, *HTTPError) { // For AGPL we only sync the default organization. return OrganizationParams{ SyncEnabled: false, diff --git a/coderd/idpsync/organizations_test.go b/coderd/idpsync/organizations_test.go index 070e5591ed512..65aab2b3e5365 100644 --- a/coderd/idpsync/organizations_test.go +++ b/coderd/idpsync/organizations_test.go @@ -1,4 +1,4 @@ -package idpsync +package idpsync_test import ( "testing" @@ -9,6 +9,7 @@ import ( "cdr.dev/slog/sloggers/slogtest" "github.com/coder/coder/v2/coderd/entitlements" + "github.com/coder/coder/v2/coderd/idpsync" "github.com/coder/coder/v2/testutil" ) @@ -18,7 +19,7 @@ func TestParseOrganizationClaims(t *testing.T) { t.Run("SingleOrgDeployment", func(t *testing.T) { t.Parallel() - s := NewAGPLSync(slogtest.Make(t, &slogtest.Options{}), entitlements.New(), SyncSettings{ + s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{}), entitlements.New(), idpsync.SyncSettings{ OrganizationField: "", OrganizationMapping: nil, OrganizationAssignDefault: true, @@ -38,7 +39,7 @@ func TestParseOrganizationClaims(t *testing.T) { t.Parallel() // AGPL has limited behavior - s := NewAGPLSync(slogtest.Make(t, &slogtest.Options{}), entitlements.New(), SyncSettings{ + s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{}), entitlements.New(), idpsync.SyncSettings{ OrganizationField: "orgs", OrganizationMapping: map[string][]uuid.UUID{ "random": {uuid.New()}, diff --git a/coderd/userauth.go b/coderd/userauth.go index a434aaac5871f..373faba1e02e5 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -669,7 +669,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { }) cookies, user, key, err := api.oauthLogin(r, params) defer params.CommitAuditLogs() - var httpErr idpsync.HttpError + var httpErr idpsync.HTTPError if xerrors.As(err, &httpErr) { httpErr.Write(rw, r) return @@ -1069,7 +1069,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { }) cookies, user, key, err := api.oauthLogin(r, params) defer params.CommitAuditLogs() - var httpErr idpsync.HttpError + var httpErr idpsync.HTTPError if xerrors.As(err, &httpErr) { httpErr.Write(rw, r) return @@ -1097,7 +1097,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { } // oidcGroups returns the groups for the user from the OIDC claims. -func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interface{}) (bool, []string, *idpsync.HttpError) { +func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interface{}) (bool, []string, *idpsync.HTTPError) { logger := api.Logger.Named(userAuthLoggerName) usingGroups := false var groups []string @@ -1118,7 +1118,7 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac slog.F("type", fmt.Sprintf("%T", groupsRaw)), slog.Error(err), ) - return false, nil, &idpsync.HttpError{ + return false, nil, &idpsync.HTTPError{ Code: http.StatusBadRequest, Msg: "Failed to sync groups from OIDC claims", Detail: err.Error(), @@ -1151,7 +1151,7 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac if len(groups) == 0 { detail = "You are currently not a member of any groups! Ask an administrator to add you to an authorized group to login." } - return usingGroups, groups, &idpsync.HttpError{ + return usingGroups, groups, &idpsync.HTTPError{ Code: http.StatusForbidden, Msg: "Not a member of an allowed group", Detail: detail, @@ -1175,7 +1175,7 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac // It would be preferred to just return an error, however this function // decorates returned errors with the appropriate HTTP status codes and details // that are hard to carry in a standard `error` without more work. -func (api *API) oidcRoles(ctx context.Context, mergedClaims map[string]interface{}) ([]string, *idpsync.HttpError) { +func (api *API) oidcRoles(ctx context.Context, mergedClaims map[string]interface{}) ([]string, *idpsync.HTTPError) { roles := api.OIDCConfig.UserRolesDefault if !api.OIDCConfig.RoleSyncEnabled() { return roles, nil @@ -1197,7 +1197,7 @@ func (api *API) oidcRoles(ctx context.Context, mergedClaims map[string]interface slog.F("type", fmt.Sprintf("%T", rolesRow)), slog.Error(err), ) - return nil, &idpsync.HttpError{ + return nil, &idpsync.HTTPError{ Code: http.StatusInternalServerError, Msg: "Login disabled until OIDC config is fixed", Detail: fmt.Sprintf("Roles claim must be an array of strings, type found: %T. Disabling role sync will allow login to proceed.", rolesRow), @@ -1358,7 +1358,7 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C if api.OIDCConfig != nil && api.OIDCConfig.SignupsDisabledText != "" { signupsDisabledText = render.HTMLFromMarkdown(api.OIDCConfig.SignupsDisabledText) } - return &idpsync.HttpError{ + return &idpsync.HTTPError{ Code: http.StatusForbidden, Msg: "Signups are disabled", Detail: signupsDisabledText, @@ -1409,7 +1409,7 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C } } if !validUsername { - return &idpsync.HttpError{ + return &idpsync.HTTPError{ Code: http.StatusConflict, Msg: fmt.Sprintf("exhausted alternatives for taken username %q", original), } @@ -1564,7 +1564,7 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C //nolint:gocritic err := api.Options.SetUserSiteRoles(dbauthz.AsSystemRestricted(ctx), logger, tx, user.ID, filtered) if err != nil { - return &idpsync.HttpError{ + return &idpsync.HTTPError{ Code: http.StatusBadRequest, Msg: "Invalid roles through OIDC claims", Detail: fmt.Sprintf("Error from role assignment attempt: %s", err.Error()), @@ -1679,7 +1679,7 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data // Trying to convert to OIDC, but the email does not match. // So do not make a new user, just block the request. if user.ID == uuid.Nil { - return database.User{}, idpsync.HttpError{ + return database.User{}, idpsync.HTTPError{ Code: http.StatusBadRequest, Msg: fmt.Sprintf("The oidc account with the email %q does not match the email of the account you are trying to convert. Contact your administrator to resolve this issue.", params.Email), } @@ -1687,7 +1687,7 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data jwtCookie, err := r.Cookie(OAuthConvertCookieValue) if err != nil { - return database.User{}, idpsync.HttpError{ + return database.User{}, idpsync.HTTPError{ Code: http.StatusBadRequest, Msg: fmt.Sprintf("Convert to oauth cookie not found. Missing signed jwt to authorize this action. " + "Please try again."), @@ -1699,13 +1699,13 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data }) if xerrors.Is(err, jwt.ErrSignatureInvalid) || !token.Valid { // These errors are probably because the user is mixing 2 coder deployments. - return database.User{}, idpsync.HttpError{ + return database.User{}, idpsync.HTTPError{ Code: http.StatusBadRequest, Msg: "Using an invalid jwt to authorize this action. Ensure there is only 1 coder deployment and try again.", } } if err != nil { - return database.User{}, idpsync.HttpError{ + return database.User{}, idpsync.HTTPError{ Code: http.StatusInternalServerError, Msg: fmt.Sprintf("Error parsing jwt: %v", err), } @@ -1727,14 +1727,14 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data oauthConvertAudit.Old = user if claims.RegisteredClaims.Issuer != api.DeploymentID { - return database.User{}, idpsync.HttpError{ + return database.User{}, idpsync.HTTPError{ Code: http.StatusForbidden, Msg: "Request to convert login type failed. Issuer mismatch. Found a cookie from another coder deployment, please try again.", } } if params.State.StateString != claims.State { - return database.User{}, idpsync.HttpError{ + return database.User{}, idpsync.HTTPError{ Code: http.StatusForbidden, Msg: "Request to convert login type failed. State mismatch.", } @@ -1746,7 +1746,7 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data if user.ID != claims.UserID || codersdk.LoginType(user.LoginType) != claims.FromLoginType || codersdk.LoginType(params.LoginType) != claims.ToLoginType { - return database.User{}, idpsync.HttpError{ + return database.User{}, idpsync.HTTPError{ Code: http.StatusForbidden, Msg: fmt.Sprintf("Request to convert login type from %s to %s failed", user.LoginType, params.LoginType), } @@ -1762,7 +1762,7 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data UserID: user.ID, }) if err != nil { - return database.User{}, idpsync.HttpError{ + return database.User{}, idpsync.HTTPError{ Code: http.StatusInternalServerError, Msg: "Failed to convert user to new login type", } @@ -1850,12 +1850,12 @@ func clearOAuthConvertCookie() *http.Cookie { } } -func wrongLoginTypeHTTPError(user database.LoginType, params database.LoginType) idpsync.HttpError { +func wrongLoginTypeHTTPError(user database.LoginType, params database.LoginType) idpsync.HTTPError { addedMsg := "" if user == database.LoginTypePassword { addedMsg = " You can convert your account to use this login type by visiting your account settings." } - return idpsync.HttpError{ + return idpsync.HTTPError{ Code: http.StatusForbidden, RenderStaticPage: true, Msg: "Incorrect login type", diff --git a/enterprise/coderd/enidpsync/enidpsync.go b/enterprise/coderd/enidpsync/enidpsync.go index 412cb647e53f5..2e5dca4798bd6 100644 --- a/enterprise/coderd/enidpsync/enidpsync.go +++ b/enterprise/coderd/enidpsync/enidpsync.go @@ -18,9 +18,9 @@ type EnterpriseIDPSync struct { *idpsync.AGPLIDPSync } -func NewSync(logger slog.Logger, entitlements *entitlements.Set, settings idpsync.SyncSettings) *EnterpriseIDPSync { +func NewSync(logger slog.Logger, set *entitlements.Set, settings idpsync.SyncSettings) *EnterpriseIDPSync { return &EnterpriseIDPSync{ - entitlements: entitlements, - AGPLIDPSync: idpsync.NewAGPLSync(logger.With(slog.F("enterprise_capable", "true")), entitlements, settings), + entitlements: set, + AGPLIDPSync: idpsync.NewAGPLSync(logger.With(slog.F("enterprise_capable", "true")), set, settings), } } diff --git a/enterprise/coderd/enidpsync/organizations.go b/enterprise/coderd/enidpsync/organizations.go index 473e08955b8af..a89246d877f05 100644 --- a/enterprise/coderd/enidpsync/organizations.go +++ b/enterprise/coderd/enidpsync/organizations.go @@ -14,7 +14,7 @@ import ( "github.com/coder/coder/v2/codersdk" ) -func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedClaims jwt.MapClaims) (idpsync.OrganizationParams, *idpsync.HttpError) { +func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedClaims jwt.MapClaims) (idpsync.OrganizationParams, *idpsync.HTTPError) { if !e.entitlements.Enabled(codersdk.FeatureMultipleOrganizations) { // Default to agpl if multi-org is not enabled return e.AGPLIDPSync.ParseOrganizationClaims(ctx, mergedClaims) @@ -30,7 +30,7 @@ func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedCl if ok { parsedOrganizations, err := idpsync.ParseStringSliceClaim(organizationRaw) if err != nil { - return idpsync.OrganizationParams{}, &idpsync.HttpError{ + return idpsync.OrganizationParams{}, &idpsync.HTTPError{ Code: http.StatusBadRequest, Msg: "Failed to sync organizations from the OIDC claims", Detail: err.Error(), diff --git a/enterprise/coderd/enidpsync/organizations_test.go b/enterprise/coderd/enidpsync/organizations_test.go index 9b9ee7ebda2de..bb2801aa39f80 100644 --- a/enterprise/coderd/enidpsync/organizations_test.go +++ b/enterprise/coderd/enidpsync/organizations_test.go @@ -1,4 +1,4 @@ -package enidpsync +package enidpsync_test import ( "context" @@ -32,7 +32,7 @@ type Expectations struct { Name string Claims jwt.MapClaims // Parse - ParseError func(t *testing.T, httpErr *idpsync.HttpError) + ParseError func(t *testing.T, httpErr *idpsync.HTTPError) ExpectedParams idpsync.OrganizationParams // Mutate allows mutating the user before syncing Mutate func(t *testing.T, db database.Store, user database.User) @@ -235,7 +235,7 @@ func TestOrganizationSync(t *testing.T) { } // Create a new sync object - sync := NewSync(logger, caseData.Entitlements, caseData.Settings) + sync := idpsync.NewSync(logger, caseData.Entitlements, caseData.Settings) for _, exp := range caseData.Exps { t.Run(exp.Name, func(t *testing.T) { params, httpErr := sync.ParseOrganizationClaims(ctx, exp.Claims) From b3144c0904ee91788d718cdeabf7aad163427f85 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 27 Aug 2024 16:05:46 -0500 Subject: [PATCH 15/23] move the config into api options --- cli/server.go | 6 ------ coderd/coderd.go | 11 +++++++++++ coderd/idpsync/idpsync.go | 13 +------------ coderd/idpsync/organization.go | 2 +- coderd/userauth.go | 9 ++------- codersdk/deployment.go | 25 +++++++++++++++++++++++++ enterprise/coderd/coderd.go | 9 +++++++++ 7 files changed, 49 insertions(+), 26 deletions(-) diff --git a/cli/server.go b/cli/server.go index 79979fea7e392..6a053ee15534d 100644 --- a/cli/server.go +++ b/cli/server.go @@ -56,7 +56,6 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/sloghuman" "github.com/coder/coder/v2/coderd/entitlements" - "github.com/coder/coder/v2/coderd/idpsync" "github.com/coder/pretty" "github.com/coder/quartz" "github.com/coder/retry" @@ -199,11 +198,6 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, set *entitlements SignupsDisabledText: vals.OIDC.SignupsDisabledText.String(), IconURL: vals.OIDC.IconURL.String(), IgnoreEmailVerified: vals.OIDC.IgnoreEmailVerified.Value(), - IDPSync: idpsync.NewSync(logger, set, idpsync.SyncSettings{ - OrganizationField: vals.OIDC.OrganizationField.Value(), - OrganizationMapping: vals.OIDC.OrganizationMapping.Value, - OrganizationAssignDefault: vals.OIDC.OrganizationAssignDefault.Value(), - }), }, nil } diff --git a/coderd/coderd.go b/coderd/coderd.go index 939a7141e6df5..20ce616eab5ba 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -38,6 +38,7 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/coderd/entitlements" + "github.com/coder/coder/v2/coderd/idpsync" "github.com/coder/quartz" "github.com/coder/serpent" @@ -243,6 +244,9 @@ type Options struct { WorkspaceUsageTracker *workspacestats.UsageTracker // NotificationsEnqueuer handles enqueueing notifications for delivery by SMTP, webhook, etc. NotificationsEnqueuer notifications.Enqueuer + + // IDPSync holds all configured values for syncing external IDP users into Coder. + IDPSync idpsync.IDPSync } // @title Coder API @@ -270,6 +274,13 @@ func New(options *Options) *API { if options.Entitlements == nil { options.Entitlements = entitlements.New() } + if options.IDPSync == nil { + options.IDPSync = idpsync.NewAGPLSync(options.Logger, idpsync.SyncSettings{ + OrganizationField: options.DeploymentValues.OIDC.OrganizationField.Value(), + OrganizationMapping: options.DeploymentValues.OIDC.OrganizationMapping.Value, + OrganizationAssignDefault: options.DeploymentValues.OIDC.OrganizationAssignDefault.Value(), + }) + } if options.NewTicker == nil { options.NewTicker = func(duration time.Duration) (tick <-chan time.Time, done func()) { ticker := time.NewTicker(duration) diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go index 7c578e56dc2b3..3f20e7a143eb7 100644 --- a/coderd/idpsync/idpsync.go +++ b/coderd/idpsync/idpsync.go @@ -11,22 +11,11 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/coderd/database" - "github.com/coder/coder/v2/coderd/entitlements" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/site" ) -// NewSync is a factory function for creating an IDP sync object. -// Due to the way we instantiate Coder, there is no way for the enterprise -// cli wrapper to pass in the enterprise IDP sync object. -// So instead, if the code is compiled with the enterprise logic, it will -// override this function to return the enterprise IDP sync object. -// For unit testing, the callers can specifically choose which "NewSync" to use. -var NewSync = func(logger slog.Logger, set *entitlements.Set, settings SyncSettings) IDPSync { - return NewAGPLSync(logger, set, settings) -} - type IDPSync interface { // ParseOrganizationClaims takes claims from an OIDC provider, and returns the // organization sync params for assigning users into organizations. @@ -57,7 +46,7 @@ type SyncSettings struct { OrganizationAssignDefault bool } -func NewAGPLSync(logger slog.Logger, _ *entitlements.Set, settings SyncSettings) *AGPLIDPSync { +func NewAGPLSync(logger slog.Logger, settings SyncSettings) *AGPLIDPSync { return &AGPLIDPSync{ Logger: logger.Named("idp-sync"), SyncSettings: settings, diff --git a/coderd/idpsync/organization.go b/coderd/idpsync/organization.go index 1a5d1904df599..1e4b913abd79f 100644 --- a/coderd/idpsync/organization.go +++ b/coderd/idpsync/organization.go @@ -16,7 +16,7 @@ import ( "github.com/coder/coder/v2/coderd/util/slice" ) -func (s AGPLIDPSync) ParseOrganizationClaims(ctx context.Context, _ jwt.MapClaims) (OrganizationParams, *HTTPError) { +func (s AGPLIDPSync) ParseOrganizationClaims(_ context.Context, _ jwt.MapClaims) (OrganizationParams, *HTTPError) { // For AGPL we only sync the default organization. return OrganizationParams{ SyncEnabled: false, diff --git a/coderd/userauth.go b/coderd/userauth.go index 373faba1e02e5..23066d26b4126 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -742,9 +742,6 @@ type OIDCConfig struct { // support the userinfo endpoint, or if the userinfo endpoint causes // undesirable behavior. IgnoreUserInfo bool - // IDPSync contains all the configuration for syncing user information - // from the external IDP. - IDPSync idpsync.IDPSync // TODO: Move all idp fields into the IDPSync struct // GroupField selects the claim field to be used as the created user's @@ -1030,7 +1027,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { return } - orgSync, orgSyncErr := api.OIDCConfig.IDPSync.ParseOrganizationClaims(ctx, mergedClaims) + orgSync, orgSyncErr := api.IDPSync.ParseOrganizationClaims(ctx, mergedClaims) if orgSyncErr != nil { orgSyncErr.Write(rw, r) return @@ -1491,9 +1488,7 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C } } - // Only OIDC really supports syncing like this. At some point, we might - // want to move this configuration and allow github to allow do org syncing. - err = api.OIDCConfig.IDPSync.SyncOrganizations(ctx, tx, user, params.OrganizationSync) + err = api.IDPSync.SyncOrganizations(ctx, tx, user, params.OrganizationSync) if err != nil { return xerrors.Errorf("sync organizations: %w", err) } diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 6efa9b73e74df..aa3c2b92686d2 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -1545,6 +1545,31 @@ when required by your organization's security policy.`, Group: &deploymentGroupOIDC, YAML: "ignoreUserInfo", }, + { + Name: "OIDC Organization Field", + Description: "This field must be set if using the organization sync feature." + + " Set to the claim to be used for organizations.", + Flag: "oidc-organization-field", + Env: "CODER_OIDC_ORGANIZATION_FIELD", + // Empty value means sync is disabled + Default: "", + Value: &c.OIDC.OrganizationField, + Group: &deploymentGroupOIDC, + YAML: "organizationField", + }, + { + Name: "OIDC Assign Default Organization", + Description: "If set to true, users will always be added to the default organization. " + + "If organization sync is enabled, then the default org is always added to the user's set of expected" + + "organizations.", + Flag: "oidc-organization-assign-default", + Env: "CODER_OIDC_ORGANIZATION_ASSIGN_DEFAULT", + // Single org deployments should always have this enabled. + Default: "true", + Value: &c.OIDC.OrganizationAssignDefault, + Group: &deploymentGroupOIDC, + YAML: "organizationAssignDefault", + }, { Name: "OIDC Group Field", Description: "This field must be set if using the group sync feature and the scope name is not 'groups'. Set to the claim to be used for groups.", diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 92225f3e44ecb..971b675f20d02 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -16,8 +16,10 @@ import ( "github.com/coder/coder/v2/coderd/appearance" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/entitlements" + "github.com/coder/coder/v2/coderd/idpsync" agplportsharing "github.com/coder/coder/v2/coderd/portsharing" "github.com/coder/coder/v2/coderd/rbac/policy" + "github.com/coder/coder/v2/enterprise/coderd/enidpsync" "github.com/coder/coder/v2/enterprise/coderd/portsharing" "golang.org/x/xerrors" @@ -78,6 +80,13 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { if options.Entitlements == nil { options.Entitlements = entitlements.New() } + if options.IDPSync == nil { + options.IDPSync = enidpsync.NewSync(options.Logger, options.Entitlements, idpsync.SyncSettings{ + OrganizationField: options.DeploymentValues.OIDC.OrganizationField.Value(), + OrganizationMapping: options.DeploymentValues.OIDC.OrganizationMapping.Value, + OrganizationAssignDefault: options.DeploymentValues.OIDC.OrganizationAssignDefault.Value(), + }) + } ctx, cancelFunc := context.WithCancel(ctx) From 72b501e952561de977346ef52dcf4ed8d0832ce6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 28 Aug 2024 09:47:07 -0500 Subject: [PATCH 16/23] change sqlc version --- coderd/database/models.go | 2 +- coderd/database/querier.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/models.go b/coderd/database/models.go index 18571981a4c92..959609d82eb79 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -1,6 +1,6 @@ // Code generated by sqlc. DO NOT EDIT. // versions: -// sqlc v1.26.0 +// sqlc v1.25.0 package database diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 974478a767579..a159735cbbf69 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -1,6 +1,6 @@ // Code generated by sqlc. DO NOT EDIT. // versions: -// sqlc v1.26.0 +// sqlc v1.25.0 package database From 521623057de96e98ee7594eddbd909fca1e590fc Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 28 Aug 2024 09:54:33 -0500 Subject: [PATCH 17/23] some cleanup --- cli/server.go | 4 ++-- coderd/coderd.go | 2 ++ coderd/idpsync/idpsync.go | 5 +++++ coderd/idpsync/organizations_test.go | 5 ++--- codersdk/deployment.go | 11 +++++++++++ 5 files changed, 22 insertions(+), 5 deletions(-) diff --git a/cli/server.go b/cli/server.go index 6a053ee15534d..6773b62f57832 100644 --- a/cli/server.go +++ b/cli/server.go @@ -108,7 +108,7 @@ import ( "github.com/coder/coder/v2/tailnet" ) -func createOIDCConfig(ctx context.Context, logger slog.Logger, set *entitlements.Set, vals *codersdk.DeploymentValues) (*coderd.OIDCConfig, error) { +func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.DeploymentValues) (*coderd.OIDCConfig, error) { if vals.OIDC.ClientID == "" { return nil, xerrors.Errorf("OIDC client ID must be set!") } @@ -669,7 +669,7 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. // Missing: // - Userinfo // - Verify - oc, err := createOIDCConfig(ctx, options.Logger, options.Entitlements, vals) + oc, err := createOIDCConfig(ctx, options.Logger, vals) if err != nil { return xerrors.Errorf("create oidc config: %w", err) } diff --git a/coderd/coderd.go b/coderd/coderd.go index 20ce616eab5ba..6998091e8847f 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -275,6 +275,8 @@ func New(options *Options) *API { options.Entitlements = entitlements.New() } if options.IDPSync == nil { + // If this is set in the options, it is probably the enterprise + // version of the code. options.IDPSync = idpsync.NewAGPLSync(options.Logger, idpsync.SyncSettings{ OrganizationField: options.DeploymentValues.OIDC.OrganizationField.Value(), OrganizationMapping: options.DeploymentValues.OIDC.OrganizationMapping.Value, diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go index 3f20e7a143eb7..37b47a37bf88b 100644 --- a/coderd/idpsync/idpsync.go +++ b/coderd/idpsync/idpsync.go @@ -16,6 +16,11 @@ import ( "github.com/coder/coder/v2/site" ) +// IDPSync is an interface, so we can implement this as AGPL and as enterprise, +// and just swap the underlying implementation. +// IDPSync exists to contain all the logic for mapping a user's external IDP +// claims to the internal representation of a user in Coder. +// TODO: Move group + role sync into this interface. type IDPSync interface { // ParseOrganizationClaims takes claims from an OIDC provider, and returns the // organization sync params for assigning users into organizations. diff --git a/coderd/idpsync/organizations_test.go b/coderd/idpsync/organizations_test.go index 65aab2b3e5365..03b1ebfa4b27b 100644 --- a/coderd/idpsync/organizations_test.go +++ b/coderd/idpsync/organizations_test.go @@ -8,7 +8,6 @@ import ( "github.com/stretchr/testify/require" "cdr.dev/slog/sloggers/slogtest" - "github.com/coder/coder/v2/coderd/entitlements" "github.com/coder/coder/v2/coderd/idpsync" "github.com/coder/coder/v2/testutil" ) @@ -19,7 +18,7 @@ func TestParseOrganizationClaims(t *testing.T) { t.Run("SingleOrgDeployment", func(t *testing.T) { t.Parallel() - s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{}), entitlements.New(), idpsync.SyncSettings{ + s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{}), idpsync.SyncSettings{ OrganizationField: "", OrganizationMapping: nil, OrganizationAssignDefault: true, @@ -39,7 +38,7 @@ func TestParseOrganizationClaims(t *testing.T) { t.Parallel() // AGPL has limited behavior - s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{}), entitlements.New(), idpsync.SyncSettings{ + s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{}), idpsync.SyncSettings{ OrganizationField: "orgs", OrganizationMapping: map[string][]uuid.UUID{ "random": {uuid.New()}, diff --git a/codersdk/deployment.go b/codersdk/deployment.go index aa3c2b92686d2..c0acf9ca01e6e 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -1570,6 +1570,17 @@ when required by your organization's security policy.`, Group: &deploymentGroupOIDC, YAML: "organizationAssignDefault", }, + { + Name: "OIDC Organization Sync Mapping", + Description: "A map of OIDC claims and the organizations in Coder it should map to. " + + "This is required because organization IDs must be used within Coder.", + Flag: "oidc-organization-mapping", + Env: "CODER_OIDC_ORGANIZATION_MAPPING", + Default: "{}", + Value: &c.OIDC.OrganizationMapping, + Group: &deploymentGroupOIDC, + YAML: "organizationMapping", + }, { Name: "OIDC Group Field", Description: "This field must be set if using the group sync feature and the scope name is not 'groups'. Set to the claim to be used for groups.", From e73919e5d3f8e7e27e0fd0b3eb1d2a74110bb551 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 28 Aug 2024 11:10:22 -0500 Subject: [PATCH 18/23] test: add full org sync tests --- coderd/database/dbauthz/dbauthz.go | 2 +- coderd/idpsync/idpsync.go | 16 ++ coderd/idpsync/idpsync_test.go | 8 + coderd/idpsync/organization.go | 7 +- coderd/members.go | 9 + coderd/userauth.go | 18 +- coderd/userauth_test.go | 6 + coderd/users.go | 4 - enterprise/coderd/enidpsync/enidpsync.go | 8 +- enterprise/coderd/enidpsync/organizations.go | 6 +- enterprise/coderd/userauth_test.go | 249 ++++++++++++++++++- 11 files changed, 304 insertions(+), 29 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 8dd903a2a9137..2da1462e8af00 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -243,7 +243,7 @@ var ( rbac.ResourceAssignOrgRole.Type: rbac.ResourceAssignOrgRole.AvailableActions(), rbac.ResourceSystem.Type: {policy.WildcardSymbol}, rbac.ResourceOrganization.Type: {policy.ActionCreate, policy.ActionRead}, - rbac.ResourceOrganizationMember.Type: {policy.ActionCreate}, + rbac.ResourceOrganizationMember.Type: {policy.ActionCreate, policy.ActionDelete, policy.ActionRead}, rbac.ResourceProvisionerDaemon.Type: {policy.ActionCreate, policy.ActionUpdate}, rbac.ResourceProvisionerKeys.Type: {policy.ActionCreate, policy.ActionRead, policy.ActionDelete}, rbac.ResourceUser.Type: rbac.ResourceUser.AvailableActions(), diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go index 37b47a37bf88b..da88851c83a93 100644 --- a/coderd/idpsync/idpsync.go +++ b/coderd/idpsync/idpsync.go @@ -22,6 +22,7 @@ import ( // claims to the internal representation of a user in Coder. // TODO: Move group + role sync into this interface. type IDPSync interface { + OrganizationSyncEnabled() bool // ParseOrganizationClaims takes claims from an OIDC provider, and returns the // organization sync params for assigning users into organizations. ParseOrganizationClaims(ctx context.Context, _ jwt.MapClaims) (OrganizationParams, *HTTPError) @@ -105,6 +106,21 @@ func ParseStringSliceClaim(claim interface{}) ([]string, error) { return nil, xerrors.Errorf("invalid claim type. Expected an array of strings, got: %T", claim) } +// IsHTTPError handles us being inconsistent with returning errors as values or +// pointers. +func IsHTTPError(err error) *HTTPError { + var httpErr HTTPError + if xerrors.As(err, &httpErr) { + return &httpErr + } + + var httpErrPtr *HTTPError + if xerrors.As(err, &httpErrPtr) { + return httpErrPtr + } + return nil +} + // HTTPError is a helper struct for returning errors from the IDP sync process. // A regular error is not sufficient because many of these errors are surfaced // to a user logging in, and the errors should be descriptive. diff --git a/coderd/idpsync/idpsync_test.go b/coderd/idpsync/idpsync_test.go index 673d6131da6ef..afb2d4060c51c 100644 --- a/coderd/idpsync/idpsync_test.go +++ b/coderd/idpsync/idpsync_test.go @@ -135,3 +135,11 @@ func TestParseStringSliceClaim(t *testing.T) { }) } } + +func TestIsHTTPError(t *testing.T) { + herr := idpsync.HTTPError{} + require.NotNil(t, idpsync.IsHTTPError(herr)) + require.NotNil(t, idpsync.IsHTTPError(&herr)) + + require.Nil(t, error(nil)) +} diff --git a/coderd/idpsync/organization.go b/coderd/idpsync/organization.go index 1e4b913abd79f..23cf8d35f84f5 100644 --- a/coderd/idpsync/organization.go +++ b/coderd/idpsync/organization.go @@ -16,10 +16,15 @@ import ( "github.com/coder/coder/v2/coderd/util/slice" ) +func (s AGPLIDPSync) OrganizationSyncEnabled() bool { + // AGPL does not support syncing organizations. + return false +} + func (s AGPLIDPSync) ParseOrganizationClaims(_ context.Context, _ jwt.MapClaims) (OrganizationParams, *HTTPError) { // For AGPL we only sync the default organization. return OrganizationParams{ - SyncEnabled: false, + SyncEnabled: s.OrganizationSyncEnabled(), IncludeDefault: s.OrganizationAssignDefault, Organizations: []uuid.UUID{}, }, nil diff --git a/coderd/members.go b/coderd/members.go index 805bdafbd0447..6f5c0c5864f08 100644 --- a/coderd/members.go +++ b/coderd/members.go @@ -2,6 +2,7 @@ package coderd import ( "context" + "fmt" "net/http" "github.com/google/uuid" @@ -43,6 +44,14 @@ func (api *API) postOrganizationMember(rw http.ResponseWriter, r *http.Request) aReq.Old = database.AuditableOrganizationMember{} defer commitAudit() + if user.LoginType == database.LoginTypeOIDC && api.IDPSync.OrganizationSyncEnabled() { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Organization sync is enabled for OIDC users, meaning manual organization assignment is not allowed for this user.", + Detail: fmt.Sprintf("User %s is an OIDC user and organization sync is enabled. Ask an administrator to resolve this in your external IDP.", user.ID), + }) + return + } + member, err := api.Database.InsertOrganizationMember(ctx, database.InsertOrganizationMemberParams{ OrganizationID: organization.ID, UserID: user.ID, diff --git a/coderd/userauth.go b/coderd/userauth.go index 23066d26b4126..9c72af249cf3a 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -669,12 +669,11 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { }) cookies, user, key, err := api.oauthLogin(r, params) defer params.CommitAuditLogs() - var httpErr idpsync.HTTPError - if xerrors.As(err, &httpErr) { - httpErr.Write(rw, r) - return - } if err != nil { + if httpErr := idpsync.IsHTTPError(err); httpErr != nil { + httpErr.Write(rw, r) + return + } logger.Error(ctx, "oauth2: login failed", slog.F("user", user.Username), slog.Error(err)) httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Failed to process OAuth login.", @@ -1066,12 +1065,11 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { }) cookies, user, key, err := api.oauthLogin(r, params) defer params.CommitAuditLogs() - var httpErr idpsync.HTTPError - if xerrors.As(err, &httpErr) { - httpErr.Write(rw, r) - return - } if err != nil { + if hErr := idpsync.IsHTTPError(err); hErr != nil { + hErr.Write(rw, r) + return + } logger.Error(ctx, "oauth2: login failed", slog.F("user", user.Username), slog.Error(err)) httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Failed to process OAuth login.", diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 8603cfcfb439a..8e1f07e24d1b8 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -366,6 +366,7 @@ func TestUserOAuth2Github(t *testing.T) { require.Equal(t, "kyle", user.Username) require.Equal(t, "Kylium Carbonate", user.Name) require.Equal(t, "/hello-world", user.AvatarURL) + require.Equal(t, 1, len(user.OrganizationIDs), "in the default org") require.Len(t, auditor.AuditLogs(), numLogs) require.NotEqual(t, auditor.AuditLogs()[numLogs-1].UserID, uuid.Nil) @@ -419,6 +420,7 @@ func TestUserOAuth2Github(t *testing.T) { require.Equal(t, "kyle", user.Username) require.Equal(t, strings.Repeat("a", 128), user.Name) require.Equal(t, "/hello-world", user.AvatarURL) + require.Equal(t, 1, len(user.OrganizationIDs), "in the default org") require.Len(t, auditor.AuditLogs(), numLogs) require.NotEqual(t, auditor.AuditLogs()[numLogs-1].UserID, uuid.Nil) @@ -474,6 +476,7 @@ func TestUserOAuth2Github(t *testing.T) { require.Equal(t, "kyle", user.Username) require.Equal(t, "Kylium Carbonate", user.Name) require.Equal(t, "/hello-world", user.AvatarURL) + require.Equal(t, 1, len(user.OrganizationIDs), "in the default org") require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) require.Len(t, auditor.AuditLogs(), numLogs) @@ -536,6 +539,7 @@ func TestUserOAuth2Github(t *testing.T) { require.Equal(t, "mathias@coder.com", user.Email) require.Equal(t, "mathias", user.Username) require.Equal(t, "Mathias Mathias", user.Name) + require.Equal(t, 1, len(user.OrganizationIDs), "in the default org") require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) require.Len(t, auditor.AuditLogs(), numLogs) @@ -598,6 +602,7 @@ func TestUserOAuth2Github(t *testing.T) { require.Equal(t, "mathias@coder.com", user.Email) require.Equal(t, "mathias", user.Username) require.Equal(t, "Mathias Mathias", user.Name) + require.Equal(t, 1, len(user.OrganizationIDs), "in the default org") require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) require.Len(t, auditor.AuditLogs(), numLogs) @@ -1270,6 +1275,7 @@ func TestUserOIDC(t *testing.T) { require.Len(t, auditor.AuditLogs(), numLogs) require.NotEqual(t, uuid.Nil, auditor.AuditLogs()[numLogs-1].UserID) require.Equal(t, database.AuditActionRegister, auditor.AuditLogs()[numLogs-1].Action) + require.Equal(t, 1, len(user.OrganizationIDs), "in the default org") } }) } diff --git a/coderd/users.go b/coderd/users.go index 3ae2d916e1de8..38dcc1bd82f6e 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1294,10 +1294,6 @@ func (api *API) CreateUser(ctx context.Context, store database.Store, req Create var user database.User err := store.InTx(func(tx database.Store) error { orgRoles := make([]string, 0) - // Organization is required to know where to allocate the user. - if len(req.OrganizationIDs) == 0 { - return xerrors.Errorf("organization ID must be provided") - } params := database.InsertUserParams{ ID: uuid.New(), diff --git a/enterprise/coderd/enidpsync/enidpsync.go b/enterprise/coderd/enidpsync/enidpsync.go index 2e5dca4798bd6..f9365aaa242d6 100644 --- a/enterprise/coderd/enidpsync/enidpsync.go +++ b/enterprise/coderd/enidpsync/enidpsync.go @@ -7,12 +7,6 @@ import ( "github.com/coder/coder/v2/coderd/idpsync" ) -func init() { - idpsync.NewSync = func(logger slog.Logger, entitlements *entitlements.Set, settings idpsync.SyncSettings) idpsync.IDPSync { - return NewSync(logger, entitlements, settings) - } -} - type EnterpriseIDPSync struct { entitlements *entitlements.Set *idpsync.AGPLIDPSync @@ -21,6 +15,6 @@ type EnterpriseIDPSync struct { func NewSync(logger slog.Logger, set *entitlements.Set, settings idpsync.SyncSettings) *EnterpriseIDPSync { return &EnterpriseIDPSync{ entitlements: set, - AGPLIDPSync: idpsync.NewAGPLSync(logger.With(slog.F("enterprise_capable", "true")), set, settings), + AGPLIDPSync: idpsync.NewAGPLSync(logger.With(slog.F("enterprise_capable", "true")), settings), } } diff --git a/enterprise/coderd/enidpsync/organizations.go b/enterprise/coderd/enidpsync/organizations.go index a89246d877f05..2c7520fc412ee 100644 --- a/enterprise/coderd/enidpsync/organizations.go +++ b/enterprise/coderd/enidpsync/organizations.go @@ -14,8 +14,12 @@ import ( "github.com/coder/coder/v2/codersdk" ) +func (e EnterpriseIDPSync) OrganizationSyncEnabled() bool { + return e.entitlements.Enabled(codersdk.FeatureMultipleOrganizations) && e.OrganizationField != "" +} + func (e EnterpriseIDPSync) ParseOrganizationClaims(ctx context.Context, mergedClaims jwt.MapClaims) (idpsync.OrganizationParams, *idpsync.HTTPError) { - if !e.entitlements.Enabled(codersdk.FeatureMultipleOrganizations) { + if !e.OrganizationSyncEnabled() { // Default to agpl if multi-org is not enabled return e.AGPLIDPSync.ParseOrganizationClaims(ctx, mergedClaims) } diff --git a/enterprise/coderd/userauth_test.go b/enterprise/coderd/userauth_test.go index e5ca9c9180290..8e5318dcc402d 100644 --- a/enterprise/coderd/userauth_test.go +++ b/enterprise/coderd/userauth_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/golang-jwt/jwt/v4" + "github.com/google/uuid" "github.com/stretchr/testify/require" "golang.org/x/xerrors" @@ -14,7 +15,9 @@ import ( "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/coderdtest/oidctest" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/db2sdk" "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/util/slice" @@ -23,11 +26,208 @@ import ( "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" "github.com/coder/coder/v2/enterprise/coderd/license" "github.com/coder/coder/v2/testutil" + "github.com/coder/serpent" ) // nolint:bodyclose func TestUserOIDC(t *testing.T) { t.Parallel() + + t.Run("OrganizationSync", func(t *testing.T) { + t.Parallel() + + t.Run("SingleOrgDeployment", func(t *testing.T) { + t.Parallel() + + runner := setupOIDCTest(t, oidcTestConfig{ + Config: func(cfg *coderd.OIDCConfig) { + cfg.AllowSignups = true + cfg.UserRoleField = "roles" + }, + }) + + claims := jwt.MapClaims{ + "email": "alice@coder.com", + } + + // Login a new client that signs up + client, resp := runner.Login(t, claims) + require.Equal(t, http.StatusOK, resp.StatusCode) + runner.AssertOrganizations(t, "alice", true, nil) + + // Force a refresh, and assert nothing has changes + runner.ForceRefresh(t, client, claims) + runner.AssertOrganizations(t, "alice", true, nil) + }) + + t.Run("MultiOrgNoSync", func(t *testing.T) { + t.Parallel() + + runner := setupOIDCTest(t, oidcTestConfig{ + Config: func(cfg *coderd.OIDCConfig) { + cfg.AllowSignups = true + }, + }) + + ctx := testutil.Context(t, testutil.WaitMedium) + second, err := runner.AdminClient.CreateOrganization(ctx, codersdk.CreateOrganizationRequest{ + Name: "second", + DisplayName: "", + Description: "", + Icon: "", + }) + require.NoError(t, err) + + claims := jwt.MapClaims{ + "email": "alice@coder.com", + } + + // Login a new client that signs up + _, resp := runner.Login(t, claims) + require.Equal(t, http.StatusOK, resp.StatusCode) + runner.AssertOrganizations(t, "alice", true, nil) + + // Add alice to new org + _, err = runner.AdminClient.PostOrganizationMember(ctx, second.ID, "alice") + require.NoError(t, err) + + // Log in again to refresh the sync. The user should not be removed + // from the second organization. + runner.Login(t, claims) + runner.AssertOrganizations(t, "alice", true, []uuid.UUID{second.ID}) + }) + + t.Run("MultiOrgWithDefault", func(t *testing.T) { + t.Parallel() + + // Chicken and egg problem. Config is at startup, but orgs are + // created at runtime. We should add a runtime configuration of + // this. + second := uuid.New() + third := uuid.New() + + // Given: 4 organizations: default, second, third, and fourth + runner := setupOIDCTest(t, oidcTestConfig{ + Config: func(cfg *coderd.OIDCConfig) { + cfg.AllowSignups = true + }, + DeploymentValues: func(dv *codersdk.DeploymentValues) { + dv.OIDC.OrganizationAssignDefault = true + dv.OIDC.OrganizationField = "organization" + dv.OIDC.OrganizationMapping = serpent.Struct[map[string][]uuid.UUID]{ + Value: map[string][]uuid.UUID{ + "second": {second}, + "third": {third}, + }, + } + }, + }) + dbgen.Organization(t, runner.API.Database, database.Organization{ + ID: second, + }) + dbgen.Organization(t, runner.API.Database, database.Organization{ + ID: third, + }) + fourth := dbgen.Organization(t, runner.API.Database, database.Organization{}) + + ctx := testutil.Context(t, testutil.WaitMedium) + claims := jwt.MapClaims{ + "email": "alice@coder.com", + "organization": []string{"second", "third"}, + } + + // Then: a new user logs in with claims "second" and "third", they + // should belong to [default, second, third]. + userClient, resp := runner.Login(t, claims) + require.Equal(t, http.StatusOK, resp.StatusCode) + runner.AssertOrganizations(t, "alice", true, []uuid.UUID{second, third}) + user, err := userClient.User(ctx, codersdk.Me) + require.NoError(t, err) + + // When: they are manually added to the fourth organization, a new sync + // should remove them. + _, err = runner.AdminClient.PostOrganizationMember(ctx, fourth.ID, "alice") + require.ErrorContains(t, err, "Organization sync is enabled") + + runner.AssertOrganizations(t, "alice", true, []uuid.UUID{second, third}) + // Go around the block to add the user to see if they are removed. + dbgen.OrganizationMember(t, runner.API.Database, database.OrganizationMember{ + UserID: user.ID, + OrganizationID: fourth.ID, + }) + runner.AssertOrganizations(t, "alice", true, []uuid.UUID{second, third, fourth.ID}) + + // Then: Log in again will resync the orgs to their updated + // claims. + runner.Login(t, jwt.MapClaims{ + "email": "alice@coder.com", + "organization": []string{"third"}, + }) + runner.AssertOrganizations(t, "alice", true, []uuid.UUID{third}) + }) + + t.Run("MultiOrgWithoutDefault", func(t *testing.T) { + t.Parallel() + + second := uuid.New() + third := uuid.New() + + // Given: 4 organizations: default, second, third, and fourth + runner := setupOIDCTest(t, oidcTestConfig{ + Config: func(cfg *coderd.OIDCConfig) { + cfg.AllowSignups = true + }, + DeploymentValues: func(dv *codersdk.DeploymentValues) { + dv.OIDC.OrganizationAssignDefault = false + dv.OIDC.OrganizationField = "organization" + dv.OIDC.OrganizationMapping = serpent.Struct[map[string][]uuid.UUID]{ + Value: map[string][]uuid.UUID{ + "second": {second}, + "third": {third}, + }, + } + }, + }) + dbgen.Organization(t, runner.API.Database, database.Organization{ + ID: second, + }) + dbgen.Organization(t, runner.API.Database, database.Organization{ + ID: third, + }) + fourth := dbgen.Organization(t, runner.API.Database, database.Organization{}) + + ctx := testutil.Context(t, testutil.WaitMedium) + claims := jwt.MapClaims{ + "email": "alice@coder.com", + "organization": []string{"second", "third"}, + } + + // Then: a new user logs in with claims "second" and "third", they + // should belong to [ second, third]. + userClient, resp := runner.Login(t, claims) + require.Equal(t, http.StatusOK, resp.StatusCode) + runner.AssertOrganizations(t, "alice", false, []uuid.UUID{second, third}) + user, err := userClient.User(ctx, codersdk.Me) + require.NoError(t, err) + + // When: they are manually added to the fourth organization, a new sync + // should remove them. + dbgen.OrganizationMember(t, runner.API.Database, database.OrganizationMember{ + UserID: user.ID, + OrganizationID: fourth.ID, + }) + runner.AssertOrganizations(t, "alice", false, []uuid.UUID{second, third, fourth.ID}) + + // Then: Log in again will resync the orgs to their updated + // claims. + runner.Login(t, jwt.MapClaims{ + "email": "alice@coder.com", + "organization": []string{"third"}, + }) + runner.AssertOrganizations(t, "alice", false, []uuid.UUID{third}) + }) + }) + t.Run("RoleSync", func(t *testing.T) { t.Parallel() @@ -54,6 +254,8 @@ func TestUserOIDC(t *testing.T) { // Force a refresh, and assert nothing has changes runner.ForceRefresh(t, client, claims) runner.AssertRoles(t, "alice", []string{}) + + runner.AssertOrganizations(t, "alice", true, nil) }) // Some IDPs (ADFS) send the "string" type vs "[]string" if only @@ -81,6 +283,7 @@ func TestUserOIDC(t *testing.T) { }) require.Equal(t, http.StatusOK, resp.StatusCode) runner.AssertRoles(t, "alice", []string{rbac.RoleTemplateAdmin().String()}) + runner.AssertOrganizations(t, "alice", true, nil) }) // A user has some roles, then on an oauth refresh will lose said @@ -118,6 +321,7 @@ func TestUserOIDC(t *testing.T) { "roles": []string{"random"}, }) runner.AssertRoles(t, "alice", []string{}) + runner.AssertOrganizations(t, "alice", true, nil) }) // A user has some roles, then on another oauth login will lose said @@ -153,6 +357,7 @@ func TestUserOIDC(t *testing.T) { require.Equal(t, http.StatusOK, resp.StatusCode) runner.AssertRoles(t, "alice", []string{}) + runner.AssertOrganizations(t, "alice", true, nil) }) // All manual role updates should fail when role sync is enabled. @@ -214,6 +419,7 @@ func TestUserOIDC(t *testing.T) { }) require.Equal(t, http.StatusOK, resp.StatusCode) runner.AssertGroups(t, "alice", []string{groupName}) + runner.AssertOrganizations(t, "alice", true, nil) }) // Tests the group mapping feature. @@ -245,6 +451,7 @@ func TestUserOIDC(t *testing.T) { }) require.Equal(t, http.StatusOK, resp.StatusCode) runner.AssertGroups(t, "alice", []string{coderGroupName}) + runner.AssertOrganizations(t, "alice", true, nil) }) // User is in a group, then on an oauth refresh will lose said @@ -284,6 +491,7 @@ func TestUserOIDC(t *testing.T) { "email": "alice@coder.com", }) runner.AssertGroups(t, "alice", []string{}) + runner.AssertOrganizations(t, "alice", true, nil) }) t.Run("AddThenRemoveOnReAuth", func(t *testing.T) { @@ -318,6 +526,7 @@ func TestUserOIDC(t *testing.T) { }) require.Equal(t, http.StatusOK, resp.StatusCode) runner.AssertGroups(t, "alice", []string{}) + runner.AssertOrganizations(t, "alice", true, nil) }) // Updating groups where the claimed group does not exist. @@ -795,8 +1004,31 @@ type oidcTestConfig struct { Userinfo jwt.MapClaims // Config allows modifying the Coderd OIDC configuration. - Config func(cfg *coderd.OIDCConfig) - FakeOpts []oidctest.FakeIDPOpt + Config func(cfg *coderd.OIDCConfig) + DeploymentValues func(dv *codersdk.DeploymentValues) + FakeOpts []oidctest.FakeIDPOpt +} + +func (r *oidcTestRunner) AssertOrganizations(t *testing.T, userIdent string, includeDefault bool, expected []uuid.UUID) { + t.Helper() + + ctx := testutil.Context(t, testutil.WaitMedium) + userOrgs, err := r.AdminClient.OrganizationsByUser(ctx, userIdent) + require.NoError(t, err) + + cpy := make([]uuid.UUID, len(expected)) + copy(cpy, expected) + hasDefault := false + userOrgIDs := db2sdk.List(userOrgs, func(o codersdk.Organization) uuid.UUID { + if o.IsDefault { + hasDefault = true + cpy = append(cpy, o.ID) + } + return o.ID + }) + + require.Equal(t, includeDefault, hasDefault, "expected default org") + require.ElementsMatch(t, cpy, userOrgIDs, "expected orgs") } func (r *oidcTestRunner) AssertRoles(t *testing.T, userIdent string, roles []string) { @@ -856,14 +1088,21 @@ func setupOIDCTest(t *testing.T, settings oidcTestConfig) *oidcTestRunner { ctx := testutil.Context(t, testutil.WaitMedium) cfg := fake.OIDCConfig(t, nil, settings.Config) + dv := coderdtest.DeploymentValues(t) + if settings.DeploymentValues != nil { + settings.DeploymentValues(dv) + } + dv.Experiments = []string{string(codersdk.ExperimentMultiOrganization)} owner, _, api, _ := coderdenttest.NewWithAPI(t, &coderdenttest.Options{ Options: &coderdtest.Options{ - OIDCConfig: cfg, + OIDCConfig: cfg, + DeploymentValues: dv, }, LicenseOptions: &coderdenttest.LicenseOptions{ Features: license.Features{ - codersdk.FeatureUserRoleManagement: 1, - codersdk.FeatureTemplateRBAC: 1, + codersdk.FeatureUserRoleManagement: 1, + codersdk.FeatureTemplateRBAC: 1, + codersdk.FeatureMultipleOrganizations: 1, }, }, }) From e37d476a6018cd0dc5ddf4ca6a6ce0cca31721ff Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 28 Aug 2024 11:40:52 -0500 Subject: [PATCH 19/23] linting --- coderd/idpsync/idpsync_test.go | 2 ++ coderd/idpsync/organization.go | 2 +- enterprise/coderd/enidpsync/organizations_test.go | 3 ++- enterprise/coderd/userauth_test.go | 4 ++-- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/coderd/idpsync/idpsync_test.go b/coderd/idpsync/idpsync_test.go index afb2d4060c51c..7dc29d903af3f 100644 --- a/coderd/idpsync/idpsync_test.go +++ b/coderd/idpsync/idpsync_test.go @@ -137,6 +137,8 @@ func TestParseStringSliceClaim(t *testing.T) { } func TestIsHTTPError(t *testing.T) { + t.Parallel() + herr := idpsync.HTTPError{} require.NotNil(t, idpsync.IsHTTPError(herr)) require.NotNil(t, idpsync.IsHTTPError(&herr)) diff --git a/coderd/idpsync/organization.go b/coderd/idpsync/organization.go index 23cf8d35f84f5..40cf9aebdaaeb 100644 --- a/coderd/idpsync/organization.go +++ b/coderd/idpsync/organization.go @@ -16,7 +16,7 @@ import ( "github.com/coder/coder/v2/coderd/util/slice" ) -func (s AGPLIDPSync) OrganizationSyncEnabled() bool { +func (AGPLIDPSync) OrganizationSyncEnabled() bool { // AGPL does not support syncing organizations. return false } diff --git a/enterprise/coderd/enidpsync/organizations_test.go b/enterprise/coderd/enidpsync/organizations_test.go index bb2801aa39f80..0b2ed1ef6521f 100644 --- a/enterprise/coderd/enidpsync/organizations_test.go +++ b/enterprise/coderd/enidpsync/organizations_test.go @@ -20,6 +20,7 @@ import ( "github.com/coder/coder/v2/coderd/idpsync" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/enterprise/coderd/enidpsync" "github.com/coder/coder/v2/testutil" ) @@ -235,7 +236,7 @@ func TestOrganizationSync(t *testing.T) { } // Create a new sync object - sync := idpsync.NewSync(logger, caseData.Entitlements, caseData.Settings) + sync := enidpsync.NewSync(logger, caseData.Entitlements, caseData.Settings) for _, exp := range caseData.Exps { t.Run(exp.Name, func(t *testing.T) { params, httpErr := sync.ParseOrganizationClaims(ctx, exp.Claims) diff --git a/enterprise/coderd/userauth_test.go b/enterprise/coderd/userauth_test.go index 8e5318dcc402d..3e94a25a1c013 100644 --- a/enterprise/coderd/userauth_test.go +++ b/enterprise/coderd/userauth_test.go @@ -1016,8 +1016,8 @@ func (r *oidcTestRunner) AssertOrganizations(t *testing.T, userIdent string, inc userOrgs, err := r.AdminClient.OrganizationsByUser(ctx, userIdent) require.NoError(t, err) - cpy := make([]uuid.UUID, len(expected)) - copy(cpy, expected) + cpy := make([]uuid.UUID, 0, len(expected)) + cpy = append(cpy, expected...) hasDefault := false userOrgIDs := db2sdk.List(userOrgs, func(o codersdk.Organization) uuid.UUID { if o.IsDefault { From a8647ce57c387596490779309efaf3d57a15b912 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 28 Aug 2024 11:48:02 -0500 Subject: [PATCH 20/23] make gen --- coderd/apidoc/docs.go | 9 +++++ coderd/apidoc/swagger.json | 9 +++++ docs/reference/api/general.md | 3 ++ docs/reference/api/schemas.md | 68 ++++++++++++++++++++-------------- docs/reference/cli/server.md | 32 ++++++++++++++++ site/src/api/typesGenerated.ts | 3 ++ 6 files changed, 96 insertions(+), 28 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 08447abb22c50..23f465ce27d69 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -11077,6 +11077,15 @@ const docTemplate = `{ "name_field": { "type": "string" }, + "organization_assign_default": { + "type": "boolean" + }, + "organization_field": { + "type": "string" + }, + "organization_mapping": { + "type": "object" + }, "scopes": { "type": "array", "items": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 11126e672609a..6cd8d84a38eb7 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -9972,6 +9972,15 @@ "name_field": { "type": "string" }, + "organization_assign_default": { + "type": "boolean" + }, + "organization_field": { + "type": "string" + }, + "organization_mapping": { + "type": "object" + }, "scopes": { "type": "array", "items": { diff --git a/docs/reference/api/general.md b/docs/reference/api/general.md index cf85bbf672057..c70c6e006f6d2 100644 --- a/docs/reference/api/general.md +++ b/docs/reference/api/general.md @@ -345,6 +345,9 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \ "ignore_user_info": true, "issuer_url": "string", "name_field": "string", + "organization_assign_default": true, + "organization_field": "string", + "organization_mapping": {}, "scopes": ["string"], "sign_in_text": "string", "signups_disabled_text": "string", diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index ee229a8cc4914..1c2898394a9be 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -1849,6 +1849,9 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "ignore_user_info": true, "issuer_url": "string", "name_field": "string", + "organization_assign_default": true, + "organization_field": "string", + "organization_mapping": {}, "scopes": ["string"], "sign_in_text": "string", "signups_disabled_text": "string", @@ -2272,6 +2275,9 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "ignore_user_info": true, "issuer_url": "string", "name_field": "string", + "organization_assign_default": true, + "organization_field": "string", + "organization_mapping": {}, "scopes": ["string"], "sign_in_text": "string", "signups_disabled_text": "string", @@ -3639,6 +3645,9 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "ignore_user_info": true, "issuer_url": "string", "name_field": "string", + "organization_assign_default": true, + "organization_field": "string", + "organization_mapping": {}, "scopes": ["string"], "sign_in_text": "string", "signups_disabled_text": "string", @@ -3652,34 +3661,37 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o ### Properties -| Name | Type | Required | Restrictions | Description | -| ----------------------- | -------------------------------- | -------- | ------------ | -------------------------------------------------------------------------------- | -| `allow_signups` | boolean | false | | | -| `auth_url_params` | object | false | | | -| `client_cert_file` | string | false | | | -| `client_id` | string | false | | | -| `client_key_file` | string | false | | Client key file & ClientCertFile are used in place of ClientSecret for PKI auth. | -| `client_secret` | string | false | | | -| `email_domain` | array of string | false | | | -| `email_field` | string | false | | | -| `group_allow_list` | array of string | false | | | -| `group_auto_create` | boolean | false | | | -| `group_mapping` | object | false | | | -| `group_regex_filter` | [serpent.Regexp](#serpentregexp) | false | | | -| `groups_field` | string | false | | | -| `icon_url` | [serpent.URL](#serpenturl) | false | | | -| `ignore_email_verified` | boolean | false | | | -| `ignore_user_info` | boolean | false | | | -| `issuer_url` | string | false | | | -| `name_field` | string | false | | | -| `scopes` | array of string | false | | | -| `sign_in_text` | string | false | | | -| `signups_disabled_text` | string | false | | | -| `skip_issuer_checks` | boolean | false | | | -| `user_role_field` | string | false | | | -| `user_role_mapping` | object | false | | | -| `user_roles_default` | array of string | false | | | -| `username_field` | string | false | | | +| Name | Type | Required | Restrictions | Description | +| ----------------------------- | -------------------------------- | -------- | ------------ | -------------------------------------------------------------------------------- | +| `allow_signups` | boolean | false | | | +| `auth_url_params` | object | false | | | +| `client_cert_file` | string | false | | | +| `client_id` | string | false | | | +| `client_key_file` | string | false | | Client key file & ClientCertFile are used in place of ClientSecret for PKI auth. | +| `client_secret` | string | false | | | +| `email_domain` | array of string | false | | | +| `email_field` | string | false | | | +| `group_allow_list` | array of string | false | | | +| `group_auto_create` | boolean | false | | | +| `group_mapping` | object | false | | | +| `group_regex_filter` | [serpent.Regexp](#serpentregexp) | false | | | +| `groups_field` | string | false | | | +| `icon_url` | [serpent.URL](#serpenturl) | false | | | +| `ignore_email_verified` | boolean | false | | | +| `ignore_user_info` | boolean | false | | | +| `issuer_url` | string | false | | | +| `name_field` | string | false | | | +| `organization_assign_default` | boolean | false | | | +| `organization_field` | string | false | | | +| `organization_mapping` | object | false | | | +| `scopes` | array of string | false | | | +| `sign_in_text` | string | false | | | +| `signups_disabled_text` | string | false | | | +| `skip_issuer_checks` | boolean | false | | | +| `user_role_field` | string | false | | | +| `user_role_mapping` | object | false | | | +| `user_roles_default` | array of string | false | | | +| `username_field` | string | false | | | ## codersdk.Organization diff --git a/docs/reference/cli/server.md b/docs/reference/cli/server.md index 1ecdd326f1b17..2291a8b7b4915 100644 --- a/docs/reference/cli/server.md +++ b/docs/reference/cli/server.md @@ -558,6 +558,38 @@ OIDC auth URL parameters to pass to the upstream provider. Ignore the userinfo endpoint and only use the ID token for user information. +### --oidc-organization-field + +| | | +| ----------- | ------------------------------------------- | +| Type | string | +| Environment | $CODER_OIDC_ORGANIZATION_FIELD | +| YAML | oidc.organizationField | + +This field must be set if using the organization sync feature. Set to the claim to be used for organizations. + +### --oidc-organization-assign-default + +| | | +| ----------- | ---------------------------------------------------- | +| Type | bool | +| Environment | $CODER_OIDC_ORGANIZATION_ASSIGN_DEFAULT | +| YAML | oidc.organizationAssignDefault | +| Default | true | + +If set to true, users will always be added to the default organization. If organization sync is enabled, then the default org is always added to the user's set of expectedorganizations. + +### --oidc-organization-mapping + +| | | +| ----------- | --------------------------------------------- | +| Type | struct[map[string][]uuid.UUID] | +| Environment | $CODER_OIDC_ORGANIZATION_MAPPING | +| YAML | oidc.organizationMapping | +| Default | {} | + +A map of OIDC claims and the organizations in Coder it should map to. This is required because organization IDs must be used within Coder. + ### --oidc-group-field | | | diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 262e58d77c76c..1bdf3578fb077 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -879,6 +879,9 @@ export interface OIDCConfig { readonly email_field: string; readonly auth_url_params: Record; readonly ignore_user_info: boolean; + readonly organization_field: string; + readonly organization_mapping: Record>>; + readonly organization_assign_default: boolean; readonly group_auto_create: boolean; readonly group_regex_filter: string; readonly group_allow_list: string[]; From 02812e471eaac46eb37f566f3834713aac9e47ba Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 28 Aug 2024 11:59:48 -0500 Subject: [PATCH 21/23] update golden files --- cli/testdata/coder_server_--help.golden | 13 +++++++++++++ cli/testdata/server-config.yaml.golden | 13 +++++++++++++ enterprise/cli/testdata/coder_server_--help.golden | 13 +++++++++++++ 3 files changed, 39 insertions(+) diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden index 54e9eeffc1248..d7bc620b32517 100644 --- a/cli/testdata/coder_server_--help.golden +++ b/cli/testdata/coder_server_--help.golden @@ -433,6 +433,11 @@ OIDC OPTIONS: groups. This filter is applied after the group mapping and before the regex filter. + --oidc-organization-assign-default bool, $CODER_OIDC_ORGANIZATION_ASSIGN_DEFAULT (default: true) + If set to true, users will always be added to the default + organization. If organization sync is enabled, then the default org is + always added to the user's set of expectedorganizations. + --oidc-auth-url-params struct[map[string]string], $CODER_OIDC_AUTH_URL_PARAMS (default: {"access_type": "offline"}) OIDC auth URL parameters to pass to the upstream provider. @@ -479,6 +484,14 @@ OIDC OPTIONS: --oidc-name-field string, $CODER_OIDC_NAME_FIELD (default: name) OIDC claim field to use as the name. + --oidc-organization-field string, $CODER_OIDC_ORGANIZATION_FIELD + This field must be set if using the organization sync feature. Set to + the claim to be used for organizations. + + --oidc-organization-mapping struct[map[string][]uuid.UUID], $CODER_OIDC_ORGANIZATION_MAPPING (default: {}) + A map of OIDC claims and the organizations in Coder it should map to. + This is required because organization IDs must be used within Coder. + --oidc-group-regex-filter regexp, $CODER_OIDC_GROUP_REGEX_FILTER (default: .*) If provided any group name not matching the regex is ignored. This allows for filtering out groups that are not needed. This filter is diff --git a/cli/testdata/server-config.yaml.golden b/cli/testdata/server-config.yaml.golden index b050b662c56fa..21308e00cca60 100644 --- a/cli/testdata/server-config.yaml.golden +++ b/cli/testdata/server-config.yaml.golden @@ -319,6 +319,19 @@ oidc: # Ignore the userinfo endpoint and only use the ID token for user information. # (default: false, type: bool) ignoreUserInfo: false + # This field must be set if using the organization sync feature. Set to the claim + # to be used for organizations. + # (default: , type: string) + organizationField: "" + # If set to true, users will always be added to the default organization. If + # organization sync is enabled, then the default org is always added to the user's + # set of expectedorganizations. + # (default: true, type: bool) + organizationAssignDefault: true + # A map of OIDC claims and the organizations in Coder it should map to. This is + # required because organization IDs must be used within Coder. + # (default: {}, type: struct[map[string][]uuid.UUID]) + organizationMapping: {} # This field must be set if using the group sync feature and the scope name is not # 'groups'. Set to the claim to be used for groups. # (default: , type: string) diff --git a/enterprise/cli/testdata/coder_server_--help.golden b/enterprise/cli/testdata/coder_server_--help.golden index 0337e239d54a4..58e7ee2400eb1 100644 --- a/enterprise/cli/testdata/coder_server_--help.golden +++ b/enterprise/cli/testdata/coder_server_--help.golden @@ -434,6 +434,11 @@ OIDC OPTIONS: groups. This filter is applied after the group mapping and before the regex filter. + --oidc-organization-assign-default bool, $CODER_OIDC_ORGANIZATION_ASSIGN_DEFAULT (default: true) + If set to true, users will always be added to the default + organization. If organization sync is enabled, then the default org is + always added to the user's set of expectedorganizations. + --oidc-auth-url-params struct[map[string]string], $CODER_OIDC_AUTH_URL_PARAMS (default: {"access_type": "offline"}) OIDC auth URL parameters to pass to the upstream provider. @@ -480,6 +485,14 @@ OIDC OPTIONS: --oidc-name-field string, $CODER_OIDC_NAME_FIELD (default: name) OIDC claim field to use as the name. + --oidc-organization-field string, $CODER_OIDC_ORGANIZATION_FIELD + This field must be set if using the organization sync feature. Set to + the claim to be used for organizations. + + --oidc-organization-mapping struct[map[string][]uuid.UUID], $CODER_OIDC_ORGANIZATION_MAPPING (default: {}) + A map of OIDC claims and the organizations in Coder it should map to. + This is required because organization IDs must be used within Coder. + --oidc-group-regex-filter regexp, $CODER_OIDC_GROUP_REGEX_FILTER (default: .*) If provided any group name not matching the regex is ignored. This allows for filtering out groups that are not needed. This filter is From e18bc8f43f20512159fa336d1260fa3a23d4d920 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 28 Aug 2024 14:42:09 -0500 Subject: [PATCH 22/23] PR comments --- coderd/coderd.go | 2 -- coderd/idpsync/idpsync.go | 12 ++++++++++++ coderd/idpsync/organization.go | 11 ----------- enterprise/coderd/enidpsync/enidpsync.go | 5 +++++ 4 files changed, 17 insertions(+), 13 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 6998091e8847f..20ce616eab5ba 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -275,8 +275,6 @@ func New(options *Options) *API { options.Entitlements = entitlements.New() } if options.IDPSync == nil { - // If this is set in the options, it is probably the enterprise - // version of the code. options.IDPSync = idpsync.NewAGPLSync(options.Logger, idpsync.SyncSettings{ OrganizationField: options.DeploymentValues.OIDC.OrganizationField.Value(), OrganizationMapping: options.DeploymentValues.OIDC.OrganizationMapping.Value, diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go index da88851c83a93..0323997755461 100644 --- a/coderd/idpsync/idpsync.go +++ b/coderd/idpsync/idpsync.go @@ -52,6 +52,18 @@ type SyncSettings struct { OrganizationAssignDefault bool } +type OrganizationParams struct { + // SyncEnabled if false will skip syncing the user's organizations. + SyncEnabled bool + // IncludeDefault is primarily for single org deployments. It will ensure + // a user is always inserted into the default org. + IncludeDefault bool + // Organizations is the list of organizations the user should be a member of + // assuming syncing is turned on. + Organizations []uuid.UUID +} + + func NewAGPLSync(logger slog.Logger, settings SyncSettings) *AGPLIDPSync { return &AGPLIDPSync{ Logger: logger.Named("idp-sync"), diff --git a/coderd/idpsync/organization.go b/coderd/idpsync/organization.go index 40cf9aebdaaeb..6d475f28ea0ef 100644 --- a/coderd/idpsync/organization.go +++ b/coderd/idpsync/organization.go @@ -30,17 +30,6 @@ func (s AGPLIDPSync) ParseOrganizationClaims(_ context.Context, _ jwt.MapClaims) }, nil } -type OrganizationParams struct { - // SyncEnabled if false will skip syncing the user's organizations. - SyncEnabled bool - // IncludeDefault is primarily for single org deployments. It will ensure - // a user is always inserted into the default org. - IncludeDefault bool - // Organizations is the list of organizations the user should be a member of - // assuming syncing is turned on. - Organizations []uuid.UUID -} - // SyncOrganizations if enabled will ensure the user is a member of the provided // organizations. It will add and remove their membership to match the expected set. func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, user database.User, params OrganizationParams) error { diff --git a/enterprise/coderd/enidpsync/enidpsync.go b/enterprise/coderd/enidpsync/enidpsync.go index f9365aaa242d6..bb21c68501e1b 100644 --- a/enterprise/coderd/enidpsync/enidpsync.go +++ b/enterprise/coderd/enidpsync/enidpsync.go @@ -7,6 +7,11 @@ import ( "github.com/coder/coder/v2/coderd/idpsync" ) +// EnterpriseIDPSync enabled syncing user information from an external IDP. +// The sync is an enterprise feature, so this struct wraps the AGPL implementation +// and extends it with enterprise capabilities. These capabilities can entirely +// be changed in the Parsing, and leaving the "syncing" part (which holds the +// more complex logic) to the shared AGPL implementation. type EnterpriseIDPSync struct { entitlements *entitlements.Set *idpsync.AGPLIDPSync From 35160086df2075f2516b97d7c60cde11993d2271 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 29 Aug 2024 09:06:57 -0500 Subject: [PATCH 23/23] fmt --- coderd/idpsync/idpsync.go | 1 - 1 file changed, 1 deletion(-) diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go index 0323997755461..73a7b9b6f530d 100644 --- a/coderd/idpsync/idpsync.go +++ b/coderd/idpsync/idpsync.go @@ -63,7 +63,6 @@ type OrganizationParams struct { Organizations []uuid.UUID } - func NewAGPLSync(logger slog.Logger, settings SyncSettings) *AGPLIDPSync { return &AGPLIDPSync{ Logger: logger.Named("idp-sync"),