Support multiple logout views (multiple use of 'oidc_logout' decorator). #126

zamzterz · 2022-02-07T19:24:33Z

~~Note: Not tested yet~~

infohash · 2022-03-07T22:00:41Z

I left a review highlighting the problem.

zamzterz · 2022-03-12T06:48:46Z

@infohash I don't see any review? Was it not published because this PR was still in draft stage?

infohash · 2022-03-12T07:02:42Z

src/flask_pyoidc/flask_pyoidc.py

@@ -269,8 +249,9 @@ def _logout(self):
        if client.provider_end_session_endpoint:
            flask.session['end_session_state'] = rndstr()

+            post_logout_uri = url_for(view_func.__name__, _external=True)


This will fail for logout view functions that are mounted under blueprints. Name of view functions under a blueprint are prefixed by the name of the associated blueprint. In large-scaled flask apps, this is very common. A user should have an option available to provide view function name that can be resolved by url_for.

You're right - I opted to leave this behavior for the draft as that's how it was before.

I've just pushed a change now to make it more general, with tests that show it works regardless of which endpoint the view function is mounted under.
The benefit as I see it with this PR is that it will automatically handle the most common cases (including setting the correct post_logout_redirect_uris in the registration request) without any changes from the user side.

infohash · 2022-03-12T21:11:25Z

src/flask_pyoidc/flask_pyoidc.py

-            return []
+    def _get_url_for_view_function(self, view_func):
+        for endpoint, f in current_app.view_functions.items():
+            if f == view_func:


This if condition will never be true if you are using flask extensions that wrap view functions to generate OpenAPI documentation (Swagger). For example if you use flask-restx, the view function object becomes <function Logout.get at 0x000001DF5DB27760> and is registered with flask app as:

>>> app.view_functions {'apiv2.logout_logout': <function View.as_view.<locals>.view at 0x000001DF5DB40670>

The reason that registered object is different here is because flask-restx uses Flask Pluggable Views to register the views. See Views.as_view(). This also means that even if you are not using any other flask extensions, implementing views with Pluggable Views will produce the same problem.

In an enterprise environment, OpenAPI compliance is a standard so all view functions are likely to be wrapped with extensions that automatically generate swagger documentation for APIs. We must not have conflicts with the use of other flask-extensions.

~~That is due to the ordering of the decorators: @auth.oidc_logout needs to be the second outermost decorator, right after the Flask @app.route(...):~~

Nvm, I didn't check the pluggable views reference.
Either way I found a way to greatly simplify it: flask.request.endpoint can be used to get the current endpoint being hit when doing the logout.
And for the registration, the docs already says:

If the logout view is mounted under a custom endpoint (other than the default, which is
the name of the view function), or if using Blueprints, you
must specify the full URL in the Flask-pyoidc configuration using post_logout_redirect_uris

I have done it in the lastest commit of #124. I'm using flask.request.url instead.

I think it would be good if we aligned on a single PR to not duplicate the efforts 🙂 .

I checked #124 and it has some unrelated changes now (e.g. the imports), and this one is ready to go, so I'd like to merge this one.
Is there anything more that should be added to this one from #124?

Our approach is exactly same 😂. I have left some reviews to address the changes.

Hehe! :)

Thanks a ton for the detailed review, I've addressed the comments! 🙇

infohash · 2022-03-13T15:22:01Z

src/flask_pyoidc/flask_pyoidc.py

        try:
-            return url_for(self._logout_view.__name__, _external=True)
+            return [url_for(view.__name__, _external=True) for view in self._logout_views]


This raises unhandled exception if post_logout_redirect_uris is not provided in ClientRegistrationInfo and the client is registered outside of request context.

... auth = OIDCAuthentication({'default': provider_config}) client = auth.clients['default'] with app.app_context(): auth._register_client(client) RuntimeError: Application was not able to create a URL adapter for request independent URL generation. You might be able to fix this by setting the SERVER_NAME config variable.

I'd say this is an expected error, and the given error message points to the solution as well: with SERVER_NAME in the Flask config it works as expected:

app.config.update({'SERVER_NAME': 'foobar', ...}) auth.init_app(app) client = auth.clients[PROVIDER_NAME1] with app.app_context(): auth._register_client(client)

This is standard Flask behavior, that it needs to either have a request context or know the SERVER_NAME to be able to figure out the URL for and endpoint.

infohash · 2022-03-13T15:37:11Z

src/flask_pyoidc/flask_pyoidc.py

-
-    def _get_url_for_logout_view(self):
-        if not self._logout_view:
-            return None


To prevent RuntimeError exception as shown below, return empty list here if self._logout_view is empty.

This will already return an empty list if self._logout_views is empty.

infohash · 2022-03-13T15:43:49Z

src/flask_pyoidc/flask_pyoidc.py

        post_logout_redirect_uris = client._provider_configuration._client_registration_info.get(
            'post_logout_redirect_uris')
        if not post_logout_redirect_uris:
-            # If not passed, try to resolve it by using logout view function.
-            _default_post_logout_redirect_uris = default_post_logout_redirect_uris()
-            # Set this as an attribute of ClientRegistrationInfo.
            client._provider_configuration._client_registration_info[


Don't remove all comments, let it be for future devs so that they can understand the reason of writing this line of code.

I found the comments in this case redundant when looking over them - they don't explain any additional info that is not already in the code itself.
Or am I missing something?

infohash · 2022-03-13T15:52:42Z

tests/test_flask_pyoidc.py

-        with self.app.test_request_context('/'):
-            assert authn._get_url_for_logout_view() == f'http://{self.CLIENT_DOMAIN}/logout'
-
-    def test_get_url_for_logout_view_should_raise_build_error_if_mounted_under_custom_endpoint(self):


Keep this test case, removing this has decreased the test coverage.

The method _get_url_for_logout_view no longer exists so this test didn't make sense after these changes.

It does.

Flask-pyoidc/src/flask_pyoidc/flask_pyoidc.py

Lines 91 to 96 in 9d08533

def _get_urls_for_logout_views(self):

try:

return [url_for(view.__name__, _external=True) for view in self._logout_views]

except BuildError:

logger.error('could not build url for logout view, it might be mounted under a custom endpoint')

raise

You can mark rest of the conversations as resolved. 🙂

infohash · 2022-03-13T16:02:54Z

src/flask_pyoidc/flask_pyoidc.py

@@ -88,42 +88,22 @@ def init_app(self, app):
            for (name, configuration) in self._provider_configurations.items()
        }

-    def _get_post_logout_redirect_uri(self, client):
-        if client.post_logout_redirect_uris:


As this method is no longer required, add a test case to cover post_logout_redirect_uris property of PyoidcFacade. Take it from my PR.

@property def post_logout_redirect_uris(self): return self._client.registration_response.get('post_logout_redirect_uris') def test_property_post_logout_redirect_uris(self): post_logout_redirect_uris = ['https://client.example.com/logout'] client_metadata = self.CLIENT_METADATA.copy( post_logout_redirect_uris=post_logout_redirect_uris) facade = PyoidcFacade(ProviderConfiguration(provider_metadata=self.PROVIDER_METADATA, client_metadata=client_metadata), REDIRECT_URI) assert facade.post_logout_redirect_uris == post_logout_redirect_uris

Fixed! 👍

infohash · 2022-03-16T17:14:26Z

Test coverage is at 99.20% 🙂. Striving for 100% is good when the code base is not large yet. This will benefit us in future PRs. You can set test coverage threshold to 98% now.

How do you bump versions? Should we keep our version scheme consistent with Python? Like keeping it 3.10.0 - 3.10.x until Python 3.11 version is released?

zamzterz · 2022-03-23T18:20:37Z

@infohash I don't think 100% coverage is necessary. And actually I'd like to refactor the tests sometime™️ for less test code to maintain - that's higher priority than increasing already high coverage. 🙂

As for the versioning: this extension is using semantic versioning, so not tied to Python versioning at all.
Next version would be 3.11.0, do you think it should be released now?
Otherwise I was planning on holding off since you mentioned you were working on the "Client/Http session" refactor already.

infohash · 2022-03-23T19:00:33Z

@zamzterz I have delegated all requests to underlying pyoidc library but test cases are failing because oic.oic.Client methods are not returning id_token_jwt. I'll have to raise PR there to support this feature.

zamzterz mentioned this pull request Feb 7, 2022

Support for custom post logout redirect URIs #124

Closed

zamzterz force-pushed the support-multiple-logout-views branch from 281b4c3 to c821ab4 Compare March 7, 2022 21:25

Support multiple logout views (multiple use of 'oidc_logout' decorator).

f9f6d7d

zamzterz force-pushed the support-multiple-logout-views branch from c821ab4 to f9f6d7d Compare March 7, 2022 21:56

zamzterz marked this pull request as ready for review March 12, 2022 06:46

infohash reviewed Mar 12, 2022

View reviewed changes

Don't rely on function name when computing the post_logout_redirect_uri.

eeaaae5

infohash reviewed Mar 12, 2022

View reviewed changes

Simplify post_logout_redirect_uri computation.

9d08533

zamzterz force-pushed the support-multiple-logout-views branch from bd7ed99 to 9d08533 Compare March 13, 2022 14:43

infohash suggested changes Mar 13, 2022

View reviewed changes

zamzterz added 2 commits March 13, 2022 19:52

[WIP]

4a9e5c0

Add back removed test.

e78012b

zamzterz merged commit 5e66a38 into main Mar 16, 2022

zamzterz deleted the support-multiple-logout-views branch March 16, 2022 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple logout views (multiple use of 'oidc_logout' decorator). #126

Support multiple logout views (multiple use of 'oidc_logout' decorator). #126

zamzterz commented Feb 7, 2022 •

edited

Loading

infohash commented Mar 7, 2022

zamzterz commented Mar 12, 2022

infohash Mar 12, 2022

zamzterz Mar 12, 2022

infohash Mar 12, 2022 •

edited

Loading

zamzterz Mar 13, 2022 •

edited

Loading

infohash Mar 13, 2022 •

edited

Loading

zamzterz Mar 13, 2022

infohash Mar 13, 2022

zamzterz Mar 13, 2022

infohash Mar 13, 2022

zamzterz Mar 13, 2022

infohash Mar 13, 2022

zamzterz Mar 13, 2022

infohash Mar 13, 2022

zamzterz Mar 13, 2022

infohash Mar 13, 2022

zamzterz Mar 13, 2022

infohash Mar 13, 2022 •

edited

Loading

infohash Mar 13, 2022

zamzterz Mar 13, 2022

infohash commented Mar 16, 2022 •

edited

Loading

zamzterz commented Mar 23, 2022

infohash commented Mar 23, 2022

	def _get_urls_for_logout_views(self):
	try:
	return [url_for(view.__name__, _external=True) for view in self._logout_views]
	except BuildError:
	logger.error('could not build url for logout view, it might be mounted under a custom endpoint')
	raise

Support multiple logout views (multiple use of 'oidc_logout' decorator). #126

Support multiple logout views (multiple use of 'oidc_logout' decorator). #126

Conversation

zamzterz commented Feb 7, 2022 • edited Loading

infohash commented Mar 7, 2022

zamzterz commented Mar 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infohash Mar 12, 2022 • edited Loading

Choose a reason for hiding this comment

zamzterz Mar 13, 2022 • edited Loading

Choose a reason for hiding this comment

infohash Mar 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infohash Mar 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infohash commented Mar 16, 2022 • edited Loading

zamzterz commented Mar 23, 2022

infohash commented Mar 23, 2022

zamzterz commented Feb 7, 2022 •

edited

Loading

infohash Mar 12, 2022 •

edited

Loading

zamzterz Mar 13, 2022 •

edited

Loading

infohash Mar 13, 2022 •

edited

Loading

infohash Mar 13, 2022 •

edited

Loading

infohash commented Mar 16, 2022 •

edited

Loading