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

Added TTL renewal of credentials in CredentialsManager [SDK-1818] #399

Merged
merged 6 commits into from
Jul 27, 2020

Conversation

Widcket
Copy link
Contributor

@Widcket Widcket commented Jul 21, 2020

Changes

  • Added a new minTTL parameter to the credentials(withScope:callback:) method to force renew the Access Token unless it remains valid for a certain amount of time, and to ensure the minimum lifetime of the renewed Access Token. This guarantees that the retrieved Access Token will always remain valid for at least minTTL.
credentialsManager.credentials(minTTL: 600) { error, newCredentials in // 600 seconds
    // newCredentials.accessToken will be valid for at least 10 minutes
}
  • Added a new minTTL parameter to the hasValid() method to check that the Access Token will remain valid for at least a certain amount of time.
if credentialsManager.hasValid(minTTL: 600) {
    // The Access Token will remain valid for at least 10 minutes
}
  • Used the existing scope parameter to force renew the Access Token when requesting a reduced scope than originally granted.
credentialsManager.credentials(withScope: "openid profile offline_access") { error, newCredentials in
    // newCredentials.accessToken will be limited to the new scope
}

References

Replaces #395

Testing

  • This change adds unit test coverage
  • This change has been tested on the latest version of the platform/language or why not

Checklist

@Widcket Widcket added this to the vNext milestone Jul 21, 2020
@Widcket Widcket requested a review from a team July 21, 2020 01:50
@cocojoe cocojoe changed the title Implemented forced renewal of credentials in CredentialsManager [SDK-1818] Added forced renewal of credentials in CredentialsManager [SDK-1818] Jul 21, 2020
Auth0/CredentialsManager.swift Outdated Show resolved Hide resolved
Auth0/CredentialsManager.swift Outdated Show resolved Hide resolved
Auth0/CredentialsManager.swift Show resolved Hide resolved
Auth0Tests/CredentialsManagerSpec.swift Outdated Show resolved Hide resolved
@Widcket Widcket requested a review from cocojoe July 21, 2020 15:04
Auth0/CredentialsManager.swift Outdated Show resolved Hide resolved
@Widcket Widcket requested a review from cocojoe July 21, 2020 17:40
@cocojoe cocojoe changed the title Added forced renewal of credentials in CredentialsManager [SDK-1818] Added TTL renewal of credentials in CredentialsManager [SDK-1818] Jul 21, 2020
@cocojoe
Copy link
Member

cocojoe commented Jul 21, 2020

@danielphillips @Tauseef-TL We had a think about your use case and wanted to have a consistent story across the native SDKs, starting with iOS. Would be great to get your feedback on the functionality added in this PR (snippet included) and ensure it also solves your use case.

@cocojoe cocojoe requested a review from lbalmaceda July 22, 2020 09:15
@cocojoe
Copy link
Member

cocojoe commented Jul 22, 2020

@lbalmaceda please can you review to ensure it aligns with your expectations around the discovery and as a heads up for Android. Thx

Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

There's a method I wouldn't add to the SDK. The rest looks good, I left some comments.

Auth0/CredentialsManager.swift Outdated Show resolved Hide resolved
Auth0/CredentialsManager.swift Show resolved Hide resolved
Auth0Tests/CredentialsManagerSpec.swift Outdated Show resolved Hide resolved
Auth0Tests/CredentialsManagerSpec.swift Outdated Show resolved Hide resolved
Auth0Tests/CredentialsManagerSpec.swift Outdated Show resolved Hide resolved
Auth0Tests/CredentialsManagerSpec.swift Show resolved Hide resolved
@tauseefm-tl
Copy link

@cocojoe From initial review, it appears that this should cover our usecases/requirements. I will need to spike this with our codebase in order to fully verify this.

@lbalmaceda
Copy link
Contributor

@Widcket I cannot comment on the diff because this method hasn't changed, but note that it also needs to accept the minTTL value since this is the only way the developer has to check for existing non-expired credentials that require a minTTL without having to take them from the keystore or refreshing them (what the method you changed does).

public func hasValid() -> Bool {

@Widcket
Copy link
Contributor Author

Widcket commented Jul 24, 2020

@Widcket I cannot comment on the diff because this method hasn't changed, but note that it also needs to accept the minTTL value since this is the only way the developer has to check for existing non-expired credentials that require a minTTL without having to take them from the keystore or refreshing them (what the method you changed does).

public func hasValid() -> Bool {

Good catch, I added the minTTL param to the hasValid method in 548ee44. Please check that logic and its tests.

@Widcket Widcket requested a review from lbalmaceda July 24, 2020 18:45
@Widcket Widcket merged commit 2874a39 into master Jul 27, 2020
@Widcket Widcket deleted the feature/forced-renewal branch July 27, 2020 12:14
@Widcket Widcket mentioned this pull request Jul 27, 2020
@Widcket
Copy link
Contributor Author

Widcket commented Jul 27, 2020

This is now out on v1.27.0.

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

Successfully merging this pull request may close these issues.

None yet

4 participants