Add method to provide Identity from Authenticator's in SPI

[tbaeg]

Description

Introduce additional methods to the SPI authenticators to directly provide an Identity.

Problem

Some authentication methods can also have authorization information (i.e. - Identity.groups population using JWT claims.. also see comments) but there is no way to provide that information to the Identity given the current implementation (only accepts a Principal).

Solution

Allow implementing classes to provide an Identity directly. This allows significantly more flexibility in shaping what properties an Identity has and better decoupling from main.

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

[wendigo]

@dain do you want to chime in?

[findepi]

Introduce additional methods to the SPI authenticators to directly provide an Identity. This allows for more flexibility in by the implementing authenticators to define what the Identity should look like.

This describes the code change, but not the context. The Why remains unclear.
What is this that you're trying to do, and is not possible in the current code?
How important and non-niche use-case is this?

[tbaeg]

Introduce additional methods to the SPI authenticators to directly provide an Identity. This allows for more flexibility in by the implementing authenticators to define what the Identity should look like.

This describes the code change, but not the context. The Why remains unclear. What is this that you're trying to do, and is not possible in the current code? How important and non-niche use-case is this?

I've updated the description to better outline the problem it's trying to solve along with additional context from another MR/comment.

[ksobolew]

I'm mostly interested in the backwards compatibility story (as I'm sure I'll be adjusting a bunch of authenticators to that new API).

• Making this method deprecated indicates that it should not be used or implemented, but it looks like a normal fallback. Is the intent to remove it entirely in the future?
• Renaming this method forces the implementers to make adjustments anyway, can this be avoided?
• Naming the new method the same as the old one is confusing - the correct course of action when someone wants to keep the current behavior is to rename the method in the interface implementation, but the error message will say that the return type is wrong, which suggests something else

[findepi]

• Making this method deprecated indicates that it should not be used or implemented, but it looks like a normal fallback.

That's how deprecation is done. If the method was no longer called, we would remove it.

[ksobolew]

True, except that it's also renamed, making it technically a new method. So implementers of this interface need to adjust anyway. A typical deprecation process would deprecate the method, but implementations would work unchanged.

[tbaeg]

Making this method deprecated indicates that it should not be used or implemented, but it looks like a normal fallback. Is the intent to remove it entirely in the future?

My preference would be to remove it entirely. I see limited/deprecated usage of the Principal (i.e. - SystemAccessControl.checkCanSetUser) that further supports removal.

Renaming this method forces the implementers to make adjustments anyway, can this be avoided?

I generally wouldn't rename the deprecated method, but the main reason was for:

1. Consistency between authenticators in it's current state.
2. The supplanting method name be authenticate. At it's core, it's really just a naming preference. (authenticate vs createAuthenticatedIdentity).

Naming the new method the same as the old one is confusing - the correct course of action when someone wants to keep the current behavior is to rename the method in the interface implementation, but the error message will say that the return type is wrong, which suggests something else

I don't disagree it is a tad confusing and I'm not against naming the new method createAuthenticatedIdentity. That said, I don't think most people have a much trouble with this and I prefer to have some pain on some method names if we can get to the desired state faster.

[ksobolew]

Sure. Like I said, I will be updating a bunch of authenticators so this affects my work. It's not a big deal for me, though it may be for others. I just wanted to make sure that this is deliberate and well-thought-out. Thanks!

[tbaeg]

Sure. Like I said, I will be updating a bunch of authenticators so this affects my work. It's not a big deal for me, though it may be for others. I just wanted to make sure that this is deliberate and well-thought-out. Thanks!

I think your concern is valid and warranted. I will happily change the name to createAuthenticatedIdentity.

[ksobolew]

Thank you :)

[kokosing]

Some authentication methods can also have authorization information (i.e. - Identity.groups population using JWT claims

Populating groups as part of the authentication is wrong path. We had that for oauth2 and we realized it caused a lot of issues problems with views. For groups you should be using group providers.

[tbaeg]

Some authentication methods can also have authorization information (i.e. - Identity.groups population using JWT claims

Populating groups as part of the authentication is wrong path. We had that for oauth2 and we realized it caused a lot of issues problems with views. For groups you should be using group providers.

I don't understand the specific problem that was encountered, but populating group information is just an example of what is possible; not a requirement. This just gives extension writers more flexibility in shaping the Identity, which I think is a step toward increased adoption without having to change authenticators in trino-main. Additionally, there is already support for groups via claims in the OAUTH2 mode here.

This is outside of the scope of this PR, but I'd love to see movement away from Authenticators to something like IdentityProvider. This would streamline Identity provisioning and provide a simple interface to implement both authentication and authorization aspects. Happy to discuss more via the slack channel for those that are interested.

[kokosing]

I don't understand the specific problem that was encountered

#15669

This is outside of the scope of this PR, but I'd love to see movement away from Authenticators to something like IdentityProvider. This would streamline Identity provisioning and provide a simple interface to implement both authentication and authorization aspects.

Can you please elaborate?

Happy to discuss more via the slack channel for those that are interested.

I like to post this on github as it is then easier to find in future.