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

Update Keycloak Authorization docs and UserInfo #13659

Merged
merged 1 commit into from Dec 3, 2020

Conversation

sberyozkin
Copy link
Member

Fixes #13652

Also a user reported that it was impossible to get a list of all the UserInfo properties

@sberyozkin sberyozkin added this to the 1.11 - master milestone Dec 3, 2020
@ghost ghost added the area/documentation label Dec 3, 2020
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -40,7 +42,9 @@ public String userNameService(@PathParam("tenant") String tenant) {
String name = getNameServiceType();
if ("tenant-d".equals(tenant) || "tenant-b-no-discovery".equals(tenant)) {
UserInfo userInfo = getUserInfo();
name = name + "." + userInfo.getString("preferred_username");
if (userInfo.getPropertyNames().contains(PREFERRED_USERNAME)) {
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 returning the property names via a getter, isn't better to introduce a UserInfo.contains(String) instead?

Copy link
Member Author

@sberyozkin sberyozkin Dec 3, 2020

Choose a reason for hiding this comment

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

@gastaldi the user just wanted to see the whole set of property names as he was not sure what is in that UserInfo, here I just added this awkward check to test the new method :-), but yeah, let me add a shortcut as well, makes sense

Copy link
Member Author

Choose a reason for hiding this comment

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

@gastaldi Just updated it, kept the getPropertyNames test check but also added contains check

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, thanks!

@sberyozkin
Copy link
Member Author

@gastaldi Only MP TCK tests have stalled which is not related; I'll just go ahead with merging it to have a feeling I've done at least something today :-)

@sberyozkin sberyozkin merged commit cdbf49a into quarkusio:master Dec 3, 2020
@sberyozkin sberyozkin deleted the minor_oidc_fixes branch December 3, 2020 17:55
Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

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

@sberyozkin I've found an issue in these changes

@@ -318,7 +318,7 @@ public class ProtectedResource {
@GET
public CompletionStage<List<Permission>> get() {
return identity.checkPermission(new AuthPermission("{resource_name}"))
.thenCompose(granted -> {
.transform(granted -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing onItem() before calling .transform(.

Copy link
Member Author

Choose a reason for hiding this comment

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

Going to update it shortly

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.

Outdated section "Checking Permissions Programmatically" in security-keycloak-authorization guide
3 participants