jeremyevans / rodauth

Ruby's Most Advanced Authentication Framework
http://rodauth.jeremyevans.net
MIT License
1.69k stars 95 forks source link

Parameterize active sessions DS and helpers by ID #137

Closed bjeanes closed 3 years ago

bjeanes commented 3 years ago

This allows clearing sessions for a specific ID (e.g. after a password reset, when session_value is blank)

It isn't clear to me if the session_value vs account_id is important or arbitrary for this feature. Because I don't understand this decision, I look forward to your advice on which this should be. All that motivates this PR is the ability to parameterise it one way or another. Unlike the remember feature (for instance, which parameterizes with a default of account_id), I have kept this default as session_value to be conservative.

jeremyevans commented 3 years ago

In general I don't want to have configuration methods take optional arguments. remove_remember_key and csrf_tag are the only configuration methods that currently do, and if not for backwards compatibility, I would probably change both. Additionally, adding optional parameters to configuration methods that don't currently take parameters is bad for backwards compatibility, because then code that starts an already defined configuration method with the optional argument would break.

One possibility is adding new configuration methods that take a mandatory parameter. Alternatively, if a simpler fix is session_value || account_id in active_sessions_ds, that may be preferable.

In general, it's best to use the session_value if it is set, because that means the user is logged in. account_id can represent a user that is not logged in, and is at greater risk of misuse. Which is used in specific features is usually dependent on the feature's implementation, since the methods were designed primarily to implement the feature, and only secondarily for being overridable by configuration methods.

bjeanes commented 3 years ago

That makes sense on some fronts. In particular, that this is in fact a breaking change if any of these methods are overridden.

How do you want to proceed?

My use case really does demand some sort of parameterisation because it is for customer support to force the logout of a specific user across all sessions. However, I have been tackling this elsewhere by newing up my own Rodauth instance and doing instance_eval { account_from_login('foo@bar.com'); whatever_else_here }. If we merely had this fallback on account_id, this approach would work for my immediate needs.

That being said, while my integration with Rodauth and its rollout to production has been broadly positive, these cases have been the only real pain points. There are lots of scenarios (particularly around customer support tooling that becomes necessary for companies with larger numbers of users, like is the case for me) where I've needed to operate on a user but not necessarily as that user. BUT, that is probably a better conversation for the mailing list.

jeremyevans commented 3 years ago

For admin access, instead of trying to modify Rodauth's internals to patch around the fact that Rodauth expects to operate in the context of the logged in user, we should probably work on an API like:

rodauth = Roda.rodauth.for_account(1)
rodauth.remove_all_active_sessions

Basically, allow you easily create a Rodauth object with a defined account_id and session_value, so that you can call whatever methods you need in that context of that account, which should work without modifying Rodauth's internals.

You are correct that something like that would best be discussed on the mailing list.

bjeanes commented 3 years ago

Ack. That sounds like a good idea. I'll bring it up in the mailing list (though, frankly, it may be early next year as I am with family now).

As to this PR, how do you want to proceed? Even with an API along the lines of your proposal, it would need to be de-coupled from session_value in particular (or, as you gesture, also set this). Shall I open a new PR which just falls back to account_id here or do you have reservations about misuse for this. For now, I am working with the data-set directly, so I am not blocked, if you'd like to think about it for a while longer.

jeremyevans commented 3 years ago

The approach I envision with for_account would return a rodauth object with a defined account_id and session_value, so it wouldn't matter which of those the feature uses. Basically, you will not need the changes in this PR with the approach I envision, so I think it would be safe to close this.

bjeanes commented 3 years ago

OK. I'll close and move to a broader discussion in the mailing list in the coming weeks. Thanks!