Skip to content

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