Skip to content

Add WWW-Authenticate header for 401 and 403 requests#51

Merged
lbalmaceda merged 3 commits intoauth0:masterfrom
jimmyjames:add-www-authenticate-header
Jun 9, 2020
Merged

Add WWW-Authenticate header for 401 and 403 requests#51
lbalmaceda merged 3 commits intoauth0:masterfrom
jimmyjames:add-www-authenticate-header

Conversation

@jimmyjames
Copy link
Copy Markdown
Contributor

Changes

As raised in #47, the WWW-Authenticate headers should be sent on the response when the request is rejected due to a missing or invalid access token. This is specified by RFC 6750.

Scenarios

  • Requests to a resource that require authorization that are rejected with a 401 due to an invalid or missing access token will have a WWW-Authenticate header included on the response.
  • Requests to a resource that are rejected with a 403 due to insufficient scopes will have a WWW-Authenticate header included on the response.

WWW-Authenticate header value

RFC 6750 does not require any specific attributes of the WWW-Authenticate header to be set. This PR simply includes an error attribute, with the following values:

  • "Invalid token" for a 401 resulting from a missing or invalid access token
  • "Insufficient scope" for a 403 resulting from a token with insufficient scopes

A couple notes on what could be added in the future, but is not included here as this change is focused on complying with RFC 6750 and enables future changes to the header attributes:

  • In the case of 401 errors, we could unwrap the AuthenticationException and use the underlying exception cause to return a more specific error (e.g., invalid signature, expired token, etc).
  • In the case of 403 errors, we might be able to included the required scopes for the resource, as was added to express-jwt-authz. That would require some more thought, since we'd need to be able to hook into the required scopes as configured by the application developer.
  • We could include attributes such as the realm or authorization_uri, if those prove to be useful for callers.

Again, those possible additions are not included in this PR for the purposes of simplicity and compliance, but we can consider them going forward.

References

Testing

Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

  • This change adds unit test coverage
  • This change adds integration test coverage
  • This change has been tested on the latest version of the platform/language or why not

Checklist

@jimmyjames jimmyjames added CH: Added medium Medium review labels Jun 5, 2020
@jimmyjames jimmyjames added this to the v1-Next milestone Jun 5, 2020
@jimmyjames jimmyjames requested a review from a team June 5, 2020 21:03
class JwtAccessDeniedHandler extends AccessDeniedHandlerImpl {

@Override
public void handle(HttpServletRequest request, HttpServletResponse response, AccessDeniedException accessDeniedException) throws IOException, ServletException {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AccessDeniedHandler specifies that handle must be public, which is why the class as package-private but the method as public

@jimmyjames
Copy link
Copy Markdown
Contributor Author

Codecov checks failing because of the lack of coverage around the configure() method of JwtWebSecurityConfigurer. There's not a great way to test this, as HttpSecurity is final (can't be mocked) and would probably require a spring integration test, which would require some type of controller under test.

@jimmyjames jimmyjames force-pushed the add-www-authenticate-header branch from dde3fd2 to 388a48d Compare June 9, 2020 16:27
@jimmyjames jimmyjames requested a review from lbalmaceda June 9, 2020 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CH: Added medium Medium review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants