furkan3ayraktar / clojure-polylith-realworld-example-app

Clojure, Polylith and Ring codebase containing real world examples (CRUD, auth, advanced patterns, etc) that adheres to the RealWorld spec and API.
MIT License
448 stars 79 forks source link

Add "Authorization" to Allow Headers #6

Closed zelark closed 5 years ago

zelark commented 5 years ago

All auth routes won't work without it. When we make a request to an auth route the authorization header is requested: Access-Control-Request-Headers: authorization,content-type

So, backend has to allow it as well.

furkan3ayraktar commented 5 years ago

@zelark Please correct me if I'm wrong.

I think Access-Control-Allow-Credentials: true is a better way of telling you are expecting authorization header than adding Authorization to Access-Control-Request-Headers. Of course, when you have this setup, client code should use XMLHttpRequest.withCredentials or credentials option in Request().

You can read more about it here.

zelark commented 5 years ago

Thanks for the links. I haven't knew about that approach. I've read it.

A little prehistory: I ran into this issue when I played with https://github.com/jacekschae/conduit. It doesn't add withCredentials to requests, instead it adds Authorization header. So, when a browser makes OPTIONS request, it automatically puts into the request Access-Control-Request-Headers: authorization header. And because the server doesn't allow this header, nothing happens after. But that frontend works with NodeJS RealWorld backend well.

To be honest, I'm new to topic about authorization. So, I have a few questions:

  1. Could you explain why you think Access-Control-Allow-Credentials: true is a better way than adding Authorization header?
  2. If we go this way, how it can be done on client? Adding withCredentials is not enough, it doesn't add Authorization header with token, because now, token is stored in Local Storage, and we have to add it manually.

P. S. As I understood, when we want to use withCredentials, the server has to set a cookie with the token.

furkan3ayraktar commented 5 years ago

I think I have replied to this too quickly. My explanation was misleading.

When I read through the RealWorld code again, I realized that we are not making the authorization through cookies, we make it by manually setting the Authorization header on the client side. The server returns the required token for authorization within the response body. In this case, you are right, we can remove Access-Control-Allow-Credentials from the code and add Authorization to Access-Control-Request-Headers.

However, if I was creating the API specs for RealWorld API, I would use cookies for authentication instead of Authorization header. In that case, after a successful login, server would return a response with Set-Cookie header which contains the token. The browser would set the returned cookie to the following requests automatically. So, in the client code, you wouldn't need to set Authorization header for all requests, everything is done automatically. Also, this setup lets server to update token when needed, without requiring user to login again. The server can return Set-Cookie header after any request, when needed. The only thing the client application would do is setting withCredentials to true. In this case, server will also need to return Access-Control-Allow-Credentials: true header for the preflight requests to inform client that it's allowed to set cookies.

Reasons that I think this second approach better are:

  1. Client does not need to know about authorization flow.
  2. Client does not need to handle saving authorization token locally for persisting the login. Browser will handle saving cookies for the next time user visits the site.
  3. Server has flexibility to update token any time for security reasons or any other reason.

There is a good answer on stackoverflow about withCredentials where you can find some more information.

Finally, would you mind also removing the line that sets Access-Control-Allow-Credentials: true, since it's not needed in this specific project?

zelark commented 5 years ago

Thanks for a great explanation. It made things clear.

Sure, I'll remove Access-Control-Allow-Credentials: true.

furkan3ayraktar commented 5 years ago

Thanks for the changes! I've merged it now.