yacoma / morepath-realworld-example-app

Exemplary real world backend API built with Morepath (Python)
https://realworld.io
MIT License
5 stars 0 forks source link

CORS support? #1

Closed jldiaz closed 6 years ago

jldiaz commented 6 years ago

Hello.

Thank you very much for the effort of putting together this real-world example code. I was working on a project which uses morepath+pony at the backend and your example provides invaluable info for the configuration and deployment.

However, as far as I can see, your backend does not support CORS. When I try, for example, the Angular client and set https://conduit.yacoma.it/api as the api_url in environments/environment.ts, the page shows "Loading articles..." and no progress happens.

If I deploy my own local server (in a machine different than the one serving the client), I can see that the browser is sending OPTIONS requests (the CORS preflight), and the backend is answering "405 Method not allowed", instead of answering with "200 OK" and the appropiate CORS headers.

In my own project I had this problem also, and solved it by adding somewhere (in views.py) the following snippet:

@App.tween_factory()
def cors_tween(app, handler):
    def cors_handler(request):
        print(request)
        preflight = request.method == 'OPTIONS'
        if preflight:
            response = Response()
        else:
            response = handler(request)
        response.headers.add('Access-Control-Allow-Origin', '*')
        response.headers.add('Access-Control-Allow-Methods', 'GET, POST, PUT, DELETE')
        response.headers.add('Access-Control-Allow-Headers', 'authorization, content-type')
        return response
    return cors_handler

But I'm not sure if this is the proprer way to do it in "real world".

Could you please provide an example showing how CORS can be added to the backend?

henri-hulski commented 6 years ago

I have to release more.cors. But it's actually ready so I can release today.

henri-hulski commented 6 years ago

@jldiaz CORS support added in 6061a061a733ad48143945c067ad20d4bed8e2ee. Could you confirm that it's working now?

jldiaz commented 6 years ago

@henri-hulski Yes! It is working nicely now. Thank you very much.

jldiaz commented 6 years ago

@henri-hulski Hum... Actually something is wrong. All routes appear to work, except

OPTIONS /api/user

which returns 404 and no CORS headers. This makes impossible to update the user profile, because the browser rejects to do the PUT if the preflight response is not successful.

I think the problem can be related to the fact that the browser does not send the Authorization token as part of the preflight. Somehow, more.cors rejects the unauthorized attempt, but it does wrong, because, first, it returns 404 instead of 401, and second, no CORS headers are included in the response (which should be for OPTIONS verb).

This is the request:

OPTIONS /api/user HTTP/2.0
Host: <obfuscated server NAME>
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Encoding: gzip, deflate, br
Accept-Language: es-ES,es;q=0.8,en;q=0.5,en-US;q=0.3
Access-Control-Request-Headers: authorization,content-type
Access-Control-Request-Method: PUT
Origin: https://angular-realworld.stackblitz.io
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
X-Forwarded-For: <obfuscated IP>

This is the answer:

HTTP/1.1 404 Not Found
Connection: close
Content-Type: text/html; charset=UTF-8
Date: Fri, 08 Jun 2018 17:50:47 GMT
Server: gunicorn/19.8.1
Content-Length: 0
henri-hulski commented 6 years ago

Thanks for checking this I will look into it asap.

henri-hulski commented 6 years ago

@jldiaz Could you open an issue in more.cors as it seems to belong there. Maybe with some more background and a reproducible use case or test. Thanks for finding this.

henri-hulski commented 6 years ago

I do not really understand why the route to /api/user doesn't work but the others are working. Are you sure you haven't a cache problem?

jldiaz commented 6 years ago

No, it is not a cache problem. I have the same results if I perform the queries from CLI, using curl or httpie. But you are in part right. The problem is not with that route only. Other routes have the same problem.

Apparently, the browser never sends the Authentication header as part of OPTIONS, so any route which requires authentication (and should produce 401 if no auth is provided), causes a 404 error instead.

Routes which are not protected, such as /api/articles or /api/tags, or /article/id/comments have not this problem, so the page is almost functional and thus I did not notice the problems.

Interestinlgy, it is even posible to create a new article, because that is a POST on /api/articles, and this route allows GET (and thus OPTIONS) without auth.

henri-hulski commented 6 years ago

If you adjust the more.cors settings and set 'allow_credentials' to True and 'allowed_origin' to the specific server URL, does that fix it?

henri-hulski commented 6 years ago

For example by adding to conduit/settings/production.yml something like:

cors:
  allowed_origin: https://conduit.yacoma.it
  allow_credentials: True
jldiaz commented 6 years ago

No luck. Still the preflight answer does not contain cors headers.

I think the problem is related to line 94 in more.cors/main.py, because when the model is (tried to be) resolved, the exception HTTPUnauthorized is raised, and thus the context is set to app.

I think that causes trouble later, when at line 117 the context is used to find out the allowed verbs. The allowed_origin list is empty.

However I think that is a "philosophical" design flaw, difficult to be solved. I can build a (not so) minimal example using morepath, more.cors and more.jwt to show the problem, and post it as an issue at more.cors.

jldiaz commented 6 years ago

I'm not sure if more.cors is to be blamed... The problem is caused in fact by this line in morepath-realworld-example-app/conduit/auth/path.py, because raising an exception inside a path causes that no model can be associated with that path, and thus no introspection can be used to check the allowed verbs (which pertain to the views).

The problem is solved if permissions are managed in a standard way, i.e: allowing the path to return the appropiate model, and leaving the permission management to the view. I discovered this in the minimal app I was preparing to illustrate the issue.

henri-hulski commented 6 years ago

Hmm but that line should raise a HTTPUnauthorized 401 error not 404.

henri-hulski commented 6 years ago

If the path returns None like in https://github.com/yacoma/morepath-realworld-example-app/blob/master/conduit/auth/path.py#L32 it raises 404.

henri-hulski commented 6 years ago

In more.cors this line is raising a 404 exception: https://github.com/morepath/more.cors/blob/master/more/cors/main.py#L109 You could change that to 401 on your site and check if the HTTP status code you get will also change.

jldiaz commented 6 years ago

The true problem is not wheter more.cors returns 401 or 404. The real problem is that the CORS headers are absent from the response (because not allowed-methods were found). Thus, when the browser receives this response, it aborts the request.

To summarize:

A solution could be to modify more.cors so that it returns App.settings.cors.allowed_verbs if the path raised HTTPUnauthorized. I made that patch and it is working ok, so far.

henri-hulski commented 6 years ago

Ok I got it. Thanks for the explanation. Your solution sounds fine as the path in conduit seems to be valid regarding the Morepath logic. So it would be good if more.cors would support it. Could you create a PR for it in more.cors?

BTW As it seems you are into the CORS thingy maybe you could also review https://github.com/morepath/more.cors/pull/2. A haven't found anyone who wanted to review it and at the end just merged it in. It looks good to me, but I'm not too experienced with CORS.

henri-hulski commented 6 years ago

Fixed in morepath/more.cors#4.