Grant Management by guymoyo · Pull Request #23 · tnorimat/keycloak

guymoyo · 2021-05-20T17:22:57Z

In the scope of this PR, I would like to show the conception of Grant Management.
Please note, it's not totally complete. If the conception is correct (I will go on then), if there are some advice or propositions - feel free to comment. Thank you!

Draft design: keycloak/keycloak-community#265

spec: https://openid.net/specs/fapi-grant-management-01.html

tnorimat · 2021-05-20T23:54:10Z

Hello @guymoyo ,
Thank you for your implementation. I will review it and add comments afterwards.

tnorimat

@guymoyo I've reviewed this PR and add some review comments. Could you check them?

…Provider

guymoyo · 2021-06-16T11:44:04Z

Hi @tnorimat, I made some changes. I added the possibility to select 'grant_id_supported' in the admin UI, also I changed the logic of this grant flow, IMO I think it makes more sense to go with 'RequiredActionProvider', WDYT?

There is some cross logic with the RAR, you will see that as comments in code.

tnorimat · 2021-06-19T01:06:06Z

Hello @guymoyo ,

As you've proposed, it is good to use Required Action instead of coding onto login service flow.

One point I would like to notice is as follows.

As discussed previously, to implement newly introduced User Grant, the following relationship is assumed.

Consent 1 --- 1 Grant

The current keycloak's Consent has the following relationship.

{Client, User} 1 --- 1 Consent

Therefore, the following relationship is newly introduced.

{Client, User} 1 --- 1 Consent 1 --- 1 Grant

I'm afraid that what you want to do by Grant Management is achieved under this relationship.

guymoyo · 2021-06-21T11:45:51Z

Hello @tnorimat,
Yes, you're right, consent is an agreement between a User and a Client. And the grant will extend that with fine-grained authorization. Therefore when the consent is canceled, all it's extended should be cancel/delete.

I would like to try this:

grant representation

` {

	"grantId" : ""

	"consentId" : "" // should be not null

	"clientId" : ""  // for quick access

	"userId" : ""   // for quick access

	"authorization_details" : {}

	"scopes" : []   // this scopes should be less than consent->grantedClientScopes

	"claims" : []   // same for the claims

	"createdDate" ""

	"lastUpdatedDate" ""
}`

if we delete a consent, all grants with that consent will be deleted.

When creating the grant, if there is not yet consent between that Client and that User, it will be created implicitly.

tnorimat · 2021-06-22T05:00:13Z

Hello @guymoyo ,

Thank you for your clarification.

guymoyo · 2021-06-28T13:05:52Z

TODO:

Incorporates the updates from the latest grant management publication (23.06.01)
Avoid modifying RealmModel directly and follow CibaConfig for the spec param
GrantEndpoint should follow --> CibaRootEndpoint concept
Code redundancies for checking should be removed and make usage of this pull request
Make grant and consent working like described in a comment upper
Remove unnecessary code, space ...
Write test
write documentation

Hi @tnorimat, here some tasks that I will continue working on, please feel free to add anything pertinent.

# Conflicts: # core/src/main/java/org/keycloak/protocol/oidc/representations/OIDCConfigurationRepresentation.java # core/src/main/java/org/keycloak/representations/AccessTokenResponse.java # core/src/main/java/org/keycloak/representations/idm/RealmRepresentation.java # model/jpa/src/main/resources/META-INF/jpa-changelog-13.0.0.xml # server-spi-private/src/main/java/org/keycloak/events/Errors.java # server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java # server-spi/src/main/java/org/keycloak/models/UserModel.java # services/src/main/java/org/keycloak/protocol/oidc/OIDCConfigAttributes.java # services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java # services/src/main/java/org/keycloak/protocol/oidc/OIDCWellKnownProvider.java # services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java # services/src/main/resources/META-INF/services/org.keycloak.authentication.RequiredActionFactory

guymoyo · 2021-07-14T18:59:11Z

Hi @tnorimat, I made some refactoring, can you please check again?

tnorimat · 2021-07-14T21:39:10Z

Hello @guymoyo ,
Yes, I will check your revised codes.

tnorimat · 2021-07-15T06:55:28Z

@guymoyo I will check them completely, but I have one suggestion at this time.

As the same as PAR and CIBA, it might be better for this grant management to be provided as preview feature.

WDYT?

guymoyo · 2021-07-15T07:20:08Z

@guymoyo I will check them completely, but I have one suggestion at this time.

As the same as PAR and CIBA, it might be better for this grant management to be provided as preview feature.

WDYT?

Yes, you're right, I forgot to change. I will change.

guymoyo · 2021-07-15T07:27:08Z

+     *
+     * @return
+     */
+    @Path("query")


@tnorimat, the request to query a grant should be something like this

https://as.example.com/grants/TSdqirmAxDa0_-DB_1bASQ

and not

https://as.example.com/grants/query/TSdqirmAxDa0_-DB_1bASQ

but following the CIBA endpoint concept, I have no choice to give a path here.
do you have an idea how can we solve that?

I'm not sure but does the following work?

@Path("/{grant_id}") @GET

no, it doesn’t work.

I see, I will find other solution.

guymoyo · 2021-07-15T07:28:25Z

+     *
+     * @return
+     */
+    @Path("revoke")


same as @Path("query")

I'm not sure but does the following work?

@Path("/{grant_id}") @DELETE

same, it doesn’t work.

I see, I will find other solution.

tnorimat

@guymoyo I've added some review comments. Could you check them?

guymoyo · 2021-07-19T11:16:16Z

Hi @tnorimat, I made the changes but still having this problem with the path of the grant management endpoint.

tnorimat · 2021-07-20T21:44:36Z

@guymoyo As I added comments, I will find other solution for the final issue.

guymoyo · 2021-09-08T08:25:49Z

Hi @tnorimat, may this help to have a good overview on Relation Grant vs Consent

tnorimat · 2021-09-08T11:17:00Z

Hello @guymoyo ,

Thank you for your reference https://bitbucket.org/openid/fapi/issues/442/grant-vs-consent-confusion ,
I will follow this discussion thread. I've looked it at a glance and found that it might be needed to discuss the definition of Consent and Grant by considering their nature and the relationship between them.

From your implementer's side, is it better to have relationship Consent 1 --- * Grant ?

guymoyo · 2021-09-08T14:49:42Z

Hello @guymoyo ,

Thank you for your reference https://bitbucket.org/openid/fapi/issues/442/grant-vs-consent-confusion ,
I will follow this discussion thread. I've looked it at a glance and found that it might be needed to discuss the definition of Consent and Grant by considering their nature and the relationship between them.

From your implementer's side, is it better to have relationship Consent 1 --- * Grant ?

Yes from the implementer's side, I think it is better to go in this way and it is actually implemented like that in this branch. but it can be changed if we have another clear relation view.

test: adding unit tests for flat, structured and recursive disclosure

guymoyo changed the title ~~grant management initial commit~~ Grant Management May 20, 2021

guymoyo commented May 20, 2021

View reviewed changes

Comment thread model/jpa/src/main/java/org/keycloak/grant/jpa/impl/GrantServiceImpl.java Outdated

tnorimat self-requested a review May 20, 2021 23:53

tnorimat reviewed May 22, 2021

View reviewed changes

guymoyo force-pushed the grant_management branch 2 times, most recently from 62ae006 to a6f786e Compare June 2, 2021 13:37

grant management

3eebd37

guymoyo force-pushed the grant_management branch from a6f786e to 3eebd37 Compare June 2, 2021 13:45

gmo added 3 commits June 2, 2021 22:31

revoke userssesion

9a6b5f0

add creation date to grantModel

cd95038

Enable grant supported in admin ui. Grant consent as a RequiredAction…

ddef15c

…Provider

remove stringUtils, add consentId to grant

a40fc66

guymoyo force-pushed the grant_management branch from b7d62c7 to a40fc66 Compare July 6, 2021 18:35

gmo added 9 commits July 6, 2021 20:44

fix MapRealmAdapter

dd08542

review

513d139

review

cae9104

review

525dccd

fix grant endpoint

ee91205

fix grant endpoint

26ea987

refactor

df4afbd

remove space

091dfca

Grant profile default to preview mode

774522a

guymoyo commented Jul 15, 2021

View reviewed changes

Comment thread services/src/main/java/org/keycloak/authentication/requiredactions/GrantManagement.java

guymoyo commented Jul 15, 2021

View reviewed changes

Comment thread services/src/main/java/org/keycloak/authentication/requiredactions/GrantManagement.java

guymoyo commented Jul 15, 2021

View reviewed changes

tnorimat reviewed Jul 18, 2021

View reviewed changes

gmo added 2 commits July 19, 2021 13:01

review

d2f3d22

review

7422eba

tnorimat pushed a commit that referenced this pull request Feb 11, 2024

Merge pull request #23 from adorsys/issue-25638-14-15-16

e5085b9

test: adding unit tests for flat, structured and recursive disclosure

Conversation

guymoyo commented May 20, 2021

Uh oh!

Uh oh!

tnorimat commented May 20, 2021

Uh oh!

tnorimat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guymoyo commented Jun 16, 2021

Uh oh!

tnorimat commented Jun 19, 2021

Uh oh!

guymoyo commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnorimat commented Jun 22, 2021

Uh oh!

guymoyo commented Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guymoyo commented Jul 14, 2021

Uh oh!

tnorimat commented Jul 14, 2021

Uh oh!

tnorimat commented Jul 15, 2021

Uh oh!

guymoyo commented Jul 15, 2021

Uh oh!

Uh oh!

Uh oh!

guymoyo Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnorimat Jul 18, 2021

Choose a reason for hiding this comment

Uh oh!

guymoyo Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

tnorimat Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

guymoyo Jul 15, 2021

Choose a reason for hiding this comment

Uh oh!

tnorimat Jul 18, 2021

Choose a reason for hiding this comment

Uh oh!

guymoyo Jul 19, 2021

Choose a reason for hiding this comment

Uh oh!

tnorimat Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

tnorimat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guymoyo commented Jul 19, 2021

guymoyo commented Jun 21, 2021 •

edited

Loading

guymoyo commented Jun 28, 2021 •

edited

Loading

guymoyo Jul 15, 2021 •

edited

Loading