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

OidcIdTokenValidator ensures clockSkew is positive number #6514

Closed
wants to merge 3 commits into from
Closed

OidcIdTokenValidator ensures clockSkew is positive number #6514

wants to merge 3 commits into from

Conversation

vishalvrv9
Copy link
Contributor

This pull request is in reference to #6443

Previously, the OidcIdTokenValidator.setClockSkew() asserted the clockSkew is not null.
This PR consists of:

  1. checks to ensure clockSkew >=0
  2. relevant test cases for both clockSkew=null (was missing as was pointed out in the discussion at OidcIdTokenValidator ensures clockSkew is positive number #6443) along with assertion tests for clockSkew >=0

Copy link
Contributor

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @vishalvrv9! Please see my comments for some minor updates. Also, can you ensure the commit message follows this format.

@jgrandja jgrandja changed the title not null and positive Assertions for OidcIdTokenValidator OidcIdTokenValidator ensures clockSkew is positive number Feb 8, 2019
@jgrandja jgrandja self-assigned this Feb 8, 2019
@rwinch
Copy link
Member

rwinch commented Feb 15, 2019

@vishalvrv9 Thanks for the updates. It appears the changes are not compiling. You can check the build by running:

./gradlew clean check

@vishalvrv9
Copy link
Contributor Author

hey @rwinch @jgrandja apologies for the delay. the build is successful now. Please review whenever convenient

@jgrandja
Copy link
Contributor

Thanks @vishalvrv9. This is ready to get merged.

Before we proceed, can you please squash the commits to 1. As well, ensure the commit message follows this format. Thank you!

@vishalvrv9
Copy link
Contributor Author

@jgrandja have squashed the commit, does this work?

@jgrandja
Copy link
Contributor

@vishalvrv9 There are 3 commits and there should only be 1 - this one 74dc68a5792cdbd5eeb0313c1c542627a7377776.

The 2 merge commits should not be there. You need to rebase on master and add that one commit. Makes sense?

@vishalvrv9
Copy link
Contributor Author

I'm sorry I tried rebasing to master and force push but the merge commits don't seem to be going away. I'm not sure if I should git revert the two merge commits and then squash everything again or if there is a better way?

Also, can't figure out why the two merge commits are appearing grayed out
image

@jgrandja
Copy link
Contributor

jgrandja commented Mar 4, 2019

@vishalvrv9 I think the easiest thing to do is start a new branch from master and than git cherry-pick da1d63987afb5923bee0d92df6bdee9b28e86b6a. This should do it.

@vishalvrv9 vishalvrv9 mentioned this pull request Mar 5, 2019
@vishalvrv9
Copy link
Contributor Author

vishalvrv9 commented Mar 5, 2019

@jgrandja I think using git pull upstream has created the merge commits (and all this hassle) from the base branch to my forked repo. I'm fairly new to git and assumed I should update my master branch before I commit anything to avoid merge conflicts (if any) I've created a new pull request #6592 but the merge commits from master still remain 😞

Apologies for the non-issue related hassle I'm creating. At this point, I just feel like starting from scratch would be more efficient but I wish there was a way out.
Could you suggest what you deem best in this scenario?

@jgrandja
Copy link
Contributor

jgrandja commented Mar 5, 2019

@vishalvrv9 Ok, let's start from scratch.

Assuming your branch name for the PR is called gh-6443-clockSkew-fix, execute the following commands in order.

Checkout branch

git checkout gh-6443-clockSkew-fix

Move/copy the branch to new name (for backup)

git branch -M gh-6443-clockSkew-fix-backup

Create new branch with same name as in PR - starting from upstream/master

git checkout -b gh-6443-clockSkew-fix upstream/master

Cherry-pick the commit in branch gh-6443-clockSkew-fix-backup -> OidcIdTokenValidator ensures clockSkew is positive number

git cherry-pick da1d63987afb5923bee0d92df6bdee9b28e86b6a

Now you can force-push gh-6443-clockSkew-fix.

Let me know if you have any questions.

@jgrandja
Copy link
Contributor

jgrandja commented Apr 1, 2019

@vishalvrv9 Do you need any further help with this? Would you like me to get this rebased and merged?

@vishalvrv9
Copy link
Contributor Author

Hey @jgrandja ,
I tried the above steps mentioned but the step

git checkout-b gh-6443-clockSkew-fix upstream/master

results in

upstream/master' is not a commit and a branch 'gh-6443-clockSkew-fix' cannot be created from it

Also tried deleting the local repo, adding upstream back & trying the above steps but the aforementioned error still perists.

@jgrandja
Copy link
Contributor

jgrandja commented Apr 8, 2019

@vishalvrv9 I re-tried the steps I outlined above and it worked on my end. Not sure why you are getting that error message.

If you don't mind?...I can get this merged as I would like to get this in for the 5.2.0.M2 release scheduled for April 15.

@vishalvrv9
Copy link
Contributor Author

@jgrandja,
Not a problem, please get this merged based on release milestones. Apologies if I was a hindrance/bottleneck as far as contributions were concerned,

@jgrandja jgrandja added type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) OIDC labels Apr 10, 2019
@jgrandja jgrandja added this to the 5.2.0.M2 milestone Apr 10, 2019
@jgrandja
Copy link
Contributor

@vishalvrv9 Not a problem at all - you were not a hindrance. Git is very powerful but it could take some time to get used to. Hopefully you learned something new and the next time will be easier :) Thanks again for your contribution! This is now in master.

@jgrandja jgrandja closed this Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants