Strategies shouldn't store by default

[ahills]

I was surprised to learn that my strategy was opening another avenue of access to my application. At minimum the #store? method should be documented in Strategies, but I also believe that this behavior goes against the grain of "default deny", a security fundamental.

I saw a reference in #111, but no explanation of the reasoning behind the decision.

[hassox]

I'm a bit concerned that this is a pretty fundamental change that would break a lot of things. I like the idea but not sure how to migrate from here without breaking everything.

[ahills]

That's also a concern of mine; maybe it'd be best if such a change didn't land until a minor version bump (for those fortunate enough to have used ~> 1.2.3). Alternately, an exception could force subclasses to implement the method, but that feels wrong for other reasons.

[hassox]

Perhaps a configuration option that let you set it to not store by default so you can just call it once?

[ahills]

That would work until you include a gem that assumes the default is true, though.

[hassox]

ah true facts. I guess the first step is to set a deprecation message when a strategy doesn't implement the store? method saying that it's going away at the next major revision. Then work towards a 2.0

[waiting-for-dev]

Besides what has been discussed so far about removing default storing in strategies (which I agree), I think it could be insufficient for unaware users in some scenarios.

For example. Say a library extends Warden just to supply a token authentication strategy. Usually, a user would not use it alone. In a Rails application, probably he would use it combined with Devise, relying on its DatabaseAuthenticatable strategy for the very first authentication (when a token does not exist, yet).

Following with this example, in the most common scenario this would not suppose a risk. Devise checks whether env['devise.skip_storage'] is set in order to decide if it should #store? the user. That key is set when CSRF token validation fails, and (having that kind of authentication) probably the application is an API which clients do not send any CSRF token.

But with users out there wanting to experiment with shiny new authentication systems, I think we can't dismiss the possibility that the frontend is Rails and it is sending valid CSRF tokens in its forms. Furthermore, Devise does a good assumption but other libraries may not.

For this reason, I think it would be desirable to have a configuration option acting like a switch between allowing user storage or not. Should that switch disallow, user would not be stored regardless of what individual strategies say. Token strategy library author could set that switch to 'disallow'.

This change could be backward compatible defaulting to 'allow'.

[waiting-for-dev]

Nothing here? It is an important security threat and it is open since 2015...

[jsmestad]

To get this merged in, we'd have to do the following:

1. Get this branch up to date with latest master
2. Get a PR where a deprecation warning is posted to STDOUT if a strategy does not overwrite the store? method. Release that in a minor patch release, notifying that upcoming versions will switch this value to false.
3. Release a new major version where store returns NotImplementedError of some sort and force strategies to upgrade.

@waiting-for-dev if you want to jump on that, I can get it merged and deployed.

[waiting-for-dev]

Thanks for the repply @jsmestad . Ok, I'll look into it.

[waiting-for-dev]

Hey, sorry, with the time I forgot which was my actual problem 😅

@jsmestad , your proposed solution would not address my concern. I'll try to explain it better. I'm the developer of warden-jwt_auth and devise-jwt. In them, a warden strategy is used which authorizes if a valid token is present in the headers.

The problem I found while integrating with devise is that users are firstly authenticated through devise DatabaseAuthenticatable strategy. Then, they get the token in the response and can use the JWT strategy from then on.

The issue is that if the session storage is not disabled and the CSRF rails protection is deactivated (which is used by devise to decide not to #store?, the DatabaseAuthenticatable strategy persists the user in the session. Therefore, the JWT strategy can be bypassed and the user is authenticated from the session.

What I think we need is a way to declare that a scope won't be persisted to the session regardless of what an individual strategy says. My users configure scopes that are mean to be authenticated through a stateless API. For them it makes no sense to persist to the session in any strategy.

Does it make sense?

[waiting-for-dev]

In fact, my proposed solution is not acceptable, because a scope could be used to authenticate both an API and an html request.

I think that the correct way to handle this would be to move the authentication from the session to a strategy. This way, having to be explicitly configured, the user would be aware of this.

[waiting-for-dev]

Hey @jsmestad , closing the issue means that no action on this will be performed? Thanks for any feedback.

[jsmestad]

@waiting-for-dev I'd be open to getting it fixed but reading your last commit it sounded like a different approach was necessary. Did I misunderstand?

[waiting-for-dev]

Yes, I think that the best fix would be to convert the authentication from the session to a strategy. WDYT?