zamzterz/Flask-pyoidc

Session remains authenticated when session_refresh_interval_seconds is None and the access token in cookies is expired

Closed this issue · 6 comments

This line checks for if the access token should be refreshed. If ProviderConfiguration.session_refresh_interval_seconds is None, the condition here will always be False.

if session.should_refresh(client.session_refresh_interval_seconds):
logger.debug('user auth will be refreshed "silently"')
return self._authenticate(client, interactive=False)

So it moves to the next condition that checks for if the session is still authenticated by verifying the existence of _session_storage.last_authenticated.

elif session.is_authenticated():

"""
flask_session is empty when the session hasn't been initialised or has expired.
Thus checking for existence of any item is enough to determine if we're authenticated.
"""
return self._session_storage.get('last_authenticated') is not None

The problem is this will be True even if the access token is expired.

My solution:

def is_authenticated(self):
    """
    flask_session is empty when the session hasn't been initialised. Checking for the
    expiration of access token is enough to determine if we're authenticated.
    """
    if self._session_storage.get('access_token_expires_at'):
        return self._session_storage.get('access_token_expires_at') > time.time()
    return False

What do you think?

The is_authenticated method is intentionally not tied to access token expiration - the browser session should still be considered valid to not force the user to login again as the access token might still be possible to refresh.
OIDCAuthentication.valid_access_token is the function that checks access_token_expires_at, to see whether the access token should be refreshed.

Then we should check the expiry of refresh token at least. Otherwise, even if the user is coming back after 6 months, he will still be able to use the session if session_refresh_interval_seconds is set to None.

The refresh token lifetime is not returned by the IdP, so that functionality is still only a best effort request.
(Also there is no guarantee the refresh token has not been invalidated by some other mechanism (for example "logout from all devices" from the IdP by the user).
The browser session lifetime can be controlled separately via Flasks config option PERMANENT_SESSION_LIFETIME.

I see refresh_expires_in in token response from Keycloak.

{
    'access_token': 'eyJhbG...',
    'expires_in': 60,
    'refresh_expires_in': 1800,
    'refresh_token': 'eyJhbG...',
    'token_type': 'Bearer',
    'id_token': {...},
    'not-before-policy': 0,
    'session_state': '5e116f...',
    'scope': 'openid profile email',
    'id_token_jwt': 'eyJhbG....'
}

OpenID Connect specs on token lifetime also has validity for refresh token.

Both those are implementation specific attributes; neither RFC 6749 (OAuth 2.0) nor OIDC specifies that response value.
So unfortunately that's not something that can be relied on being in the response.

Then how about this:

If ProviderConfiguration.session_refresh_interval_seconds is None, access token will not be refreshed automatically so verify the validity of the access token and re-prompt the user for login if the access token is no longer valid. Now the user session is truly secure.

def is_authenticated(self):
    """
    flask_session is empty when the session hasn't been initialised. Checking for the
    expiration of access token is enough to determine if we're authenticated.
    """
    # If ProviderConfiguration.session_refresh_interval_seconds is None,
    # access token will not be refreshed automatically so verify the
    # validity of the access token and re-prompt the user for login if the
    # access token is no longer valid.
    if self.access_token_expires_at and self._session_refresh_interval_seconds is None:
        return self.access_token_expires_at > time.time()
    return self.last_authenticated is not None