Potential bug in remember logic for login when login is skipped

[doubliez]

Describe the bug

When login is skipped and I call "accept login request" with only a subject (without remember params), the oauth2_authentication_session cookie is reset to be a session cookie instead of maintaining the same expiry date it had on first login (based on remember_for value).

Reproducing the bug

Steps to reproduce the behavior:

Should be reproducible with the sample login-consent app provided by Hydra, which is what I based my implementation on.

In the sample code here: https://github.com/ory/hydra-login-consent-node/blob/f4605748c2500f113813bc87b21c4875fc04694d/routes/login.js#L30

Only the subject is passed when accepting the login request (in case it was skipped). This for me causes the oauth2_authentication_session cookie to be reset to a session cookie. If I specify the remember params, the lifetime of the cookie is set accordingly. However for skipped login I don't want to have to re-specify remember params, it should not touch the cookie at all in my opinion.

Expected behavior

When accepting a skipped login request, the cookie used to remember the user should not be altered, and maintain its expiry date if it was set on first login through remember_for param.

Environment

• Version: oryd/hydra:v1.0.0 (Docker image)
• Environment: Docker

[aeneasr]

This is intended behavior. If a user is requested to sign in again, and remember for is not set, then the cookie has to be overwritten as the user now chose not to remember the login. Not handling according to this logic would keep the cookie around even though the user chose not to stay logged in.

[doubliez]

@aeneasr In this case the user is not explicitly requested to sign in again, there is no user interaction. I'm using a prompt=none silent refresh within a hidden iframe in my SPA, and this behavior means that even though on the first (fresh) login the user chose to remember its session, when the first silent refresh occurs this will override the decision of the user...

For me that's unintended behavior. When login is skipped, Hydra should not override the initial decision of the user to remember the login. This defeats the purpose of the "remember me" feature when using silent refresh.

But maybe I'm doing something wrong?

[aeneasr]

It’s not possible for hydra to know that. You can set remember to !skip || request.form.remember (pseudocode) to achieve the behavior you want. I believe this is documented but if not it definiteöy shouöd be!

…

On 14. Sep 2019, at 18:36, Thibault Doubliez ***@***.***> wrote: @aeneasr In this case the user is not explicitly requested to sign in again, there is no user interaction. I'm using a prompt=none silent refresh within a hidden iframe in my SPA, and this behavior means that even though on the first (fresh) login the user chose to remember its session, when the first silent refresh occurs this will override the decision of the user... For me that's unintended behavior. When login is skipped, Hydra should not override the initial decision of the user to remember the login. This defeats the purpose of the "remember me" feature when using silent refresh. But maybe I'm doing something wrong? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[doubliez]

To be clear, on first login, the session is remembered for 30 days using:

hydra.acceptLoginRequest(challenge, {
    subject: user.id,
    remember: true,
    remember_for: 2592000
});

(when the user checks the "Remember me" checkbox)

This appropriately sets the expiry date of the oauth2_authentication_session cookie to 30 days from now. But because this gets unexpectedly reset on subsequent skipped logins, I had to work around it by keeping track of the remainder TTL for the user session myself, and re-specify the remember parameters based on that when login is skipped:

if (response.skip) {
    hydra.acceptLoginRequest(challenge, {
        subject,
        remember: true,
        remember_for: remainingTTLForSession
    });
}

This way I can maintain the expected expiry date on the session.

On a side note about remember_for, the documentation says:

RememberFor sets how long the authentication should be remembered for in seconds. If set to 0, the authorization will be remembered indefinitely.

However when set to 0, the oauth2_authentication_session cookie will be a session cookie, therefore the login will only be remembered as long as the user doesn't close its browser. Which is correct behavior in my opinion, but the documentation is misleading on that, it will not be "indefinitely" remembered.

[aeneasr]

Oh I see - the cookie should definitely not be reset on consecutive logins when remember=true. rememberFor should be ignored in that case (needs to be documented) Would you be up for investigating the cause/contributing a PR? :) I think the documentation is mot up to date and a value of -1 should do the trick - but I need to confirm that (back in the office on monday)

…

On 15. Sep 2019, at 04:24, Thibault Doubliez ***@***.***> wrote: To be clear, on first login, the session is remembered for 30 days using: hydra.acceptLoginRequest(challenge, { subject: user.id, remember: true, remember_for: 2592000 }); (when the user checks the "Remember me" checkbox) This appropriately sets the expiry date of the oauth2_authentication_session cookie to 30 days from now. But because this gets unexpectedly reset on subsequent skipped logins, I had to work around it by keeping track of the remainder TTL for the user session myself, and re-specify the remember parameters based on that when login is skipped: if (response.skip) { hydra.acceptLoginRequest(challenge, { subject, remember: true, remember_for: remainingTTLForSession }); } This way I can maintain the expected expiry date on the session. On a side note about remember_for, the documentation says: RememberFor sets how long the authentication should be remembered for in seconds. If set to 0, the authorization will be remembered indefinitely. However when set to 0, the oauth2_authentication_session cookie will be a session cookie, therefore the login will only be remembered as long as the user doesn't close its browser. Which is correct behavior in my opinion, but the documentation is misleading on that, it will not be "indefinitely" remembered. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[doubliez]

From what I can see the oauth2_authentication_session cookie is set at the end of the verifyAuthentication function here:

hydra/consent/strategy_default.go

Lines 437 to 451 in 32e23bc

	cookie, _ := s.r.CookieStore().Get(r, CookieAuthenticationName)
	cookie.Values[CookieAuthenticationSIDName] = sessionID
	if session.RememberFor >= 0 {
	cookie.Options.MaxAge = session.RememberFor
	}
	cookie.Options.HttpOnly = true
	if s.c.ServesHTTPS() {
	cookie.Options.Secure = true
	}
	if err := cookie.Save(r, w); err != nil {
	return nil, errors.WithStack(err)
	}
	return session, nil

It seems to me that the following condition should be added just before that:

if session.Remember && session.LoginRequest.Skip {
    return session, nil
}

Because when the session was remembered and login is skipped, the cookie shouldn't be altered.

[aeneasr]

Yes! Additionally, we should add modify this

https://github.com/ory/hydra/blob/master/consent/strategy_default.go#L439-L441

to

	if session.RememberFor == -1 {
		cookie.Options.MaxAge = // some very large value
	} else if session.RememberFor >= 0 {
		cookie.Options.MaxAge = session.RememberFor
	}

Would you be up for a PR?

[doubliez]

Sure: #1564

I was unsure about adding a magic number tied to RememberFor == -1, it feels unnecessary but feel free to add it.

My only complain was that the documentation was incorrect in saying that RememberFor == 0 means "indefinitely remembered session". The documentation should be changed to say that a value of 0 will make it a session cookie, and therefore make the login persist until the user restarts its browser.

[aeneasr]

My only complain was that the documentation was incorrect in saying that RememberFor == 0 means "indefinitely remembered session". The documentation should be changed to say that a value of 0 will make it a session cookie, and therefore make the login persist until the user restarts its browser

Yes, definitely. Would you be up to fixing that as well? :)