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

ElytronPasswordIdentityProvider does not forward attributes from SecurityIdentity #44804

Closed
MelkorCC opened this issue Nov 28, 2024 · 8 comments · Fixed by #44896
Closed

ElytronPasswordIdentityProvider does not forward attributes from SecurityIdentity #44804

MelkorCC opened this issue Nov 28, 2024 · 8 comments · Fixed by #44896
Labels
area/security kind/bug Something isn't working
Milestone

Comments

@MelkorCC
Copy link

Describe the bug

In Wildfly Elytron it is possible to request additional attributes using attribute mappings. Quarkus does also enable this via the configs (example for ldap).

Quarkus SecurityIdentity interface and implementation has getAttribute and getAttributes methods available and likewise the builder used to construct it has setAttribute and setAttributes methods, however ElytronPasswordIdentityProvider does not forward the attributes from the retrieved Wildfly SecrutityIdentityto it.

Expected behavior

The attributes should be set on the builder and thus ultimately be available on the injected SecurityIdentity.

Actual behavior

The attributes are not available on injected SecurityIdentity.

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

@MelkorCC MelkorCC added the kind/bug Something isn't working label Nov 28, 2024
Copy link

quarkus-bot bot commented Nov 28, 2024

/cc @sberyozkin (security)

@geoand
Copy link
Contributor

geoand commented Nov 28, 2024

@MelkorCC as you seem to have analyzed the code, would you be willing to contribute the necessary fix?

Thanks

@MelkorCC
Copy link
Author

MelkorCC commented Nov 28, 2024

Sure, however i have two questions:

  • What about ElytronTokenIdentityProvider and ElytronTrustedIdentityProvider? They also use a Wildfly SecurityIdentity and build a Quarkus SecurityIdentity from it, so in theory it would also possible to change them to pass the attributes from the Wildfly SecurityIdentity into the QuarkusSecurityIdentity.Builder. I don't know if these two would ever have any attributes, but given that the attributes would then be set to EMPTY on the Wildfly SecurityIdentity and thus not set any attributes on the QuarkusSecurityIdentity.Builder i also don't see how it would cause any problems.

  • Do you have any recommendation with regards to test coverage? I am unsure how to test the authenticate method (and thus setting the attributes), when i did a quick look at other classes implementing the IdentityProvider interface i didn't see any tests for any of them (it is ofc possible that i missed them).

@MelkorCC
Copy link
Author

So, i looked into this a bit yesterday after work. The fix is pretty trivial, as already mentioned everything is already there and i just needed to forward it to the builder. Great.

I also figured out that the IdentityProviders are covered by the integration tests rather than unittests, but this is where my problem started. I can't run the intergration tests. Every one that i tried (even the GreetingResourceTest mentioned in CONTRIBUTING.md) gave me the same error:

Caused by: java.lang.IllegalStateException: No config found for class io.quarkus.vertx.http.runtime.HttpBuildTimeConfig

Given that i was unable to find any information about this on github i feel like i missed some very obvious step when setting up the Quarkus repo locally. But i am unable to spot the step i might be missing.

@sberyozkin
Copy link
Member

sberyozkin commented Nov 29, 2024

@MelkorCC Thanks for giving it a try, can you open a draft PR request, updating the main source only to copy those attributes, and I can help next week to add a test

@MelkorCC
Copy link
Author

MelkorCC commented Dec 2, 2024

@sberyozkin i reverted my attempts with adding the tests for an ldap setup and created the draft pr

@sberyozkin
Copy link
Member

Thanks @MelkorCC, I've commented in the PR, can give me a hint what to update, for example, in the DB user name and password entry, for the extra attributes be available, I'll then update one of the integration tests to verify the attribute(s) are copied

@MelkorCC
Copy link
Author

MelkorCC commented Dec 3, 2024

@sberyozkin thanks again for your time and work on this issue

@gsmet gsmet modified the milestones: 3.18 - main, 3.17.4 Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants