On Thu, Feb 10, 2022 at 2:48 PM Mark Thomas <ma...@apache.org> wrote: > > On 10/02/2022 12:01, Rémy Maucherat wrote: > > On Tue, Feb 8, 2022 at 2:13 PM <micha...@apache.org> wrote: > >> > >> This is an automated email from the ASF dual-hosted git repository. > >> > >> michaelo pushed a commit to branch main > >> in repository https://gitbox.apache.org/repos/asf/tomcat.git > >> > >> > >> The following commit(s) were added to refs/heads/main by this push: > >> new c3edf43 Add support for additional user attributes in > >> TomcatPrincipal > >> c3edf43 is described below > >> > >> commit c3edf437da20af0f11edc0ad6d893399b01e6287 > >> Author: Carsten Klein <c.kl...@datagis.com> > >> AuthorDate: Wed Jan 12 15:06:42 2022 +0100 > >> > >> Add support for additional user attributes in TomcatPrincipal > >> > >> This closes #463 > > > > -1 (veto) > > > > Unfortunately, the more I think about this, the more it seems wrong to me. > > ACK. I won't tag until this has been reverted or you retract the veto. > > I will note that if I get to the point where I am ready to tag and this > is the only issue remaining to resolve I will revert the change prior to > the tag - although I'd prefer that michaelo did that before then. > > > This makes the realm an object store, which is not a good idea at all, > > as it will: > > - Encourage all sorts of bad hacks and practices for users > > - Force us to gradually add more and more features to this generic > > object store feature > > - Have massive discrepancies between realms, with the users trying to > > figure out pros and cons between realms > > - Is a proprietary feature which ties up users to Tomcat > > Just trying to understand the concern at this point. > > You think the risk is that developers will push data in the Realm and > then access from the authenticated principal?
This will seem convenient. If building an e-commerce thingie, you could imagine storing payment info, all misc info, shopping cart. The only thing preventing this is that there is no setAttribute(String name, Object value), but what if someone comes along with a PR later ? Is that a "no" then ? Why would this be more a "no" than for this one ? This is clearly my concern now [following lingering thoughts about how this could be (ab)used]. Again I would like to apologize for taking so long until reaching this position, I know this wastes a lot of time ... My wider concerns were initially triggered when there was suddenly some intent to add the feature to the UserDatabase (which is a R/W database for users, so there the feature clearly becomes general object storage - even though the PR only contained a mock impl). > > More importantly: application data storage should remain the > > prerogative of the application, Tomcat should not attempt to do that. > > The only thing Tomcat should provide is a principal name, which is > > then the primary key for the third party object store. There is zero > > reason to integrate this within the realm or principal API. It also > > feels like it can never be complete or appropriate. > > Looking at the proposed change for the DataSourceRealm I can see it has > clearly defined limits. I can see how we might get requests to push back > those limits in the future. > > I have a general concern / unease that doing this in an application > specific manner would be relatively easy but doing it generically could > get complex. > > > I apologize for having to do this, after initially helping out with > > this PR, but this is one of those situations where the more I look > > about it, the worse I feel about it. > > I do recognise the use case of wanting to expose data that is already in > the Realm to the application. Yes, that was what I was thinking about too. But then it still encourages putting more stuff in the realm backend (since now you can, you probably know how it works by now). The use case in the PR is the DataSourceRealm (not the LDAP one), so let's assume it is a JDBC thing. Retrieving the data from another data source connecting to the same backend in the application might (supposedly) be less efficient (but if you pull a lot of data from the DB, it will suddenly be impossible to tell the difference), but is portable and you could store things properly (and more importantly avoid asking us to add code to automagically map to Java collections ... that's just for starters obviously). > I wonder whether this is a case where we can just provide some hooks > and, if the developer chooses to (ab)use them, that is up to them. > > I'm thinking something like a getUserAttributes() method on Realm that > returns null by default. If an application wants to populate additional > attributes, then it needs to provide a custom realm that overrides that > method. The issue with this approach is it requires a custom realm. > > Given your concerns, backing this out while we discuss it some more > seems like the right approach to me. If there's firm consensus that this feature is final for there, then I could drop the veto without the need to punt it to another API and custom realms. I wouldn't be happy about it, but this would not get out of hand. That separate API idea has merits since our simple default feature impl in DataSourcerealm could, for example, get in the way for a user who would like to fully exploit this. Who knows ... The feature should be better javadoc-ized in that scenario, with explicit warnings in TomcatPrincipal such as: - this is only meant to expose extra data which might be already present in the realm backends - this is not an object store and will not become one in the future - this will not be extended further (no collection type mapping, no setAttribute, etc) - existing realms where this is not a natural feature will not be extended to expose additional arbitrary attributes (again to avoid encouraging use as an object store) - the UserDatabase, which is meant for R/W, will never add support for this feature I can volunteer to add the warnings. Rémy > Mark > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
