Skip to content

Ability to specify Custom headers #1166

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

Merged
merged 7 commits into from
Dec 13, 2017
Merged

Ability to specify Custom headers #1166

merged 7 commits into from
Dec 13, 2017

Conversation

RuslanZavacky
Copy link
Contributor

@RuslanZavacky RuslanZavacky commented Dec 4, 2017

Problem
When using Sentry on premises, Sentry can be behind of authorization or any other service (or both). Part of the verification process comes from the headers passed, for example, csrf-token. Currently, there is no simple way how we can pass custom header to every sentry request. And as an additional complication, headers should be computed, as the token is not static.

Solution
To avoid any breaking changes, we can pass additional headers as options (via globalOptions), it is already supported by most of the libraries. To solve an issue with computed header value, we can pass function and execute it at a time of the headers evaluation.

Solution Problem
As it seems, and the comment in the raven.js code , as soon as you step into the Headers game, you might have issues with CORS. I am not too worried about it, as it is possible (for on-premises) to solve that, by using the same domain and do routing.
What I am struggling now with, is to make tests to work correctly and test things as they should.


  • If you've added code that should be tested, please add tests.
  • If you've modified the API (e.g. added a new config or public method), update the docs and TypeScript declaration file.
  • Ensure your code lints and the test suite passes (npm test).

@RuslanZavacky
Copy link
Contributor Author

This update is Backwards Compatible and shouldn't inflict any migration steps.

@RuslanZavacky
Copy link
Contributor Author

Hey @kamilogorek, I would greatly appreciate if you'd be able to look into that PR and give your feedback :)

@kamilogorek
Copy link
Contributor

Very nice implementation, especially that you include tests and docs as well, thanks! As this is completely new functionality, can I get another "ay" from someone? cc: @getsentry/javascript

As you are already modifying docs, can you update transport section, so it also lists options as one of the properties it receives?

@RuslanZavacky
Copy link
Contributor Author

As you are already modifying docs, can you update transport section, so it also lists options as one of the properties it receives?

@kamilogorek in this one, are you talking about this part in docs:

https://github.com/getsentry/raven-js/pull/1166/files#diff-fd40cf2be7711772de9d8316da038cceR244

It does seem transport is described there, or there is another place? :) I am happy to update it, can you please link the place where to look?

@kamilogorek
Copy link
Contributor

Yes, this one. It's listing url, data, auth, and 2 callbacks, but no mention of options which allow developer to access all global options.

@RuslanZavacky
Copy link
Contributor Author

Done )

@kamilogorek
Copy link
Contributor

Thanks, @RuslanZavacky! I love when PRs include code, tests and docs. Great work! :)

@kamilogorek kamilogorek merged commit fbfea0f into getsentry:master Dec 13, 2017
@kamilogorek
Copy link
Contributor

Released in 3.21.0

@RuslanZavacky
Copy link
Contributor Author

Thank you @kamilogorek :)

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