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

Check for authorization header. #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sunnyrjuneja
Copy link

It appears that Swagger-UI correctly sets an authorization header when
added in the interface (api key field). However, if the endpoint has
the option "authorizations" set with "oauth2", it will override the
value. See #13.

@@ -26,5 +26,10 @@ class API < Grape::API
request.params.as_json
end

desc 'Get Authorization header.', authorizations: { oauth2: [] }
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if this actually did OAuth2 :) I would rename the endpoint either way to oauth2.

Copy link
Author

Choose a reason for hiding this comment

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

I can set it up but it'll add the dep of doorkeeper (and activerecord). Okay, sounds good.

Copy link
Member

Choose a reason for hiding this comment

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

I think a dep is fine, we want this to be closer to the real world.

@sunnyrjuneja
Copy link
Author

@dblock Happy to improve these tests w/ feedback but how can we land a fix in Swagger UI? Do you know roughly where the offending code is?

@sunnyrjuneja
Copy link
Author

It would be nice if this actually did OAuth2 :)

I ended up doing this but was forced to mock doorkeeper tokens which turned into a mess. Decided this was easiest way to mock. Anymore feedback @dblock?

It appears that Swagger-UI correctly sets an authorization header when
added in the interface (api key field). However, if the endpoint has
the option "authorizations" set with "oauth2", it will override the
value. See ruby-grape#13.
@dblock
Copy link
Member

dblock commented Aug 25, 2015

I think as far as tests go this is good.

@sunnyrjuneja
Copy link
Author

Cool, is the next move to open an issue at Swagger-UI?

@dblock
Copy link
Member

dblock commented Aug 25, 2015

I think so. Supposedly once that's fixed there we can verify that this test passes.

@supagroova
Copy link

@sunnyrjuneja did you ever manage to find a solution to this issue? I have the same problem from what I can tell and would like to investigate further to try and provide a solution.

@sunnyrjuneja
Copy link
Author

Hey @supagroova excuse my brevity since I'm on my phone. There are some references PRs in repos above. I essentially had to do a few work around but was able to solve my problem. If you're unable to figure it out, I'm happy to chat on Skype or GHangout for your problem.

@matfiz
Copy link

matfiz commented Jan 14, 2016

Hey @sunnyrjuneja @supagroova! I have somehow managed to get it up and running. What I did was I have changed the way the GrapeSwaggerRails.options.headers are passed to SwaggerUi.
The problem in the original implementation is that the headers are read into swaggerUi.api.clientAuthorizations after the SwaggerUi is loaded. It means upon the request to Swagger JSON, the headers are not set!

The solution I have found is that instead of reading it after SwaggerUi.load(), I enter them directly to the SwaggerUi initializer (look how authorizations are loaded in https://github.com/swagger-api/swagger-ui/blob/master/src/main/javascript/SwaggerUi.js#L100).

The working code after changes is:

    auth_headers = {}
    <% GrapeSwaggerRails.options.headers.each_with_index do |(key, value), index| %>
      <%=raw "auth_headers['header_#{index}'] = new SwaggerClient.ApiKeyAuthorization('#{CGI.escapeHTML(key)}', '#{CGI.escapeHTML(value)}', 'header');" %>
    <% end %>

    window.swaggerUi = new SwaggerUi({
      url: options.app_url + options.url,
      dom_id: "swagger-ui-container",
      supportHeaderParams: true,
      supportedSubmitMethods: ['get', 'post', 'put', 'delete', 'patch'],
      onComplete: function(swaggerApi, swaggerUi){
        if('console' in window) {
          console.log("Loaded SwaggerUI")
          console.log(swaggerApi);
          console.log(swaggerUi);
        }
        $('pre code').each(function(i, e) {hljs.highlightBlock(e)});
      },
      onFailure: function(data) {
        if('console' in window) {
          console.log("Unable to Load SwaggerUI");
          console.log(data);
        }
      },
      docExpansion: options.doc_expansion,
      apisSorter: "alpha",
      authorizations: auth_headers
    });

You can see the full new index.html.erb here: https://github.com/GeecoLABs/grape-swagger-rails/blob/master/app/views/grape_swagger_rails/application/index.html.erb.

If you find it of any interest @dblock, I can prepare a separate PR.

@sunnyrjuneja
Copy link
Author

@matfiz That looks great. Thanks mat!

@supagroova
Copy link

@matfiz nice, I like it! I ended up using the latest swagger-ui version and doing the following based on the code found here - https://github.com/swagger-api/swagger-ui/blob/master/dist/index.html#L81

Then I changed it as such:

      function addApiKeyAuthorization(){
        var key = encodeURIComponent($('#input_apiKey')[0].value);
        if(key && key.trim() != "") {
            apiKeyAuth = new SwaggerClient.ApiKeyAuthorization("Authorization", "Bearer "+key, "header");
            window.swaggerUi.api.clientAuthorizations.add("oauth2", apiKeyAuth);
            log("added key " + key);
        }
      }

The key difference is that once I changed the key api_key to oauth2 the headers were then included in the subsequent ajax calls.

I like your approach too and will try it out as I'd like to use this gem and keep swagger-ui out of my app code.

@supagroova
Copy link

@matfiz looking at your code again it would seem to me that the api_key you're setting in you're headers must be defined at app initialisation no?

My approach was to let the end user input their api_key into the #input_apiKey input and have that applied to all ajax calls.

I'm thinking I might fork the project, update it to the latest version of swagger-ui and then add this functionality. Might be of interest to you guys?

@matfiz
Copy link

matfiz commented Jan 15, 2016

@supagroova Pls have a look at all the changes I made to index.html.erb https://github.com/GeecoLABs/grape-swagger-rails/blob/master/app/views/grape_swagger_rails/application/index.html.erb

In my solution, the api_key can be initialized both at app initialization or after entering #input_apiKey.

What you suggested: window.swaggerUi.api.clientAuthorizations.add("oauth2", apiKeyAuth); with changing api-key to oauth2 actually should not affect anything, as this just changes the key in authz array.

I think I was solving a problem a little bit different that yours. I needed the header to be included in the api call to fetch swagger_doc.json. window.swaggerUi.api is not availale before window.swaggerUi.load(), so the clientAuthorizations.add() cannot be used for that. That's why I've included the manualy constructed authz in the swaggerUi init options.

I hope it has clarified :) I am also happy to provide a clean PR with the changes I've described.

