Repository: zamzterz/Flask-pyoidc

Issue:
Error responses using undefined _redirect_uri_endpoint attribute

In some cases (e.g. when the auth state goes stale after much time) we rightfully receive an error that should be passed on to the client

```python
Exception on /redirect_uri [POST]

Traceback (most recent call last):

  File "../flask_pyoidc/flask_pyoidc.py", line 156, in _handle_authentication_response

    flask.session.pop('nonce'))

  File "../lib/python3.7/site-packages/flask_pyoidc/auth_response_handler.py", line 60, in process_auth_response

    raise AuthResponseErrorResponseError(auth_response.to_dict())

flask_pyoidc.auth_response_handler.AuthResponseErrorResponseError: {'error': 'login_required', 'state': 'm92iSPHP7qMBXSkQ'}

 

During handling of the above exception, another exception occurred:

 

Traceback (most recent call last):

  File "../lib/python3.7/site-packages/flask/app.py", line 2447, in wsgi_app

    response = self.full_dispatch_request()

  File "../lib/python3.7/site-packages/flask/app.py", line 1952, in full_dispatch_request

    rv = self.handle_user_exception(e)

  File "../lib/python3.7/site-packages/flask_cors/extension.py", line 165, in wrapped_function

    return cors_after_request(app.make_response(f(*args, **kwargs)))

  File "../lib/python3.7/site-packages/flask_cors/extension.py", line 165, in wrapped_function

    return cors_after_request(app.make_response(f(*args, **kwargs)))

  File "../lib/python3.7/site-packages/flask/app.py", line 1821, in handle_user_exception

    reraise(exc_type, exc_value, tb)

  File "../lib/python3.7/site-packages/flask/_compat.py", line 39, in reraise

    raise value

  File "../lib/python3.7/site-packages/flask/app.py", line 1950, in full_dispatch_request

    rv = self.dispatch_request()

  File "../lib/python3.7/site-packages/flask/app.py", line 1936, in dispatch_request

    return self.view_functions[rule.endpoint](**req.view_args)

  File "../lib/python3.7/site-packages/flask_pyoidc/flask_pyoidc.py", line 158, in _handle_authentication_response

    return self._handle_error_response(e.error_response, is_processing_fragment_encoded_response)

  File "../lib/python3.7/site-packages/flask_pyoidc/flask_pyoidc.py", line 186, in _handle_error_response

    return '/' + self._redirect_uri_endpoint + '?error=1'

AttributeError: 'OIDCAuthentication' object has no attribute '_redirect_uri_endpoint'
```

Unfortunately it looks like the error handler tries to return a nonexistant attribute (holdover from old code?) for the redirect uri endpoint. I'm guessing this should be 
`self._redirect_uri_config.endpoint` instead.

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

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

```diff
@@            Coverage Diff             @@
##           master      #96      +/-   ##
==========================================
+ Coverage   97.36%   97.81%   +0.44%     
==========================================
  Files           9        9              
  Lines         570      549      -21     
  Branches       62       62              
==========================================
- Hits          555      537      -18     
+ Misses          8        6       -2     
+ Partials        7        6       -1     
```


| [Impacted Files](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/96?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/flask\_pyoidc/flask\_pyoidc.py](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/96/diff?src=pr&el=tree#diff-c3JjL2ZsYXNrX3B5b2lkYy9mbGFza19weW9pZGMucHk=) | `94.91% <100.00%> (+1.61%)` | :arrow_up: |
| [src/flask\_pyoidc/provider\_configuration.py](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/96/diff?src=pr&el=tree#diff-c3JjL2ZsYXNrX3B5b2lkYy9wcm92aWRlcl9jb25maWd1cmF0aW9uLnB5) | `96.05% <0.00%> (-0.06%)` | :arrow_down: |
| [example/app.py](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/96/diff?src=pr&el=tree#diff-ZXhhbXBsZS9hcHAucHk=) | `100.00% <0.00%> (ø)` | |
| [example/test\_example\_app.py](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/96/diff?src=pr&el=tree#diff-ZXhhbXBsZS90ZXN0X2V4YW1wbGVfYXBwLnB5) | `100.00% <0.00%> (ø)` | |
| [src/flask\_pyoidc/user\_session.py](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/96/diff?src=pr&el=tree#diff-c3JjL2ZsYXNrX3B5b2lkYy91c2VyX3Nlc3Npb24ucHk=) | `100.00% <0.00%> (ø)` | |
| [src/flask\_pyoidc/pyoidc\_facade.py](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/96/diff?src=pr&el=tree#diff-c3JjL2ZsYXNrX3B5b2lkYy9weW9pZGNfZmFjYWRlLnB5) | `100.00% <0.00%> (ø)` | |
| [src/flask\_pyoidc/auth\_response\_handler.py](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/96/diff?src=pr&el=tree#diff-c3JjL2ZsYXNrX3B5b2lkYy9hdXRoX3Jlc3BvbnNlX2hhbmRsZXIucHk=) | `100.00% <0.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/96?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/96?src=pr&el=footer). Last update [4aa29e4...6734c42](https://codecov.io/gh/zamzterz/Flask-pyoidc/pull/96?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/src/flask_pyoidc/flask_pyoidc.py b/src/flask_pyoidc/flask_pyoidc.py
index 5ca699f..cd00461 100644
--- a/src/flask_pyoidc/flask_pyoidc.py
+++ b/src/flask_pyoidc/flask_pyoidc.py
@@ -183,7 +183,7 @@ class OIDCAuthentication:
             # if the current request was from the JS page handling fragment encoded responses we need to return
             # a URL for the error page to redirect to
             flask.session['error'] = error_response
-            return '/' + self._redirect_uri_endpoint + '?error=1'
+            return '/' + self._redirect_uri_config.endpoint + '?error=1'
         return self._show_error_response(error_response)
 
     def _show_error_response(self, error_response):


Test patch:
diff --git a/tests/test_flask_pyoidc.py b/tests/test_flask_pyoidc.py
index 427cdf9..e507adc 100644
--- a/tests/test_flask_pyoidc.py
+++ b/tests/test_flask_pyoidc.py
@@ -290,6 +290,24 @@ class TestOIDCAuthentication(object):
             assert session.access_token == access_token
             assert response == '/test'
 
+    def test_handle_error_response_POST(self):
+        state = 'test_state'
+
+        authn = self.init_app()
+        error_resp = {'state': state, 'error': 'invalid_request', 'error_description': 'test error'}
+
+        with self.app.test_request_context('/redirect_uri',
+                                           method='POST',
+                                           data=error_resp,
+                                           mimetype='application/x-www-form-urlencoded'):
+            UserSession(flask.session, self.PROVIDER_NAME)
+            flask.session['state'] = state
+            flask.session['nonce'] = 'test_nonce'
+            response = authn._handle_authentication_response()
+            assert flask.session['error'] == error_resp
+            assert response == '/redirect_uri?error=1'
+
+
     def test_handle_authentication_response_without_initialised_session(self):
         authn = self.init_app()