Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rich Authorization Requests (RAR) #266

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

guymoyo
Copy link

@guymoyo guymoyo commented Apr 21, 2021

No description provided.

@guymoyo guymoyo changed the title draft RAR Rich Authorization Requests (RAR) design draft Apr 21, 2021
Copy link
Contributor

@tnorimat tnorimat left a comment

Choose a reason for hiding this comment

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

@guymoyo I've read this PR and had not any comment. I agree with your idea of processing authorization details by SPI provider implementation.

@guymoyo guymoyo marked this pull request as ready for review May 14, 2021 09:30
Copy link

@thomasdarimont thomasdarimont left a comment

Choose a reason for hiding this comment

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

Thanks @guymoyo for your proposal, this looks promising :)

I just have a few remarks inline.

design/rich-authorization-requests.md Show resolved Hide resolved
design/rich-authorization-requests.md Show resolved Hide resolved
design/rich-authorization-requests.md Outdated Show resolved Hide resolved
@guymoyo
Copy link
Author

guymoyo commented Aug 5, 2021

Thanks @guymoyo for your proposal, this looks promising :)

I just have a few remarks inline.

Thanks @thomasdarimont, I will check your remarks.

@guymoyo guymoyo changed the title Rich Authorization Requests (RAR) design draft Rich Authorization Requests (RAR) Aug 7, 2021
@VinodAnandan
Copy link

Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

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

Finally added review to this specification. Sorry for being late...

I've added few points inline and marking this as "Request changes" until these are discussed and addressed somehow.

One general note: There is other specification FAPI Lodging Intent https://bitbucket.org/openid/fapi/src/master/Financial_API_Lodging_Intent.md . It seems to me that specification tries to address very similar problem like this RAR specification (Insufficient capabilities of the OAuth2 scope parameter and the ways how to overcome those limited capabilities). Will be probably good to also take the other specification into account and makes sure that they are somehow compatible with each other. The specifications are both drafts and both have same/similar authors, so it is probably not two completely independent efforts :-)

It seems to me that some intersection for these specifications is for example generating the consent screen (as it seems that RAR may also need some capabilities on the consent screen).

But I don't have any concrete ideas for now, so let's first do RAR and then lodging intent or vice-versa. Keycloak may soon need "dynamic scopes" feature, which may cause some intersections with this, but let's follow-up later to see what exactly is needed and if there are any conflicts between those capabilities.

design/rich-authorization-requests.md Show resolved Hide resolved
design/rich-authorization-requests.md Show resolved Hide resolved
design/rich-authorization-requests.md Show resolved Hide resolved
design/rich-authorization-requests.md Show resolved Hide resolved
design/rich-authorization-requests.md Show resolved Hide resolved
design/rich-authorization-requests.md Outdated Show resolved Hide resolved
design/rich-authorization-requests.md Outdated Show resolved Hide resolved
design/rich-authorization-requests.md Outdated Show resolved Hide resolved
@tnorimat
Copy link
Contributor

@mposolda
For #266 (review) , OIDF's FAPI site mentions that FAPI 1.0 lodged intent be replaced with OAuth PAR + OAuth RAR. Therefore, at this time, it might be OK for us to focus only on supporting RAR.

@tnorimat
Copy link
Contributor

tnorimat commented Sep 22, 2021

@guymoyo , @mposolda , @thomasdarimont ,
As an design document, it seems to be OK to be merged after the current review comments are resolved.

IMO, when implementing it, the following points might be difficult to handle.

  • showing an consent screen with respect to "authroization_details"
  • enriching "authroization_details" on UI
  • managing consents with respect to "authroization_details"

The last point is also related to dynamic scopes and FAPI 2.0 Grant Management for OAuth 2.0.

I think the current keycloak's consent model may need to be modified.

Also, on implementation, I think keycloak need not include any concrete RAR processor provider because it is highly dependent on specific use-cases. However, arquillian integration tests need to include some concrete RAR processor.

@mposolda
Copy link
Contributor

@guymoyo , @mposolda , @thomasdarimont , As an design document, it seems to be OK to be merged after the current review comments are resolved.

IMO, when implementing it, the following points might be difficult to handle.

* showing an consent screen with respect to "authroization_details"

* enriching "authroization_details" on UI

* managing consents with respect to "authroization_details"

The last point is also related to dynamic scopes and FAPI 2.0 Grant Management for OAuth 2.0.

I think the current keycloak's consent model may need to be modified.

Also, on implementation, I think keycloak need not include any concrete RAR processor provider because it is highly dependent on specific use-cases. However, arquillian integration tests need to include some concrete RAR processor.

@tnorimat These are very good points. It will be good to consider RAR as a more powerful alternative to classic "scope" parameter, however dynamic scopes have lots of common things with RAR.

It turns that Keycloak team has some internal requirements and it looks that it may need to prioritize the work on dynamic scopes soon. There are some use-cases to dynamic scopes discussed here keycloak/keycloak#8486 and some other discussion started here keycloak/keycloak#8488 (comment) .

Overally, we will need to figure some way how to do RAR and dynamic scopes together and make sure that whatever is done will match the requirements for both these use-cases.

For now, I am not 100% sure how to move forward. I suggest to anyone to comment on the mentioned discussions for dynamic scopes and eventually add feedback to them.

@stianst
Copy link
Contributor

stianst commented Sep 29, 2021

I haven't had time to read through this proposal yet, but I've come to the conclusion that we must not extend "scopes" without considering RAR, and at the same time we must not implement RAR as a separate thing. The two have very similar needs: a) client includes some additional details in authz request, b) some policies decide if it's permitted or not, c) the user optionally consents to some stuff, and d) some claims are added to tokens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants