Skip to content

Commit

Permalink
feat: improve and clean up error handling
Browse files Browse the repository at this point in the history
  • Loading branch information
@aeneasr
aeneasr committed Nov 17, 2020
1 parent edc54c2 commit b727367
Show file tree
Hide file tree
Showing 10 changed files with 180 additions and 78 deletions.
18 changes: 9 additions & 9 deletions client/validator.go
View file
Expand Up @@ -76,7 +76,7 @@ func (v *Validator) Validate(c *Client) error {
c.TokenEndpointAuthMethod = "client_secret_basic"
} else if c.TokenEndpointAuthMethod == "private_key_jwt" {
if len(c.JSONWebKeysURI) == 0 && c.JSONWebKeys == nil {
return errors.WithStack(fosite.ErrInvalidRequest.WithHint("When token_endpoint_auth_method is \"private_key_jwt\", either jwks or jwks_uri must be set."))
return errors.WithStack(fosite.ErrInvalidRequest.WithHint("When token_endpoint_auth_method is 'private_key_jwt', either jwks or jwks_uri must be set."))
}
if c.TokenEndpointAuthSigningAlgorithm != "" && !isSupportedAuthTokenSigningAlg(c.TokenEndpointAuthSigningAlgorithm) {
return errors.WithStack(fosite.ErrInvalidRequest.WithHint("Only RS256, RS384, RS512, PS256, PS384, PS512, ES256, ES384 and ES512 are supported as algorithms for private key authentication."))
Expand All @@ -98,20 +98,20 @@ func (v *Validator) Validate(c *Client) error {
for k, origin := range c.AllowedCORSOrigins {
u, err := url.Parse(origin)
if err != nil {
return errors.WithStack(fosite.ErrInvalidRequest.WithHint(fmt.Sprintf("Origin URL %s from allowed_cors_origins could not be parsed: %s", origin, err)))
return errors.WithStack(fosite.ErrInvalidRequest.WithHintf("Origin URL %s from allowed_cors_origins could not be parsed: %s", origin, err))
}

if u.Scheme != "https" && u.Scheme != "http" {
return errors.WithStack(fosite.ErrInvalidRequest.WithHint(fmt.Sprintf("Origin URL %s must use https:// or http:// as HTTP scheme.", origin)))
return errors.WithStack(fosite.ErrInvalidRequest.WithHintf("Origin URL %s must use https:// or http:// as HTTP scheme.", origin))
}

if u.User != nil && len(u.User.String()) > 0 {
return errors.WithStack(fosite.ErrInvalidRequest.WithHint(fmt.Sprintf("Origin URL %s has HTTP user and/or password set which is not allowed.", origin)))
return errors.WithStack(fosite.ErrInvalidRequest.WithHintf("Origin URL %s has HTTP user and/or password set which is not allowed.", origin))
}

u.Path = strings.TrimRight(u.Path, "/")
if len(u.Path)+len(u.RawQuery)+len(u.Fragment) > 0 {
return errors.WithStack(fosite.ErrInvalidRequest.WithHint(fmt.Sprintf("Origin URL %s must have an empty path, query, and fragment but one of the parts is not empty.", origin)))
return errors.WithStack(fosite.ErrInvalidRequest.WithHintf("Origin URL %s must have an empty path, query, and fragment but one of the parts is not empty.", origin))
}

c.AllowedCORSOrigins[k] = u.String()
Expand All @@ -131,7 +131,7 @@ func (v *Validator) Validate(c *Client) error {
}

if c.UserinfoSignedResponseAlg != "none" && c.UserinfoSignedResponseAlg != "RS256" {
return errors.WithStack(fosite.ErrInvalidRequest.WithHint("Field userinfo_signed_response_alg can either be \"none\" or \"RS256\"."))
return errors.WithStack(fosite.ErrInvalidRequest.WithHint("Field userinfo_signed_response_alg can either be 'none' or 'RS256'."))
}

var redirs []url.URL
Expand All @@ -143,13 +143,13 @@ func (v *Validator) Validate(c *Client) error {
redirs = append(redirs, *u)

if strings.Contains(r, "#") {
return errors.WithStack(fosite.ErrInvalidRequest.WithHint("Redirect URIs must not contain fragments (#)"))
return errors.WithStack(fosite.ErrInvalidRequest.WithHint("Redirect URIs must not contain fragments (#)."))
}
}

if c.SubjectType != "" {
if !stringslice.Has(v.conf.SubjectTypesSupported(), c.SubjectType) {
return errors.WithStack(fosite.ErrInvalidRequest.WithHint(fmt.Sprintf("Subject type %s is not supported by server, only %v are allowed.", c.SubjectType, v.conf.SubjectTypesSupported())))
return errors.WithStack(fosite.ErrInvalidRequest.WithHintf("Subject type %s is not supported by server, only %v are allowed.", c.SubjectType, v.conf.SubjectTypesSupported()))
}
} else {
if stringslice.Has(v.conf.SubjectTypesSupported(), "public") {
Expand Down Expand Up @@ -187,7 +187,7 @@ func (v *Validator) Validate(c *Client) error {
func (v *Validator) ValidateSectorIdentifierURL(location string, redirectURIs []string) error {
l, err := url.Parse(location)
if err != nil {
return errors.WithStack(fosite.ErrInvalidRequest.WithHint(fmt.Sprintf("Value of sector_identifier_uri could not be parsed: %s", err)))
return errors.WithStack(fosite.ErrInvalidRequest.WithHintf("Value of sector_identifier_uri could not be parsed because %s.", err))
}

if l.Scheme != "https" {
Expand Down
20 changes: 10 additions & 10 deletions consent/handler.go
View file
Expand Up @@ -105,7 +105,7 @@ func (h *Handler) DeleteConsentSession(w http.ResponseWriter, r *http.Request, p
client := r.URL.Query().Get("client")
allClients := r.URL.Query().Get("all") == "true"
if subject == "" {
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter "subject" is not defined but should have been.`)))
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter 'subject' is not defined but should have been.`)))
return
}

Expand All @@ -121,7 +121,7 @@ func (h *Handler) DeleteConsentSession(w http.ResponseWriter, r *http.Request, p
return
}
default:
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter both "client" and "all" is not defined but one of them should have been.`)))
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter both 'client' and 'all' is not defined but one of them should have been.`)))
return
}

Expand Down Expand Up @@ -155,7 +155,7 @@ func (h *Handler) DeleteConsentSession(w http.ResponseWriter, r *http.Request, p
func (h *Handler) GetConsentSessions(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
subject := r.URL.Query().Get("subject")
if subject == "" {
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter "subject" is not defined but should have been.`)))
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter 'subject' is not defined but should have been.`)))
return
}

Expand Down Expand Up @@ -216,7 +216,7 @@ func (h *Handler) GetConsentSessions(w http.ResponseWriter, r *http.Request, ps
func (h *Handler) DeleteLoginSession(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
subject := r.URL.Query().Get("subject")
if subject == "" {
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter "subject" is not defined but should have been.`)))
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter 'subject' is not defined but should have been.`)))
return
}

Expand Down Expand Up @@ -262,7 +262,7 @@ func (h *Handler) GetLoginRequest(w http.ResponseWriter, r *http.Request, ps htt
)

if challenge == "" {
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter "challenge" is not defined but should have been.`)))
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter 'challenge' is not defined but should have been.`)))
return
}

Expand Down Expand Up @@ -318,7 +318,7 @@ func (h *Handler) AcceptLoginRequest(w http.ResponseWriter, r *http.Request, ps
r.URL.Query().Get("challenge"),
)
if challenge == "" {
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter "challenge" is not defined but should have been.`)))
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter 'challenge' is not defined but should have been.`)))
return
}

Expand Down Expand Up @@ -406,7 +406,7 @@ func (h *Handler) RejectLoginRequest(w http.ResponseWriter, r *http.Request, ps
r.URL.Query().Get("challenge"),
)
if challenge == "" {
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter "challenge" is not defined but should have been.`)))
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter 'challenge' is not defined but should have been.`)))
return
}

Expand Down Expand Up @@ -481,7 +481,7 @@ func (h *Handler) GetConsentRequest(w http.ResponseWriter, r *http.Request, ps h
r.URL.Query().Get("challenge"),
)
if challenge == "" {
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter "challenge" is not defined but should have been.`)))
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter 'challenge' is not defined but should have been.`)))
return
}

Expand Down Expand Up @@ -546,7 +546,7 @@ func (h *Handler) AcceptConsentRequest(w http.ResponseWriter, r *http.Request, p
r.URL.Query().Get("challenge"),
)
if challenge == "" {
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter "challenge" is not defined but should have been.`)))
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter 'challenge' is not defined but should have been.`)))
return
}

