jeremyevans / rodauth

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

Unable to get an JWT refresh token using an access token from an expired session #183

Closed renchap closed 2 years ago

renchap commented 2 years ago

As discussed on the Google Group, if you use active_sessions + jwt_refresh and allow_refresh_with_expired_access_token? true, then you are not able to get a new access token using your refresh token + access token, as the session is expired.

Here is a test showing this behaviour:

# spec/jwt_refresh.rb
  it "should allow refreshing token when providing expired access token if configured with active_sessions but session is expired" do
    rodauth do
      enable :active_sessions, :login, :logout, :jwt_refresh, :close_account
      hmac_secret SecureRandom.random_bytes(32)
      jwt_secret '1'
      jwt_access_token_period{1800}
      allow_refresh_with_expired_jwt_access_token? true

      session_inactivity_deadline{50}
      session_lifetime_deadline{100}
    end
    roda(:jwt) do |r|
      rodauth.check_active_session
      r.rodauth
      rodauth.require_authentication
      response['Content-Type'] = 'application/json'
      {'authenticated_by' => rodauth.authenticated_by}.to_json
    end

    res = jwt_refresh_login
    refresh_token = res.last['refresh_token']

    res = json_request("/jwt-refresh", :refresh_token=>refresh_token)
    jwt_refresh_validate(res)
    refresh_token = res.last['refresh_token']

    json_request('/').must_equal [200, {"authenticated_by"=>["password"]}]

    # We expire the current session
    DB[:account_active_session_keys].update(:last_use=>Time.now - 15, :created_at=>Time.now - 150)

    # Our access token is correctly expired
    json_request('/').must_equal [401, {"reason"=>"inactive_session", "error"=>"This session has been logged out"}]

    # But we should be able to use our refresh token to get a new access token, hence a new session
    res = json_request("/jwt-refresh", :refresh_token=>refresh_token)
    jwt_refresh_validate(res)
    refresh_token = res.last['refresh_token']

    json_request('/').must_equal [200, {"authenticated_by"=>["password"]}]
  end

I think this is not the correct behaviour, as the refresh token have their own expiration, they should not check the expiration of the session (but should probably still check if the session has not been logged out globally).

jeremyevans commented 2 years ago

I did some research on this. There are a few related commits to allowing JWT refresh with an expired access token (f89179ca9718ba6bd356330b5f9272b4a5a60216, 1843f9b70162e938dedc2bdbcdfe8fc1ca5962bb, e9bfc815ef544fcbbee363a1ed275f8ec09377f5). All of the previous cases focus on allowing decoding of the expired JWT. The difference in this example is that instead of access token being expired, you are expiring the active session. Because the session is no longer considered active, the request stops at check_active_session. The current behavior makes sense and is not a bug. You are asking rodauth to check for active sessions, and the session is not active at the point of the request, therefore the request fails.

Basically, in your app, you don't want to do that check for active sessions if the current route is the jwt_refresh route. That is best done as a conditional on the jwt_refresh route in the user code, IMO. I suppose it could be handled automatically by Rodauth, in one of two ways:

  1. Modify the active_sessions feature to check if the jwt_refresh feature is loaded, and if so and the current request is for the jwt_refresh route, have currently_active_session? return true even if the session is not active.

  2. Add another feature that depends on jwt_refresh and active_sessions, and overrides currently_active_session? to do something like super || request.path == jwt_refresh_path.

I'm definitely against option 1, as interdependencies between features that are not strict dependencies is discouraged, and there isn't a good way to do the integration that doesn't result in something similar to respond_to?(:jwt_refresh_route). I'm willing to consider option 2, though I don't think it will do what you want, see below.

Neither approach will fix your spec. Your spec still wouldn't work for two reasons. The first reason is this line:

json_request('/').must_equal [401, {"reason"=>"inactive_session", "error"=>"This session has been logged out"}]

This will attempt to request the root page (not the jwt_refresh page), but since the active session has expired, you get logged out, and a new access token will be provided in that case, which won't have the jwt refresh information. If you remove the above line, this spec still fails at the final line:

json_request('/').must_equal [200, {"authenticated_by"=>["password"]}]

That's because refreshing a JWT does not update the active session, which has already expired. So even after refreshing the JWT, you still don't have an active session. Updating last_use on the active session when refreshing won't fix it, since the active session has an expired lifetime, it's not just expired due to inactivity. The only way to fix that would be to change active_sessions to add the current session as a new active session when refreshing the token. That seems like it would introduce a vulnerability, as it would allow you to circumvent active session lifetime restrictions via JWT refreshes. So I definitely would not consider that approach in Rodauth.

Ultimately, if your active_session expires, you shouldn't be doing a jwt refresh, you should be doing a login. Or as you mentioned in the Google Group, you could use jwt_refresh for JWT sessions, and active_sessions for non-JWT sessions.

renchap commented 2 years ago

Thanks a lot for your research and explanation, I understand the implications much better now!

I would like to keep the ability for the user to log out all sessions globally, including the JWT-based sessions. But for those sessions, having an expiration does not really make sense because both the access token and the refresh token already have their own lifetime, so this feature of active_sessions is redundant.

Would it make sense to do something like this to achieve this result?

session_inactivity_deadline { 1.day.to_i unless use_jwt? }
session_lifetime_deadline { 30.days.to_i unless use_jwt? }
jeremyevans commented 2 years ago

That looks like it would work.

renchap commented 2 years ago

After reading the code for active_sessions, I am not sure it will in fact work as sessions can be removed from the table by remove_inactive_sessions which will use the current session configured deadline.

In many cases the user will have both a JWT-based (mobile app) and web-based session (desktop browser), and the mobile session record will be deleted when remove_inactive_sessions is invoked from a web request 😞

jeremyevans commented 2 years ago

I think your analysis is correct. If you don't want that behavior, you might have to add a column to the active sessions table, flagged differently for JWT and non-JWT, and have the removal of inactive sessions not include JWT sessions. That should be possible using the existing configuration methods. This does beg the question of how you would expire JWT active sessions, though.