Repository: zamzterz/Flask-pyoidc

Issue:
Issues refreshing tokens when 'session_refresh_interval_seconds' is set
With the current state of flask_pyoidc we're running into the issue of not silently refreshing the tokens when it's desired.

Our current setup is as follows: API endpoints are protected using flask_pyoidc, with the session_refresh_interval_seconds providing `prompt=none` refreshing after 5 minutes.

The first issue we encountered was running into the silent refreshing when the initial authentication has not been successfully established yet.
The second issue was the refreshing being triggered based on the previous auth_time, which didn't change with the request. Instead, what worked was taking the expiry time provided by the OIDC instance to trigger the silent refresh that way.

To illustrate a proposed way of fixing it we've created this change (this is not PR-quality code by any means):
https://github.com/fredldotme/Flask-pyoidc/commit/8a062a1d9d281a80421c1e3adadd84c50ae12c7a

This forces the refresh after 5 minutes. Note that we've added 20 seconds of headroom before the expiry time runs out.

I'd like to get your suggestions on this topic and maybe a way forward in the form of code being merged into upstream.

Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/70?src=pr&el=h1) Report
> Merging [#70](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/70?src=pr&el=desc) into [master](https://codecov.io/gh/zamzterz/Flask-pyoidc/commit/cf3f5f8ed0507d310c70b40d13b49dd2a7b708b4?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/70/graphs/tree.svg?width=650&token=xbSknAkssg&height=150&src=pr)](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/70?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage   97.01%   97.02%   +0.01%     
==========================================
  Files           8        8              
  Lines         502      504       +2     
  Branches       55       55              
==========================================
+ Hits          487      489       +2     
  Misses          8        8              
  Partials        7        7
```


| [Impacted Files](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/70?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/flask\_pyoidc/user\_session.py](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/70/diff?src=pr&el=tree#diff-c3JjL2ZsYXNrX3B5b2lkYy91c2VyX3Nlc3Npb24ucHk=) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/70?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/70?src=pr&el=footer). Last update [cf3f5f8...b9a6354](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/70?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/src/flask_pyoidc/user_session.py b/src/flask_pyoidc/user_session.py
index 962f4db..4578db0 100644
--- a/src/flask_pyoidc/user_session.py
+++ b/src/flask_pyoidc/user_session.py
@@ -11,7 +11,15 @@ class UserSession:
     Wraps comparison of times necessary for session handling.
     """
 
-    KEYS = ['access_token', 'current_provider', 'id_token', 'id_token_jwt', 'last_authenticated', 'userinfo']
+    KEYS = [
+        'access_token',
+        'current_provider',
+        'id_token',
+        'id_token_jwt',
+        'last_authenticated',
+        'last_session_refresh',
+        'userinfo'
+    ]
 
     def __init__(self, session_storage, provider_name=None):
         self._session_storage = session_storage
@@ -36,10 +44,11 @@ class UserSession:
 
     def should_refresh(self, refresh_interval_seconds=None):
         return refresh_interval_seconds is not None and \
+               self._session_storage.get('last_session_refresh') is not None and \
                self._refresh_time(refresh_interval_seconds) < time.time()
 
     def _refresh_time(self, refresh_interval_seconds):
-        last = self._session_storage.get('last_authenticated', 0)
+        last = self._session_storage.get('last_session_refresh', 0)
         return last + refresh_interval_seconds
 
     def update(self, access_token=None, id_token=None, id_token_jwt=None, userinfo=None):
@@ -55,11 +64,13 @@ class UserSession:
             if value:
                 self._session_storage[session_key] = value
 
-        auth_time = int(time.time())
+        now = int(time.time())
+        auth_time = now
         if id_token:
             auth_time = id_token.get('auth_time', auth_time)
 
         self._session_storage['last_authenticated'] = auth_time
+        self._session_storage['last_session_refresh'] = now
         set_if_defined('access_token', access_token)
         set_if_defined('id_token', id_token)
         set_if_defined('id_token_jwt', id_token_jwt)


Test patch:
diff --git a/tests/test_user_session.py b/tests/test_user_session.py
index 6507603..1689161 100644
--- a/tests/test_user_session.py
+++ b/tests/test_user_session.py
@@ -45,17 +45,17 @@ class TestUserSession(object):
 
     def test_should_not_refresh_if_authenticated_within_refresh_interval(self):
         refresh_interval = 10
-        session = self.initialised_session({'last_authenticated': time.time() + (refresh_interval - 1)})
+        session = self.initialised_session({'last_session_refresh': time.time() + (refresh_interval - 1)})
         assert session.should_refresh(refresh_interval) is False
 
     def test_should_refresh_if_supported_and_necessary(self):
         refresh_interval = 10
         # authenticated too far in the past
-        session_storage = {'last_authenticated': time.time() - (refresh_interval + 1)}
+        session_storage = {'last_session_refresh': time.time() - (refresh_interval + 1)}
         assert self.initialised_session(session_storage).should_refresh(refresh_interval) is True
 
-    def test_should_refresh_if_supported_and_not_previously_authenticated(self):
-        assert self.initialised_session({}).should_refresh(10) is True
+    def test_should_not_refresh_if_not_previously_authenticated(self):
+        assert self.initialised_session({}).should_refresh(10) is False
 
     @pytest.mark.parametrize('data', [
         {'access_token': 'test_access_token'},
@@ -71,7 +71,11 @@ class TestUserSession(object):
 
         self.initialised_session(storage).update(**data)
 
-        expected_session_data = {'last_authenticated': auth_time, 'current_provider': self.PROVIDER_NAME}
+        expected_session_data = {
+            'last_authenticated': auth_time,
+            'last_session_refresh': auth_time,
+            'current_provider': self.PROVIDER_NAME
+        }
         expected_session_data.update(**data)
         assert storage == expected_session_data
 
@@ -81,6 +85,15 @@ class TestUserSession(object):
         session.update(id_token={'auth_time': auth_time})
         assert session.last_authenticated == auth_time
 
+    @patch('time.time')
+    def test_update_should_update_last_session_refresh_timestamp(self, time_mock):
+        now_timestamp = 1234
+        time_mock.return_value = now_timestamp
+        data = {}
+        session = self.initialised_session(data)
+        session.update()
+        assert data['last_session_refresh'] == now_timestamp
+
     def test_trying_to_update_uninitialised_session_should_throw_exception(self):
         with pytest.raises(UninitialisedSession):
             UserSession(session_storage={}).update()