Async security check #869

Jyhess · 2019-02-05T17:27:17Z

Fixes #868 .

Allow to use coroutines with Aiohttp in security handlers.

Changes proposed in this pull request:

Move all function from connexion.operations.security to a class in connexion.security.SecurityHandlerFactory
Specialize this class for AioHttp and Flask, and instantiate them in dedicated apis
AioHttp specialization now support coroutines
pass_context_arg_name option is supported for security handlers

Jyhess · 2019-05-09T16:44:39Z

I rebased the branch on top of master.
Is there something else I can do to move further on this pull request?

elsnoon · 2019-05-21T21:54:06Z

Hi guys, when we can expect this to be merged?

It would be great to have async security check. At the moment we have some workaround solution with middleware.

Thanks 🙂

ghost · 2019-06-25T14:09:31Z

Is there any progress on this?

hjacobs · 2019-10-15T16:44:22Z

Sorry for the delay. Could you rebase the PR and fix the conflicts? We will take a look afterwards.

cognifloyd · 2019-12-04T20:37:34Z

I will rebase this one once #849 is merged. (Thanks @Jyhess for giving me access so I can move the PRs forward)

dtkav · 2019-12-05T00:15:31Z

connexion/exceptions.py


 class OAuthResponseProblem(OAuthProblem):
-    def __init__(self, token_response, **kwargs):
-        self.token_response = token_response


what are the implications of removing this?

Some history here - every security handler used to raise OAuthResponseProblem in case of an error, so including it (or inheriting from it) was for backwards compatibility.
Hopefully we can drop that requirement at some point.

Oh. Well, I just got rid of it everywhere where it was in use, and added it where it made sense.

If we need to adjust the PR to split that out into another PR then fine. This PR wore me out, so if changes need to occur I'll wait a bit to make them.

dtkav · 2019-12-05T00:17:51Z

connexion/security/aiohttp_security_handlers_factory.py

+logger = logging.getLogger('connexion.api.security')
+
+
+class AioHttpSecurityHandlerFactory(SecurityHandlerFactory):


are we going to have to write one of these for every framework we support?
Is there a more generic version of this like AsyncSecurityHandlerFactory?

Right now, yes, each framework would need its own SecurityHandlerFactory.
As far as making it more generic - I'm of two minds:

I think we should assume this is AioHttp-specific until we implement another async framework. At that time, refactoring this into a reusable AsyncSecurityHandler that can be reused by both of them would make sense.

The only thing that uses aiohttp in this class is get_token_info_remote in how it uses self.client_session. If request is a ConnexionRequest (vs a framework specific request) then the other functions are very generic, so it should be simple to have an AbstractAsyncSecurityHandlerFactory that the AioHttpSecurityHandlerFactory extends to implement the get_token_info_remote method.

After I rebase this, I can add the AbstractAsyncSecurityHandlerFactory if you think that's worth it to help adding future frameworks. The risk there is that there actually is something specific to aiohttp here than I'm not seeing by reading, so we'll have to refactor again at that point to only include the generic stuff.

And now that I've typed all that, I think that I will go ahead and do that. I'll leave the rest of my comments here for future reference on the thinking.

dtkav · 2019-12-05T00:29:28Z

connexion/security/aiohttp_security_handlers_factory.py

+                # Must be created in a coroutine
+                self.client_session = aiohttp.ClientSession()
+            headers = {'Authorization': 'Bearer {}'.format(token)}
+            token_request = yield from self.client_session.get(token_info_url, headers=headers, timeout=5)


5 is a magic number here - it might make sense to add as a field on the base class in case someone wants to change it.

cognifloyd

Wow. good catches. Here are my follow up comments as I try to understand the code :)

cognifloyd · 2019-12-06T00:28:32Z

connexion/security/aiohttp_security_handlers_factory.py

+logger = logging.getLogger('connexion.api.security')
+
+
+class AioHttpSecurityHandlerFactory(SecurityHandlerFactory):


Right now, yes, each framework would need its own SecurityHandlerFactory.
As far as making it more generic - I'm of two minds:

I think we should assume this is AioHttp-specific until we implement another async framework. At that time, refactoring this into a reusable AsyncSecurityHandler that can be reused by both of them would make sense.

The only thing that uses aiohttp in this class is get_token_info_remote in how it uses self.client_session. If request is a ConnexionRequest (vs a framework specific request) then the other functions are very generic, so it should be simple to have an AbstractAsyncSecurityHandlerFactory that the AioHttpSecurityHandlerFactory extends to implement the get_token_info_remote method.

After I rebase this, I can add the AbstractAsyncSecurityHandlerFactory if you think that's worth it to help adding future frameworks. The risk there is that there actually is something specific to aiohttp here than I'm not seeing by reading, so we'll have to refactor again at that point to only include the generic stuff.

And now that I've typed all that, I think that I will go ahead and do that. I'll leave the rest of my comments here for future reference on the thinking.

connexion/security/aiohttp_security_handlers_factory.py

+                # Must be created in a coroutine
+                self.client_session = aiohttp.ClientSession()
+            headers = {'Authorization': 'Bearer {}'.format(token)}
+            token_request = yield from self.client_session.get(token_info_url, headers=headers, timeout=5)


connexion/apis/abstract.py

        logger.debug('pass_context_arg_name: %s', pass_context_arg_name)
        self.pass_context_arg_name = pass_context_arg_name

+        self.security_handler_factory = self.make_security_handler_factory(pass_context_arg_name)


connexion/decorators/security.py

