From dad057b6393548ad389ead07c2cce5b3ac2811e0 Mon Sep 17 00:00:00 2001 From: zeripath Date: Fri, 10 Mar 2023 06:14:43 +0000 Subject: [PATCH] Handle OpenID discovery URL errors a little nicer when creating/editing sources (#23397) When there is an error creating a new openIDConnect authentication source try to handle the error a little better. Close #23283 Signed-off-by: Andrew Thornton Co-authored-by: techknowlogick --- cmd/admin.go | 11 ++++++++- options/locale/locale_en-US.ini | 2 ++ routers/web/admin/auths.go | 23 +++++++++++++++++++ .../auth/source/oauth2/source_register.go | 4 ++++ templates/admin/auth/source/oauth.tmpl | 2 +- 5 files changed, 40 insertions(+), 2 deletions(-) diff --git a/cmd/admin.go b/cmd/admin.go index b913b817bd..f9fb1b6c68 100644 --- a/cmd/admin.go +++ b/cmd/admin.go @@ -7,6 +7,7 @@ package cmd import ( "errors" "fmt" + "net/url" "os" "strings" "text/tabwriter" @@ -469,11 +470,19 @@ func runAddOauth(c *cli.Context) error { return err } + config := parseOAuth2Config(c) + if config.Provider == "openidConnect" { + discoveryURL, err := url.Parse(config.OpenIDConnectAutoDiscoveryURL) + if err != nil || (discoveryURL.Scheme != "http" && discoveryURL.Scheme != "https") { + return fmt.Errorf("invalid Auto Discovery URL: %s (this must be a valid URL starting with http:// or https://)", config.OpenIDConnectAutoDiscoveryURL) + } + } + return auth_model.CreateSource(&auth_model.Source{ Type: auth_model.OAuth2, Name: c.String("name"), IsActive: true, - Cfg: parseOAuth2Config(c), + Cfg: config, }) } diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 3695bd0384..6f0d06a6e8 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2808,6 +2808,8 @@ auths.still_in_used = The authentication source is still in use. Convert or dele auths.deletion_success = The authentication source has been deleted. auths.login_source_exist = The authentication source '%s' already exists. auths.login_source_of_type_exist = An authentication source of this type already exists. +auths.unable_to_initialize_openid = Unable to initialize OpenID Connect Provider: %s +auths.invalid_openIdConnectAutoDiscoveryURL = Invalid Auto Discovery URL (this must be a valid URL starting with http:// or https://) config.server_config = Server Configuration config.app_name = Site Title diff --git a/routers/web/admin/auths.go b/routers/web/admin/auths.go index 8ce45720fe..d2953f753d 100644 --- a/routers/web/admin/auths.go +++ b/routers/web/admin/auths.go @@ -271,6 +271,15 @@ func NewAuthSourcePost(ctx *context.Context) { } case auth.OAuth2: config = parseOAuth2Config(form) + oauth2Config := config.(*oauth2.Source) + if oauth2Config.Provider == "openidConnect" { + discoveryURL, err := url.Parse(oauth2Config.OpenIDConnectAutoDiscoveryURL) + if err != nil || (discoveryURL.Scheme != "http" && discoveryURL.Scheme != "https") { + ctx.Data["Err_DiscoveryURL"] = true + ctx.RenderWithErr(ctx.Tr("admin.auths.invalid_openIdConnectAutoDiscoveryURL"), tplAuthNew, form) + return + } + } case auth.SSPI: var err error config, err = parseSSPIConfig(ctx, form) @@ -305,6 +314,10 @@ func NewAuthSourcePost(ctx *context.Context) { if auth.IsErrSourceAlreadyExist(err) { ctx.Data["Err_Name"] = true ctx.RenderWithErr(ctx.Tr("admin.auths.login_source_exist", err.(auth.ErrSourceAlreadyExist).Name), tplAuthNew, form) + } else if oauth2.IsErrOpenIDConnectInitialize(err) { + ctx.Data["Err_DiscoveryURL"] = true + unwrapped := err.(oauth2.ErrOpenIDConnectInitialize).Unwrap() + ctx.RenderWithErr(ctx.Tr("admin.auths.unable_to_initialize_openid", unwrapped), tplAuthNew, form) } else { ctx.ServerError("auth.CreateSource", err) } @@ -389,6 +402,15 @@ func EditAuthSourcePost(ctx *context.Context) { } case auth.OAuth2: config = parseOAuth2Config(form) + oauth2Config := config.(*oauth2.Source) + if oauth2Config.Provider == "openidConnect" { + discoveryURL, err := url.Parse(oauth2Config.OpenIDConnectAutoDiscoveryURL) + if err != nil || (discoveryURL.Scheme != "http" && discoveryURL.Scheme != "https") { + ctx.Data["Err_DiscoveryURL"] = true + ctx.RenderWithErr(ctx.Tr("admin.auths.invalid_openIdConnectAutoDiscoveryURL"), tplAuthEdit, form) + return + } + } case auth.SSPI: config, err = parseSSPIConfig(ctx, form) if err != nil { @@ -408,6 +430,7 @@ func EditAuthSourcePost(ctx *context.Context) { if err := auth.UpdateSource(source); err != nil { if oauth2.IsErrOpenIDConnectInitialize(err) { ctx.Flash.Error(err.Error(), true) + ctx.Data["Err_DiscoveryURL"] = true ctx.HTML(http.StatusOK, tplAuthEdit) } else { ctx.ServerError("UpdateSource", err) diff --git a/services/auth/source/oauth2/source_register.go b/services/auth/source/oauth2/source_register.go index 3527d54b65..82a36acaa6 100644 --- a/services/auth/source/oauth2/source_register.go +++ b/services/auth/source/oauth2/source_register.go @@ -36,6 +36,10 @@ func (err ErrOpenIDConnectInitialize) Error() string { return fmt.Sprintf("Failed to initialize OpenID Connect Provider with name '%s' with url '%s': %v", err.ProviderName, err.OpenIDConnectAutoDiscoveryURL, err.Cause) } +func (err ErrOpenIDConnectInitialize) Unwrap() error { + return err.Cause +} + // wrapOpenIDConnectInitializeError is used to wrap the error but this cannot be done in modules/auth/oauth2 // inside oauth2: import cycle not allowed models -> modules/auth/oauth2 -> models func wrapOpenIDConnectInitializeError(err error, providerName string, source *Source) error { diff --git a/templates/admin/auth/source/oauth.tmpl b/templates/admin/auth/source/oauth.tmpl index b7ee00822f..1080937f91 100644 --- a/templates/admin/auth/source/oauth.tmpl +++ b/templates/admin/auth/source/oauth.tmpl @@ -24,7 +24,7 @@ -
+