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

Refactor RSA verification: Replace jsrsasign with crypto-js #268

Merged
merged 11 commits into from Jan 30, 2020

Conversation

lbalmaceda
Copy link
Contributor

Changes

jsrsasign library was not compatible with the Hermes JS engine, currently optional but still officially supported by the React Native platform for Android. This PR changes the RSA signature check implementation to use crypto-js instead.

No breaking changes are introduced on this build.

References

Resolves #251

Testing

The unit tests were updated. To evaluate Hermes support, it has been tested this way:

  1. Enable Hermes for your android app by changing the app/build.gradle file
 project.ext.react = [
     entryFile: "index.js",
     enableHermes: true,  // clean and rebuild if changing
 ]
  1. Clean and Rebuild
cd android && ./gradlew clean assemble
  1. Run the android app in a "release" variant
react-native run-android --variant release
  • This change adds unit test coverage
  • This change has been tested on the latest version of the platform/language or why not

Checklist

@lbalmaceda lbalmaceda added this to the v2-Next milestone Jan 16, 2020
@lbalmaceda lbalmaceda requested a review from a team January 16, 2020 20:47
@lbalmaceda lbalmaceda added the medium This PR may require moderate effort to action, or contains many changes to review label Jan 17, 2020
src/jwt/__tests__/base64.spec.js Outdated Show resolved Hide resolved
expect(base64.padding(base64.padding('abc'))).toBe('abc=');
});

it('decode to hex', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest you reword this to use present simple describing what it should do, instead of simply repeating the function name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see other choices here. The method's responsibility is pretty dumb, TBH. Only decoding a base64 into a HEX value. What do you have in mind?

src/jwt/__tests__/jwt.spec.js Show resolved Hide resolved
src/jwt/__tests__/jwt.spec.js Outdated Show resolved Hide resolved
src/jwt/base64.js Outdated Show resolved Hide resolved
src/jwt/rsa-verifier.js Outdated Show resolved Hide resolved
src/jwt/rsa-verifier.js Outdated Show resolved Hide resolved
src/jwt/rsa-verifier.js Outdated Show resolved Hide resolved
src/jwt/signatureVerifier.js Outdated Show resolved Hide resolved
src/jwt/signatureVerifier.js Outdated Show resolved Hide resolved
Copy link
Contributor

@Widcket Widcket left a comment

Choose a reason for hiding this comment

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

LGTM

@lbalmaceda lbalmaceda merged commit 72d8cd9 into master Jan 30, 2020
@lbalmaceda lbalmaceda deleted the temp-idtv-2 branch January 30, 2020 18:54
@lbalmaceda lbalmaceda modified the milestones: v2-Next, v2.2.0 Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CH: Changed medium This PR may require moderate effort to action, or contains many changes to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[2.1.0] TypeError: undefined is not a function, js engine: hermes
2 participants