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

OidcIdTokenValidator ensures clockSkew is positive number #6443

Open
jgrandja opened this Issue Jan 15, 2019 · 10 comments

Comments

Projects
None yet
2 participants
@jgrandja
Copy link
Collaborator

jgrandja commented Jan 15, 2019

OidcIdTokenValidator.setClockSkew() should assert that the clockSkew is >= 0.

@vishalvrv9

This comment has been minimized.

Copy link

vishalvrv9 commented Jan 15, 2019

Hey @jgrandja
New Lurker here. Would be more than willing to help. 😄

From my initial analysis, it seems like I need to update OidcIdTokenValidator.java i.e.

within OidcIdTokenValidator.java,

Assert.notNull(clockSkew, "clockSkew cannot be null"); inside the setClockSkew method should be changed to Assert.isTrue(clockSkew >= 0, "clockSkew should be greater than 0");

Please let me know if I'm missing something.

Also, was curious as to why this validation is being changed? to avoid negative Duration values?

EDIT: Forgot to insert validation inside Assert.isTrue 😅

@jgrandja

This comment has been minimized.

Copy link
Collaborator Author

jgrandja commented Jan 15, 2019

@vishalvrv9 Thanks for the offer! This is yours. Yes, you have it right as far as what changes are required. The main reason for this change is to prevent the user from configuring a negative value. Also, please ensure you write a test for this. Thanks so much!

@vishalvrv9

This comment has been minimized.

Copy link

vishalvrv9 commented Jan 20, 2019

Hey @jgrandja,
I was struggling with writing suitable test cases for this particular change.

Firstly, I tried looking for a test case within OidcIdTokenValidatorTests for the previous use case i.e. Assert.notNull(clockSkew, "clockSkew cannot be null"); but there was no test where the clockSkew was mocked as null.

Another thing I noticed is OidcIdTokenValidatorTests contain xxxxThenHasErrors test cases for OAuth2Error while the Assert spring class would throw IllegalArgumentException or IllegalStateException. Do I need to write concerning test cases somewhere else? (I don't know if I'm making sense at this point because I'm new to the code base)

Then, I went through this to understand the function of the clockSkew. I now understand the purpose of a clockSkew better, but from a test case perspective; still no luck.

Could you help me move forward and let me know what I'm missing here?

@jgrandja

This comment has been minimized.

Copy link
Collaborator Author

jgrandja commented Jan 21, 2019

@vishalvrv9

Assert.notNull(clockSkew, "clockSkew cannot be null"); but there was no test where the clockSkew was mocked as null.

You're right and this test needs to be there. Looks like it was missed. Thanks for pointing that out. Can you please add this test as well. For an example of how to write this test take a look at OidcIdTokenDecoderFactoryTests.setJwtValidatorFactoryWhenNullThenThrowIllegalArgumentException. It should be the same like this one.

For the negative Duration test, it should be named setClockSkewWhenNegativeSecondsThenThrowIllegalArgumentException and you can pass in Duration.ofSeconds(-1).

As an FYI, our test names follow this naming convention: methodNameWhen<condition>Then<Expectation>

Does this help?

@jgrandja

This comment has been minimized.

Copy link
Collaborator Author

jgrandja commented Feb 1, 2019

Hi @vishalvrv9. How are things going?

@vishalvrv9

This comment has been minimized.

Copy link

vishalvrv9 commented Feb 3, 2019

Hey @jgrandja,
Apologies for the late reply. My computer gave up and I finished setting up dev environments, paths etc onto my new machine.

About the issue, I have made the necessary changes. The junit tests are passing.
But when I try building the project, using
./gradlew build OR ./gradlew clean build integrationTest
the build is failing.
The error I'm getting is:
image
image

It seems like an encoding error because of the author's name Eddú Meléndez.

Could you help me mitigate this?
(note: I'm making sure I'm in the correct project path & I'm using intellij on MacOS)

Update: Just went through the contributing guidelines again and changed the encoding to Latin-1 (ISO-8859-1) for the entire project. The error is still persisting.

@jgrandja

This comment has been minimized.

Copy link
Collaborator Author

jgrandja commented Feb 4, 2019

@vishalvrv9 Are you using the native Terminal application packaged with Mac OSX? You might want to look at this article.

Also, I use iTerm2 for my CLI and would highly recommend this over Terminal. I've never had any encoding issues with iTerm2. See if the same problem persists using iTerm2 and let me know.

Also, use this command to do a full build -> ./gradlew --refresh-dependencies clean build

@vishalvrv9

This comment has been minimized.

Copy link

vishalvrv9 commented Feb 5, 2019

Hey @jgrandja
Was using the built in IDE terminal for intellij. Using iterm2 henceforth and the gh-6443-clockSkew-assertion did the trick.

New impediment:
I cloned the spring-security project and created a local branch for the same. Checked out to the concerned feature-branch and have committed the changes locally. But when I try pushing the changes using git push origin gh-6443-clockSkew-assertion, I run into a permission problem saying

remote: Permission to spring-projects/spring-security.git denied to vishalvrv9. fatal: unable to access 'https://github.com/spring-projects/spring-security.git/': The requested URL returned error: 403

Note: I've signed the contributor license agreement. Also, apologies for being sloppy and an untimely delivery. Will be extra careful in the future. 😞

@jgrandja

This comment has been minimized.

Copy link
Collaborator Author

jgrandja commented Feb 5, 2019

@vishalvrv9

I cloned the spring-security project and created a local branch for the same

You need to fork spring-security and than clone your fork locally. Than you will push your local branch to your fork. From your fork, you'll be able to create a PR from the branch. Makes sense?

@vishalvrv9

This comment has been minimized.

Copy link

vishalvrv9 commented Feb 7, 2019

@jgrandja
Have submitted the PR #6514 :)

@jgrandja jgrandja self-assigned this Feb 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment