Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use resolver in security middleware #1553

Merged
merged 2 commits into from
Jun 20, 2022

Conversation

RobbeSneyders
Copy link
Member

Continues on #1514.

This PR continues the implementation of the security middleware, which was started in #1514. I took some shortcuts in the original PR, one of them the naive 'resolution' of the operation_id by fetching it from the spec instead of relying on a resolver.

operation_id = operation.get('operationId')

This PR replaces this with resolution via resolver.

You'll see that I rely on the original Connexion Operation classes for this, instead of on the Resolver directly. I propose to further hollow out the Operation classes during the refactoring into a middleware stack, until it becomes a 'data class' providing an interface for the specification of the operation across openapi versions. This is similar to the Specification class, but on an operation level.

Specific middleware operations such as SecurityOperation and RoutingOperation can then use this Operation object to initialize themselves. I added this for the RoutingOperation as a separate commit.

@coveralls
Copy link

coveralls commented Jun 11, 2022

Pull Request Test Coverage Report for Build 2531413147

  • 35 of 35 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 94.38%

Totals Coverage Status
Change from base Build 2416536603: 0.04%
Covered Lines: 2670
Relevant Lines: 2829

💛 - Coveralls

Copy link
Member

@Ruwann Ruwann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM! Had 1 question though.

I really like the idea of making the operation class a simple class that only contains functionality to the operation object as specified in the spec (e.g. fetch parameters from the common definitions, or in case of security, get the applicable security for this operation in case it's defined both globally and on the operation).

connexion/apis/abstract.py Show resolved Hide resolved
try:
self.add_operation(path, method)
except ResolverError:
# ResolverErrors are handled in routing middleware
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we propagate the error in this case?

If the resolving fails, it is normally already raised in the routing middleware, or am I mistaken?
But if it is already raised there, then we don't need to account for it here?

So, not sure what the flow is here :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolution is done at start up. So

  • if the ResolverError is not handled in the routing middleware, start up will fail and this code will indeed never be reached.
  • If the ResolverError is handled in the routing middleware, it will still be raised here during start-up, but a request will never reach the operation since it will already be handled by the resolver_error_handler in the routing middleware.

I can add some more explanation to the comment, but open to suggestions on how to handle it more clearly in code.

@RobbeSneyders RobbeSneyders force-pushed the feature/security-middleware-resolver branch from f928a79 to 396ce08 Compare June 20, 2022 20:36
@RobbeSneyders RobbeSneyders merged commit abbc8ff into main Jun 20, 2022
@RobbeSneyders RobbeSneyders deleted the feature/security-middleware-resolver branch June 20, 2022 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants