Fix oauth2 issuer treatment and exception handling

[ascopes]

---

Bug fixes:

Fixes to OAuth2 introspection processing, fixing a couple of misinterpretations of
the implementation spec, and amending logging to aid in debugging.

• Amended treatment of OAuth2 'iss' claim

  Prior to this commit, the OAuth2 resource server code is failing any issuer
  that is not a valid URL. This does not correspond to
  https://datatracker.ietf.org/doc/html/rfc7662#page-7 which redirects to
  https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.1, defining an
  issuer as being a "StringOrURI", which is defined at
  https://datatracker.ietf.org/doc/html/rfc7519#page-5 as being
  an "arbitrary string value" that "MUST be a URI" only for
  "any value containing a ':'".

  While OIDC does require this field to be a valid URL, the use of the resource
  server itself should not require all integrations to be using OIDC under the
  hood, since that is irrelevant implementation detail as far as a REST API
  using OAuth2 resource server introspection for authentication and authorization
  is concerned.

  The issue currently is that an issuer that is not a valid URL may be
  provided, which will automatically result in the request being aborted
  due to being invalid.

  I have removed the check entirely, since while the claim could be invalid,
  it is still a response that the OAuth2 introspection endpoint has provided.
  In the liklihood that interpretations of this behaviour are different for
  the OAuth2 server implementation in use, this currently stops Spring
  Security from being able to be used at all without implementing a custom
  introspector from scratch.

  It is also worth noting that the spec does not specify whether it is
  valid to normalize issuers or not if they are valid URLs. This may cause
  other unintended side effects as a result of this change, so it is
  safer to disable it entirely. Best practises such as those outlined at
  https://curity.io/resources/learn/jwt-best-practices/#4-always-check-the-issuer always suggest
  that issuers must "match exactly the value you expect it to be" which
  is not necessarily guaranteed by converting to java.net.URL (since there is no
  concrete specification to say that the URL class must conform to JWT semantics).

  This issue is currently causing issues in existing applications we have that use this library,
  which is what has sparked the change to update this. The change itself may be
  considered breaking if the documentation refers to the issuer as being stored
  as a URL object. However, if the documentation only implies that .toString can be performed,
  then the underlying logic of this change would likely just be considered implementation
  detail instead.

---

Minor fixes and improvements:

• Fixed potential NullPointerException in opaque token introspection

  It appears Nimbus does not check the presence of the Content-Type
  header before parsing it in some versions, and since prior to this
  commit, the code is .toString()-ing the result, a malformed response
  (such as that from a misbehaving cloud gateway) that does not include
  a Content-Type would currently throw a NullPointerException.

  In addition to this, I have added a little more information to the
  log output for this module on the standard and reactive implementations
  to aid in debugging authorization/authentication issues much more
  easily. A failed introspection will now log the reason to debug logs
  to make this simpler to debug.

• Fix typo in headers asciidoc. Would have opened a separate PR for
  this but it felt like a little bit of an overkill to remove two characters!

• Fixed final field warnings in opaque token introspectors

• Ensuring consistency in error handling of opaque providers/managers

  The OpaqueTokenAuthenticationProvider now propagates the cause of
  introspection exceptions in the same way that the reactive
  OpaqueTokenReactiveAuthenticationManager does.

  Fixed a final field warning on both OpaqueTokenAuthenticationProvider
  and OpaqueTokenReactiveAuthenticationManager.

• Replace usages of deprecated OAuth2IntrospectionClaimNames

  Replace all usages of OAuth2IntrospectionClaimNames with
  the suggested OAuth2TokenIntrospectionClaimNames.

  There does not appear to be any further usages of OAuth2IntrospectionClaimNames,
  so it should be suitable for removal when appropriate in accordance with the
  deprecation policy.

[jzheaux]

Thanks, @ascopes, for the PR and for your thorough research and feedback!

The PR generally seems acceptable - could you please also address the following?

• Spring Security prefers trace logging for exceptional cases.
• It appears you should also run ./gradlew format to satisfy the build.
• I think it would be helpful to demonstrate the NPE in a test - the test should fail before your change and succeed after

