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

oAuth token refresh #4

Merged
merged 1 commit into from
May 6, 2015
Merged

oAuth token refresh #4

merged 1 commit into from
May 6, 2015

Conversation

rkoster
Copy link
Contributor

@rkoster rkoster commented May 6, 2015

This PR uses the oauth2 lib for getting the UAA token.
This comes with the added benefit of support for refresh tokens.

@rkoster
Copy link
Contributor Author

rkoster commented May 6, 2015

cc @longnguyen11288

@simonjohansson
Copy link
Contributor

Ive added .travis.yml to master if you want to rebase in that into this branch instead of commiting it here :)

@rkoster
Copy link
Contributor Author

rkoster commented May 6, 2015

@simonjohansson have removed the travis related changes from this branch, have cherrypicked the README change onto master.

@simonjohansson
Copy link
Contributor

Coolio!

Ill take a look at this PR.

@simonjohansson
Copy link
Contributor

When doing a godep save it pulls in a dep that is not in Godeps.json

diff --git a/Godeps/Godeps.json b/Godeps/Godeps.json
index 3cf77bf..89b3662 100644
--- a/Godeps/Godeps.json
+++ b/Godeps/Godeps.json
@@ -30,6 +30,11 @@
                        "Rev": "754d38fccfc6e52083355014b6abc8d626445dfd"
                },
                {
+                       "ImportPath": "github.com/smartystreets/assertions",
+                       "Comment": "1.5.0-380-g4727d76",
+                       "Rev": "4727d767ce8a81811c40eedc2a836c85aebb4d09"
+               },
+               {
                        "ImportPath": "github.com/smartystreets/goconvey/convey",
                        "Comment": "1.5.0-386-geb2e83c",
                        "Rev": "eb2e83c1df892d2c9ad5a3c85672da30be585dfd"

@rkoster
Copy link
Contributor Author

rkoster commented May 6, 2015

I'm only able to reproduce the above Godeps.json diff when checking out the current master

@simonjohansson
Copy link
Contributor

@rkoster
I might be doing something wrong, but here is how I reproduce it

➜  go-cfclient git:(oauth_token_refresh) git checkout master 
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
➜  go-cfclient git:(master) git branch -d oauth_token_refresh
warning: deleting branch 'oauth_token_refresh' that has been merged to
         'refs/remotes/origin/oauth_token_refresh', but not yet merged to HEAD.
Deleted branch oauth_token_refresh (was da150b3).
➜  go-cfclient git:(master) git pull
Already up-to-date.
➜  go-cfclient git:(master) git status
On branch master
Your branch is up-to-date with 'origin/master'.
nothing to commit, working directory clean
➜  go-cfclient git:(master) git log --name-status | cat | head -n 5
commit f292098e5581d2dbcb4e796dd3d7801f29efe8cd
Merge: 3a58fc7 e519aa5
Author: Long Nguyen <long.nguyen11288@gmail.com>
Date:   Wed May 6 08:45:44 2015 -0400

➜  go-cfclient git:(master) git checkout -b oauth_token_refresh origin/oauth_token_refresh
Branch oauth_token_refresh set up to track remote branch oauth_token_refresh from origin.
Switched to a new branch 'oauth_token_refresh'
➜  go-cfclient git:(oauth_token_refresh) git log --name-status | cat | head -n 5                       
commit da150b310d0d02369e73c227380335dd9b86e853
Author: Ruben Koster <rkoster@starkandwayne.com>
Date:   Wed May 6 12:08:19 2015 +0200

    Switch to oauth2 lib for UAA token (comes with refresh token support)
➜  go-cfclient git:(oauth_token_refresh) godep save
➜  go-cfclient git:(oauth_token_refresh) ✗ git diff | cat   
diff --git a/Godeps/Godeps.json b/Godeps/Godeps.json
index 3cf77bf..89b3662 100644
--- a/Godeps/Godeps.json
+++ b/Godeps/Godeps.json
@@ -30,6 +30,11 @@
            "Rev": "754d38fccfc6e52083355014b6abc8d626445dfd"
        },
        {
+           "ImportPath": "github.com/smartystreets/assertions",
+           "Comment": "1.5.0-380-g4727d76",
+           "Rev": "4727d767ce8a81811c40eedc2a836c85aebb4d09"
+       },
+       {
            "ImportPath": "github.com/smartystreets/goconvey/convey",
            "Comment": "1.5.0-386-geb2e83c",
            "Rev": "eb2e83c1df892d2c9ad5a3c85672da30be585dfd"

@rkoster
Copy link
Contributor Author

rkoster commented May 6, 2015

After following the above steps there where still no changes on my machine

@simonjohansson
Copy link
Contributor

fakeUAAServer is great!

@simonjohansson
Copy link
Contributor

Maybe better to wait 0s instead of 3s to speed up the tests?

gomega.Eventually(client.GetToken(), "3s").Should(gomega.Equal("bearer foobar3"))
|
v
gomega.Eventually(client.GetToken(), "0s").Should(gomega.Equal("bearer foobar3"))

@simonjohansson
Copy link
Contributor

| After following the above steps there where still no changes on my machine

How weird, probably my machine that have some issues then.

"token_type": "bearer",
"access_token": "foobar" + strconv.Itoa(count),
"refresh_token": "barfoo",
"expires_in": 3,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you talking about this time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with regard to the expires_in time, the token will be seen as expired before the 3 seconds have elapsed. Since an expiryDelta is taken into account.

@simonjohansson
Copy link
Contributor

Besides my missunderstanding of the test case, and my weird environment on my machine this looks great!

What is the rules in this repo, more than one review to merge?

@rkoster
Copy link
Contributor Author

rkoster commented May 6, 2015

As can be seen on travis, the token refresh test only task 0.10s.
test

@rkoster
Copy link
Contributor Author

rkoster commented May 6, 2015

cc @longnguyen11288

@lnguyen
Copy link
Member

lnguyen commented May 6, 2015

I think one person reviewing is fine. Nothing to strict. @simonjohansson

simonjohansson added a commit that referenced this pull request May 6, 2015
@simonjohansson simonjohansson merged commit 7d7a7da into master May 6, 2015
@simonjohansson
Copy link
Contributor

@longnguyen11288 ok, coolio.

@shinji62 shinji62 deleted the oauth_token_refresh branch May 18, 2015 01: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.

None yet

3 participants