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

whitelist Authorization header via SharedVarnishConstants #26534

Merged
merged 2 commits into from Jan 10, 2019

Conversation

davidsbailey
Copy link
Member

follow-on to #26383 to allow Authorization header through Varnish and CloudFront. HttpCache cannot depend on SharedConstants because that file is not available in chef, so we have to jump through some extra hoops.

@davidsbailey
Copy link
Member Author

davidsbailey commented Jan 9, 2019

@sureshc , can you please review? CC @wjordan . Main points of interest are (1) HttpCache config and (2) the arrangement of the shared constants between lib/cdo and HttpCache.

I've verified on an adhoc that this is working end-to-end now to access spotify URLs via the startWebRequest block in App Lab. You can see it working here: https://adhoc-spotify-whitelist-studio.cdn-code.org/projects/applab/TP3c4-GKEI4pvQ5HayE-1w/view

although the access token expires after just a few minutes so if you get a 401 I may just need to update the program with a new access token

@davidsbailey davidsbailey merged commit 31a6f99 into staging Jan 10, 2019
@davidsbailey davidsbailey deleted the spotify-shared branch January 10, 2019 18:11
@wjordan
Copy link
Contributor

wjordan commented Jan 10, 2019

@davidsbailey glad you got this working. I don't know if it was necessary to create a new module to reference this constant 'without pulling in anything else in HttpCache', as we already have several shared constants in HttpCache (referenced through ScriptConfig) similarly used by Dashboard.

In any case, maintaining two different places for storing the same type of constant is now less than ideal. Either we should move the existing script constants out of HttpCache into SharedVarnishConstants or move the newly-added constant from SharedVarnishConstants into HttpCache, removing the new module.

@davidsbailey
Copy link
Member Author

Apologies @wjordan for merging this before you had a chance to respond. That's fine, I can merge SharedVarnishConstants into HttpCache. Please see follow-on PR #26553 .

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.

None yet

3 participants