From 872d2d1e1c0d01c965fda7f4e307abb499a30d99 Mon Sep 17 00:00:00 2001 From: Karl Persson Date: Tue, 7 Mar 2023 09:57:25 +0100 Subject: [PATCH] AuthN: Login error handling (#64239) * Social: Fix type so it appears in error responses * AuthN: construct errutil.Error from social.Error * login: Check for errutil.Error and use public message * Login: redirectURLWithErrorCookie for authn errors Co-authored-by: Jo --- pkg/api/login.go | 6 ++++++ pkg/api/login_oauth.go | 18 ++---------------- pkg/login/social/common.go | 2 +- pkg/services/authn/clients/oauth.go | 18 ++++++++++++++---- 4 files changed, 23 insertions(+), 21 deletions(-) diff --git a/pkg/api/login.go b/pkg/api/login.go index efa715efa38..d22f0a2f88f 100644 --- a/pkg/api/login.go +++ b/pkg/api/login.go @@ -25,6 +25,7 @@ import ( "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" + "github.com/grafana/grafana/pkg/util/errutil" "github.com/grafana/grafana/pkg/web" ) @@ -441,5 +442,10 @@ func getLoginExternalError(err error) string { return createTokenErr.ExternalErr } + gfErr := &errutil.Error{} + if errors.As(err, gfErr) { + return gfErr.Public().Message + } + return err.Error() } diff --git a/pkg/api/login_oauth.go b/pkg/api/login_oauth.go index d302f63ea19..68a2dc7544a 100644 --- a/pkg/api/login_oauth.go +++ b/pkg/api/login_oauth.go @@ -24,7 +24,6 @@ import ( "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" - "github.com/grafana/grafana/pkg/util/errutil" "github.com/grafana/grafana/pkg/web" ) @@ -90,7 +89,7 @@ func (hs *HTTPServer) OAuthLogin(ctx *contextmodel.ReqContext) { if code == "" { redirect, err := hs.authnService.RedirectURL(ctx.Req.Context(), authn.ClientWithPrefix(name), req) if err != nil { - hs.handleAuthnOAuthErr(ctx, "failed to generate oauth redirect url", err) + ctx.Redirect(hs.redirectURLWithErrorCookie(ctx, err)) return } @@ -109,7 +108,7 @@ func (hs *HTTPServer) OAuthLogin(ctx *contextmodel.ReqContext) { cookies.DeleteCookie(ctx.Resp, OauthStateCookieName, hs.CookieOptionsFromCfg) if err != nil { - hs.handleAuthnOAuthErr(ctx, "failed to perform login for oauth request", err) + ctx.Redirect(hs.redirectURLWithErrorCookie(ctx, err)) return } @@ -380,19 +379,6 @@ func (hs *HTTPServer) hashStatecode(code, seed string) string { return hex.EncodeToString(hashBytes[:]) } -func (hs *HTTPServer) handleAuthnOAuthErr(c *contextmodel.ReqContext, msg string, err error) { - gfErr := &errutil.Error{} - if errors.As(err, gfErr) { - if gfErr.Public().Message != "" { - c.Handle(hs.Cfg, gfErr.Public().StatusCode, gfErr.Public().Message, err) - return - } - } - - c.Logger.Warn(msg, "err", err) - c.Redirect(hs.Cfg.AppSubURL + "/login") -} - type LoginError struct { HttpStatus int PublicMessage string diff --git a/pkg/login/social/common.go b/pkg/login/social/common.go index 309ede7cd0e..1dfea668312 100644 --- a/pkg/login/social/common.go +++ b/pkg/login/social/common.go @@ -12,7 +12,7 @@ import ( ) var ( - errMissingGroupMembership = Error{"user not a member of one of the required groups"} + errMissingGroupMembership = &Error{"user not a member of one of the required groups"} ) type httpGetResponse struct { diff --git a/pkg/services/authn/clients/oauth.go b/pkg/services/authn/clients/oauth.go index 480d4b82d14..906c29a0dee 100644 --- a/pkg/services/authn/clients/oauth.go +++ b/pkg/services/authn/clients/oauth.go @@ -6,6 +6,7 @@ import ( "crypto/sha256" "encoding/base64" "encoding/hex" + "errors" "fmt" "net/http" "strings" @@ -42,11 +43,16 @@ var ( errOAuthInvalidState = errutil.NewBase(errutil.StatusUnauthorized, "auth.oauth.state.invalid", errutil.WithPublicMessage("Provided state does not match stored state")) errOAuthTokenExchange = errutil.NewBase(errutil.StatusInternal, "auth.oauth.token.exchange", errutil.WithPublicMessage("Failed to get token from provider")) + errOAuthUserInfo = errutil.NewBase(errutil.StatusInternal, "auth.oauth.userinfo.error") - errOAuthMissingRequiredEmail = errutil.NewBase(errutil.StatusUnauthorized, "auth.oauth.email.missing") - errOAuthEmailNotAllowed = errutil.NewBase(errutil.StatusUnauthorized, "auth.oauth.email.not-allowed") + errOAuthMissingRequiredEmail = errutil.NewBase(errutil.StatusUnauthorized, "auth.oauth.email.missing", errutil.WithPublicMessage("Provider didn't return an email address")) + errOAuthEmailNotAllowed = errutil.NewBase(errutil.StatusUnauthorized, "auth.oauth.email.not-allowed", errutil.WithPublicMessage("Required email domain not fulfilled")) ) +func fromSocialErr(err *social.Error) error { + return errutil.NewBase(errutil.StatusUnauthorized, "auth.oauth.userinfo.failed", errutil.WithPublicMessage(err.Error())).Errorf("%w", err) +} + var _ authn.RedirectClient = new(OAuth) func ProvideOAuth( @@ -106,13 +112,17 @@ func (c *OAuth) Authenticate(ctx context.Context, r *authn.Request) (*authn.Iden // exchange auth code to a valid token token, err := c.connector.Exchange(clientCtx, r.HTTPRequest.URL.Query().Get("code"), opts...) if err != nil { - return nil, err + return nil, errOAuthTokenExchange.Errorf("failed to exchange code to token: %w", err) } token.TokenType = "Bearer" userInfo, err := c.connector.UserInfo(c.connector.Client(clientCtx, token), token) if err != nil { - return nil, errOAuthTokenExchange.Errorf("failed to exchange code to token: %w", err) + var sErr *social.Error + if errors.As(err, &sErr) { + return nil, fromSocialErr(sErr) + } + return nil, errOAuthUserInfo.Errorf("failed to get user info: %w", err) } if userInfo.Email == "" {