jeremyevans / rodauth

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

Logout no longer active session when remembered #316

Closed janko closed 1 year ago

janko commented 1 year ago

If calling both rodauth.check_active_session and rodauth.load_memory in the routing tree, when the session becomes no longer active, the user will not be logged out if remembered. This is because the user will be autologged back in from the remember cookie on first request after the session was cleared. We fix this by also forgetting the user when the session is found to no longer be active.

jeremyevans commented 1 year ago

I'm not sure whether it makes sense to do this by default. In the spec given, the user is logged out, they are just logged in by the remember cookie immediately after being logged out. Any custom session keys would be lost, for example, and if they were logged in via password before the active session was removed, they will show as being logged in via the remember cookie after.

If someone has a shorter active session lifetime and/or inactivity deadline than the remember period, I'm not sure why an active session expiring should force removing the remember key from the browser. After all, if the user closed the browser before the active session expired, and reopened it afterward, they would still be logged in automatically via the remember token. I don't see a reason we should force different behavior for the case where they are using the site when the active session is expired.

Maybe we can allow for opt in to this behavior via a configuration method?

janko commented 1 year ago

Thanks for the feedback! I haven't considered that the user would still be autologged in via the remember cookie if they had their browser closed at the time their session expired.

I was implementing session revoking functionality that GitHub has, which currently deletes the session key record, and then expects that session to be logged out. Even if this PR was merged as is, due to the autologin scenario you described, it wouldn't guarantee the session would actually be logged out, which is the whole purpose of session revoking.

I was thinking if there is a way to prevent the autologin via remember cookie in this scenario, but I didn't find it with the current design. If you have any ideas, I would love to hear them. Otherwise, I think we can close this PR, as it doesn't fulfill the intended goal.

jeremyevans commented 1 year ago

If the user purposely chooses to logout, that deletes both the current active session and the remember cookie. If a global logout is used, all active sessions are deleted. However, the remember key is not removed, so a global logout can still result in the remember key being used in other browsers. Potentially we should remove the remember key as well when there is a global logout?

janko commented 1 year ago

The global logout already seems to remove the remember key: https://github.com/jeremyevans/rodauth/blob/f4ea6e8d94bd80f7eff8d96b90aadef0d3fb42ad/lib/rodauth/features/active_sessions.rb#L127-L135

In my case I don't want to logout globally, just revoke a specific session. However, I could rotate the remember key in this case. Sure, it might cause some other sessions to be logged out where they would otherwise be remembered, due to their remember cookie referencing the old token. But I think it's still better than not actually logging out from that browser.

class SettingsController < ApplicationController
  def revoke_session
    current_account.active_session_keys
      .where(session_id: params[:id])
      .delete_all

    # rotate remember token
    rodauth.remove_remember_key
    rodauth.remember_login

    redirect_to settings_sessions_path
  end
end

This would probably be too specific behavior for a configuration option in Rodauth, and in my case I want to do it in the custom endpoint that deletes the active session, which is outside of Rodauth. So, I'll close this PR, thanks again for the input 🙂