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?
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.
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.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
