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

CredentialsManager function to clear and revoke the refresh token #312

Merged
merged 14 commits into from
Oct 15, 2019

Conversation

stevehobbsdev
Copy link

@stevehobbsdev stevehobbsdev commented Oct 2, 2019

Changes

This PR adds a new method revoke to CredentialsManager, which boths clears credentials and revokes the refresh token, if available. It accepts a callback for when the revocation call has finished.

When called, it first checks to make sure that it has:

  • Valid credentials stored
  • A refresh token

If there is no refresh token, then the credentials are cleared as normal without the revoke endpoint being called.

Otherwise, it uses its internal instance of Authentication to call the revoke endpoint, passing the refresh token. If this call is successful, the credentials are cleared. If not, the credentials remain and the callback is invoked with an error.

Use Case

The revoke function could be used in most cases where the user's session is to be cleared, as it's designed to clear credentials even if there is no refresh token. Using clear would be beneficial if the session is to be cleared but the developer explicitly does not want to revoke the refresh token.

Usage

The PR adds one method to the CredentialsManager API:

credentialsManager.revoke { error in
    guard error == nil else {
        print("Failed to revoke the refresh token: \(error)")
    }

    print("Token revoked and credentials cleared!")
}

Testing

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

Checklist

@stevehobbsdev stevehobbsdev marked this pull request as ready for review October 2, 2019 13:41
@stevehobbsdev stevehobbsdev requested a review from a team October 2, 2019 13:41
@stevehobbsdev
Copy link
Author

The unit tests need a little re-working to try and avoid the use of the now-deprecated clear() method.

@stevehobbsdev stevehobbsdev removed the request for review from a team October 3, 2019 15:15
@cocojoe
Copy link
Member

cocojoe commented Oct 4, 2019

@stevehobbsdev fix your tests, appear to be breaking for everything other than tvOS in Swift 5 😄 So they would also be failing locally then? Let me know if you want to pair on this.

Copy link
Member

@cocojoe cocojoe left a comment

Choose a reason for hiding this comment

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

Few areas to discuss

App/Assets.xcassets/AppIcon.appiconset/Contents.json Outdated Show resolved Hide resolved
App/Base.lproj/Main.storyboard Outdated Show resolved Hide resolved
Auth0/CredentialsManager.swift Outdated Show resolved Hide resolved
Auth0/CredentialsManager.swift Outdated Show resolved Hide resolved
Auth0/CredentialsManager.swift Outdated Show resolved Hide resolved
Steve Hobbs added 2 commits October 4, 2019 11:21
They needed a sprinkling of `waitUntil` blocks so that the tests didn't
trip over themselves with clearing credentials asynchronously while
other tests were running.
Steve Hobbs added 4 commits October 7, 2019 11:43
May fix timeout issue when running in CI environment
* Tests have been reverted to a previous state where they used `clear`
to remove credentials between tests, now that `clear` is no longer
depcreated
* Appropriate use of `waitUntil` has been restored into the tests so
that they run properly
damieng
damieng previously approved these changes Oct 7, 2019
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@cocojoe cocojoe added this to the vNext milestone Oct 15, 2019
@stevehobbsdev stevehobbsdev merged commit 97d19cf into master Oct 15, 2019
@stevehobbsdev stevehobbsdev deleted the creds-manager-revoke-token branch October 15, 2019 12:38
@stevehobbsdev stevehobbsdev mentioned this pull request Oct 15, 2019
sam-w pushed a commit to LoungeBuddy/Auth0.swift that referenced this pull request Mar 11, 2020
…th0#312)

* Added CredentialsManager.clearAndRevokeToken(), superceding .clear()

* Added deprecation notice above the old clear() method

* Added an example of `clearAndRevokeToken` to the readme

* Fixed unit tests

They needed a sprinkling of `waitUntil` blocks so that the tests didn't
trip over themselves with clearing credentials asynchronously while
other tests were running.

* Reverted changes to icons and storyboard

* Removed the default case in favour of .success

* Bumped timeout values inline with other waitUntil calls

May fix timeout issue when running in CI environment

* Bumped timeout for clearAndRevoke for multi manager tests

* Reversed 'clear' deprecation, reverted tests

* Tests have been reverted to a previous state where they used `clear`
to remove credentials between tests, now that `clear` is no longer
depcreated
* Appropriate use of `waitUntil` has been restored into the tests so
that they run properly

* Renamed clearAndRevokeToken to revoke, updated README

* Applied readme changes based on review feedback

* Added more detail to doc comments for revoke method

* Reworded test spec for clarity
sam-w added a commit to LoungeBuddy/Auth0.swift that referenced this pull request Mar 11, 2020
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