ended up with a global factory funciton

coder · Emyrk · Aug 30, 2024 · Aug 28, 2024 · Aug 26, 2024 · Aug 26, 2024
commit 9c12b18ab37de207b26dd4ee156bee69be2d5356
diff --git 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
@@ -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
@@ -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
@@ -13,19 +13,18 @@ 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
 	ctx = dbauthz.AsSystemRestricted(ctx)
 	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
 }