@dblock
Copy link
Member

dblock commented Jan 15, 2016

I haven't dug in the discussion above, but I am interested in fixing anything that's broken here, please make pull requests!

davidbrewer pushed a commit to davidbrewer/grape-swagger-rails that referenced this pull request Mar 3, 2016
Previous to this update, when I tried to do API-key authorization using
headers rather than the query string, Swagger-UI would not send my
authorization headers. This update basically makes the authorization
options get set AFTER Swagger-UI has finished initializing, which seems
to make the system much happier. Changes based on a conversation on a
mostly-unrelated pull request,
ruby-grape#25
@davidbrewer
Copy link
Contributor

@supagroova You saved my life with your addApiKeyAuthorization example above. Could not for the life of me get api keys via Authorization headers working until I followed your example. Thanks!

@dblock: I'm not sure how best to make a clean pull request out of this change... there are still some weird issues in the resulting behavior (such as some javascript errors that remain, and the curl field not getting updated), but I think those weird issues might already be present and be related to multipart forms rather than header-based authorization. I suspect an update to the underlying Swagger-UI code might fix some of these issues but I'm a bit out of my depth when it comes to the other consequences.

Can you take a look at this commit and tell me if it seems worth trying to make a pull request out of this?

davidbrewer@5701ef5

Apologies if this has all gotten a bit far afield from the original purpose of this pull request!

@supagroova
Copy link

@davidbrewer It took me quite some time to work out that fix, so I'm glad that helped you as well! :-)

@dblock
Copy link
Member

dblock commented Mar 6, 2016

I commented in #48.

davidbrewer pushed a commit to davidbrewer/grape-swagger-rails that referenced this pull request Mar 7, 2016
Previous to this update, when I tried to do API-key authorization using
headers rather than the query string, Swagger-UI would not send my
authorization headers. This update basically makes the authorization
options get set AFTER Swagger-UI has finished initializing, which seems
to make the system much happier. Changes based on a conversation on a
mostly-unrelated pull request,
ruby-grape#25
davidbrewer pushed a commit to davidbrewer/grape-swagger-rails that referenced this pull request Mar 8, 2016
Previous to this update, when I tried to do API-key authorization using
headers rather than the query string, Swagger-UI would not send my
authorization headers. This update basically makes the authorization
options get set AFTER Swagger-UI has finished initializing, which seems
to make the system much happier. Changes based on a conversation on a
mostly-unrelated pull request,
ruby-grape#25
@dblock
Copy link
Member

dblock commented Apr 27, 2016

I believe #48 fixes this, I am going to close it.

@dblock dblock closed this Apr 27, 2016
@dblock dblock reopened this Apr 27, 2016
@dblock
Copy link
Member

dblock commented Apr 27, 2016

It still fails:

  1) Swagger #options #api_auth:oauth2 adds a token when the route specifies oauth2 authorization
     Failure/Error: expect(page).to have_css 'span.attribute', text: 'Authorization'
       expected to find css "span.attribute" with text "Authorization" but there were no matches
     # ./spec/features/swagger_spec.rb:108:in `block (4 levels) in <top (required)>'

Want to take a look with all the recent changes @sunnyrjuneja or @davidbrewer?

@sunnyrjuneja
Copy link
Author

I can take a look this afternoon. I'll block out sometime for 1pm PST.

@dblock
Copy link
Member

dblock commented Apr 27, 2016

Given all the 💰 we pay you you're now totally on a deadline :)

@sunnyrjuneja
Copy link
Author

Hey @dblock I gave a pretty honest look at this. I think I can help but do you mind explaining how to run the test server without running tests?

I tried bundle exec rackup config.ru in grape-swagger-rails/spec/dummy but I get Can't read from server. It may not have the appropriate access-control-origin settings.

@dblock
Copy link
Member

dblock commented Apr 28, 2016

You're on the right track, it's cause the default is pointing to the wrong place. Change the URL in the green box to http://localhost:9292.

screen shot 2016-04-28 at 9 19 18 am

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.

5 participants