Add support for additional user attributes to TomcatPrincipal #463
Conversation
michael-o
left a comment
michael-o
left a comment
There was a problem hiding this comment.
The first and foremost question I have: Is this save to be backported to previous Tomcat versions?
| * <p> | ||
| * Attribute names and naming conventions are maintained by the Tomcat | ||
| * components that contribute to this map, like some of the Realm | ||
| * implementations. |
There was a problem hiding this comment.
Are those really maintained? I thought the dev/admin requests the realm to load attribute values. So the attribute names are not necessarily mandated?!
There was a problem hiding this comment.
Don't know whether it's really the exact word. However, every attribute provider (aka Realm) can use it's own naming schema and conventions. Some may use prefixes (user.displayName) others may not, who knows. We make no rules or assumptions here but only refer to the each attribute provider's documentation.
There was a problem hiding this comment.
In other words, when querying an attribute named "displayName", a Realm could add this under a key like "realm.displayName" or "user.displayName". For that, the (maybe custom) Realm may have a config option userAttributePrefix or this could even be hard-coded.
Such prefixes might make sense, if, in the future, for example, several components could contribute to the Principal's attributes (like the Authenticator or a new component SocialUserDataProvider querying user information from Facebook etc.).
It should be. I do not see any reasons why this cannot be backported. |
The big question is here is whether the |
|
We could always remove the new attributes methods from the BTW, shouldn't we commit/merge the example mentioned above with this PR? Listing the Principal's additional attributes is completely independent from the Realm implementations. The same is likely true for the new Common Features / Additional User Attributes section in realm-howto.xml. |
|
If we limit the back-port to Tomcat 9+, we are on Java 8 and can use default methods to maintain compatibility. |
I guess, that's the way to go. Shall I implement these in my branch already? However, I will commit all changes only after we've agreed on all open questions. |
|
@cklein05 Please apply all requested changes. I want to merge with main and proceed with Tomcat 9+ as @markt-asf has written. |
|
What about the example JSP application? Shouldn't we add support for dumping user attributes with this commit? See #463 (comment) (2nd paragraph) |
Personally, I wouldn't do this. This will limit the semantics of your PR, taking away to work with interface only. My custom principal uses this one as well so Tomcat can internally use the provided methods otherwise it feels foreign to Tomcat. |
|
I'm not talking about removing methods from the interface. In the 2nd paragraph of this comment I was asking whether to include the additions to Tomcat's example JSP application located under |
My bad, yes. Please reuse the example as well. |
|
Done. |
|
@cklein05 The default methods are on not required on main since it is still bleeding edge, we can break API here. Default methods are required for backports to 9 and 10 only. |
|
Removed |
|
@michael-o Will provide a fix for Travis CI failed ASAP. |
Thanks, will then run it locally. |
|
@michael-o Should be fine now. |
|
@cklein05 Please provide backport PRs for 10.0.x and 9.0.x with default methods. |
3 participants
Add support for additional user attributes to the
TomcatPrincipalinterface and theGenericPrincipalclass.