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

Conditional user attribute authenticator misfunction #14837

Open
gilvansfilho opened this issue Oct 11, 2022 · 5 comments
Open

Conditional user attribute authenticator misfunction #14837

gilvansfilho opened this issue Oct 11, 2022 · 5 comments
Labels

Comments

@gilvansfilho
Copy link
Contributor

Describe the bug

Conditional user attribute authenticator are a bit confusing.

The docs say: This checks if the user has set up the required attribute. There is a possibility to negate output, which means the user should not have the attribute. but help text says Flow is executed only if the user attribute exists and has the expected value.

So what is correct expected behavior? Once there is a config to set expected attribute value I think the way this should work is:

  • if user dont have the specified attribute the conditional should always evaluate to false regardless of the value negate checkbox
  • if user have the specified attribute AND expected value are equals of user attribute value the conditional should evaluate to true except if negate are true
  • if user have the specified attribute AND expected value are not equals of user attribute value the conditional should evaluate to false except if negate are true

However for this works properly Expected attribute value must be mandatory and today that do not is.

Following scenario illustrate better the problem:

  • "foo" as user attribute (Attribute name)
  • "" as expected value (Expected attribute value)
  • false as Negate output
  • user have "foo" attribute with "bar" value

If server admin are trying to validate if user has "foo" attribute with value equals to "" this will work as expected evaluating to false as "bar" != "" but otherwise if server admin are trying to validate only if user has "foo" attribute regardless their value this will not work as expected (expected true but evaluated to false)

Version

19.0.3

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Anything else?

No response

@gilvansfilho gilvansfilho added kind/bug Categorizes a PR related to a bug status/triage labels Oct 11, 2022
@stianst stianst added the area/authentication Indicates an issue on Authentication area label Oct 11, 2022
@gilvansfilho
Copy link
Contributor Author

I want to work on it after correct behavior are defined.

@mposolda mposolda added this to the Backlog milestone Apr 4, 2023
@mposolda
Copy link
Contributor

mposolda commented Apr 4, 2023

Thanks for the report, but unfortunately due the amount of other reported issues and other priorities, Keycloak team does not have time to properly triage this bug. So preliminary added to Backlog for now.
It will be helpful if:

  • You can verify if still applicable in latest Keycloak released version. If not, then it is welcome to close this issue.
  • If you figure that this may not be a valid bug (for example just a mistake in configuration etc), it will be also welcome to close this issue
  • If still applicable in latest version, it will be welcome to add the comment as well, that this was still reproduced with latest Keycloak version as it is very valuable info. Anyone is welcome to comment with this or add other relevant comments to this issue.

@rmartinc
Copy link
Contributor

Hi @gilvansfilho!

The code for the user condition is here. I think you are right and maybe the condition can check first if attributeName and attributeValue are defined, if they are not, just return false (it's bad configured). I also agree that the Attribute name and the Expected attribute value should be compulsory in the configuration (if the admin wants to configure any value, he/she can use a regex). Feel free to tune any text if you think they are misleading.

Very sorry for commenting so late. I'm adding the help wanted and moving the issue to backlog for now.

@keycloak-github-bot
Copy link

Due to the amount of issues reported by the community we are not able to prioritise resolving this issue at the moment.

If you are affected by this issue, upvote it by adding a 👍 to the description. We would also welcome a contribution to fix the issue.

@gilvansfilho
Copy link
Contributor Author

gilvansfilho commented Mar 15, 2024

Change behaviour of that authenticator could lead to unexpected behaviour for ones which already use so I was thinking if is not better to deprecate that and create a new one with enhanced behaviour?

WDYT @rmartinc ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants