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

Rename encryption_method to signing_method #53

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

maxencehenneron
Copy link

This library does not do encryption and the JWT tokens are readable by anyone.

Using the word "encryption" makes it seem it is safe to store sensible data in the payload when it is clearly not. Encrypted JWT tokens are called JWE tokens and are not supported by this library or by ruby-jwt.

What this library does however is signing the tokens to ensure they were created by a trusted party.

I renamed encryption_method to signing_method and updated the README to specify "signing" instead of "encryption".
It is still possible to use encryption_method for backward compatibility but this now deprecated.

@@ -101,14 +121,15 @@
expect(decoded_token[1]["alg"]).to eq "HS256"
end

it "creates a signed encrypted JWT token with a custom dynamic payload" do

Copy link
Collaborator

Choose a reason for hiding this comment

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

Layout/EmptyLines: Extra blank line detected.

@@ -65,10 +65,10 @@
expect(decoded_token[1]["alg"]).to eq "none"
end

it "creates a signed encrypted JWT token" do
it "creates a signed JWT token" do
Copy link
Collaborator

Choose a reason for hiding this comment

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

RSpec/RepeatedDescription: Don't repeat descriptions within an example group.

Copy link
Member

@nbulaj nbulaj left a comment

Choose a reason for hiding this comment

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

While I agree with the changes we can't just rename methods and - what is more important - config options as it will break existing projects. We have to at least deprecate them first and only then remove/rename.

@saurabhlime
Copy link

When can this be merged?

@maxencehenneron
Copy link
Author

While I agree with the changes we can't just rename methods and - what is more important - config options as it will break existing projects. We have to at least deprecate them first and only then remove/rename.

Just saw your reply, backward compatibility is actually kept with https://github.com/doorkeeper-gem/doorkeeper-jwt/pull/53/files#diff-74e4c64e2f216da896bfaa36c9fa1f58c3e9cbcb042932181d997d3a51510b7fR43

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

4 participants