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 adding multiple realms #15915

Merged
merged 1 commit into from Mar 23, 2021
Merged

Allow adding multiple realms #15915

merged 1 commit into from Mar 23, 2021

Conversation

ruromero
Copy link
Contributor

Signed-off-by: ruromero rromerom@redhat.com

When defining multiple realms only the first one was registered.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Thanks for this, great catch!

I added a small question inline.

builder.getValue().addRealm(realmName, realm.getValue());
public RuntimeValue<SecurityDomain.Builder> addRealm(RuntimeValue<SecurityDomain.Builder> builder, String realmName,
RuntimeValue<SecurityRealm> realm) {
return new RuntimeValue<>(builder.getValue().addRealm(realmName, realm.getValue()).build());
Copy link
Member

@gsmet gsmet Mar 22, 2021

Choose a reason for hiding this comment

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

I agree the build() call is missing but do you really need to return the builder? It seems useless to me but I might miss something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it is indeed not needed, I will remove it. The build call is enough

Copy link
Member

Choose a reason for hiding this comment

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

Cool, thanks. I squashed the two commits and force pushed. Let's wait for CI!

@gsmet gsmet added triage/waiting-for-ci Ready to merge when CI successfully finishes triage/backport? labels Mar 22, 2021
@ruromero
Copy link
Contributor Author

@gsmet please, check this last change as it was preventing me from authenticating using the DefaultRoleDecoder.
Maybe it would be better to move it to the wildfly-elytron-auth-server library.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

The second change makes sense too but please get rid of the lambdas you added.

})
.setRoleDecoder(authorizationIdentity -> CDI.current()
.select(DefaultRoleDecoder.class).get()
.decodeRoles(authorizationIdentity))
Copy link
Member

Choose a reason for hiding this comment

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

Please don't do that, we are not using lambdas in runtime code because it consumes memory.

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 didn't know. Will revert immediately

@ruromero ruromero force-pushed the multirealm branch 2 times, most recently from e2ee0e6 to edfe887 Compare March 23, 2021 07:14
public Roles decodeRoles(AuthorizationIdentity authorizationIdentity) {
return CDI.current().select(DefaultRoleDecoder.class).get().decodeRoles(authorizationIdentity);
}
});
Copy link
Member

Choose a reason for hiding this comment

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

Am I missing something or did you lose the call to build()? :)

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 am the one losing my head... sorry 😅

Signed-off-by: ruromero <rromerom@redhat.com>
@gsmet gsmet merged commit 6b79752 into quarkusio:main Mar 23, 2021
@quarkus-bot quarkus-bot bot added this to the 1.14 - main milestone Mar 23, 2021
@gsmet
Copy link
Member

gsmet commented Mar 23, 2021

Merged, thanks!

@ruromero ruromero deleted the multirealm branch March 23, 2021 12:00
@gsmet gsmet modified the milestones: 1.14 - main, 1.13.0.Final Mar 23, 2021
@ruromero
Copy link
Contributor Author

ruromero commented Apr 8, 2021

@gsmet would it be possible to backport it to 1.11? For next releases of JBPM/RHPAM we will be depending on Quarkus 1.11.
Thanks in advance

@gsmet
Copy link
Member

gsmet commented Apr 8, 2021

@ruromero sure, I already added the appropriate label as I thought it would be a good fix for 1.11.

Everything with the triage/backport-1.11? will be considered for 1.11. We might reconsider things when we have the full list but this one certainly looks like a good candidate!

@ruromero
Copy link
Contributor Author

ruromero commented Apr 8, 2021

Thanks for the clarification

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants