From 05349ddb5c9d62a59ab1b997984d0b97f24df247 Mon Sep 17 00:00:00 2001 From: kolaente Date: Mon, 18 Nov 2024 10:34:30 +0100 Subject: [PATCH] feat!: config for auth providers now use a map instead of an array The config values for openid providers now use a map with the provider as key instead of an array. For example before: auth: openid: providers: - name: foo clientid: ... now becomes: auth: openid: providers: foo: clientid: ... This allows us to read values for openid providers from files using the same syntax as everywhere and makes the configuration more predictable. It also allows configuring providers through env variables, though it is still required to set at least one value via the config file because Vikunja won't discover the provider otherwise. --- config-raw.json | 3 +- pkg/config/config.go | 4 ++ pkg/modules/auth/auth.go | 3 +- pkg/modules/auth/openid/cron.go | 10 ++--- pkg/modules/auth/openid/openid.go | 7 ++-- pkg/modules/auth/openid/providers.go | 56 ++++++++++++++++------------ 6 files changed, 47 insertions(+), 36 deletions(-) diff --git a/config-raw.json b/config-raw.json index d7e110eb2..4884d8420 100644 --- a/config-raw.json +++ b/config-raw.json @@ -665,9 +665,10 @@ }, { "key": "providers", - "comment": "A list of enabled providers. This expects an array, which means you cannot use enviroment variables.", + "comment": "A list of enabled providers. You can freely choose the ``. Note that you must add at least one key to a config file if you want to read values from an environment variable as the provider won't be known to Vikunja otherwise.", "children": [ { + "key": "", "children": [ { "key": "name", diff --git a/pkg/config/config.go b/pkg/config/config.go index 8357e279a..273d0a429 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -443,6 +443,10 @@ func getConfigValueFromFile(configKey string) string { func readConfigValuesFromFiles() { keys := viper.AllKeys() for _, key := range keys { + if strings.Contains(key, "auth.openid.providers") { + // Setting openid provider values will remove everything but the value from file + continue + } // Env is evaluated manually at runtime, so we need to check this for each key value := getConfigValueFromFile(key + ".file") if value != "" { diff --git a/pkg/modules/auth/auth.go b/pkg/modules/auth/auth.go index 45fd3d8b5..f78702387 100644 --- a/pkg/modules/auth/auth.go +++ b/pkg/modules/auth/auth.go @@ -21,9 +21,8 @@ import ( "net/http" "time" - "code.vikunja.io/api/pkg/db" - "code.vikunja.io/api/pkg/config" + "code.vikunja.io/api/pkg/db" "code.vikunja.io/api/pkg/models" "code.vikunja.io/api/pkg/user" "code.vikunja.io/api/pkg/web" diff --git a/pkg/modules/auth/openid/cron.go b/pkg/modules/auth/openid/cron.go index dfb153e6e..7fe4b58e3 100644 --- a/pkg/modules/auth/openid/cron.go +++ b/pkg/modules/auth/openid/cron.go @@ -17,13 +17,13 @@ package openid import ( - "code.vikunja.io/api/pkg/log" - "code.vikunja.io/api/pkg/models" - "xorm.io/builder" - "xorm.io/xorm" - "code.vikunja.io/api/pkg/cron" "code.vikunja.io/api/pkg/db" + "code.vikunja.io/api/pkg/log" + "code.vikunja.io/api/pkg/models" + + "xorm.io/builder" + "xorm.io/xorm" ) func RemoveEmptySSOTeams(s *xorm.Session) (err error) { diff --git a/pkg/modules/auth/openid/openid.go b/pkg/modules/auth/openid/openid.go index ac9d435c6..f1fcc06a1 100644 --- a/pkg/modules/auth/openid/openid.go +++ b/pkg/modules/auth/openid/openid.go @@ -24,14 +24,14 @@ import ( "strconv" "strings" - "code.vikunja.io/api/pkg/web/handler" - "code.vikunja.io/api/pkg/db" "code.vikunja.io/api/pkg/log" "code.vikunja.io/api/pkg/models" "code.vikunja.io/api/pkg/modules/auth" "code.vikunja.io/api/pkg/user" "code.vikunja.io/api/pkg/utils" + "code.vikunja.io/api/pkg/web/handler" + "github.com/coreos/go-oidc/v3/oidc" petname "github.com/dustinkirkland/golang-petname" "github.com/labstack/echo/v4" @@ -100,7 +100,6 @@ func HandleCallback(c echo.Context) error { provider, err := GetProvider(providerKey) log.Debugf("Provider: %v", provider) if err != nil { - log.Error(err) return handler.HandleHTTPError(err) } if provider == nil { @@ -114,7 +113,6 @@ func HandleCallback(c echo.Context) error { if err != nil { var rerr *oauth2.RetrieveError if errors.As(err, &rerr) { - log.Error(err) details := make(map[string]interface{}) if err := json.Unmarshal(rerr.Body, &details); err != nil { @@ -122,6 +120,7 @@ func HandleCallback(c echo.Context) error { return handler.HandleHTTPError(err) } + log.Error(err) return c.JSON(http.StatusBadRequest, map[string]interface{}{ "message": "Could not authenticate against third party.", "details": details, diff --git a/pkg/modules/auth/openid/providers.go b/pkg/modules/auth/openid/providers.go index 5e14c1b31..8e9ec5a71 100644 --- a/pkg/modules/auth/openid/providers.go +++ b/pkg/modules/auth/openid/providers.go @@ -17,14 +17,12 @@ package openid import ( - "regexp" "strconv" - "strings" - - "code.vikunja.io/api/pkg/log" "code.vikunja.io/api/pkg/config" + "code.vikunja.io/api/pkg/log" "code.vikunja.io/api/pkg/modules/keyvalue" + "github.com/coreos/go-oidc/v3/oidc" "golang.org/x/oauth2" ) @@ -43,14 +41,14 @@ func GetAllProviders() (providers []*Provider, err error) { return nil, nil } - rawProvider := rawProviders.([]interface{}) + rawProvider := rawProviders.(map[string]interface{}) - for _, p := range rawProvider { + for key, p := range rawProvider { var pi map[string]interface{} var is bool pi, is = p.(map[string]interface{}) // JSON config is a map[string]interface{}, other providers are not. Under the hood they are all strings so - // it is save to cast. + // it is safe to cast. if !is { pis := p.(map[interface{}]interface{}) pi = make(map[string]interface{}, len(pis)) @@ -59,21 +57,21 @@ func GetAllProviders() (providers []*Provider, err error) { } } - provider, err := getProviderFromMap(pi) + provider, err := getProviderFromMap(pi, key) if err != nil { - if provider != nil { - log.Errorf("Error while getting openid provider %s: %s", provider.Name, err) - continue - } - log.Errorf("Error while getting openid provider: %s", err) + log.Errorf("Error while getting openid provider %s: %s", key, err) + continue + } + + if provider == nil { + log.Errorf("Could not openid provider %s, please check your config", key) continue } providers = append(providers, provider) - k := getKeyFromName(pi["name"].(string)) - err = keyvalue.Put("openid_provider_"+k, provider) + err = keyvalue.Put("openid_provider_"+key, provider) if err != nil { return nil, err } @@ -107,19 +105,29 @@ func GetProvider(key string) (provider *Provider, err error) { return } -func getKeyFromName(name string) string { - reg := regexp.MustCompile("[^a-z0-9]+") - return reg.ReplaceAllString(strings.ToLower(name), "") -} +func getProviderFromMap(pi map[string]interface{}, key string) (provider *Provider, err error) { + + for _, configKey := range []string{ + // Values from environment variables are evaluated at runtime, hence we need to check them explicitly + // through viper to make sure we catch all of them. + "name", + "logouturl", + "scope", + "authurl", + "clientsecret", + "clientid", + } { + valueFromFile := config.GetConfigValueFromFile("auth.openid.providers." + key + "." + configKey) + if valueFromFile != "" { + pi[configKey] = valueFromFile + } + } -func getProviderFromMap(pi map[string]interface{}) (provider *Provider, err error) { name, is := pi["name"].(string) if !is { return nil, nil } - k := getKeyFromName(name) - logoutURL, ok := pi["logouturl"].(string) if !ok { logoutURL = "" @@ -130,8 +138,8 @@ func getProviderFromMap(pi map[string]interface{}) (provider *Provider, err erro scope = "openid profile email" } provider = &Provider{ - Name: pi["name"].(string), - Key: k, + Name: name, + Key: key, AuthURL: pi["authurl"].(string), OriginalAuthURL: pi["authurl"].(string), ClientSecret: pi["clientsecret"].(string),