Expand Down Expand Up @@ -625,7 +625,7 @@ func (h *Handler) RejectConsentRequest(w http.ResponseWriter, r *http.Request, p
r.URL.Query().Get("challenge"),
)
if challenge == "" {
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter "challenge" is not defined but should have been.`)))
h.r.Writer().WriteError(w, r, errors.WithStack(fosite.ErrInvalidRequest.WithHint(`Query parameter 'challenge' is not defined but should have been.`)))
return
}

Expand Down
34 changes: 15 additions & 19 deletions consent/strategy_default.go
View file
Expand Up @@ -332,7 +332,7 @@ func (s *DefaultStrategy) obfuscateSubjectIdentifier(cl fosite.Client, subject,
if c, ok := cl.(*client.Client); ok && c.SubjectType == "pairwise" {
algorithm, ok := s.r.SubjectIdentifierAlgorithm()[c.SubjectType]
if !ok {
return "", errors.WithStack(fosite.ErrInvalidRequest.WithHint(fmt.Sprintf(`Subject Identifier Algorithm "%s" was requested by OAuth 2.0 Client "%s", but is not configured.`, c.SubjectType, c.OutfacingID)))
return "", errors.WithStack(fosite.ErrInvalidRequest.WithHintf(`Subject Identifier Algorithm '%s' was requested by OAuth 2.0 Client '%s' but is not configured.`, c.SubjectType, c.OutfacingID))
}

if len(forcedIdentifier) > 0 {
Expand All @@ -350,7 +350,7 @@ func (s *DefaultStrategy) verifyAuthentication(w http.ResponseWriter, r *http.Re
ctx := r.Context()
session, err := s.r.ConsentManager().VerifyAndInvalidateLoginRequest(ctx, verifier)
if errors.Is(err, sqlcon.ErrNoRows) {
return nil, errors.WithStack(fosite.ErrAccessDenied.WithDebug("The login verifier has already been used, has not been granted, or is invalid."))
return nil, errors.WithStack(fosite.ErrAccessDenied.WithDebug("The login verifier has already been used, has not been granted or is invalid."))
} else if err != nil {
return nil, err
}
Expand All @@ -361,7 +361,7 @@ func (s *DefaultStrategy) verifyAuthentication(w http.ResponseWriter, r *http.Re
}

if session.RequestedAt.Add(s.c.ConsentRequestMaxAge()).Before(time.Now()) {
return nil, errors.WithStack(fosite.ErrRequestUnauthorized.WithDebug("The login request has expired, please try again."))
return nil, errors.WithStack(fosite.ErrRequestUnauthorized.WithDebug("The login request has expired. Please try again."))
}

if err := validateCsrfSession(r, s.r.CookieStore(), cookieAuthenticationCSRFName, session.LoginRequest.CSRF, s.c.CookieSameSiteLegacyWorkaround(), s.c.ServesHTTPS()); err != nil {
Expand Down Expand Up @@ -643,7 +643,7 @@ func (s *DefaultStrategy) generateFrontChannelLogoutURLs(ctx context.Context, su
for _, c := range clients {
u, err := url.Parse(c.FrontChannelLogoutURI)
if err != nil {
return nil, errors.WithStack(fosite.ErrServerError.WithHint(fmt.Sprintf("Unable to parse frontchannel_logout_uri: %s", c.FrontChannelLogoutURI)).WithDebug(err.Error()))
return nil, errors.WithStack(fosite.ErrServerError.WithHintf("Unable to parse frontchannel_logout_uri because %s.", c.FrontChannelLogoutURI).WithDebug(err.Error()))
}

urls = append(urls, urlx.SetQuery(u, url.Values{
Expand Down Expand Up @@ -767,12 +767,12 @@ func (s *DefaultStrategy) issueLogoutVerifier(w http.ResponseWriter, r *http.Req

if len(state) > 0 {
// state can only be set if it's an RP-initiated logout flow. If not, we should throw an error.
return nil, errors.WithStack(fosite.ErrInvalidRequest.WithHint("Logout failed because query parameter state is set but id_token_hint is missing"))
return nil, errors.WithStack(fosite.ErrInvalidRequest.WithHint("Logout failed because query parameter state is set but id_token_hint is missing."))
}

if len(requestedRedir) > 0 {
// post_logout_redirect_uri can only be set if it's an RP-initiated logout flow. If not, we should throw an error.
return nil, errors.WithStack(fosite.ErrInvalidRequest.WithHint("Logout failed because query parameter post_logout_redirect_uri is set but id_token_hint is missing"))
return nil, errors.WithStack(fosite.ErrInvalidRequest.WithHint("Logout failed because query parameter post_logout_redirect_uri is set but id_token_hint is missing."))
}

session, err := s.authenticationSession(w, r)
Expand Down Expand Up @@ -818,37 +818,33 @@ func (s *DefaultStrategy) issueLogoutVerifier(w http.ResponseWriter, r *http.Req
mksi := mapx.KeyStringToInterface(claims)
if !claims.VerifyIssuer(s.c.IssuerURL().String(), true) {
return nil, errors.WithStack(fosite.ErrInvalidRequest.
WithHint(
fmt.Sprintf(
`Logout failed because issuer claim value "%s" from query parameter id_token_hint does not match with issuer value from configuration "%s"`,
WithHintf(
`Logout failed because issuer claim value '%s' from query parameter id_token_hint does not match with issuer value from configuration '%s'.`,
mapx.GetStringDefault(mksi, "iss", ""),
s.c.IssuerURL().String(),
),
),
)
}

now := time.Now().UTC().Unix()
if !claims.VerifyIssuedAt(now, true) {
return nil, errors.WithStack(fosite.ErrInvalidRequest.
WithHint(
fmt.Sprintf(
`Logout failed because iat claim value "%.0f" from query parameter id_token_hint is before now ("%d")`,
WithHintf(
`Logout failed because iat claim value '%.0f' from query parameter id_token_hint is before now ('%d').`,
mapx.GetFloat64Default(mksi, "iat", float64(0)),
now,
),
),
)
}

hintSid := mapx.GetStringDefault(mksi, "sid", "")
if len(hintSid) == 0 {
return nil, errors.WithStack(fosite.ErrInvalidRequest.WithHint("Logout failed because query parameter id_token_hint is missing sid claim"))
return nil, errors.WithStack(fosite.ErrInvalidRequest.WithHint("Logout failed because query parameter id_token_hint is missing sid claim."))
}

// It doesn't really make sense to use the subject value from the ID Token because it might be obfuscated.
if hintSub := mapx.GetStringDefault(mksi, "sub", ""); len(hintSub) == 0 {
return nil, errors.WithStack(fosite.ErrInvalidRequest.WithHint("Logout failed because query parameter id_token_hint is missing sub claim"))
return nil, errors.WithStack(fosite.ErrInvalidRequest.WithHint("Logout failed because query parameter id_token_hint is missing sub claim."))
}

// Let's find the client by cycling through the audiences. Typically, we only have one audience
Expand All @@ -872,7 +868,7 @@ func (s *DefaultStrategy) issueLogoutVerifier(w http.ResponseWriter, r *http.Req

if cl == nil {
return nil, errors.WithStack(fosite.ErrInvalidRequest.
WithHint("Logout failed because none of the listed audiences is a registered OAuth 2.0 Client"))
WithHint("Logout failed because none of the listed audiences is a registered OAuth 2.0 Client."))
}

if len(requestedRedir) > 0 {
Expand All @@ -881,7 +877,7 @@ func (s *DefaultStrategy) issueLogoutVerifier(w http.ResponseWriter, r *http.Req
if w == requestedRedir {
u, err := url.Parse(w)
if err != nil {
return nil, errors.WithStack(fosite.ErrServerError.WithHint(fmt.Sprintf("Unable to parse post_logout_redirect_uri: %s", w)).WithDebug(err.Error()))
return nil, errors.WithStack(fosite.ErrServerError.WithHint(fmt.Sprintf("Unable to parse post_logout_redirect_uri '%s' because %s.", w, err)).WithDebug(err.Error()))
}

f = u
Expand All @@ -890,7 +886,7 @@ func (s *DefaultStrategy) issueLogoutVerifier(w http.ResponseWriter, r *http.Req

if f == nil {
return nil, errors.WithStack(fosite.ErrInvalidRequest.
WithHint("Logout failed because query parameter post_logout_redirect_uri is not a whitelisted as a post_logout_redirect_uri for the client"),
WithHint("Logout failed because query parameter post_logout_redirect_uri is not a whitelisted as a post_logout_redirect_uri for the client."),
)
}

Expand Down
4 changes: 2 additions & 2 deletions consent/subject_identifier_algorithm_pairwise.go
View file
Expand Up @@ -43,9 +43,9 @@ func (g *SubjectIdentifierAlgorithmPairwise) Obfuscate(subject string, client *c
// sub = SHA-256 ( sector_identifier || local_account_id || salt ).
var id string
if len(client.SectorIdentifierURI) == 0 && len(client.RedirectURIs) > 1 {
return "", errors.WithStack(fosite.ErrInvalidRequest.WithHint(fmt.Sprintf("OAuth 2.0 Client %s has multiple redirect_uris but no sector_identifier_uri was set which is not allowed when performing using subject type pairwise. Please reconfigure the OAuth 2.0 client properly.", client.OutfacingID)))
return "", errors.WithStack(fosite.ErrInvalidRequest.WithHintf("OAuth 2.0 Client %s has multiple redirect_uris but no sector_identifier_uri was set which is not allowed when performing using subject type pairwise. Please reconfigure the OAuth 2.0 client properly.", client.OutfacingID))
} else if len(client.SectorIdentifierURI) == 0 && len(client.RedirectURIs) == 0 {
return "", errors.WithStack(fosite.ErrInvalidRequest.WithHint(fmt.Sprintf("OAuth 2.0 Client %s neither specifies a sector_identifier_uri nor a redirect_uri which is not allowed when performing using subject type pairwise. Please reconfigure the OAuth 2.0 client properly.", client.OutfacingID)))
return "", errors.WithStack(fosite.ErrInvalidRequest.WithHintf("OAuth 2.0 Client %s neither specifies a sector_identifier_uri nor a redirect_uri which is not allowed when performing using subject type pairwise. Please reconfigure the OAuth 2.0 client properly.", client.OutfacingID))
} else if len(client.SectorIdentifierURI) > 0 {
id = client.SectorIdentifierURI
} else {
Expand Down
12 changes: 6 additions & 6 deletions consent/types.go
View file
Expand Up @@ -111,15 +111,15 @@ func (e *RequestDeniedError) toRFCError() *fosite.RFC6749Error {
}

if e.Code == 0 {
e.Code = fosite.ErrInvalidRequest.Code
e.Code = fosite.ErrInvalidRequest.CodeField
}

return &fosite.RFC6749Error{
Name: e.Name,
Description: e.Description,
Hint: e.Hint,
Code: e.Code,
Debug: e.Debug,
ErrorField: e.Name,
DescriptionField: e.Description,
HintField: e.Hint,
CodeField: e.Code,
DebugField: e.Debug,
}
}

Expand Down
26 changes: 13 additions & 13 deletions consent/types_test.go
View file
Expand Up @@ -21,10 +21,10 @@ func TestToRFCError(t *testing.T) {
valid: true,
},
expect: &fosite.RFC6749Error{
Name: "not empty",
Description: "",
Code: fosite.ErrInvalidRequest.Code,
Debug: "",
ErrorField: "not empty",
DescriptionField: "",
CodeField: fosite.ErrInvalidRequest.CodeField,
DebugField: "",
},
},
{
Expand All @@ -34,20 +34,20 @@ func TestToRFCError(t *testing.T) {
valid: true,
},
expect: &fosite.RFC6749Error{
Name: "request_denied",
Description: "not empty",
Code: fosite.ErrInvalidRequest.Code,
Debug: "",
ErrorField: "request_denied",
DescriptionField: "not empty",
CodeField: fosite.ErrInvalidRequest.CodeField,
DebugField: "",
},
},
{
input: &RequestDeniedError{valid: true},
expect: &fosite.RFC6749Error{
Name: "request_denied",
Description: "",
Hint: "",
Code: fosite.ErrInvalidRequest.Code,
Debug: "",
ErrorField: "request_denied",
DescriptionField: "",
HintField: "",
CodeField: fosite.ErrInvalidRequest.CodeField,
DebugField: "",
},
},
} {
Expand Down

0 comments on commit b727367

Please sign in to comment.