Extends the conditional user attribute authenticator to check the att…#20189
Extends the conditional user attribute authenticator to check the att…#20189mposolda merged 14 commits intokeycloak:mainfrom
Conversation
…ributes of the joined groups. Also includes a small change to the documentation to clarify this behavior. Closes keycloak#20007
|
|
||
| `Condition - User Attribute`:: | ||
| This checks if the user has set up the required attribute. | ||
| This checks if the user or any of the joined groups have set up the required attribute. |
There was a problem hiding this comment.
Shall we make clear that attributes are not inherited from parent groups but only those the user is a direct member of?
I'm not 100% sure if the attributes should always be resolved from groups in case they are unavailable from the user. It might end up with additional queries to the database even though you are not relying on attributes from groups or not using groups at all. It might happen that groups (and their attributes) are already being fetched before entering this authenticator, but I'm not sure.
There was a problem hiding this comment.
@pedroigor Maybe we can add additional option to this authenticator like Allow group attributes - When false (the default), the behaviour would be the same like today (so not checking groups). When true, it can also check attributes from groups (the tooltip should have the info about the fact that using group attributes can have some performance impact). WDYT?
|
@dmartinol Added one comment inline in addition to Pedro's comment. Also the automated test is missing (for example |
@mposolda I tried both the default undertow and the auth-server-quarkus profile but both fail with some mismatch with dependency classes/interfaces. E.g. with auth-server-quarkus profile I get:
Any hints? |
|
@dmartinol The easiest way to test mvn clean install -DskipTests
cd testsuite/integration-arquillian/tests/base/
mvn clean test -Dtest=org.keycloak.testsuite.forms.BrowserFlowTest
# or
mvn clean test -Pauth-server-quarkus -Dtest=org.keycloak.testsuite.forms.BrowserFlowTest
# for a specific test
mvn clean test -Dtest=org.keycloak.testsuite.forms.BrowserFlowTest#testFlowDisabledWhenConditionalAuthenticatorIsDisabledUsually the non-quarkus test is easier as it picks the deps without requiring more sub-projects (just As @mposolda commented I would try to add an extra option to the conditional, this is very similar to Regards! |
…e evaluation of group attributes
Extends the conditional user attribute authenticator to check the att… keycloak#20189
|
@mposolda, all |
|
@pedroigor @mposolda anything else is needed? |
pedroigor
left a comment
There was a problem hiding this comment.
@dmartinol @mposolda LGTM.
|
@mposolda Anything else? |
|
@dmartinol @pedroigor Thanks for the update and review! |
Extends the conditional user attribute authenticator to check the attributes of the joined groups.
Also includes a small change to the documentation to clarify this behavior.
Closes #20007