From 3bebbe040906692a52845949318c12d944275991 Mon Sep 17 00:00:00 2001 From: Bethuel Date: Mon, 29 May 2023 14:48:19 +0300 Subject: [PATCH] Refactor IdP Config Structure (#879) --- management/cmd/management.go | 6 --- management/server/idp/auth0.go | 21 ++++---- management/server/idp/auth0_test.go | 2 +- management/server/idp/azure.go | 32 +++++++----- management/server/idp/idp.go | 67 +++++++++++++++++++++++--- management/server/idp/keycloak.go | 18 ++++--- management/server/idp/keycloak_test.go | 24 ++++++++- management/server/idp/zitadel.go | 25 ++++++---- management/server/idp/zitadel_test.go | 2 +- 9 files changed, 141 insertions(+), 56 deletions(-) diff --git a/management/cmd/management.go b/management/cmd/management.go index 2efef7093ca..eee8c5c5712 100644 --- a/management/cmd/management.go +++ b/management/cmd/management.go @@ -394,12 +394,6 @@ func loadMgmtConfig(mgmtConfigPath string) (*server.Config, error) { } log.Infof("loaded OIDC configuration from the provided IDP configuration endpoint: %s", oidcEndpoint) - log.Infof("configuring IdpManagerConfig.OIDCConfig.Issuer with a new value %s,", oidcConfig.Issuer) - config.IdpManagerConfig.OIDCConfig.Issuer = strings.TrimRight(oidcConfig.Issuer, "/") - - log.Infof("configuring IdpManagerConfig.OIDCConfig.TokenEndpoint with a new value %s,", oidcConfig.TokenEndpoint) - config.IdpManagerConfig.OIDCConfig.TokenEndpoint = oidcConfig.TokenEndpoint - log.Infof("overriding HttpConfig.AuthIssuer with a new value %s, previously configured value: %s", oidcConfig.Issuer, config.HttpConfig.AuthIssuer) config.HttpConfig.AuthIssuer = oidcConfig.Issuer diff --git a/management/server/idp/auth0.go b/management/server/idp/auth0.go index fba5ad55acf..f568f51de51 100644 --- a/management/server/idp/auth0.go +++ b/management/server/idp/auth0.go @@ -32,10 +32,10 @@ type Auth0Manager struct { // Auth0ClientConfig auth0 manager client configurations type Auth0ClientConfig struct { Audience string - AuthIssuer string `json:"-"` + AuthIssuer string ClientID string ClientSecret string - GrantType string `json:"-"` + GrantType string } // auth0JWTRequest payload struct to request a JWT Token @@ -110,9 +110,7 @@ type auth0Profile struct { } // NewAuth0Manager creates a new instance of the Auth0Manager -func NewAuth0Manager(oidcConfig OIDCConfig, config Auth0ClientConfig, - appMetrics telemetry.AppMetrics) (*Auth0Manager, error) { - +func NewAuth0Manager(config Auth0ClientConfig, appMetrics telemetry.AppMetrics) (*Auth0Manager, error) { httpTransport := http.DefaultTransport.(*http.Transport).Clone() httpTransport.MaxIdleConns = 5 @@ -120,13 +118,14 @@ func NewAuth0Manager(oidcConfig OIDCConfig, config Auth0ClientConfig, Timeout: 10 * time.Second, Transport: httpTransport, } - helper := JsonParser{} - config.AuthIssuer = oidcConfig.Issuer - config.GrantType = "client_credentials" + + if config.AuthIssuer == "" { + return nil, fmt.Errorf("auth0 IdP configuration is incomplete, AuthIssuer is missing") + } if config.ClientID == "" { - return nil, fmt.Errorf("auth0 IdP configuration is incomplete, clientID is missing") + return nil, fmt.Errorf("auth0 IdP configuration is incomplete, ClientID is missing") } if config.ClientSecret == "" { @@ -137,6 +136,10 @@ func NewAuth0Manager(oidcConfig OIDCConfig, config Auth0ClientConfig, return nil, fmt.Errorf("auth0 IdP configuration is incomplete, Audience is missing") } + if config.GrantType == "" { + return nil, fmt.Errorf("auth0 IdP configuration is incomplete, GrantType is missing") + } + credentials := &Auth0Credentials{ clientConfig: config, httpClient: httpClient, diff --git a/management/server/idp/auth0_test.go b/management/server/idp/auth0_test.go index b4a5cbf20a0..fecee936ba2 100644 --- a/management/server/idp/auth0_test.go +++ b/management/server/idp/auth0_test.go @@ -461,7 +461,7 @@ func TestNewAuth0Manager(t *testing.T) { for _, testCase := range []test{testCase1, testCase2} { t.Run(testCase.name, func(t *testing.T) { - _, err := NewAuth0Manager(OIDCConfig{}, testCase.inputConfig, &telemetry.MockAppMetrics{}) + _, err := NewAuth0Manager(testCase.inputConfig, &telemetry.MockAppMetrics{}) testCase.assertErrFunc(t, err, testCase.assertErrFuncMessage) }) } diff --git a/management/server/idp/azure.go b/management/server/idp/azure.go index 3b9d2ff3811..56d2ca4e8a6 100644 --- a/management/server/idp/azure.go +++ b/management/server/idp/azure.go @@ -37,13 +37,12 @@ type AzureManager struct { // AzureClientConfig azure manager client configurations. type AzureClientConfig struct { - ClientID string - ClientSecret string - ObjectID string - - GraphAPIEndpoint string `json:"-"` - TokenEndpoint string `json:"-"` - GrantType string `json:"-"` + ClientID string + ClientSecret string + ObjectID string + GraphAPIEndpoint string + TokenEndpoint string + GrantType string } // AzureCredentials azure authentication information. @@ -75,8 +74,7 @@ type azureExtension struct { } // NewAzureManager creates a new instance of the AzureManager. -func NewAzureManager(oidcConfig OIDCConfig, config AzureClientConfig, - appMetrics telemetry.AppMetrics) (*AzureManager, error) { +func NewAzureManager(config AzureClientConfig, appMetrics telemetry.AppMetrics) (*AzureManager, error) { httpTransport := http.DefaultTransport.(*http.Transport).Clone() httpTransport.MaxIdleConns = 5 @@ -84,11 +82,7 @@ func NewAzureManager(oidcConfig OIDCConfig, config AzureClientConfig, Timeout: 10 * time.Second, Transport: httpTransport, } - helper := JsonParser{} - config.TokenEndpoint = oidcConfig.TokenEndpoint - config.GraphAPIEndpoint = "https://graph.microsoft.com" - config.GrantType = "client_credentials" if config.ClientID == "" { return nil, fmt.Errorf("azure IdP configuration is incomplete, clientID is missing") @@ -98,10 +92,22 @@ func NewAzureManager(oidcConfig OIDCConfig, config AzureClientConfig, return nil, fmt.Errorf("azure IdP configuration is incomplete, ClientSecret is missing") } + if config.TokenEndpoint == "" { + return nil, fmt.Errorf("azure IdP configuration is incomplete, TokenEndpoint is missing") + } + + if config.GraphAPIEndpoint == "" { + return nil, fmt.Errorf("azure IdP configuration is incomplete, GraphAPIEndpoint is missing") + } + if config.ObjectID == "" { return nil, fmt.Errorf("azure IdP configuration is incomplete, ObjectID is missing") } + if config.GrantType == "" { + return nil, fmt.Errorf("azure IdP configuration is incomplete, GrantType is missing") + } + credentials := &AzureCredentials{ clientConfig: config, httpClient: httpClient, diff --git a/management/server/idp/idp.go b/management/server/idp/idp.go index 876680e27d3..489ea3ae9cd 100644 --- a/management/server/idp/idp.go +++ b/management/server/idp/idp.go @@ -19,17 +19,23 @@ type Manager interface { GetUserByEmail(email string) ([]*UserData, error) } -// OIDCConfig specifies configuration for OpenID Connect provider -// These configurations are automatically loaded from the OIDC endpoint -type OIDCConfig struct { +// ClientConfig defines common client configuration for all IdP manager +type ClientConfig struct { Issuer string TokenEndpoint string + ClientID string + ClientSecret string + GrantType string } +// ExtraConfig stores IdP specific config that are unique to individual IdPs +type ExtraConfig map[string]string + // Config an idp configuration struct to be loaded from management server's config file type Config struct { ManagerType string - OIDCConfig OIDCConfig `json:"-"` + ClientConfig *ClientConfig + ExtraConfig ExtraConfig Auth0ClientCredentials Auth0ClientConfig AzureClientCredentials AzureClientConfig KeycloakClientCredentials KeycloakClientConfig @@ -82,13 +88,58 @@ func NewManager(config Config, appMetrics telemetry.AppMetrics) (Manager, error) case "none", "": return nil, nil case "auth0": - return NewAuth0Manager(config.OIDCConfig, config.Auth0ClientCredentials, appMetrics) + auth0ClientConfig := config.Auth0ClientCredentials + if config.ClientConfig != nil { + auth0ClientConfig = Auth0ClientConfig{ + Audience: config.ExtraConfig["Audience"], + AuthIssuer: config.ClientConfig.Issuer, + ClientID: config.ClientConfig.ClientID, + ClientSecret: config.ClientConfig.ClientSecret, + GrantType: config.ClientConfig.GrantType, + } + } + + return NewAuth0Manager(auth0ClientConfig, appMetrics) case "azure": - return NewAzureManager(config.OIDCConfig, config.AzureClientCredentials, appMetrics) + azureClientConfig := config.AzureClientCredentials + if config.ClientConfig != nil { + azureClientConfig = AzureClientConfig{ + ClientID: config.ClientConfig.ClientID, + ClientSecret: config.ClientConfig.ClientSecret, + GrantType: config.ClientConfig.GrantType, + TokenEndpoint: config.ClientConfig.TokenEndpoint, + ObjectID: config.ExtraConfig["ObjectID"], + GraphAPIEndpoint: config.ExtraConfig["GraphAPIEndpoint"], + } + } + + return NewAzureManager(azureClientConfig, appMetrics) case "keycloak": - return NewKeycloakManager(config.OIDCConfig, config.KeycloakClientCredentials, appMetrics) + keycloakClientConfig := config.KeycloakClientCredentials + if config.ClientConfig != nil { + keycloakClientConfig = KeycloakClientConfig{ + ClientID: config.ClientConfig.ClientID, + ClientSecret: config.ClientConfig.ClientSecret, + GrantType: config.ClientConfig.GrantType, + TokenEndpoint: config.ClientConfig.TokenEndpoint, + AdminEndpoint: config.ExtraConfig["AdminEndpoint"], + } + } + + return NewKeycloakManager(keycloakClientConfig, appMetrics) case "zitadel": - return NewZitadelManager(config.OIDCConfig, config.ZitadelClientCredentials, appMetrics) + zitadelClientConfig := config.ZitadelClientCredentials + if config.ClientConfig != nil { + zitadelClientConfig = ZitadelClientConfig{ + ClientID: config.ClientConfig.ClientID, + ClientSecret: config.ClientConfig.ClientSecret, + GrantType: config.ClientConfig.GrantType, + TokenEndpoint: config.ClientConfig.TokenEndpoint, + ManagementEndpoint: config.ExtraConfig["ManagementEndpoint"], + } + } + + return NewZitadelManager(zitadelClientConfig, appMetrics) default: return nil, fmt.Errorf("invalid manager type: %s", config.ManagerType) } diff --git a/management/server/idp/keycloak.go b/management/server/idp/keycloak.go index 8d35640bec2..90dbf94ff47 100644 --- a/management/server/idp/keycloak.go +++ b/management/server/idp/keycloak.go @@ -37,8 +37,8 @@ type KeycloakClientConfig struct { ClientID string ClientSecret string AdminEndpoint string - TokenEndpoint string `json:"-"` - GrantType string `json:"-"` + TokenEndpoint string + GrantType string } // KeycloakCredentials keycloak authentication information. @@ -82,8 +82,7 @@ type keycloakProfile struct { } // NewKeycloakManager creates a new instance of the KeycloakManager. -func NewKeycloakManager(oidcConfig OIDCConfig, config KeycloakClientConfig, - appMetrics telemetry.AppMetrics) (*KeycloakManager, error) { +func NewKeycloakManager(config KeycloakClientConfig, appMetrics telemetry.AppMetrics) (*KeycloakManager, error) { httpTransport := http.DefaultTransport.(*http.Transport).Clone() httpTransport.MaxIdleConns = 5 @@ -91,10 +90,7 @@ func NewKeycloakManager(oidcConfig OIDCConfig, config KeycloakClientConfig, Timeout: 10 * time.Second, Transport: httpTransport, } - helper := JsonParser{} - config.TokenEndpoint = oidcConfig.TokenEndpoint - config.GrantType = "client_credentials" if config.ClientID == "" { return nil, fmt.Errorf("keycloak IdP configuration is incomplete, clientID is missing") @@ -104,10 +100,18 @@ func NewKeycloakManager(oidcConfig OIDCConfig, config KeycloakClientConfig, return nil, fmt.Errorf("keycloak IdP configuration is incomplete, ClientSecret is missing") } + if config.TokenEndpoint == "" { + return nil, fmt.Errorf("keycloak IdP configuration is incomplete, TokenEndpoint is missing") + } + if config.AdminEndpoint == "" { return nil, fmt.Errorf("keycloak IdP configuration is incomplete, AdminEndpoint is missing") } + if config.GrantType == "" { + return nil, fmt.Errorf("keycloak IdP configuration is incomplete, GrantType is missing") + } + credentials := &KeycloakCredentials{ clientConfig: config, httpClient: httpClient, diff --git a/management/server/idp/keycloak_test.go b/management/server/idp/keycloak_test.go index 589dc4cd973..115306d7db4 100644 --- a/management/server/idp/keycloak_test.go +++ b/management/server/idp/keycloak_test.go @@ -56,9 +56,29 @@ func TestNewKeycloakManager(t *testing.T) { assertErrFuncMessage: "should return error when field empty", } - for _, testCase := range []test{testCase1, testCase2, testCase3} { + testCase4Config := defaultTestConfig + testCase4Config.TokenEndpoint = "" + + testCase4 := test{ + name: "Missing TokenEndpoint Configuration", + inputConfig: testCase3Config, + assertErrFunc: require.Error, + assertErrFuncMessage: "should return error when field empty", + } + + testCase5Config := defaultTestConfig + testCase5Config.GrantType = "" + + testCase5 := test{ + name: "Missing GrantType Configuration", + inputConfig: testCase3Config, + assertErrFunc: require.Error, + assertErrFuncMessage: "should return error when field empty", + } + + for _, testCase := range []test{testCase1, testCase2, testCase3, testCase4, testCase5} { t.Run(testCase.name, func(t *testing.T) { - _, err := NewKeycloakManager(OIDCConfig{}, testCase.inputConfig, &telemetry.MockAppMetrics{}) + _, err := NewKeycloakManager(testCase.inputConfig, &telemetry.MockAppMetrics{}) testCase.assertErrFunc(t, err, testCase.assertErrFuncMessage) }) } diff --git a/management/server/idp/zitadel.go b/management/server/idp/zitadel.go index 35cd907002b..fedfc3d4a77 100644 --- a/management/server/idp/zitadel.go +++ b/management/server/idp/zitadel.go @@ -30,9 +30,9 @@ type ZitadelManager struct { type ZitadelClientConfig struct { ClientID string ClientSecret string - GrantType string `json:"-"` - TokenEndpoint string `json:"-"` - ManagementEndpoint string `json:"-"` + GrantType string + TokenEndpoint string + ManagementEndpoint string } // ZitadelCredentials zitadel authentication information. @@ -85,8 +85,7 @@ type zitadelProfile struct { } // NewZitadelManager creates a new instance of the ZitadelManager. -func NewZitadelManager(oidcConfig OIDCConfig, config ZitadelClientConfig, - appMetrics telemetry.AppMetrics) (*ZitadelManager, error) { +func NewZitadelManager(config ZitadelClientConfig, appMetrics telemetry.AppMetrics) (*ZitadelManager, error) { httpTransport := http.DefaultTransport.(*http.Transport).Clone() httpTransport.MaxIdleConns = 5 @@ -94,11 +93,7 @@ func NewZitadelManager(oidcConfig OIDCConfig, config ZitadelClientConfig, Timeout: 10 * time.Second, Transport: httpTransport, } - helper := JsonParser{} - config.TokenEndpoint = oidcConfig.TokenEndpoint - config.ManagementEndpoint = fmt.Sprintf("%s/management/v1", oidcConfig.Issuer) - config.GrantType = "client_credentials" if config.ClientID == "" { return nil, fmt.Errorf("zitadel IdP configuration is incomplete, clientID is missing") @@ -108,6 +103,18 @@ func NewZitadelManager(oidcConfig OIDCConfig, config ZitadelClientConfig, return nil, fmt.Errorf("zitadel IdP configuration is incomplete, ClientSecret is missing") } + if config.TokenEndpoint == "" { + return nil, fmt.Errorf("zitadel IdP configuration is incomplete, TokenEndpoint is missing") + } + + if config.ManagementEndpoint == "" { + return nil, fmt.Errorf("zitadel IdP configuration is incomplete, ManagementEndpoint is missing") + } + + if config.GrantType == "" { + return nil, fmt.Errorf("zitadel IdP configuration is incomplete, GrantType is missing") + } + credentials := &ZitadelCredentials{ clientConfig: config, httpClient: httpClient, diff --git a/management/server/idp/zitadel_test.go b/management/server/idp/zitadel_test.go index 736422ab83b..28d26aedd10 100644 --- a/management/server/idp/zitadel_test.go +++ b/management/server/idp/zitadel_test.go @@ -57,7 +57,7 @@ func TestNewZitadelManager(t *testing.T) { for _, testCase := range []test{testCase1, testCase2, testCase3} { t.Run(testCase.name, func(t *testing.T) { - _, err := NewZitadelManager(OIDCConfig{}, testCase.inputConfig, &telemetry.MockAppMetrics{}) + _, err := NewZitadelManager(testCase.inputConfig, &telemetry.MockAppMetrics{}) testCase.assertErrFunc(t, err, testCase.assertErrFuncMessage) }) }