"Remember Me" functionality

bravegag commented 5 years ago

This PR corresponds to the comments under issues #360. We have tested all the functionality in the play-authenticate-usage example.

bravegag commented 5 years ago

We missed to add the messages in all languages :)

bravegag commented 5 years ago

The checks seem to be broken on the smaller example. I fixed it.

bravegag commented 5 years ago

This looks like travis is getting an old PA artifact from a repo other than local ... ?

bravegag commented 5 years ago

Hi @joscha can you please take a look at the CI? looks like it is picking an old PA artifact

joscha commented 5 years ago

@bravegag looking great.

A few things:

can you change the code so it is a backwards compatible addition? The CI fails because there were a few parameters added to the core without overloading the available methods, but just changing the ones that are there. This will break any custom providers. As PA was built with custom providers in mind, there are quite a few people out there that use their own. I think some of these would break if the core was changed in a way that is incompatible.
Can you explain the Cookie series a little bit, I am not following what exactly this does
Can we guard the unlink in some way, maybe disable it by default and/or only allow unlinking of whitelisted providers? Is adding the unlinking needed for the CookieProvider or could we just invalidate the record? What happens if I log into a lot of different machines/browser windows - each time we would add a cookie, entry, right? I can see a potential overflow attack there - could we limit the number of attachable sessions?

bravegag commented 5 years ago

Hi @joscha, thanks! we will do 1) and 3) as you ask.

2) the cookie series helps detect and protect against potential cookie theft. Whenever there is a login with "Remember me" a new series is generated and this is implemented according to the spec here: http://jaspan.com/improved_persistent_login_cookie_best_practice

The Solution

My solution to this problem is based on the observation that since each token can only be used once, a remembered login session actually consists of a series of such tokens. When an attacker successfully steals and uses T_0 he is issued T_1, but the victim is still holding T_0. The database no longer has a copy of T_0 and thus cannot differentiate it from an arbitrary invalid token.

However, if the series of tokens is itself given an identity that must be presented along with the current token, the system can notice that the victim is presenting a valid series identifier along with an invalid token. Assuming that the series identifiers are as hard to guess as tokens, the only way a user could present a valid series identifier with an invalid token is if some other user previously presented the same valid series identifier with a valid token. This requires that two different users held the same series and token pair at the same time and therefore indicates that a theft has occurred.

bravegag commented 5 years ago

Hi @joscha

Can you please help us (@alexeysavenkov and I) push this through, as soon as that's done we are gonna work on the 2 Step Auth with Google Authenticator integration bit.

Without 1) and possibly 2) the "Remember Me" functionality won't work. I think it doesn't make sense to overload the methods as you suggest and by doing so then either enjoy the feature "Remember Me" or not. I believe this is error prone and confusing. If we want new functionality inevitably we might end up not being backward compatible, having release notes and migration guides ... "Remember Me" requires passing the context around and not session. If you have suggestions we can check but as I see it this is the only way.

@alexeysavenkov can you please also comment?

alexeysavenkov commented 5 years ago

Hi @joscha,

We changed the Session argument to Context in several methods, because to authorize with cookie Session is not enough, we need both Session and cookies. Context gives us both.
Whether we invalidate the record or delete it depends on UserService implementation. Personally, I don't think we need to store invalidated cookies in database in usage sample.
Not sure if it's possible to guard unlink somehow and if we need it. I think it's clear what unlink does, so it's unlikely that somebody will accidentally call it in custom providers.
Yes, we can limit the number of cookie sessions. E.g. if the limit is 10, and user authorizes with 11th cookie, the oldest one gets dismissed.

joscha commented 5 years ago

We changed the Session argument to Context in several methods, because to authorize with cookie Session is not enough, we need both Session and cookies. Context gives us both.

Context was different in Java and Scala in the past, hence why there was no easy way of using it. I am not saying we can't use Context, however I think backwards compatibility is not optional unless we also want to open the pandora's box of migration, etc. Ideally we could find a way to add this new feature whilst not breaking all custom providers out there at the same time.

alexeysavenkov commented 5 years ago

@joscha One of the solutions is to have pairs of methods: with session, and with context. And to mark those which receive session as deprecated, because they are not able to do cookie authorization.

bravegag commented 5 years ago

@joscha I'm migrating my fork to 0.9.0 ... can we not merge away this PR?

bravegag commented 5 years ago

@josha The PR fork master branch is now at the same level as 0.9.1-SNAPSHOT.

KrushnaOrigoNetworks commented 5 years ago

Hi @joscha, I do not find those changes in 0.9.1-SNAPSHOT. Can you please let me know in which branch it is available? I am trying to run the app available at https://github.com/bravegag/play-authenticate-usage-scala and it fails with message "value isAuthorizedWithCookie is not a member of com.feth.play.module.pa.PlayAuthenticate"

joscha / play-authenticate

"Remember Me" functionality #363