AbstractOAuth2Token modifications: #6808
Conversation
* remove issuedAt and expiresAt properties (redundancyand potential incoherence with claims) * promote claims (AKA attributes) as abstracted member
There was a problem hiding this comment.
@ch4mpy There is a significant amount of changes in this PR which complicates the process and requires quite a bit of time for review. As well, the changes here touch on a few core OAuth2 classes which is very risky.
This PR cannot be accepted for a number of reasons as I've detailed in my comments. On a go forward basis, please keep PR's as minimal as possible to help with the review process. You may need to split a PR into multiple PR's to keep the changes contained. However, if this is the case, than submit one first and allow the process to go through before you submit the next one. This will help us significantly and ultimately save everyone time.
| * | ||
| * @param tokenValue the token value | ||
| */ | ||
| protected AbstractOAuth2Token(String tokenValue) { |
There was a problem hiding this comment.
This type of change can only be applied in a major release - please see Semantic Versioning - otherwise, this breaks compatibility.
| public @Nullable Instant getExpiresAt() { | ||
| return this.expiresAt; | ||
| } | ||
| public abstract @Nullable Instant getExpiresAt(); |
There was a problem hiding this comment.
If the expires_in is provided in the Access Token Response it should be supplied via the constructor. It doesn't make sense to me to have an abstract method for overriding expiredAt that is also optional (but recommended) as per spec.
Furthermore, abstract methods/operations are typically provided to enable polymorphic behaviour in more complex classes. It's not that common to provide the same for a simple POJO attribute such as getExpiresAt() - it's either null or not-null nothing more.
There was a problem hiding this comment.
Does it make sens to have issue and expiration instants both as plein members AND claims (or attributes) in most token implementations ?
To me, issue and expiration claims (or attributes) access is polymorphic as claims could have different name and instant serialization depend on the token type. Some token types could even have no claim (attributes) and require instant members for that.
| protected AbstractOAuth2Token(String tokenValue) { | ||
| this(tokenValue, null, null); | ||
| } | ||
| private final Map<String, Object> attributes; |
There was a problem hiding this comment.
Adding Map<String, Object> attributes here doesn't make sense as there may be no attributes available for an instance of an OAuth 2.0 Token. A refresh token is an example of this as it's typically opaque. At a minimum, there is a token value and from a logical standpoint there will likely be a expires_at and if that is available than we can assume there is an issued_at. All other attributes are optional.
As an FYI, the concept of claims was introduced in the OpenID Connect related specifications NOT standard OAuth 2.0. For example, there is no concept of claims with GitHub and Facebook since they are not OpenID Connect compliant and instead implement standard OAuth 2.0.
There was a problem hiding this comment.
issued_at and expires_at seemed good candidates for being stored in a Map<String, Object> attributes, just like for other token types.
| */ | ||
| protected AbstractOAuth2Token(String tokenValue, @Nullable Instant issuedAt, @Nullable Instant expiresAt) { | ||
| protected AbstractOAuth2Token(final String tokenValue, final Map<String, Object> attributes) { |
There was a problem hiding this comment.
This change breaks compatibility - see previous comment
|
|
||
| @Override | ||
| public Instant getIssuedAt() { | ||
| return getClaimAsInstant("iat"); |
There was a problem hiding this comment.
The iat claim is defined by the OpenID Connect related specifications as well as Jwt spec. However, this claim is likely not available for standard OAuth 2.0 providers like GitHub and Facebook. But it doesn't mean they don't provide their own attribute value for issueAt but using a different attribute name.
There was a problem hiding this comment.
couldn't it be solved with more polymorphisme?
|
|
||
| @Override | ||
| public Instant getExpiresAt() { | ||
| return getClaimAsInstant("exp"); |
There was a problem hiding this comment.
Same reasons as described for iat.
jgrandja
added
the
in: oauth2
jgrandja
added
the
status: declined
I can imagine. As per my opening comment in #6807:
|
To illustrate
#6807
Make it possible to inject granted-authorities converter in JwtAuthenticationConverter constructor
JwtGrantedAuthoritiesConverter modifications (which currently is both a granted-authority and a scope converter.):
TokenAttributesScopesConverter behavior modification: scan and merge all "claims to scan" (instead of first found only)