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

Added ldap_user_federation attribute trust_email #267

Merged

Conversation

lathspell
Copy link
Contributor

Something strange happened with Pull Request 251, one of my git-rebase actions seems to have closed it?

The problem with Keycloak 7.0.1 is that the GUI does not support the "trust_email" attribute in this version. It is not in the Admin Webinterface nor in the representation that is reported back in the API.

How do you usually cope with such attributes? Is there a good way to throw an error depending on the version? If you can give me an example, I could try to enhance the patch.

original Text from Pull Request 251:

fixes #234

It's a simple boolean value, I copy & paste fixed it whereever I found ValidatePasswordPolicy/validate_password_policy.

Verified that it can in fact turn the setting on and off in Keycloak 8.0.1.

@mrparkers
Copy link
Contributor

That makes sense, thanks for the explanation.

I went ahead and updated CI to pass a KEYCLOAK_VERSION environment variable when running the tests. Could you update the failing test to check this using os.Getenv and skip the test if the version is set to 7.0.1? Also, if you update the docs for this new attribute to say that it only works on Keycloak 8+ that would be great.

Everything else LGTM.

@lathspell lathspell force-pushed the ldap_user_federation_with_trust_email branch 2 times, most recently from 08be1e6 to 360ca71 Compare May 8, 2020 12:37
@lathspell
Copy link
Contributor Author

lathspell commented May 8, 2020

I added a utility function that extracts the Keycloak major version from the new environment variable KEYCLOAK_VERSION.

The test sets the first and second value for "trustEmail" to false if the Keycloak version is <8. This should work as the variable is fetched using parseBoolAndTreatEmptyStringAsFalse().

The test still fails though and I cannot figure out why. In the output I see changes in batch_size_for_sync, user_object_classesand validate_password_policy which are all totally unrelated to trustEmail. See https://circleci.com/gh/mrparkers/terraform-provider-keycloak/1780?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

I could not figure out how to do single step debugging into the test (IntellIJ just stopped) or where exactly this output comes from. So you need to help a bit here.

secondTrustEmail = !firstTrustEmail
} else {
firstTrustEmail = false
secondTrustEmail = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this is still failing.

It might be best to remove the trustEmail stuff from this test and make a separate test specifically for testing this attribute. Then we can skip that test if the version of Keycloak doesn't support it.

@lathspell lathspell force-pushed the ldap_user_federation_with_trust_email branch from 360ca71 to c889abc Compare September 4, 2020 08:30
@lathspell
Copy link
Contributor Author

I rebased the patch to the current master branch.
As Keycloak 7.0.1 is no longer tested, the test case now runs perfectly :-)

@mrparkers
Copy link
Contributor

Sorry for just now getting back to this @lathspell.

Since we don't need to look at the KEYCLOAK_VERSION environment variable anymore, would you mind updating this PR to remove the checking for this?

@lathspell lathspell force-pushed the ldap_user_federation_with_trust_email branch from 705150f to 340ec21 Compare March 1, 2021 09:07
@lathspell
Copy link
Contributor Author

I rebased the patch. Is it ok to merge now?

Copy link
Contributor

@mrparkers mrparkers left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the PR!

@mrparkers mrparkers merged commit 1f8eebb into keycloak:master Mar 26, 2021
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.

Missing attribute "trustEmail" in keycloak_ldap_user_federation
2 participants