Refresh expiring tokens before making requests

[seif-at-sap]

Does this PR modify CLI v6 or v7?

This change affects both CLI v6 and v7.
In addition, the change is back ported to the unrefactored Legacy Code in the cf package.

What Need Does It Address?

See #1582.

Who Is The Functionality For?

All users of cli.

How Often Will This Functionality Be Used?

Every time a v6, v7, or legacy command is issued.

Possible Drawbacks

None.

Why Should This Be In Core?

Bugfix of core functionality.

Description of the Change

JWT Access Tokens will be automatically refreshed when they are about to expire or are already expired. This avoids unauthenticated requests that are known to fail. If the token refresh fails, the request will not be retried.

Alternate Designs

This PR implements the proposed design from #1582 and https://www.pivotaltracker.com/n/projects/892938/stories/164213372.

Applicable Issues

#1582
#1143

How Urgent Is The Change?

Urgent.

Other Relevant Parties

None

[cfdreddbot]

❌ Hey seif-at-sap!

All pull request submitters and commit authors must have a Contributor License Agreement (CLA). Click here for details on the CLA process.

The following github user @seif-at-sap is not covered by a CLA.

After the CLA process is complete, this pull request will need to be closed & reopened. DreddBot will then validate the CLA(s).

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/164773969

The labels on this github issue will be updated when the story is started.

[seif-at-sap]

Close due to missing CLA.

[cfdreddbot]

✅ Hey seif-at-sap! The commit authors and yourself have already signed the CLA.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/164774081

The labels on this github issue will be updated when the story is started.

[bwasmith]

Hey @seif-at-sap ,

Thanks for this! It all looks really good, we are excited to have this PR.

@a-shan and I were reviewing this today, and ran our integration tests on your changes. We want to update you on a few things.

We had some failing tests, mainly integration tests for our verbose logging flags. We will fix this to correctly assert on the new request ordering.

A question we had for you was, did you consider using the Golang JWT package to parse the OAuth access token dynamically? The access token should be able to give its expiration date. It seems like this would be preferable to maintaining an extra field AccessTokenExpiryDate in the config.json.

We saw this SO post which refers to this OAuth parsing library.

Please let us know what you think of this, and if you would be opposed to making this update.

Thanks again, looking forward to hearing from you soon.

Brendan and Alex

[seif-at-sap]

Hi @bwasmith and @a-shan,
thanks for your positive and swift feedback!

A question we had for you was, did you consider using the Golang JWT package to parse the OAuth access token dynamically? The access token should be able to give its expiration date. It seems like this would be preferable to maintaining an extra field AccessTokenExpiryDate in the config.json.

I was not aware the UAA would always include an exp claim. Dynamically parsing the JWT and using this particular claim seems indeed preferable. It avoids changes to the configuration layer altogether. I have updated the PR accordingly.
However, there is one downside to this approach: it is much more vulnerable to clock skew between the UAA and the client, since the exp claim is set to a total timestamp according to the clock of the UAA. To counter this, I increased the refresh threshold to one minute. I do not expect bigger skews to be a common occurrence.

We saw this SO post which refers to this OAuth parsing library.

The project already depends on github.com/SermoDigital/jose. I opted for this library to avoid adding another dependency.

[tjvman]

Hi @seif-at-sap, it looks like there's an issue with the changes that makes cf marketplace error with not a compact JWS if not logged in.

I believe that the root cause is that this method doesn't validate that the returned access token is a non-empty string (which it potentially can be, since the IsLoggedIn method in actor/sharedaction returns true if either the access or refresh token is non-empty).

We'll gladly take another look to validate the PR once you've fixed this issue.

[seif-at-sap]

Hi @tjvman, thank you for the review!
I have updated the such that an unauthenticated marketplace command works (neither access token nor refresh token is set).
If only the refresh token is set (or the access token is invalid), the token will be automatically refreshed.

[bwasmith]

Merged, thank you @seif-at-sap !

We updated a test because our verbose_flag_test was recently split out into separate files 211081c

[ryenus]

Hi, @seif-at-sap and @bwasmith I think one possible draw back is whether this change is thread-safe.

See the linked issue #2232.