Repository navigation
User attribute value length extension - #26196
Conversation
07bd213 to
c17a3fe
Compare
|
What about |
c17a3fe to
f1248a3
Compare
Updated. |
b8f95a5 to
7c6bdd5
Compare
There was a problem hiding this comment.
To prevent a denial of service attempt, we should have a maximum length of attributes defined.
We discussed that the declarative user profile could have a configurable length of an attribute. For the time being and also when UP is disabled, we need to have a (possibly hard coded) maximum length of user attributes.
There was a problem hiding this comment.
Reading the comment, it looks like we previously supported searching for attributes longer than 255 characters in the federated attributes, and the code has changed to no longer support this. This would then be a regression. It is also difficult to explain to users why user attribute search lengths are different in both places.
Nitpick: I didn't find the method referenced here. It seems to me that the check is done in searchForUserByUserAttributeStream, therefore the comment might needs updating.
| //this should never happen as the exception should be thrown already by JpaUserProvider#getUsersByUserAttributeStream, but leaving it here as additional check | |
| //this should never happen as the exception should be thrown already by JpaUserProvider#searchForUserByUserAttributeStream, but leaving it here as additional check |
|
@pedroigor - I found that when using LDAP, people were able to use longer user attributes (for example for photos) even before. The maximum length validation for unmanaged LDAP attributes applies here as well, so people who are using attribute values in LDAP which are larger than 2048 characters (ca. 1500 for binary data) will need to make those attributes managed and add a length validation. I assume this is ok as no user should post an unlimited amount of data to a field in LDAP. Tests are now green again, and docs updated. |
There was a problem hiding this comment.
Thank you for the PR @vramik and @ahus1. I added a few comments. Maybe I am nitpicking too much, sorry for that.
I also created a commit to refactor the lambda for hash collision check: mhajas@14d9e2f but I don't want to just push it as I am not sure it is much better, feel free to add it if you like it.
|
|
||
| try (PreparedStatement ps = connection.prepareStatement("SELECT t.ID, t.VALUE" + | ||
| " FROM " + getTableName("FED_USER_ATTRIBUTE") + " t" + | ||
| " WHERE CHAR_LENGTH(t.VALUE) > 2024"); |
There was a problem hiding this comment.
How sure are we that CHAR_LENGTH in MySQL returns the same results as String.length in Java?
There was a problem hiding this comment.
Unicode character have sometimes a strange behavior when it comes to lengths, and you're right that I should check this. See below for an example:
https://stackoverflow.com/questions/64213394/string-encoding-with-emoji-in-java
System.out.println("🇻🇺".length())
// -> 4SELECT length("🇻🇺"), char_length("🇻🇺")
FROM dual;
-- -> 8, 2
I didn't find an equivalent for Java's length() in MySQL, so I'll do the following:
- Change the SQL to use
LENGTH()to select possibly more rows than necessary. - Add additional filtering in Java to ensure I'm updating only those where Java's string length is longer.
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
Signed-off-by: Michal Hajas <mhajas@redhat.com>
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
|
@mhajas - tests are green and your review comments have been addressed in separate commits. Ready for another round of review. Thanks! |
| for (AttributeValidatorMetadata validator : metadata.getValidators()) { | ||
| if (validator.getValidatorId().equals(LengthValidator.ID)) { | ||
| lengthValidatorExists = true; | ||
| break; |
There was a problem hiding this comment.
Can't this whole thing be simplified a bit by just returning from here instead of breaking out of two loops just to skip the rest of the method? i.e. remove the flag, and return if the id matches here.
There was a problem hiding this comment.
Yes, that work work. I'll add anther commit. The data is in-memory IMHO already, anyway.
There was a problem hiding this comment.
Commit added. Build will hopefully be green. Ready for a final round of review?
| public void searchByLongAttributeValue() { | ||
| testingClient.server().run(session -> { | ||
| // random string with suffix that makes it case-sensitive | ||
| String longValue = RandomStringUtils.random(2999, true, true) + "v"; |
There was a problem hiding this comment.
Also, a question: shouldn't this size of attribute fail the length check? Or is a length validator added somewhere that I'm missing?
There was a problem hiding this comment.
AFAIK this creates a lambda that runs directly on the server and accesses the model. This doesn't go via the declarative user profile layer, so it can write any data it wants.
There was a problem hiding this comment.
ah, true.. thanks for the info!
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
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.x509.X509BrowserLoginTest#changeLocaleOnX509InfoPageKeycloak CI - Java Distribution IT (windows-latest - temurin - 19) |
Oh, covfefe!! |
|
Thank you for everyone who helped with this task in the discussions, ideas and iterations until we got to this point to get this merged. I hope we got it correct in all necessary the variant where attributes are used. Let's keep an eye on this in the coming weeks before the next KC24 to spot unexpected regressions. Parts of this change wouldn't have been possible without the declarative user profile. With that in place, it was possible to have both a default length validation and a UI to configure the length of each field. Thank you! |
|
If I get this right, this change didn't increase the limit for group attributes though, did it? |
|
@overheadhunter - no, it didn't. Feel free to open a new issue for that if none exists. |
Closes #9758
The PR adds a possibility to store and search by long user attribute values. It works for both regular and federated user attributes.
For both tables it uses two new columns in the database schema. One BLOB where the actual value is stored, second with its hash. On the hash column there is also an index created to allow effective search.
This approach could lead to very low chance that there would occur conflicts within the hash value, due to that there has been requirement to verify that user actually has indeed the specified value before returning it.
This check has some drawbacks:
UserAttributeFederatedStorage#getUsersByUserAttributeStreamas it returns only user IDs (Stream<String>)UserEntity-UserAttributeEntityassociations from LAZY to EAGER as for each returned user its attributes needs to be loaded as well.Ad. 3. for most scenarios we load attributes anyway as they are needed, but in case calling
UsersResource#getUserswithbriefRepresentationset to true (by default it is false) in has the mentioned impact