Skip to content

scope parameter in refresh flow#12043

Merged
mposolda merged 1 commit intokeycloak:mainfrom
eosc-kc:12009-scope-refreshflow
Dec 20, 2023
Merged

scope parameter in refresh flow#12043
mposolda merged 1 commit intokeycloak:mainfrom
eosc-kc:12009-scope-refreshflow

Conversation

@cgeorgilakis
Copy link
Copy Markdown
Contributor

Closes #12009

@cgeorgilakis
Copy link
Copy Markdown
Contributor Author

Moreover, UserInfo must be restricted based on Access Token scopes. For example access token produced by refresh flow with scope parameter.
I have implemented it by filtering default Client scopes based on access token scope. If you have another idea , we could discuss it.

@cgeorgilakis cgeorgilakis force-pushed the 12009-scope-refreshflow branch 4 times, most recently from 2075110 to 42804c5 Compare October 21, 2022 08:44
@cgeorgilakis
Copy link
Copy Markdown
Contributor Author

I have rebase it to main.

I would like to mention that based on documentation only access token scopes are filtering based on scope parameter. Refresh token scope - if must be returned - are not changed.

Could you review the PR in order Keycloak to be compliant with OAuth refresh token flow?

Copy link
Copy Markdown
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.

@cgeorgilakis Sorry for late response. Thanks for this PR! Added few minor comments inline.

Could you please also rebase this PR on top of latest main? This should also trigger the GH actions build.

Comment thread services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java Outdated
Comment thread services/src/main/java/org/keycloak/protocol/oidc/endpoints/UserInfoEndpoint.java Outdated
@mposolda mposolda self-assigned this Dec 11, 2023
@cgeorgilakis
Copy link
Copy Markdown
Contributor Author

@cgeorgilakis Sorry for late response. Thanks for this PR! Added few minor comments inline.

Could you please also rebase this PR on top of latest main? This should also trigger the GH actions build.

I will make corrections and rebase. I will only do it next week due to conference participation.

Copy link
Copy Markdown
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.

@cgeorgilakis Thanks, much better now. I've added 2 minor comments regarding tests. One is follow-up of your previous change. Second is something I overlooked before...

There is also DCO check failing in your PR. This is new check added recently to Keycloak due CNCF requirements about adding signatures to the commits. See https://github.com/keycloak/keycloak/blob/main/CONTRIBUTING.md#developers-certificate-of-origin . Is it possible to squash commits in this PR and make sure that your signature is added?

@cgeorgilakis
Copy link
Copy Markdown
Contributor Author

@cgeorgilakis Thanks, much better now. I've added 2 minor comments regarding tests. One is follow-up of your previous change. Second is something I overlooked before...

There is also DCO check failing in your PR. This is new check added recently to Keycloak due CNCF requirements about adding signatures to the commits. See https://github.com/keycloak/keycloak/blob/main/CONTRIBUTING.md#developers-certificate-of-origin . Is it possible to squash commits in this PR and make sure that your signature is added?

Of course, I will do it before merging. Could you see my last changes and comments before proccessing to this?

Copy link
Copy Markdown
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.

@cgeorgilakis Thanks! I see your update in RefreshTokenTest.

So it looks that remaining points are to update OfflineTokenTest and fix DCO and this PR is good to go IMO.

@mposolda
Copy link
Copy Markdown
Contributor

@cgeorgilakis Thanks for your latest changes. It seems that if you squash the commits and add the signature to fix DCO, we should be good.

Closes keycloak#12009

Signed-off-by: cgeorgilakis-grnet <[email protected]>
@cgeorgilakis cgeorgilakis force-pushed the 12009-scope-refreshflow branch from 182d141 to 87c9549 Compare December 20, 2023 08:24
@cgeorgilakis
Copy link
Copy Markdown
Contributor Author

@cgeorgilakis Thanks for your latest changes. It seems that if you squash the commits and add the signature to fix DCO, we should be good.

Done.

Copy link
Copy Markdown
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.

@cgeorgilakis Thanks! I hope to merge once tests are ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for scope parameter in the refresh flow

2 participants