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

Make Security Definition and Security Requirement as Python Objects #124

Conversation

macisamuele
Copy link
Collaborator

@macisamuele macisamuele commented Oct 6, 2016

The goal of this PR is to target #123.

As @jlumpe the initial implementation of the security object added by PR #112 was not 100% compliant with the swagger specs.

To achieve a better matching of the specs I have created specific classes for Security Definition and Security Requirement. In this way will be easier to integrate the handling of oauth2 security format.

Main differences:

  • security_objects property of operation is removed: the definition was incomplete and is replaced by security_requirements property which returns the list of Security Requirement objects
  • the parameters injected due to security specification cannot override parameters required by Path Object or Operation Object (is needed because different constraint could be injected) and are marked as optional parameters (because of the OR between different security requirements, check example3 on issue Fix handling of security requirements to match spec #123)
  • unmarshal_request performs proper validation of security related parameters:
    • checks that 1 and only one security requirement is matched
    • in case of multiple security requirements are matched, checks that one matching security requirement is a superset of all the others (contains all the others, ie. example5 on issue Fix handling of security requirements to match spec #123)

I know that reviewing it could be hard ... so I added a long set of tests to guarantee a good coverage of the modifications.

NOTE: I'll update CHANGELOG.rst after the end of the review

@coveralls
Copy link

coveralls commented Oct 6, 2016

Coverage Status

Coverage increased (+0.2%) to 97.523% when pulling 0aa191a on macisamuele:issue_123-security_object_definitions_and_security_handling_improvement into bea65fc on Yelp:master.

@jlumpe
Copy link
Contributor

jlumpe commented Oct 6, 2016

Can confirm that this is working for me using my simple spec where the API key can be in the query or header.

@macisamuele
Copy link
Collaborator Author

I have added test care with apiKey security with parameter in query.

@jlumpe the test modified is checking even that case.

@macisamuele macisamuele force-pushed the issue_123-security_object_definitions_and_security_handling_improvement branch 2 times, most recently from 640a765 to 129b62a Compare October 6, 2016 20:46
@coveralls
Copy link

coveralls commented Oct 6, 2016

Coverage Status

Coverage increased (+0.2%) to 97.523% when pulling 129b62a on macisamuele:issue_123-security_object_definitions_and_security_handling_improvement into bea65fc on Yelp:master.

@macisamuele
Copy link
Collaborator Author

@sjaensch if you have some time in the next days could you have a look to this? Thanks a lot

Copy link
Contributor

@sjaensch sjaensch left a comment

Choose a reason for hiding this comment

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

lg2m, sorry for the long review time turnaround. Let's get this shipped!

:raise: SwaggerSecurityValidationError
"""

security_types = set([
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to create a temporary list here, you can just remove the square brackets.


# is the longest security definition a superset for all the others?
# if no there is no way to discriminate the security definition matched
exists_superset = all(
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is something that the spec allows? Is it something that we expect to encounter in the real world? I don't see how it would make sense, at least in the context of API keys, to require either one or more than one. Wouldn't you then just say "hey the one is enough"?

macisamuele and others added 4 commits October 21, 2016 14:03
Security parameters validation
Prevent overriding parameters from security object
validate_security_object have to raises only for apiKey security definitions
@macisamuele macisamuele force-pushed the issue_123-security_object_definitions_and_security_handling_improvement branch from 7260c2b to df8b4bf Compare October 21, 2016 12:04
@coveralls
Copy link

coveralls commented Oct 21, 2016

Coverage Status

Coverage increased (+0.2%) to 97.536% when pulling df8b4bf on macisamuele:issue_123-security_object_definitions_and_security_handling_improvement into dac21a8 on Yelp:master.

@sjaensch
Copy link
Contributor

🛳

@coveralls
Copy link

coveralls commented Oct 21, 2016

Coverage Status

Coverage increased (+0.2%) to 97.536% when pulling dffc0d4 on macisamuele:issue_123-security_object_definitions_and_security_handling_improvement into dac21a8 on Yelp:master.

@coveralls
Copy link

coveralls commented Oct 21, 2016

Coverage Status

Coverage increased (+0.2%) to 97.536% when pulling dffc0d4 on macisamuele:issue_123-security_object_definitions_and_security_handling_improvement into dac21a8 on Yelp:master.

@macisamuele macisamuele merged commit 8ac640c into Yelp:master Oct 21, 2016
@macisamuele macisamuele deleted the issue_123-security_object_definitions_and_security_handling_improvement branch October 21, 2016 18:40
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.

4 participants