Repository navigation
KEYCLOAK-14536 UserModel preparation for UserProfile - #7080
Conversation
|
We'll need to consider a slightly less intrusive way to achieve this. Through deprecation/migration. The fields on the UserModel should remain as is, but marked as @deprecated. These can then delegate to attributes instead of fields. Perhaps this can even be done in an non-intrusive way of returning given_name attribute if it's set, with a fallback to firstName. Alternatively, we could consider migrating, but that could be very expensive for those that have millions of entries. We could even go as far as allowing reading firstName field with getAttribute(given_name). @mposolda @hmlnarik @pedroigor we really need some input on this one. It would be nice to get to the point where we don't have basic fields on the user-model and it's all just attributes, but how to get there is another thing. |
There was a problem hiding this comment.
As an initial and base implementation for the user profile stuff, I would also do it differently as @stianst suggested.
Before continuing reviewing other parts I would just revert the changes that remove first/last references from the model, deprecate them, and use a fallback approach to read them as attributes.
I think obtaining/set the first/name is a so common operation that maybe it even makes sense to not deprecate them but have them there as a more convenient way to obtain both given and family names.
In regards to migration, I also think that we can't do much without impact deployments with a huge number of entries. However, this can be done either by migrating on demand (when a user updates these attributes) and when a realm us actually using the upcoming user profile SPI (not the legacy).
There was a problem hiding this comment.
I agree with @pedroigor regarding keeping firstName,lastName getters/setters on UserModel. And they are quite used methods, so even deprecating them is not needed IMO.
- I would just add the methods like this to UserModel interface directly:
default void setFirstName(String firstName) {
setSingleAttribute("firstName");
}
etc for other getters/setters
-
My vote is even to keep the name of attributes as firstName/lastName. I personally like firstName/lastName better than "given_name"/"family_name" . Moreover I don't see much benefit with aligning to OIDC protocol naming? It rather seems that changing "firstName" to "given_name" etc will mean some more migration work (configuration of all mappers etc) and potential for more bugs reported.
-
On the other hand, we can change JPA model to use attributes for writing instead of properties? The properties can be used just as fallback for reading from previous version. I can imagine something like this on the JPA UserAdapter:
public String getFirstName() {
// This reads from the "firstName" attribute as implemented on the UserModel interface
String firstName = super.getFirstName();
// Fallback to read from FIRST_NAME property on USER_ENTITY table
if (firstName == null) {
firstName = userEntity.getFirstName();
}
}
The setters on UserEntity can be removed entirely. Also the UserAdapter.setFirstName setter can be removed. This means that all new writes will go to attributes and reads will allow fallback to read from FIRST_NAME , LAST_NAME properties.
This approach can allow better flexibility for removing getters/properties entirely in the future. And at the same time, there won't be a need for the expensive bulk users DB migration
- We can use same approach for UserRepresentation - write to attributes and read from attributes with the fallback to read from getters.
In other words, only needed thing for this PR might be the change on UserModel and then slight change on the JPA model and UserRepresentation JSON reading/writing.
|
Regarding the other properties, I can imagine that "email" can be a bit tricky regarding DB model. It is due the fact that we have unique constraint in the DB model for the "email" and I assume we still want to keep this constraint? So for the JPA model etc, I think we will still write "email" as the column EMAIL instead of the attribute just to enforce the constraint is addressed. |
|
@stianst , @mposolda , @pedroigor Thanks for your feedback! Here is my summary of the discussion so far and what I would do:
|
I am the one, who postponed to keep the attribute names to "firstName" / "lastname" instead of changing to "given_name" / "family_name" . If you guys have different opinion, then feel free to rename. Just repeating, that I don't see so much advantages of rename (Keycloak doesn't necessarily need to be tightly-coupled to OIDC as OIDC is just one of the protocols supported by Keycloak) comparing to the possible bugs during migration (EG. all mappers may need to be renamed for the migration and maybe some more various issues...)
+1 . Doing things in more PRs is always good. So JPA model can be done later.
+1 from me.
+1 . It seems the main reason to keep the username and email columns in DB are the DB unique constraints, which we probably still want to preserve. Will be nice if "username" is also subject to the UserProfile as I mentioned in the keycloak-dev. This is related to the support for username to be writable on registration form and when creating by admin, but readable in further edits. It will also allow us to remove the realm switch "Edit username" , but that can be done in some further PRs in my opinion. |
a447825 to
c4cebdc
Compare
|
@mposolda I updated the PR with the changes discussed above, i.e. There are a few points to discuss though:
|
|
There are still the Cross DC tests failing, everything else seems to work fine. I'm trying to debug those but that'll take some time. |
|
@Martin-Idel-SI Feel free to ignore cross-dc tests. They are broken right now and separate JIRA for fixing them exists. However if you volunteer to fix them, it will be great! :) Just a warning that it may be non-trivial effort and will be probably good to send separate PR to keycloak master as fixing cross-dc tests is not directly related to this User Profile work. Just checking for the case that you are interested in cross-dc. See https://github.com/keycloak/keycloak/blob/master/testsuite/integration-arquillian/HOW-TO-RUN.md#cross-dc-tests for how to run cross-dc tests. |
|
@Martin-Idel-SI I will take a more look at your changes and proposal for this PR next week. Sorry for the delay - had some other priority task this week... |
|
@mposolda No worries, next week is fine! Good to know about the Cross DC tests. Yes, I saw the "How to run" but as of yet, something isn't working correctly from IntelliJ. If I get my setup working and see an easy fix I'll let you know asap of course, but I'm not going to put too much effort into that if they also fail on master ;-) . |
c4cebdc to
88b5551
Compare
There was a problem hiding this comment.
@Martin-Idel-SI Thanks for your changes and I am sorry for later review... Please see my comment below
@stianst Do you have a chance to also take a look in this PR please and at some comments above?
I am adding my opinion on the questions you had in the comment above.
It turns out that FTL doesn't recognize default methods on classes as the JavaBeans
specification doesn't (didn't?). There is a workaround since version 2.3.26 (we use 2.3.29) but we'd
have to sacrifice compatibility with templates for earlier versions. So either we can't have default
methods inUserModelor (preferred imho) we go with default methods as suggested and we
introduce this flag, potentially breaking some old FTL templates:
I think this change is OK, but I am not 100% sure. @stianst WDYT?
I noticed that we still have one bit of possible migration to discuss: What if a user has a custom attribute called "email", "username", "firstName" or "lastName" in the database? What if that field doesn't match the real email, etc? We have to have some sort of migration strategy. I stumbled across this since there is one test realm which actually has a user with email as attribute which makes one test fail:
keycloak/testsuite/integration-arquillian/tests/base/src/test/resources/model/testrealm.json
Line 132 in c4cebdc
"old-email": "bburke@redhat.com"
I renamed it in the test, but I think we have to discuss what to do here.
Regarding the last problem, we probably should forbid creating custom attributes of that type, but I don't know how best to support this in the GUI, or should we just list email, firstName, lastName and username as custom attributes, too?
I think this is corner case and I would ignore this issue. If someone has an attribute "email" in addition to the property "email", then they already have some issues in the mappers etc. Your change in the test is OK in my opinion.
I was wondering: Apart from the base test suite, is there another testsuite which runs some tests against federated storage? Changes around the UserModel seemed to result in less test failures for federated storage than I would have thought so I wonder how good test coverage in that area is and whether we need to do some manual testing to see that everything still works - in any case I'll have to look into potential test failures on your CI.
I think it will be good to run the cluster tests and cross-dc tests. Namely the UserInvalidationClusterTest and InvalidationCrossDCTest. This will be good as I am not 100% confident that your changes in model/infinispan don't have any impact on user invalidations. Here are some hints for run those tests:
https://github.com/keycloak/keycloak/blob/master/testsuite/integration-arquillian/HOW-TO-RUN.md#cluster-tests
https://github.com/keycloak/keycloak/blob/master/testsuite/integration-arquillian/HOW-TO-RUN.md#cross-dc-tests
Besides that, it will be good to run admin console tests and account console tests to doublecheck there is no impact of your freemarker change. I will take care of it and will run the tests in our environment. But will be good to first address my other comments.
There was a problem hiding this comment.
I think it it not very good to remove those methods from UserModelDelegate? IMO you always want to call delegate rather than rely on default implementations from UserModel. In some cases, the methods on the delegate can do something different that call "setAttribute/getAttribute" . I hope it is possible to revert changes in UserModelDelegate class entirely.
There was a problem hiding this comment.
The problem is that this introduces potential inconsistencies:
I have two ways to access and/or change those attributes. If one way behaves differently than the other, this will lead to subtle and hard to debug bugs. That's why I would've deprecated and deleted those methods altogether but I can see that the effort involved is much larger as of now (for instance ftl templates rely on those methods), so we keep those methods.
This is in general true for all subclasses of UserModel. For the AbstractUserAdapterFederatedStorage this is discussed in the other comment and there is a solution to ensure consistent behaviour as long as the implementation does not override the methods to get or set attributes.
I didn't see any other implementation of that interface I thought was modeled for inheritance, but if it is possible to provide UserModel implementations via SPI then we definitely should think about this again.
There was a problem hiding this comment.
Replied to your other comment. Hopefully once "getUsername" and "setUsername" are not implemented on UserModel, the changes in Use 10000 rModelDelegate will be possible to revert, so that it always calls the delegate instead of rely on the default implementations.
There was a problem hiding this comment.
It's possible (see the two last commits). However, please note the duplicate code I had to introduce to keep consistency!
There was a problem hiding this comment.
@stianst Is this change ok or do you see any issues with that? More details in this comment : #7080 (comment)
There was a problem hiding this comment.
Not sure to be honest as I don't know what the implications really are here. It seems this requires FreeMarker to do additional reflection of the classes, which could be expensive? Also, what would break in existing templates with this?
There was a problem hiding this comment.
Yes in the sense that it requires FreeMarker to also see default methods, but I'd think this should be done in the same pass where Freemarker retrieves the other getter/setter methods, so this shouldn't have a large overhead (if any).
However, some things may break: Here is an overview: https://freemarker.apache.org/docs/api/freemarker/template/Configuration.html#Configuration-freemarker.template.Version- - since I have no idea what features are used, it's hard for me to judge if this is a problem. I ran some console tests and all seemed fine, so I was confident enough to propose this change.
As I said, the alternative would be to not use default methods but duplicate the methods to all implementing classes.
There was a problem hiding this comment.
Just an idea, but will it help to use the "wrapper" object, which has the methods getX/setX overriden? I can imagine some change like this:
- freemarkerContextAttributes.put("user", userModel);
+ UserModelDelegate wrapper = new UserModelDelegate(userModel);
+ freemarkerContextAttributes.put("user", wrapper);
Can it help?
If not, is it the possibility to have some other Wrapper class similar to for example org.keycloak.forms.login.freemarker.model.ProfileBean ?
If nothing of those helps (or it becomes unecessary complex and ugly code), if you have an opportunity to add the details what exactly the error is and where it happens, it will be helpful. Hopefully we can figure something.
The set of version in the Configuration may work as the last possibility, but will be nice to avoid since we are not 100% sure of all the possible side-effects.
There was a problem hiding this comment.
I'm sure we'll figure something out!
The problem is actually very simple: I found it first in the resource-detail.ftl. The corresponding AuthorizationBean contains permissions which contain a RequesterBean which contains a UserModel (the requester). Now, without this change, the ftl-template cannot be rendered correctly, because permission.requester.username is null in this line:
And the reason it's null is because this refers to the method getUsername on the UserModel- but it doesn't exist for FTL as default methods don't exist for without this change (this is due to the JavaBean specification which doesn't understand default methods).
Now, clearly that's just one instance of where this fails as the UserModel is also present in other beans. Also, since get/setUsername are not default implemented anymore this particular issue will not arise anymore, but the same problem will be whenever we need get/setEmail or firstName, lastName.
So for me, there would be several solutions to this:
- directly grab the attribute of the user in the FTL templates via something like
permission.requester.attributes["username"][0]or similar. This is not very nice and very error prone as I don't see a good way to find all instances where this could be a problem. Also, this breaks custom templates. - Add the configuration option as above: least intrusive from a code perspective which is why I proposed this change first, but may have unwanted side-effects. I've never really worked with FTL, so I can't say much about it
- Remove the default methods and just implement them on all subclasses. Since we do essentially the same for
getUsernameit seems those subclasses aren't too many due to the hierarchy of subclasses. - Add a wrapper object to all places where UserModel is used in the beans. Sounds weird and error prone and results in quite a few changes
- Add an abstract subclass of UserModel and derive all classes that don't override the default methods from this subclass: I don't particularly like to add yet another level to the hierarchy, it's already quite deep
- Make UserModel an abstract class instead of an interface (I haven't tested it though): This should also work, but may not be nice from an architectural standpoint.
I'm probably going to try out the third option. This leads to some code duplication, but the code changes are easiest to get right, so maybe that's the best way to go?
There was a problem hiding this comment.
As I pointed, if there is some easy way to address this like for example using this in the AuthorizationBean constructor: this.user = new UserModelDelegate(user);, it will be fine to me.
The solution with adding to subclasses however works for me as well.
There was a problem hiding this comment.
I ended up adding subclasses - please have a look. I feel that this approach is the safest, as everything looks as before for FTL templates.
All tests already ran beforehand since the only test failure I saw was actually related to the username field and I had already removed the default methods before. I tried using wrapper objects but was left unsatisfied whether I actually solved all problems and if somebody uses custom FTL files, those could easily break if I overlook something.
I could add a comment to the wrapper class (and the one set of duplicate methods in the infinispan/UserAdapter) to move to default methods once this is no longer a problem in FTL (maybe you will make a switch to a newer version for different reasons).
88b5551 to
26074c8
Compare
|
@mposolda Thanks for your input, I updated the PR again implementing the changes and rebased it to make it mergable in principle (for now, I'll only add commits to make it easier for you to review the changes I put on top, so this time I added two commits). I ran the crossdc and cluster test locally and they ran fine with the exception of the EDIT: Sadly, there are CI failures. As I can't reproduce those locally, this will take some time to debug and if I look at the code caching can be the only culprit I can think of... |
|
@Martin-Idel-SI The test failure in AuthenticationSessionFailoverClusterTest.failoverDuringAuthentication does not have anything to do with your changes. No need to look at it. JIRA exists for this and fix is in progress as far as I know. The same applies to cross-dc server and adapter tests. However the errors in LDAPProvidersIntegrationNoImportTest are most likely caused by the changes. I will take a look at your changes now... |
|
@Martin-Idel-SI The changes are ok to me. Just 2 things:
Thanks for all your work on this. The User SPI refactoring is unfortunately the part, which is a bit of pain the UserStorage SPI, backwards compatibility, lots of the tests affected etc... Hopefully the follow-up work on the UserProfile will not be that painful... :-) |
|
@mposolda We knew that changing how data is stored and accessed in the UserModel would likely be very painful, although I was pleasantly surprised by how few changes we actually need. The architecture seems to be quite well done! I'm confident introducing the SPIs will be less painful since it was rather straightforward to get something like the Legacy Profile in place (see the PR by bs-matil). Yes, the two tests failing outside of the crossdc tests are definitely related to my changes. EDIT: I can now reproduce them, so should hopefully be able to fix them soon or provide another problem to analyze :D |
f8f2701 to
caaa04c
Compare
|
@mposolda TravisCI is still not happy about the CrossDC tests but they do run locally and I see a test failure in your Keycloak.X test suite - does that have anything to do with my changes? |
There was a problem hiding this comment.
@Martin-Idel-SI Thanks for the update! The PR looks good to me. I've added one (hopefully) last minor comment inline.
Besides that, it will be good to add some small note to the documentation (Upgrading guide) about the changes in the user model. See the file https://github.com/keycloak/keycloak-documentation/blob/master/upgrading/topics/keycloak/changes.adoc for more details.
@stianst @hmlnarik @pedroigor Do you have any more feedback to this PR?
|
@mposolda No worries, since this is a rather intrusive change it's definitely good to be careful. Yes, I was thinking about where to put some documentation, thanks for pointing this out! Here is the corresponding PR: keycloak/keycloak-documentation#962 Regarding ticketing: Would you like me to create some task ticket describing the changes or rather a subtask to the UserProfile epic? I'll then continue with the database changes we discussed above but that'll probably take a while since I'll be unavailable in July. |
|
@Martin-Idel-SI Thanks for the update! I've approved your PR for the documentation. I've added Andrew (our documentation guru) as a reviewer to this PR as well to check if it is fine from the perspective of english grammar and other things like that. Yes, it will be good to have JIRA under UserProfile epic for this. We try to have 1 commit per 1 PR and 1 JIRA. Once you do the JIRA, you can squash the commits in this PR and add the JIRA number to the commit message (See other Keycloak commit messages for the details). |
- In order to make lastName/firstName/email/username field configurable in profile we need to store it as an attribute - Keep database as is for now (no impact on performance, schema) - Keep field names and getters and setters (no impact on FTL files) Fix tests with logic changes - PolicyEvaluationTest: We need to take new user attributes into account - UserTest: We need to take into account new user attributes Potential impact on users: - When subclassing UserModel, consistency issues may occur since one can now set e.g. username via setSingleAttribute also - When using PolicyEvaluations, the number of attributes has changed
2a098e6 to
ed6e791
Compare
|
@mposolda Thank you for your approving the documentation PR: However, I added one more line to the documentation because when going over the changes one last time I remembered that this change may impact users of the policy evaluation feature (see the test change that was necessary). I opened a subtask as requested and squashed to one commit as anticipated. I just like to keep several commits until most discussion is over because that makes it easier to revert changes and review new changes, but since hopefully most major changes are done now, I'm fine with squashing! |
There was a problem hiding this comment.
@Martin-Idel-SI Thanks for your work on this!
This PR is a preparation for adding the user model soon.
As discussed in several threads (e.g. here: keycloak/keycloak-community#104), it would be nice to no longer store attributes such as "firstName" or "lastName" as fields in the user model so that they are not mandatory in a future user profile. I believe this is also in line with how you are planning to evolve the Keacloak model
In addition, it was suggested to also change the name from firstName/lastName to the OIDC naming convention (i.e. given_name/family_name).
This PR outlines how we would approach this change:
firstNameandlastName. According to the discussion, we'd also think about migrationemail. I thinkusernameis too special to merit migration.There are a few things I noticed when preparing this migration:
keycloak/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/PolicyEvaluationTest.java
Line 616 in 419d9c6
What do you thing about those?
Regarding the breaking change with built in mappers, as far as I can see this should be avoidable if we do not rename the attribute from
lastNametofamily_namebut keep the old name and just migrate it from a field to an attribute.Finally, it's difficult to assess how well the tests cover that no real functionality change happened here. I went through all code changes and to me there are the following areas of interest:
Finally, what I believe remains to be done before merging this once there is an agreement on whether this is a viable strategy: