Auth code flow update to fix issue #70 #86
Conversation
jpengar
requested review from
diegogonmar,
chrishowell,
bigludo7 and
trehman-gsma
|
Please reviewers, may I ask for your review of this PR to apply the same change as in the GSMA doc, if there are no other objections? |
| ExpO-->>FE: 302<br>Location: invoker_callback?code=Operatorcode | ||
| end | ||
| end | ||
| Note over FE,ExpO: Consent capture |
There was a problem hiding this comment.
The description of the process as "Consent Capture" is ambiguous here. It could be interpreted to mean "wait until user consent is obtained before issuing the operatorCode", but that is not what is intended here. A fuller description, such as "Start asynchronous end user consent capture process" would be better.
There was a problem hiding this comment.
@eric-murray Thank you for your comment! But actually I don't see how it applies to Auth code flow. In the case of Auth code flow, the consent capture is not an asynchronous process like in, say, CIBA. In fact, in the case of Auth code flow, the application is redirected to a "consent capture page" where the user can browse and grant or revoke consent for the requested resources.
And what is particularly clarified in this PR is that if the user doesn't consent to the access (where explicit consent is required by the legal basis), the code is still provided in the GET /authorize response.
There was a problem hiding this comment.
Hi @jpengar
It was precisely because an authorisation code is returned whether or not the end user consents that I had assumed the "Consent Capture" did not need to be synchronous.
Let's see what the OpenId Connect core specification has to say about this:
Once the End-User is authenticated, the Authorization Server MUST obtain an authorization decision before releasing information to the Relying Party. When permitted by the request parameters used, this MAY be done through an interactive dialogue with the End-User that makes it clear what is being consented to or by establishing consent via conditions for processing the request or other means (for example, via previous administrative consent). Sections 2 and 5.3 describe information release mechanisms.
So "an interactive dialogue" is but one option for OpenId Connect. But I now understand that CAMARA are mandating this approach, and that "Consent Capture" consequently must be synchronous?
This is an important point of difference from the OpenId Connect specifications which needs to be properly documented by CAMARA. Specifying this as "Consent Capture" is insufficient.
There was a problem hiding this comment.
@eric-murray To date, the agreed upon and defined Auth code flow captures user consent (when required) via an interactive browsing dialog to which the user is redirected (as documented).
Also, there is nothing preventing an application from executing the Auth code flow at any time, not necessarily at the same time the network resource (CAMARA API) is consumed. So, for example, consent could be obtained at the time of user onboarding.
In any case, this topic is not related to the specific change being made is this PR. "Consent Capture" was also specified before this change. So please, let's not block this PR change because of this parallel discussion. Would you please open a new issue and contribute with your documentation improvement suggestion to avoid confusion?
There was a problem hiding this comment.
@jpengar
The other changes have changed the meaning of "Consent capture" here. Previously the rule was that, if the user had not consented and the "Consent capture" process was then started, the OperatorCode would be returned "when user grants consent / otherwise flow fails".
In other words, if end user consent could not be obtained (for whatever reason), then the flow could be terminated with an appropriate error.
But now - it appears the OperatorCode must always be returned once the end user identifier is validated, but only once "Consent capture" is complete. Subsequent text states:
- Once the user has given consent, the authorization code flow continues by redirecting to the API invoker redirect_uri (invoker_callback) and including the authorization code (OperatorCode).
Previously, if the end user did not consent, that could be interpreted as "otherwise flow fails". But now the flow cannot fail. In that case, the above text could now be interpreted as we must wait until we know whether the end user has consented or has refused consent, and then proceed to return the OperatorCode.
But what if the end user does nothing? Or if prompt=none (as it will be for Number Verification)? Or they are using a device which cannot support interactive dialogues and a slower method of consent capture must be initiated? We just ..... wait? This cannot be the intention. Asynchronous consent capture must be an option if the /authorize request now cannot fail because consent cannot be obtained in good time.
Whether intentional or not, the proposed changes appear to remove the option to terminate the flow in the event that end user consent cannot be completed synchronously, without allowing it to be completed asynchronously. I would suggest to clarify the "Consent capture" process by rewording this to "Start end user consent capture process following Section 3.1.2.4 of the OpenID Connect Core 1.0 specification".
There was a problem hiding this comment.
I would suggest to clarify the "Consent capture" process by rewording this to "Start end user consent capture process following Section 3.1.2.4 of the OpenID Connect Core 1.0 specification".
@eric-murray I'm fine with your rewording suggestion. Added in commit 1ead9a5
There was a problem hiding this comment.
I am NOT OK with this PR because I think that (10)-(12) are not needed in https://github.com/camaraproject/IdentityAndConsentManagement/blob/75a2ee2ac4e30e4f07783f7059941ed46a83e257/documentation/CAMARA-API-access-and-user-consent.md ?
I think that user authentication and consent collection happen in "[Standard OIDC Auth Code Flow between Invoker and API Exposure Platform]" (7) - (8).
If consent is not given then the AZ does not return a code but OIDC error.
Having a code means "The AZ is OK with exchanging the code for an access token". No further consent checks are needed on the token endpoint.
Please remove the "alt" section at the token-endpoint.
ps: We should always have PKCE
| FE-->>-BE: GET invoker_callback?code=OperatorCode | ||
| BE->>ExpO: POST /token<br> code=OperatorCode | ||
| ExpO->>BE: 200 OK <br> {OperatorAccessToken} | ||
| alt If Consent is Granted or Consent not needed for legal basis |
There was a problem hiding this comment.
I am NOT OK with this PR because I think that (10)-(12) are not needed in https://github.com/camaraproject/IdentityAndConsentManagement/blob/75a2ee2ac4e30e4f07783f7059941ed46a83e257/documentation/CAMARA-API-access-and-user-consent.md ?
I think that user authentication and consent collection happen in "[Standard OIDC Auth Code Flow between Invoker and API Exposure Platform]" (7) - (8).
If consent is not given then the AZ does not return a code but OIDC error.
Having a code means "The AZ is OK with exchanging the code for an access token". No further consent checks are needed on the token endpoint.
There was a problem hiding this comment.
@AxelNennker I understand that you might disagree. But the documentation update is only intended to document in CAMARA the solution already agreed and discussed in GSMA to address the front-end flow PII leakage issue originally raised.
This is the problem of overlapping docs in GSMA and CAMARA as raised by @bigludo7 in issue #82. But for now, we would need to keep the documentation in sync in both places, GSMA & CAMARA, when it applies. And just moving this agreed change from GSMA to CAMARA is already taking weeks.
There was a problem hiding this comment.
Why should Camara copy something here that deviates from the OIDC standard?
OIDC is defined so that when you have the code all requirements to get the access token were checked - the user was authenticated and gave consent. If the user failed to authenticate or did not give consent then there is no code. So, there is no "alt"-section in OIDC when the code is being exchanged for the access token.
As I understand this, we/Camara would deviate from the OIDC standard.
Sorry, that GSMA OGW discussed and agreed upon something that is not acceptable.
I would not do this even if we see this as an intermediate step.
This PR is the place where we can and should fix this.
There was a problem hiding this comment.
I'd like to amend my comment. My interpretation is based on OIDC core which handles user authentication and consent collection, and OIDC does not discuss the case where the user declines consent and still some other scopes are allowed by an access token.
I think that users are going to be surprised when the client uses an API that they "think" they declined. Surprises are harmful for the public perception of Camara business.
GDPR requires, that "consent must be freely given, specific, informed and unambiguous". If a user declines consent and we create an access token afterwards anyway, then they will question all four points.
So, I think a privacy respecting flow would be to ask for consent and create an access token if consent is granted.
And ask for a separate access token using client credentials flow for those APIs that do not require consent.
But my opinion that this flow violates OIDC was too strong. OIDC is a profile of OAuth2 and in OAuth2 it is OK that the AZ issues an access token for a subset of the requested scopes.
I am glad that we discussed this and that we did not just accept this PR because it was discussed and agreed upon somewhere else.
|
As per above last comments/reviews and our discussions in last WG meeting (20/12), can we then merge this PR? @eric-murray your suggestion is already added and it also has the approval of Orange and DT representatives. If there are no further objections, I will merge it this week after a reasonable time. |
What type of PR is this?
What this PR does / why we need it:
Auth code flow update. Fixes #70
Adds further clarification for scenarios where user consent is not given.
I'm afraid the flow is not specific enough. It says "otherwise the flow fails", but does not specify how. The scenario raised in issue #70 may not be 100% clear.
Actually, if the user doesn't consent to the access (where explicit consent is required by the legal basis), the
codeis still provided in the GET /authorize response. And then in the POST /token operation (where client_id is authenticated) is where the flow will return an error if there is no consent for the requested scopes.Which issue(s) this PR fixes:
Fixes #70
Special notes for reviewers:
This PR replicates the changes made in the GSMA documentation https://github.com/GSMA-Open-Gateway/Open-Gateway-Documents/pull/61, which fixed https://github.com/GSMA-Open-Gateway/Open-Gateway-Documents/issues/53. Issue #70 was a clone of GSMA Issue 53.
As agreed, issue #70 was discussed in the context of the GSMA technical stream. And if something needs to be piggybacked into CAMARA, it will be raised later for everyone in CAMARA to review. As a result, this PR replicates the same changes that were merged into the GSMA documentation.
I'm aware that this doc overlap and "duplicate" update is a good example of what @bigludo7 raised in issue #82. But for now, we would need to keep documentation in sync in both places, GSMA & CAMARA, when it applies.
Changelog input
N/A
Additional documentation
N/A