Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Impersonation for non-persistent authenticators #648

Closed
segy opened this issue Nov 24, 2023 · 23 comments
Closed

Impersonation for non-persistent authenticators #648

segy opened this issue Nov 24, 2023 · 23 comments

Comments

@segy
Copy link
Contributor

segy commented Nov 24, 2023

Hi

we were planning to implement impersonation for a stateless authenticator and it looked logical to me, to implement ImpersonationInterface, so we can use auth component for that. But this interface extends PersistenceInterface. I don't see any logic in this inheritance. Could you please explain, if this is needed? If not, I can create PR for the necessary changes.

Thank you

@ADmad
Copy link
Member

ADmad commented Nov 24, 2023

How do you expect impersonation for stateless authentication to work?

@segy
Copy link
Contributor Author

segy commented Nov 24, 2023

we have this scenario (simplified) for JwtAuthenticator:

  • we have user encoded in the token
  • when we start the impersonation, the service will return a modified token containing also impersonation information
  • when he stops, it returns the standard token

@segy
Copy link
Contributor Author

segy commented Nov 24, 2023

and the thing is, i couldn't find any reason for ImpersonationInterface to extend the PersistenceInterface. even the SessionAuthenticator (that is the only class implementing this) has a code implements PersistenceInterface, ImpersonationInterface, so the only change here would be to remove extends PersistenceInterface.

@markstory
Copy link
Member

when we start the impersonation, the service will return a modified token containing also impersonation information
when he stops, it returns the standard token

Where will you store the standard token if authentication is stateless?

@segy
Copy link
Contributor Author

segy commented Nov 27, 2023

nowhere. if we carry information about the impersonator, we can simply issue a new one when he decides to stop impersonating.

@markstory
Copy link
Member

nowhere. if we carry information about the impersonator, we can simply issue a new one when he decides to stop impersonating.

So how would stopImpersonating and isImpersonating work? If you just want to manage the impersonation credentials yourself you should be able to do that already.

@segy
Copy link
Contributor Author

segy commented Nov 28, 2023

Of course I am able to do that. But sorry... I don't understand, what we are talking about :) If you insist, that the ImpersonationInterface extending PersistenceInterface is necessary and you don't want to change it, just close this.

@segy
Copy link
Contributor Author

segy commented Nov 28, 2023

but to answer your question: the isImpersonating obtains the request (and the token, which contains this information, is in the header). the impersonate and also stopImpersonating allows us to modify the response, so this logic can be handled in the authenticator.

@ADmad
Copy link
Member

ADmad commented Nov 28, 2023

I don't understand, what we are talking about :)

Whenever someone opens an RFC we first try to understand the use case before we take a decision on whether to go ahead with the changes, hence the questions.

@segy
Copy link
Contributor Author

segy commented Nov 29, 2023

i added a proposed change, so you can consider.

@markstory
Copy link
Member

But sorry... I don't understand, what we are talking about :) If you insist, that the ImpersonationInterface extending PersistenceInterface is necessary and you don't want to change it, just close this.

Part of the ImpersonationInterface is that it remembers what your identity was before the impersonation began, and can restore that with stopImpersonating and check whether or not a user is being impersonated with isImpersonating.

I'm interested in how you see these method working with stateless authentication methods? For example basic auth or token based auth? Is that behavior you are expecting from the framework or are application authenticators that implement the ImpersonationInterface going to need to figure that out?

@segy
Copy link
Contributor Author

segy commented Dec 1, 2023

OK. But do you think, that the fact, that it will be impossible for some authenticators to implement ImpersonationInterface, is a good reason for it to extend PersistenceInterface? I think that this is a responsibility of the authenticator that implements it to handle all the methods that are required by the interface. What is the benefit of that inheritance in this case?

@markstory
Copy link
Member

I think that this is a responsibility of the authenticator that implements it to handle all the methods that are required by the interface. What is the benefit of that inheritance in this case?

I agree that each authenticator will need to manage the state themselves. Having the inheritance formalizes that there needs to be some persistent state managed somewhere, and gives application developers the necessary hooks to have state clearing integrated well. For example AuthenticationService::clearIdentity both resets impersonation and removes the identity.

Impersonation requires some state to be persisted somewhere and having an authenticator pretend it is stateless but also have persistent state feels awkward.

@segy
Copy link
Contributor Author

segy commented Dec 4, 2023

so even this is possible to achieve with JwtAuthenticator, you think it's not worth merging?

@segy
Copy link
Contributor Author

segy commented Dec 6, 2023

please let me know. i wanted to have this process unified using component and if this is not merged, i'd need to think of other solution. thanks

@markstory
Copy link
Member

 so even this is possible to achieve with JwtAuthenticator, you think it's not worth merging?

This isn't possible with the JwtAuthenticator today. Or are you saying it could be done with JWT authenticator.

@segy
Copy link
Contributor Author

segy commented Dec 7, 2023

Yes. I wanted to say it could be done.

@segy
Copy link
Contributor Author

segy commented Dec 14, 2023

so please can we somehow close this? either as accepted or rejected? thank you

@markstory
Copy link
Member

markstory commented Dec 15, 2023

so please can we somehow close this? either as accepted or rejected? thank you

My issue with this change is that removing the PersistentInterface isn't very logical as if we do, the authenticator needs to have state stored somewhere for the component/service logic to work. If that state is required, what benefits are there to storing it somewhere else?

so even this is possible to achieve with JwtAuthenticator, you think it's not worth merging?

If you're proposing we have a stateful JwtAuthenticator that enables impersonation, then that is something that is more interesting. As a framework I think we should be helping enable common patterns, and having JWT authentication alongside an impersonation mechanic fits nicely with those goals.

Edit: To clarify, the 'state' could be held within the JWT token and not elsewhere.

@ADmad
Copy link
Member

ADmad commented Dec 15, 2023

To clarify, the 'state' could be held within the JWT token and not elsewhere.

That's what he suggested in an earlier comment #648 (comment).

When impersonification is started they generate a new token which contains additional info regarding the user being impersonated and on stopping revert back to a "normal" token.

How the token switching is done can be left to the users and we don't need to do anything extra in the plugin, just update the ImpersonationInterface as suggested.

@markstory
Copy link
Member

When impersonification is started they generate a new token which contains additional info regarding the user being impersonated and on stopping revert back to a "normal" token.

Ok. Good to see that we're thinking about the token switching in a similar way.

How the token switching is done can be left to the users and we don't need to do anything extra in the plugin, just update the ImpersonationInterface as suggested.

Shouldn't we at least provider an implementation of JwtToken with Impersonation? Having looked at the code further and talked through how the impersonation would work with a stateless authenticator, I am sold on removing the inheritance with PersistenceInterface.

@ADmad
Copy link
Member

ADmad commented Dec 17, 2023

Shouldn't we at least provider an implementation of JwtToken with Impersonation?

I am not opposed to it if someone wants to provide an implementation :)

@dereuromark
Copy link
Member

So is this still to be kept open? The related PR got merged.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants