Skip to content

Allow empty security array for endpoints #729

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

Merged
merged 4 commits into from
Dec 7, 2018

Conversation

fotos
Copy link
Contributor

@fotos fotos commented Dec 3, 2018

Fixes a bug where empty arrays for endpoints (operations) are excluded from the generated schema.

The OpenAPI 2.0 spec for security:

A declaration of which security schemes are applied for the API as a whole. The list of values describes alternative security schemes that can be used (that is, there is a logical OR between the security requirements). Individual operations can override this definition.

This is useful in the following scenario:

You define a top level security requirement (e.g. OAuth2) that applies to the whole API and then selectively whitelist endpoints that are public.

For example:

securityDefinitions:
  oauth:
    type: oauth2
    flow: password
    tokenUrl: 'http://127.0.0.1/oauth/token'
    scopes:
      read: Grants read access
      write: Grants write access
security:
  - oauth:
      - read

paths:
  /users:
    post:
      summary: Create User
      security: []

Which renders like this in Swagger UI:

public

Note the absence of 🔓 for /users.

There are a couple of more places where #blank? is used in Grape::Endpoint. Namely in #swagger_object, #info_object, #license_object, and #contact_object. I didn't change those since, for example, the #license_object can't be blank (needs at least a name).

This change will produce empty arrays in #method_object, for keys such as parameters or tags. The generated schema is still valid since an empty parameters or tags is allowed. Especially for parameters it's allowed to override / amend (if defined) but not to remove. The empty array is successfully ignored by Swagger UI. This PR will make the generated schema a bit more verbose yet, still, valid. Another alternative is to call Object#presence in #params_object and #tag_object.

In the last commit I remove the default empty description to bring the generated schema (and specs) closer to what they were originally (before this PR).

☝️ Let me know whether I should include / make those changes.

@coveralls
Copy link

coveralls commented Dec 3, 2018

Coverage Status

Coverage decreased (-0.0009%) to 99.213% when pulling 4f9df95 on fotos:allow-empty-security-array into 83a6e8d on ruby-grape:master.

@fotos
Copy link
Contributor Author

fotos commented Dec 7, 2018

@dblock and/or @LeFnord, when you get the chance, I would appreciate some feedback for this PR.

I'd rather have a fix upstream (here 😄) than having to fork the project for a small change / fix.

(Given the opportunity, thanks for all the hard work on Grape and the surrounding ecosystem 🎉)

@dblock dblock merged commit 4777b87 into ruby-grape:master Dec 7, 2018
@dblock
Copy link
Member

dblock commented Dec 7, 2018

This makes total sense, I have merged this.

@dblock
Copy link
Member

dblock commented Dec 7, 2018

@fotos You can make another PR for the remaining things with tests, please. Much appreciated, thanks for the good work!

@fotos
Copy link
Contributor Author

fotos commented Dec 7, 2018

@dblock thanks. 🙇

I'll iterate on another PR and trim the unnecessary parameters / tags.

Is it possible to get 0.32.1 out? 🙏

@fotos fotos deleted the allow-empty-security-array branch December 7, 2018 16:37
@dblock
Copy link
Member

dblock commented Dec 7, 2018

Happy to make a release soon if @LeFnord is not around, let's see that other PR :)

@fotos
Copy link
Contributor Author

fotos commented Dec 7, 2018

@dblock here you go: #731 😃

@LeFnord
Copy link
Member

LeFnord commented Dec 7, 2018

I'm here, but need a bit to have a look on all the new stuff 😉

LeFnord pushed a commit to LeFnord/grape-swagger that referenced this pull request Feb 9, 2019
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