Remember user consent and make consent page configurable

[Kehrlann]

Fixes gh-283

[jgrandja]

Thanks for the PR @Kehrlann ! Please see review comments.

[jgrandja]

Is this a valid condition?

[Kehrlann]

I think so.

Scenario 1

Requested scopes:

• openid [REQUIRED to authenticate the user]
• notification.read [OPTIONAL, e.g. the app wants to alert users if they have notifications about X]

The user would see a consent screen with just notification.read, which they may decide NOT to consent to ; they may "untick" the item and click submit anyway (which is what the current consent page does).
Which would mean "yes, give a token to this client, but without any of theses scopes that I can see".

Scenario 2

Requested scopes:

• message.read [REQUIRED]
• message.write [OPTIONAL]

Previously approved scopes:

• message.read

Same as above, with the user un-ticking "message.write"

[jgrandja]

I think we should still display the previous consented scopes

[Kehrlann]

We could, but then they should be disabled, because otherwise the users may think they are revoking consent, or not granting consent for this particular token request, despite them having not granted consent before.

The more I think about it, the more I think it should be visible but grayed-out.

---

Example flow if we just keep it as is.

Token request #1:

User sees message.read and message.write, and only ticks the second:

• message.read
• message.write

Then, consent for message.write is persisted. There is no way to revoke consent yet.

Token request #2

User sees message.read and message.write again, and this times ticks only the first:

• message.read
• message.write

The user would expect to only consent to message.read. But they have already consented to message.write, so despite them unticking message.write, the client app will obtain a token with the message.write scope as requested.

[Kehrlann]

Done, please review and/or change the "previously approved scopes" message:

You have already granted the following permissions to the above app:

[jgrandja]

@Kehrlann The current implementation stores authorized scopes in OAuth2Authorization.attributes using key OAuth2Authorization.AUTHORIZED_SCOPE_ATTRIBUTE_NAME. We need to account for this in the PR since the storage is moving.

I wonder if we remove the key or possibly keep it? If we keep it then the OAuth2AuthorizationService would need to use the OAuth2AuthorizationConsentService to load the consents into the OAuth2Authorization attribute. Thoughts?

[Kehrlann]

Re: OAuth2Authorization storing the authorized scopes, that's an interesting point.

In this PR, we save all previously approved scopes in the OAuth2Authorization, through:

Set<String> authorizedScopes = userConsentRequestContext.getScopes();
[...]
if (previousConsent != null) {
	authorizedScopes.addAll(previousConsent.getScopes());
}
[...]
OAuth2Authorization authorization = OAuth2Authorization.from(userConsentRequestContext.getAuthorization())
		.token(authorizationCode)
		.attributes(attrs -> {
			attrs.remove(OAuth2ParameterNames.STATE);
			attrs.put(OAuth2Authorization.AUTHORIZED_SCOPE_ATTRIBUTE_NAME, authorizedScopes);
		})
		.build();

(code is here)

The information is duplicated, but it works and avoid re-loading the consents when issuing a token. I think I like it as is, that is the ConsentService is only used at authorization time, to determine:

• if consent is required
• which scopes should we ask consent for

---

However, I believe you have found a bug in the current implementation! Imagine we have previously approved scopes:

• banana.eat
• milk.drink

And the authorization request has the following requested scopes:

• apple.eat
• banana.eat

If the user consents to apple.eat, I believe the token will have:

• apple.eat
• banana.eat
• milk.drink -> 💥 the authorization request never asked for this scope

[Kehrlann]

Fixed the issue, ready for review.

Re: ProviderSettings, as discussed, I'm leaving userConsentUri out.

[jgrandja]

Thanks for all the updates @Kehrlann ! This is now in main.