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

The getAttributes method in UserAttributeLDAPStorageMapper does not work for email or other UserModel properties #19352

Merged
merged 1 commit into from Mar 30, 2023

Conversation

rmartinc
Copy link
Contributor

Closes #10412

The getAttributes method in the proxy for AlwaysReadValueFromLDAP is not returning the LDAP value for model properties (email, firstName, lastName,...). Therefore those attributes are read directly from the internal DB and the data is outdated. This PR makes the getAttributes act like getAttributeStream. Little test added.

As commented in the issue #10412 the reported problem was already fixed by a previous commit but the root bug is still there.

@rmartinc rmartinc requested review from a team as code owners March 27, 2023 09:09
@rmartinc rmartinc requested a review from a team March 27, 2023 09:09
@rmartinc rmartinc requested a review from a team as a code owner March 27, 2023 09:09
Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

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

@rmartinc Thanks!

@mposolda mposolda requested a review from ahus1 March 27, 2023 12:32
@rmartinc
Copy link
Contributor Author

@ahus1 I took a look at the model tests and they just use the LDAP provider normally, so I think it's OK and just fixing the LDAP part is enough. But better if you double check it. 😄

@mposolda
Copy link
Contributor

@ahus1 Adding you as a reviewer to doublecheck if this method also affects new store and something should be changed there? Or is it not applicable in new store?

Copy link
Contributor

@ahus1 ahus1 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 reaching out. There is no need to change the new store around this. Once the new store is picked up for LDAP again, we'll have a look at the changes in the old store and apply them if and when needed.

@mposolda
Copy link
Contributor

@ahus1 Cool, Thanks!

@mposolda mposolda merged commit 89dfeee into keycloak:main Mar 30, 2023
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Token contains old DB values with "Always Read Value From LDAP" mapper setting
3 participants