Repository navigation
fix empty attributes parsing - #26729
Conversation
828e813 to
2ce0263
Compare
vmuzikar
left a comment
There was a problem hiding this comment.
As far as I can tell, LGTM.
|
Thank you for this! 👍 Only thing I can add is that, what if the user somehow, e.g. via API or Admin UI, stores a value that is not empty but can not be parsed, e.g. the string "a"? Wouldn't that result in the same behaviour? |
sschu
left a comment
There was a problem hiding this comment.
LGTM. Just questions/nitpicks.
There was a problem hiding this comment.
Are the wildcard imports here intended?
There was a problem hiding this comment.
Nopes, that was a mistake, but not fixing that now as that would require a new run in CI ;)
There was a problem hiding this comment.
If we want to prevent this we should have some kind of shared linter/IDE configuration.
There was a problem hiding this comment.
Nitpick, I would use realmName.
There was a problem hiding this comment.
Would have possibly changed that, if it wasn't for the fact that will result in a new CI run ;)
The Admin UI only allows adding numbers to those fields, but that could happen if you do it through the Admin UI. We really need to get some improvements around validation for things like that, but won't cover it in this as I'd like to get the most pressing thing sorted out first, and get a 23.0.6 release out, hopefully today. |
mhajas
left a comment
There was a problem hiding this comment.
getUserActionTokenLifespans is doing a substring of the entry key so we need to get only something instead of whole RealmAttributes.ACTION_TOKEN_GENERATED_BY_USER_LIFESPAN + ".something" in the test. That may be a reason for a new CI run.
|
A link to the issue in the PR description would be good so the tracking on the board works correctly. |
2ce0263 to
36b9530
Compare
Fixed this, although it doesn't matter for this fix, as the exception is thrown regardless. |
ghost
left a comment
There was a problem hiding this comment.
Unreported flaky test detected, please review
|
Somehow the "new-realm" seems to affect other tests. It causes 404 when setting the realm and assertion failures |
864e2cf to
55ae655
Compare
Resolves keycloak#26597, resolves keycloak#26665 Signed-off-by: stianst <stianst@gmail.com>
55ae655 to
2419960
Compare
Just noticed that, should be fixed now |
ghost
left a comment
There was a problem hiding this comment.
Unreported flaky test detected, please review
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.admin.realm.RealmTest#testNoUserProfileProviderComponentUponRealmChangeKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#removeRealmKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#updateRealmAttributesKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#createRealmFromJsonKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#renameRealmKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#updateRealmEventsConfigKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#clientSessionStatsKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#loginAfterRemoveRealmKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#getRealmRepresentationKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#createRealmWithPasswordPolicyFromJsonWithValidPasswordsKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#testSupportedOTPApplicationsKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#createRealmEmptyKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#updateRealmWithNewRepresentationKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#getRealmsKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#excludesFieldsFromAttributesKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#pushNotBeforeWithSamlAppKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#convertOIDCClientDescriptionKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#createRealmWithPasswordPolicyFromJsonWithInvalidPasswordsKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#deleteSessionKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#convertSAMLClientDescriptionKeycloak CI - Legacy Store IT (mariadb) org.keycloak.testsuite.admin.realm.RealmTest#testNoUserProfileProviderComponentUponRealmChangeKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#removeRealmKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#updateRealmAttributesKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#createRealmFromJsonKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#renameRealmKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#updateRealmEventsConfigKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#clientSessionStatsKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#loginAfterRemoveRealmKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#getRealmRepresentationKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#createRealmWithPasswordPolicyFromJsonWithValidPasswordsKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#testSupportedOTPApplicationsKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#createRealmEmptyKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#updateRealmWithNewRepresentationKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#getRealmsKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#excludesFieldsFromAttributesKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#pushNotBeforeWithSamlAppKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#convertOIDCClientDescriptionKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#createRealmWithPasswordPolicyFromJsonWithInvalidPasswordsKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#deleteSessionKeycloak CI - Legacy Store IT (postgres) org.keycloak.testsuite.admin.realm.RealmTest#convertSAMLClientDescriptionKeycloak CI - Legacy Store IT (postgres) |
1 flaky test on run #10890 ↗︎Details:
|
|||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| Client Scopes test > Client Scope filter list items > should filter items by Protocol All |
Test Replay
Screenshots
|
|
Review all test suite changes for PR #26729 ↗︎
…oak#26729) Resolves keycloak#26597, resolves keycloak#26665 Signed-off-by: stianst <stianst@gmail.com>
…oak#26729) Resolves keycloak#26597, resolves keycloak#26665 Signed-off-by: stianst <stianst@gmail.com>
…oak#26729) (keycloak#26737) Resolves keycloak#26597, resolves keycloak#26665 Signed-off-by: stianst <stianst@gmail.com> Signed-off-by: Krzysztof Walczewski <kwalczewski@gmail.com>
Ideally the admin console shouldn't be sending empty values, and we should also have validation to prevent saving empty values, but for the time being this resolves the issue, and also is a fix for those that have upgraded and broken their installation.
Resolves #26597, resolves #26665