[ascopes]

Thanks, @ascopes, for the PR and for your thorough research and feedback!
...

Thanks for the points! Will try and find some time in the morning to incorporate these changes and push it.

[ascopes]

demonstrate the NPE in a test

Confirmation of NPE on the non-reactive Nimbus introspector when Content-Type is missing using the main branch.

It appears you should also run ./gradlew format to satisfy the build.

Applied. Thanks for making me aware of that!

Spring Security prefers trace logging for exceptional cases.

Should be fixed now.

---

Should be ready for a review again, @jzheaux. Thanks for taking the time to go through this.

---

On a side note... it appears OAuth2IntrospectionClaimAccessor is deprecated in favour of OAuth2TokenIntrospectionClaimAccessor, but the OAuth2IntrospectionAuthenticatedPrincipal implementation only implements OAuth2IntrospectionClaimAccessor and not the replacement class. I have updated the latter to also implement the new version. Likewise I spotted one other place where issuers are being treated as URLs. Since this is part of the public API, I have updated the interface to mark that method as deprecated, and have then added a new method getIssuerName that returns the String issuer instead.

[juncevich]

You use 2nd-time AUTHORITY_PREFIX as constant. The first one was in NimbusReactiveOpaqueTokenIntrospector.java. Is it better to extract constant in one place? What are you thinking about it?

[ascopes]

I was thinking this, but it appears that this constant is used in many more places than just here. Since this was already in the file, I just moved the line it was on (see line 70 on the previous version)

I was tempted to do this but I didn't want to make this PR too large and increase the scope of it too much. I was considering whether or not to draft a separate PR once this fix has gone through to get all the declarations for "ROLE_" and "SCOPE_" and put them in one place.

I would be very tempted to make a class in the spring security core to deal with this so that all prefixes including ROLE_ and SCOPE_ can be kept together, but I was attempting to avoid doing this for now so that my changes didn't span across multiple modules in spring-security.

If you think it would be a good idea to do in this PR, though, I can certainly take a look at the weekend when I am not working! :)

[juncevich]

Oh, I see. You are absolutely right about not blowing this PR. Good idea about extracting "ROLE_" and "SCOPE_". For me, it looks like an independent PR.

[jzheaux]

Thanks, @ascopes, for the updates.

Now that SpringOne has passed, I've had time again to take another look. Please see my feedback below.

[ascopes]

Will rebase onto spring-projects/spring-security@master once I have applied the requested changes.

[ascopes]

@jzheaux made the suggested changes, I assume you are still happy for things like the typo fix in the documentation to go through with this, since it is relatively minor?

Let me know if there is anything else you want me to do! Also, with regards to @juncevich's comment above, do you agree with making a separate PR or issue afterwards to consider this?

[jzheaux]

Thanks for the updates, @ascopes. Yes, I think that fixing the typo in the documentation is fine as it is an obvious fix and is in a separate commit.

In preparation for merging, will you please squash your feedback updates into the appropriate commits? Also please make sure to run format for each commit so that needed formatting changes are applied atomically.

do you agree with making a separate PR or issue afterwards to consider this?

I'd probably recommend an issue first. Scope is a concept unique to the oauth2 modules, so it's not yet clear to me what object would go into core that would contain both the role and scope concepts.

[ascopes]

Thanks for the updates, @ascopes. Yes, I think that fixing the typo in the documentation is fine as it is an obvious fix and is in a separate commit.

In preparation for merging, will you please squash your feedback updates into the appropriate commits? Also please make sure to run format for each commit so that needed formatting changes are applied atomically.

do you agree with making a separate PR or issue afterwards to consider this?

I'd probably recommend an issue first. Scope is a concept unique to the oauth2 modules, so it's not yet clear to me what object would go into core that would contain both the role and scope concepts.

Should be updated now; tests appear to be passing, checkstyle appears to be passing, and I've run format on the respective commits.

Also rebased onto main again.

Please can you re-review when you have time? Thanks!

[jzheaux]

Thanks for your detailed contribution, @ascopes! This is now merged into main.