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

Implement cookie forwarding for api proxy #232

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

lo1tuma
Copy link
Member

@lo1tuma lo1tuma commented Dec 19, 2013

The cookie name on the client gets prefixed with the name of the api and a separator. This prevents overwriting a cookie with the same name from a different api host and ensures that the cookies retrieved from the client will forwarded only to the correct api host.

@spikebrehm
Copy link
Member

This is interesting. What are your use cases?

I don't think we want to include this as a default for Rendr. Passing through all client-side cookies to the API seems like a very app-specific change that does not generalize to most use cases.

It's easy to pass a custom apiProxy middleware to rendr.createServer(). Perhaps you could release a small module like rendr-cookie-api-proxy or something?

@lo1tuma
Copy link
Member Author

lo1tuma commented Dec 20, 2013

Our API is not stateless and requires a session id sent in a cookie.

This PR doesn’t forwards all client-side cookies to all api hosts. If we get a set-cookie header in the response of an api host we will save this header in a new cookie to the client. Only these persisted set-cookie headers will then be sent to the original api host.

cc: @c089

@spikebrehm
Copy link
Member

Ah, I see, thanks for explaining @lo1tuma. It still seems to me that this does not belong in the library because it is specific to your use case. At Airbnb, we have all sorts of overrides in our DataAdapter for this type of logic.

@c089
Copy link
Member

c089 commented Dec 23, 2013

Passing our own apiProxy would require us to copy the existing apiProxy just to add a bit of functionality. While I agree that the cookie handling probably shouldn't be in the default apiProxy, we were not able to extend on it because in the DataAdapter we only have the original req from the client, our request to the api and it's response, but not the res to the client.

Maybe we could change the default apiProxy to pass the res (responseToClient) object to the dataAdapter as well, so that we could handle cookies there?

Or could you think of a better way to make the apiProxy extensible?

@c089
Copy link
Member

c089 commented Dec 27, 2013

After toying with some ideas, @lo1tuma and I came up with the following proposal how we could make this extensible without copying the apiProxy. If @spikebrehm likes it we'd be happy to contribute the implementation, as well as rewrite the existing x-forwarded-for header and cookie forwarding stuff to make use of it:


Currently, when custom behavior for the apiProxy is required, users have to copy the whole apiProxy to make custom changes. While some custom behavior can be placed in the dataAdapter, this does not allow more complex things, which is why we proposed adding cookie handling to the core apiProxy where it does not belong (see #232). This proposal tries to make both the apiProxy and dataAdapter extensible using small, reusable modules. Once rendr supports configuring different data adapters per configured API, this will allow to enable a extension module for all configured APIs by setting it on the ApiProxy or just a specific API by setting it on a DataAdapter.

This would require two extension points: One before the DataAdapter is used to make the request and one when it receives the response. We propose implementing this using two events which will be emitted by both the apiProxy and the dataAdapter(s):

  • one event sent before issuing the request to the API, where the handler function can access the request objects (and possible also the responseToClient though I'm not sure if that makes sense):

    apiProxy.on('before', function addXForwardedForHeader(requestFromClient, requestToApi) {
         requestToApi.headers['X-Forwarded-For'] = ...
         // ...
    });
    
  • one event when the response is received, with access to the response objects (and possible also the request objects, though I'm not sure if that makes sense):

    apiProxy.on('after', function copyCookiesFromApiResToClientRes(responseToClient, responseFromApi) {
      //...
    })
    
  • a shortcut for including extensions that use both before and after hooks:

    apiProxy.use(passthroughCookies))
    // equivalent to:
    //   apiProxy.on('before', passthroughCookies.before ))
    //   apiProxy.on('after', passthroughCookies.after ))
    

@@ -142,6 +142,8 @@ RestAdapter.prototype.apiDefaults = function(api, req) {
delete api.body;
}

// disable global cookie jar
api.jar = false;
Copy link
Member

Choose a reason for hiding this comment

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

this will no longer be necessary when #248 is merged

@bigethan
Copy link
Contributor

bigethan commented Apr 2, 2014

I like this event extension a lot, as we'll rarely be changing the core apiProxy functionality, and just adding stuff to it.

@pjanuario
Copy link

👍

@pjanuario
Copy link

This was one of the reason why I created https://github.com/pjanuario/rendr-auth-rest-adapter and opened this issue #457

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