[openstreetmap/openstreetmap-website] Problem with deny_access and request formats during tests (#2064)

Andy Allan notifications at github.com
Wed Nov 14 18:25:17 UTC 2018


I'm attempting to refactor changeset_comments_controller to use CanCanCan, and there's a problem with how our tests run, and how they interact with with our `deny_access` handler, and API request format handling in general.

Until now, you can call `post "api/0.6/changeset/comment/:id/hide"` with any request format you like, either html or xml or plain text or whatever, and since there's no payload it doesn't really matter very much. The action ignores the request format and always responds with XML (`render :xml => comment.changeset.to_xml.to_s`).

We take advantage of this in our tests, which are mostly of the format `post :destroy, :params => { :id => comment.id }` which is sending a text/html request behind the scenes. You can see the difference between "accept anything, always respond in XML" and "only accept XML requests" if you change the controller action to e.g.

```ruby
    # Return a copy of the updated changeset
    respond_to do |format|
      format.xml { render :xml => comment.changeset.to_xml.to_s }
    end
```
This would now throw an error on any request that's not XML, including the requests from the test suite.

The immediate problem is that our `deny_access` handler now uses the request format to decide whether to redirect requests to the login page (for html requests, assumed to be website requests) or not (for any other type of request). So when porting these API methods to CanCanCan, instead of receiving a 403 response (when you're logged in but don't have permissions, e.g. not a moderator) you get redirected to an html error page instead. This is because the test suite makes all its requests as text/html by default. Previously that hasn't mattered, but now it does.

Possible solutions include:

* Just change the test suite and accept the redirects. For example,
   ```diff
       # not a moderator
       post :restore, :params => { :id => comment.id }
  -   assert_response :forbidden
  +   assert_response :redirect
  +   assert_redirected_to :controller => :errors, :action => :forbidden
  ```
  But I think this is very misleading, since that's not how we're designing the XML API. I don't think it's a good idea to add in the redirect for real API responses either, since not all libraries follow redirects.

* Just change the test suite. For example, 
  ```diff
       # not a moderator
  -    post :restore, :params => { :id => comment.id }
  +    post :restore, :params => { :id => comment.id, :format => "xml" }
       assert_response :forbidden
  ```
  This could be tedious and repetitive though. A variation would involve e.g. `api_post` as a wrapper around `post` to set the format, perhaps. Although it makes the tests pass, any real-world API user sending text/html (are there any? Who knows?) would still be affected and get a redirect.

* Set a default format in a `before_action` handler in the controller
   ```diff
     class ChangesetCommentsController < ApplicationController
      skip_before_action :verify_authenticity_token, :except => [:index]
  +  before_action :set_default_request_format, :only => [:create, :destroy, :restore]
      before_action :authorize_web, :only => [:index]
     ...
  +  def set_default_request_format
  +     request.format = "xml"
  +  end
  ```
  This needs to be done in a `before_action`, because `deny_access` can get called before any code in the action has been run. However, unless you know the intricacies of what we're doing, it could appear meaningless to other developers. 

* Doing something even more clever with `deny_access` to figure out the appropriate redirect/status response choice. I haven't figured anything out that would avoid having to mark particular actions as "this action is part of the API and is going to respond in XML, so don't redirect to the login page, even if the request happens to be in html format".

* Do something more fundamental, like refactoring api methods into their own namespace, and giving them a different `deny_access` handler. Perhaps the cleanest solution?

(I'm not going to suggest addressing the underlying topic of accepting any request format, by using `respond_to` in actions, since that has the potential to break existing API clients who might be sending requests in any old format. But I'll call this a candidate solution for a future API change.)





-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/issues/2064
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20181114/b38c35a6/attachment.html>


More information about the rails-dev mailing list