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

Deprecate 'X-SPREE-TOKEN' header 2 #3029

Merged
merged 1 commit into from
Jan 18, 2019

Conversation

twist900
Copy link
Contributor

@twist900 twist900 commented Jan 9, 2019

Description

This PR is a continuation of #2996. It keeps the X-SPREE-TOKEN alongside the newly introduced bearer authorization token for backward compatibility. A deprecation message is displayed when the X-SPREE-TOKEN header is detected.

Also, it fixes some linting issue.

ref #2934

Checklist:

  • Pull Request guidelines are respected
  • Documentation/Readme have been updated accordingly
  • Changes are covered by tests (if possible)
  • Each commit has a meaningful message attached that described WHAT changed, and WHY

@twist900 twist900 changed the title Twist900/authorization header Deprecate 'X-SPREE-TOKEN' header 2 Jan 9, 2019
@twist900 twist900 force-pushed the twist900/authorization-header branch from c94fb91 to 33b020b Compare January 9, 2019 10:38
Copy link
Contributor

@aitbw aitbw left a comment

Choose a reason for hiding this comment

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

Thanks a lot for completing this PR, @twist900 ! 💪

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Left a code formatting comment, but overall it's a great work.

I'm also not sure if it's better to squash commits since git looking at the last commit alone outside this PR could look like we are adding the spree_token into an already existing bearer_token instead of the opposite. What do you think?

end
helper_method :api_key

def bearer_token
pattern = /^Bearer /
header = request.headers["Authorization"]
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove one space after header, it's aligned to pattern on the line above but if that line changes in the future we'll be forced to change this one as well and this is not ideal for git history. What do you think?

@twist900 twist900 force-pushed the twist900/authorization-header branch from 33b020b to 41a30ee Compare January 9, 2019 22:37
Move from custom X-Spree-Token header to Authorization header.
Keep support of the deprecated X-Spree-Token header for
backward compatibility.
@twist900 twist900 force-pushed the twist900/authorization-header branch from 41a30ee to 88068cb Compare January 9, 2019 23:11
Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

🍰 Thanks!

Copy link
Contributor

@ericsaupe ericsaupe left a comment

Choose a reason for hiding this comment

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

Great changes, thanks!

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

6 participants