-    :type token: str
-    :rtype: dict
-    """
-    token_request = session.get(token_info_url, headers={'Authorization': 'Bearer {}'.format(token)}, timeout=5)


connexion/exceptions.py


 class OAuthResponseProblem(OAuthProblem):
-    def __init__(self, token_response, **kwargs):
-        self.token_response = token_response


connexion/security/aiohttp_security_handlers_factory.py

+            if token_info is self.no_value:
+                return self.no_value
+            if token_info is None:
+                raise OAuthResponseProblem(description=exception_msg)


connexion/security/security_handler_factory.py

+            if token_info is None:
+                raise OAuthResponseProblem(description=exception_msg)


cognifloyd · 2019-12-13T00:38:49Z

K. I'm not entirely confident about my fixes for generic auth issues.

cognifloyd · 2020-05-30T08:41:36Z

K. I just rebased this on master, removing the OAuthResponseProblem changes. Those exception changes/cleanups are in my unauthorized_problem branch.

Next steps:

satisfy travis
adjust to use async/await instead of coroutine decorator

coveralls · 2020-06-04T00:53:15Z

Coverage increased (+0.6%) to 96.875% when pulling caacc78 on Jyhess:async_security_check into bed4b95 on zalando:master.

cognifloyd · 2020-06-09T06:35:04Z

@dtkav could you review when you get a chance?

cognifloyd · 2020-07-07T15:56:44Z

@hjacobs this is ready for review

chbndrhnns · 2020-07-20T15:48:52Z

@jmcs I saw you did some work recently on this project. This PR seems to be ready for quite some time. Any chance to have a look and approve/review?

I am in a position where I would need such functionality for performance reasons and I would prefer the changes in the official repo instead of forking my own version of connexion just to get async security handlers.

jmcs · 2020-07-20T16:01:29Z

@chbndrhnns @cognifloyd, I'll review this PR in the next few days.

jmcs · 2020-07-22T06:50:58Z

👍

Jyhess force-pushed the async_security_check branch 2 times, most recently from 89bfff0 to 3fb9249 Compare February 6, 2019 15:26

spec-first deleted a comment Feb 6, 2019

Jyhess changed the title ~~WIP Async security check~~ Async security check Feb 12, 2019

Jyhess force-pushed the async_security_check branch 3 times, most recently from a5876e4 to 8912e28 Compare May 9, 2019 16:31

dtkav reviewed Dec 5, 2019

View reviewed changes

cognifloyd reviewed Dec 6, 2019

View reviewed changes

cognifloyd force-pushed the async_security_check branch from 8912e28 to bfe0f53 Compare December 12, 2019 23:41

cognifloyd force-pushed the async_security_check branch 12 times, most recently from 25d5448 to 4795d7b Compare December 13, 2019 09:45

cognifloyd force-pushed the async_security_check branch from e1ca702 to 07d20e6 Compare May 30, 2020 08:35

cognifloyd force-pushed the async_security_check branch 4 times, most recently from 1b058bc to 0252ff7 Compare June 3, 2020 21:00

Julien Sagnard and others added 9 commits June 3, 2020 19:58

Preparation refactoring to introduce security factory class

8af9744

Move SecurityHandlerFactory in its own module

1fedb4f

Specialized Security handlers for Flask and AioHttp

c15896e

Add support for context keyword in security handlers

89ca634

Fix CI

c036948

move aiohttp_security -> async_security

537e46d

Make abstract async security handler

c6e4f38

Cleanup security imports

b1c4330

Document flow of methods in security_handler_factory

1663d41

cognifloyd force-pushed the async_security_check branch from e75b210 to 6bb75db Compare June 4, 2020 01:04

Use async/await in security

caacc78

cognifloyd force-pushed the async_security_check branch from 6bb75db to caacc78 Compare June 4, 2020 09:09

jmcs self-requested a review July 20, 2020 16:00

jmcs merged commit 8830d56 into spec-first:master Jul 22, 2020

Ruwann mentioned this pull request Jul 2, 2021

Bugfix/missing infofuncs #1375

Merged

a-a-abramov mentioned this pull request Mar 14, 2022

Basicauth documentation and example issues #1485

Closed

RobbeSneyders mentioned this pull request Apr 1, 2022

pass request context to authentication functions #1227

Closed

Ruwann mentioned this pull request Apr 22, 2023

Remove context keyword from security handlers #1690

Merged

		logger = logging.getLogger('connexion.api.security')


		class AioHttpSecurityHandlerFactory(SecurityHandlerFactory):

		if token_info is None:
		raise OAuthResponseProblem(description=exception_msg)

Uh oh!

Async security check #869

Async security check #869

Uh oh!

Conversation

Jyhess commented Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jyhess commented May 9, 2019

Uh oh!

elsnoon commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 25, 2019

Uh oh!

hjacobs commented Oct 15, 2019

Uh oh!

cognifloyd commented Dec 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtkav Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cognifloyd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

cognifloyd commented Dec 13, 2019

Uh oh!

cognifloyd commented May 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cognifloyd commented Jun 9, 2020

Uh oh!

cognifloyd commented Jul 7, 2020

Uh oh!

chbndrhnns commented Jul 20, 2020

Uh oh!

jmcs commented Jul 20, 2020

Uh oh!

jmcs commented Jul 22, 2020

Uh oh!

Reviewers

Assignees

Jyhess commented Feb 5, 2019 •

edited

Loading

elsnoon commented May 21, 2019 •

edited

Loading

dtkav Dec 5, 2019 •

edited

Loading

cognifloyd commented May 30, 2020 •

edited

Loading

coveralls commented Jun 4, 2020 •

edited

Loading