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

TokenManager, AccessToken migration #299

Merged
merged 4 commits into from
May 28, 2024

Conversation

mohssenfathi
Copy link
Contributor

Description

New TokenManager and AccessToken classes were added in a previous PR. This PR migrates any classes still using TokenManager_DEPRECATED and AccessToken_DEPRECATED to the new ones. It also removes the deprecated classes.

Changes

AccessGroup Support

To keep feature parity, support was added for setting a custom access group when performing any keychain operations.

Updated call sites

UberCore and UberRides classes (RidesRequestView, LoginManager, etc.) were updated to reference a new instance of TokenManager instead of the static TokenManager_DEPRECATED. Logic and functionality should remain the same.
Note: LoginManager will be removed eventually.

Additional AccessToken initializers

An AccessToken extension with custom inits was added to support UberRides classes.

Testing

  • Unit tests were added for new KeychainUtility logic
  • UberRides tests will be updated and added in a later PR, there are dependencies that need to be migrated first.
  • Manually tested using sample app
token_migration.mov

@mohssenfathi mohssenfathi marked this pull request as ready for review May 22, 2024 16:04
@@ -145,6 +149,12 @@ public final class KeychainUtility: KeychainUtilityProtocol {
) == noErr
}

/// Updates the accessGroup used for subsequent operations
/// - Parameter accessGroup: The new accessGroup
public func setAccessGroup(_ accessGroup: String) {

Choose a reason for hiding this comment

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

can we update the API to receive more than just the key instead? maybe a struct with the key and access group

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was mostly keeping parity with the legacy API since it made migration easier, but I can update to accept this as a parameter with a default nil value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Please take a look @renan-ssoares

@mohssenfathi mohssenfathi merged commit 4d4e46d into uber:2.x May 28, 2024
4 checks passed
@mohssenfathi mohssenfathi deleted the token-manager-migration branch May 28, 2024 17:38
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.

2 participants