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

Allow mapping of UserSessionNotes into UserInfo #15370

Merged
merged 1 commit into from Mar 7, 2023

Conversation

sonOfRa
Copy link
Contributor

@sonOfRa sonOfRa commented Nov 7, 2022

Fixes #15369

Simply adds the interface as a marker, which is then picked up by OIDCAttributeMapperHelper#addIncludeInTokensConfig to add the configuration option automatically.

@sonOfRa sonOfRa marked this pull request as draft November 7, 2022 10:48
@sonOfRa sonOfRa marked this pull request as ready for review November 7, 2022 14:52
@sonOfRa sonOfRa force-pushed the usersession-userinfo branch 2 times, most recently from 6981014 to c8f643e Compare November 9, 2022 18:49
@thomasdarimont
Copy link
Contributor

This seems to be a duplicate of #15263

@mposolda
Copy link
Contributor

mposolda commented Mar 2, 2023

@thomasdarimont Thanks for pointing that!

@sonOfRa This PR may require test (see #15263 for the details). I am keeping this PR open for now. When test is added, we can merge that one PR, which will first add a test :-) Another hint: AbstractKerberosSingleRealmTest.credentialDelegationTest() uses UserSessionNote mapper under the covers. Maybe it helps to just add userInfo request into that test to make sure that claim is returned in userInfo?

@mposolda mposolda self-assigned this Mar 2, 2023
@mposolda mposolda added the missing/tests Tests are missing label Mar 2, 2023
@mposolda mposolda linked an issue Mar 2, 2023 that may be closed by this pull request
@sonOfRa sonOfRa requested review from a team as code owners March 3, 2023 15:39
@sonOfRa sonOfRa requested a review from a team March 3, 2023 15:39
@sonOfRa sonOfRa requested a review from a team as a code owner March 3, 2023 15:39
@sonOfRa
Copy link
Contributor Author

sonOfRa commented Mar 3, 2023

I added a test as an additional test in AbstractKerberosSingleRealmTest, thanks for pointing me in the right direction.

That required a bit of fiddling with the SPNEGO helper methods so I could actually get the access token as a string, rather than just the parsed object.

I also added an overload to UserSessionNoteMapper.createClaimMapper(...) which retains the old behaviour for the old method call (no adding of claims to the userinfo endpoint by default), and adds a userInfo parameter so the behaviour can be toggled.

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.

@sonOfRa Thanks! Overaly looks like you're in good direction. Would be just good to avoid introducing so much code duplicities and instead re-use as much as possible for various tests - added some inline comments regarding that.

return assertSpnegoLoginTokenResponse("kerberos-app", loginUsername, expectedUsername, password);
}

protected OAuthClient.AccessTokenResponse assertSpnegoLoginTokenResponse(String clientId, String loginUsername, String expectedUsername, String password) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

IS it possible to update existing method assertSuccessfulSpnegoLogin to use your newly added method to avoid so much code duplications?

I am thinking about something around:

 protected AccessToken assertSuccessfulSpnegoLogin(String clientId, String loginUsername, String expectedUsername, String password) throws Exception {
        OAuthClient.AccessTokenResponse tokenResponse = assertSpnegoLoginTokenResponse(.. arguments of your new method here ...;

        AccessToken token = oauth.verifyToken(tokenResponse.getAccessToken());
        Assert.assertEquals(userId, token.getSubject());
        Assert.assertEquals(expectedUsername, token.getPreferredUsername());
         return token;
}

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 got rid of that method entirely. The return value AccessToken is only used in one other test, which I adjusted by adding an addition oauth.verifyToken(response.getAccessToken()) call there.

This feels much cleaner to me, and with relatively minimal overhead of running an extra token verification in a few cases.

@@ -216,4 +213,53 @@ public void credentialDelegationTest() throws Exception {

events.clear();
}

@Test
public void credentialDelegationUserInfoTest() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use existing method credentialDelegationTest() and just add the call to userInfo to that method?

I know that you need to use AccessTokenResponse, but you can possibly just use your new method to retrieve AccessTokenResponse and then do simply:

AccessToken token = oauth.verifyToken(tokenResponse.getAccessToken());

And then you have both tokenResponse and token though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, integrated the test into the existing credentialDelegationTest()

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.

@sonOfRa That works, Thanks! I hope to merge once tests are OK

@mposolda mposolda removed the missing/tests Tests are missing label Mar 7, 2023
@mposolda mposolda merged commit 96c1cf3 into keycloak:main Mar 7, 2023
48 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
3 participants