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

Add rspec test cases #40

Merged
merged 1 commit into from
Jun 19, 2020
Merged

Add rspec test cases #40

merged 1 commit into from
Jun 19, 2020

Conversation

btalbot
Copy link
Contributor

@btalbot btalbot commented Jun 18, 2020

These currently cover about 97% of the code. They also expect other pending PR fixes to be merged before they
will pass. These PR include fixes for RoR extentions, nonce-valdation, name in info hash, and default-scope.

Error handling and various other corner cases are missing from these tests and should be added when possible.

These currently cover about 97% of the code. They also expect other pending PR fixes to be merged before they
will pass.  These PR include fixes for RoR extentions, nonce-valdation, name in info hash, and default-scope.

Error handling and various other corner cases are missing from these tests and should be added when possible.
@btalbot
Copy link
Contributor Author

btalbot commented Jun 18, 2020

I should note that I'm still learning about how apple handles OAuth2 / OIDC vs how it is done by google or facebook. These tests are based on some tests from omniauth-google-oauth2 which seems to be widely used and stable but also different enough that there could be some misunderstanding.

At the very least, test coverage would help avoid RoR extensions sneaking in and various syntax errors.

The RSA key signing complexity at the top of the tests is due to how JWT.decode is used as it requires RSA keys to be available when decoding and validating the id_token. As real apple keys cannot be used (we don't have the private keys) test keys are injected and used instead.

@nhosoya
Copy link
Owner

nhosoya commented Jun 19, 2020

Awesome!!
I actually ran this test and found it using the RoR extension 😭

return {} unless request.params['user'].present?

@nhosoya
Copy link
Owner

nhosoya commented Jun 19, 2020

I merged #41 and all the tests passed.

Copy link
Owner

@nhosoya nhosoya left a comment

Choose a reason for hiding this comment

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

Thanks. I will also set up the CI later.

@nhosoya nhosoya merged commit 27b1170 into nhosoya:master Jun 19, 2020
@btalbot btalbot deleted the add-spec-tests branch June 19, 2020 02:59
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