Skip to content

Conversation

@alexbakerdev
Copy link

@alexbakerdev alexbakerdev commented May 16, 2019

Problem

See #197

Solution

Add a COOKIE_NAME env var to the option struct

Notes

It looks like there are already some tests around custom cookie name values, and I couldn't find any tests based around environment variables I couldn't find any, but if there are some tests needed do let me know!

@alexbakerdev
Copy link
Author

alexbakerdev commented May 16, 2019

Just saw #196 which seems to make this unnecessary, will leave it open in case that PR won't be merged for some time.

@jphines
Copy link
Contributor

jphines commented May 16, 2019

@alexbakerdev were you able to verify this functionality in your application?

Per your example,

Cookie Name Domain
_sso_proxy_staging staging.example.com
_sso_proxy example.com

SSO sets cookies per domain (no wildcards) so I would expect the applications on each of these domains not receive the other's _sso_proxy cookie as sent by the browser. This existing application behavior moderately negates the usefulness of this change as described in #197

@alexbakerdev
Copy link
Author

alexbakerdev commented May 16, 2019

Ah yup, as you say by default the cookie is set per domain however we had an interesting use case where we had multiple backend apis that were exposed on different subdomains.
I.e. frontend.domain.com, api.domain.com

Setting the cookie per domain in this case causes issues; since the proxy needs to initiate the authorisation redirect flow for each domain, from client-side at frontend.domain.com making a CORS request to api.domain.com (when you don't already have a valid cookie) the proxy will redirect to the auth server which will fail, as it doesn't support cors.

We solved this by setting the cookie domain to domain.com so the same cookie is used for each of its subdomains, but this caused issues because of the nested nature of the staging sub domain.

I get its a super weird use case, but I feel like there could be other use cases where environments may share a domain for whatever reason, and a similar problem will arise.

EDIT: Also yeah, our fork is solving this problem for us right now.

@jphines
Copy link
Contributor

jphines commented May 17, 2019

@alexbakerdev Thanks for the write-up! This makes more sense in given the context!

@jphines jphines merged commit 50c1a91 into buzzfeed:master May 17, 2019
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.

2 participants