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

bug: Fix auth header for push #1080

Merged
merged 2 commits into from
Apr 25, 2019
Merged

bug: Fix auth header for push #1080

merged 2 commits into from
Apr 25, 2019

Conversation

jrconlin
Copy link
Contributor

Closes #1079

Pull Request checklist

  • Quality: This PR builds and tests run cleanly
    • cargo test --all produces no test failures
    • cargo clippy --all --all-targets --all-features runs without emitting any warnings
    • cargo fmt does not produce any changes to the code
    • ./gradlew ktlint detekt runs without emitting any warnings
    • Note: For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

* include auth type prefix
* fix URL for channel list (note:
  mozilla-services/autopush#1330

Closes #1079
@jrconlin jrconlin requested review from thomcc and jonalmeida April 25, 2019 02:18
@jrconlin jrconlin added this to the 2019-04-26 milestone Apr 25, 2019
return Err(CommunicationError(format!(
"Unhandled client error {} : {:?}",
requested.status,
String::from_utf8_lossy(&requested.body)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, is there any risk of PII in the body?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quite the opposite. Our push servers return extended help info whenever they hit an error. This info was getting lost in how the message was being returned.

}
if self.auth.is_none() {
self.auth = response["secret"].as_str().map(ToString::to_string);
println!("### Got Auth {:?}", self.auth);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: get rid of the println before landing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, crap! Thanks!

@jrconlin jrconlin merged commit 594f4e3 into master Apr 25, 2019
@jrconlin jrconlin deleted the bug/1079 branch April 25, 2019 15:29
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.

Fix AUTH header for push component
2 participants