Consider session not logged in if unverified grace period expired

[janko]

Checking whether the unverified grace period expired in require_login created a discrepancy with the logged_in? method, where require_login could now return a login required response while logged_in? still returns true. I introduced this back in https://github.com/jeremyevans/rodauth/pull/211.

This can be problematic in scenarios where the application does some logic based on logged_in? before calling require_login. Checking unverified grace period expiration would also be skipped in potential applications that check only logged_in? without calling require_login. Here is an example similar to what we had in a Rails app:

class App < Roda
  plugin :rodauth do
    # ...
  end

  route do |r|
    # Would call `nil.owned_club` when unverified grace period has expired,
    # because `rodauth.account_from_session` wouldn't find the account.
    @current_club = current_account.owned_club if rodauth.authenticated?

    r.on "api" do
      rodauth.require_authentication
      # ...
    end
  end

  private

  def current_account
    account = rodauth.account || rodauth.account_from_session if rodauth.logged_in?
    @current_account ||= Account.load(account) if account
  end
end

We fix this by checking whether unverified grace period expired in logged_in?, which will then be applied to require_login as well. We also remove clearing of session in this case, because having the expired account ID in the session shouldn't be problematic now that logged_in? will return false.

[jeremyevans]

Thanks for the patch! I agree about changing logged_in? to return false for unverified accounts. I think it may still be safest to continue clearing the session once the grace period expires in require_login, because you aren't sure what the session contains, and it may contain things that should not be available after the grace period expires. Not clearing the session could theoretically introduce security issues in such cases. For example, rodauth.session_value would return the account id for the unverified account if the session was not cleared. That being said, this is definitely a tradeoff, as there are advantages for not clearing the session. I'd be OK with an configuration method for whether to clear the session in this case. What are your thoughts on that?

[janko]

Sounds reasonable, I updated the code to add back clearing of the session 👍🏻 My main motivation behind removing it was to simplify code, so I didn't add a configuration option, I was thinking it would make sense to add it only if it's requested.