cklein05 commented on pull request #428: URL: https://github.com/apache/tomcat/pull/428#issuecomment-1010954501
@rmaucher I'm with @michael-o here. Having these extra attributes with the Principal is useful per se. Since any Realm can log you on and create a Principal, it should not matter, which Realm queries and provides these attributes. The more Realms can do so, the better. You want to avoid changing the other Realms except JNDIRealm? Why? What's wrong with my code? Focusing on DataSourceRealm you're saying > ... it only seems to be adding a lot of code which will have lower performance. It does not only add a lot of code. It actually works as it should. I've only added the code required to actually query these attributes in a safe way. Also, it does not really lower performance. If there are no attributes defined, it only takes two extra method calls with a total of two `null` checks, except for the first login attempt in the Realm's lifetime. For the _normal case_ that is not really a performance killer, is it? If there are attributes defined, there is one extra SQL query executed for the _normal case_ (plus one extra SQL query executed for the first login attempt in the Realm's lifetime). Of course, that takes some time. However, if the Realm didn't perform that query, the application will have to do that after login, which in turn will take the same time or even more, since the application must use another JDBC connection from the pool. After all, I don't see an effective performance problem with the DataSourceRealm code here. If you do, could you please explain in more detail? You where saying > adding the feature to the other realms is pointless with a fixed list of attribute. Yes, that may be right. However, even that fixed list of attributes gives the application the opportunity to obtain the User's fullName property (if defined), which otherwise was just not accessible. Actually, the user's full name (aka display name) is one of the most important attributes users may want, as it is much more end-user friendly than a cryptic logon name. Now, with support for arbitrary attributes, the feature is even more useful. I personally do not need these attributes with the UserDatabaseRealm, since we actually do not use tomcat-users.xml in any setup. However, since that is Tomcat's default Realm, I believe it should support these attributes as well. E. g. I've extended example application at `/examples/jsp/security/protected` to list additional user attributes from the Principal. I guess it would be just nice, if people can test these without configuring another Realm first. Nevertheless, doing this in steps is an option. How to proceed? Are you able to merge the PR partially? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
