+
Skip to content

Conversation

patrick-primesign
Copy link
Contributor

@patrick-primesign patrick-primesign commented Nov 24, 2023

Add functions and set existing functions to protected in BackchannelAuthenticationCallbackEndpoint to allow developers easier customization when overriding.

Two new methods added when approving device code, one before and one after approving, to allow additional checks.

These changes doesn't change the default behaviour if the class is not overridden.

Closes #24992

@patrick-primesign patrick-primesign requested a review from a team as a code owner November 24, 2023 08:10
Copy link
Contributor

Choose a reason for hiding this comment

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

@patrick-primesign - while I leave the final comment for the core team, here is something I noticed:

You make this protected so a derived class can access it. At the same time you're passing it in to some methods. Can we be consistent to only pass it to methods, as I would assume it would make the code simpler to follow?

As you've been adding protected methods / making methods protected, it might be good to add some Javadoc to them explaining how they are intended to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahus1 yeah, I agree, we needed it in our implementation for getRawBearerToken(), which normally would use the http headers.
I will add response and headers as parameters getRawBearerToken, to make it more readable or consistent.
Javadoc I will also add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed some changes to the commit.

@patrick-primesign patrick-primesign force-pushed the fb-KEYCLOAK-546-preapprove branch 2 times, most recently from 44bca59 to 4a28f6c Compare November 24, 2023 11:35
Copy link
Contributor

@ahus1 ahus1 left a comment

Choose a reason for hiding this comment

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

Looking at the code, the changes still look a bit too much. From the Javadocs it is still hard to see the intent when to override which method.

It would be good if you would share more information about your usecase / the class you extend, plus maybe a configuration hint how you're configuring Keycloak to use the other endpoint instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

The logic is hard to understand as denyRequest() might have already been called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually we don't use this, so I will remove it, as the only reason it is there is that we possibly could use it. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of wrapping this here, I'd suggest only to change the line above:

Status status = extractStatusFromResponse(response);

... and then someone can override the one method extractStatusFromResponse.

Or thinking one step further, add the status to the BackchannelAuthCallbackContext, and it can be part of the extendable verifyAuthenticationRequest(). If doing that, the rest of the code can be left as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For our use case this unfortunately doesn't make sense. We don't verify the request itself, we have to check other things (from the database), e.g. our own timeout and other factors which will turn the status to UNAUTHORIZED if they are not met.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems legit. The method extractStatusFromResponse should extract some kind of status from the response. But Keycloak might have a saying as well, without checking on any status from the response.

@mposolda mposolda self-assigned this Nov 24, 2023
@patrick-primesign patrick-primesign force-pushed the fb-KEYCLOAK-546-preapprove branch from 4a28f6c to 8989417 Compare November 27, 2023 08:48
@mposolda
Copy link
Contributor

@patrick-primesign @mschallar Could you please rebase this PR? I hope we can possibly merge it once it is rebased and all the tests are OK.

…uthenticationCallbackEndpoint to allow developers easier customization when overriding.

Two new methods added when approving device code, one before and one after approving, to allow additional checks.

Closes keycloak#24992

Signed-off-by: Patrick Weiner <patrick.weiner@prime-sign.com>
@patrick-primesign patrick-primesign force-pushed the fb-KEYCLOAK-546-preapprove branch from 8989417 to 03997fe Compare December 17, 2024 09:09
@patrick-primesign
Copy link
Contributor Author

patrick-primesign commented Dec 17, 2024

@mposolda Done. Thanks for merging.

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.

@mposolda mposolda merged commit 2eae680 into keycloak:main Dec 20, 2024
77 checks passed
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.

Allow more extensive Override of BackchannelAuthenticationCallbackEndpoint

4 participants

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载