Skip to content

Commit

Permalink
Keychain Migration (#180)
Browse files Browse the repository at this point in the history
* First go at migrating keychain passwords if the app needs to.

* Updating Share Extension Entitlements

* Updating new Provisioning Profiles / Bundle Identifiers

* KeychainMigrator: Fixing Swift 4 error

* Share Extension: Adding legacy Keychain Access Group

* Fixing AppGroup Identifiers

* Adding previous-application-identifiers Entitlement

* KeychainMigrator: Switching to guard let

* Updating Release Target

* Updating Podfile

* Updating Testing Target

* KeychainPasswordItem: Adding source reference

* KeychainMigrator: Splitting into methods for testability purposes

* Implements KeychainMigrator Unit Tests

* Updates Project

* Adding AdHoc Target

* KeychainMigratorTests: Updating Unit Test

* KeychainMigrator: Wiring Constants

* KeychainMigrator: Specifying current accessGroup

* Updates CocoaPods Integration

* Updates CocoaPods Integration. Mark II

* Updates KeychainMigrator Tests

* SPAppDelegate: Updates migration call

* Fixing variable name in test.

* KeychainMigrator: Adding event tracking

* KeychainMigrator: Failsafe Mechanism
  • Loading branch information
roundhill authored Nov 27, 2017
1 parent 1bed600 commit 85a4511
Show file tree
Hide file tree
Showing 20 changed files with 1,114 additions and 283 deletions.
8 changes: 7 additions & 1 deletion Podfile
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
source 'https://github.com/CocoaPods/Specs.git'

platform :ios, '9.0'
platform :ios, '10.0'
inhibit_all_warnings!
use_frameworks!

Expand Down Expand Up @@ -30,6 +30,12 @@ abstract_target 'Automattic' do
pod 'Simperium', '0.8.19'
pod 'WordPress-AppbotX', :git => 'https://github.com/wordpress-mobile/appbotx.git', :commit => '479d05f7d6b963c9b44040e6ea9f190e8bd9a47a'
pod 'WordPress-Ratings-iOS', '0.0.2'

# Testing Target
#
target 'SimplenoteTests' do
inherit! :search_paths
end
end

# Extension Target
Expand Down
2 changes: 1 addition & 1 deletion Podfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,6 @@ SPEC CHECKSUMS:
WordPress-AppbotX: b5abc0ba45e3da5827f84e9f346c963180f1b545
WordPress-Ratings-iOS: b04108f7a45867844ba47a240bb6295ca747eebf

PODFILE CHECKSUM: 0fda9886a82c8a61adc629fbefbde69dafe16cf6
PODFILE CHECKSUM: f6767479ddf21dd5c487fc2774dad6048bd1262c

COCOAPODS: 1.3.1
833 changes: 575 additions & 258 deletions Simplenote.xcodeproj/project.pbxproj

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions Simplenote/Classes/SPOptionsViewController.m
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ - (void)doneAction:(id)sender

- (NSInteger)numberOfSectionsInTableView:(UITableView *)tableView
{
#if BETA_DISTRIBUTION
#if INTERNAL_DISTRIBUTION
return SPOptionsViewSectionsCount;
#else
return SPOptionsViewSectionsCount - 1;
Expand Down Expand Up @@ -222,7 +222,7 @@ - (NSString *)tableView:(UITableView *)tableView titleForHeaderInSection:(NSInte

- (NSString *)tableView:(UITableView *)tableView titleForFooterInSection:(NSInteger)section
{
#if BETA_DISTRIBUTION
#if INTERNAL_DISTRIBUTION
if (section == SPOptionsViewSectionsDebug) {
return [[NSString alloc] initWithFormat:@"Beta Distribution Channel\nv%@ (%@)", [NSString stringWithFormat:@"%@", [[NSBundle mainBundle] objectForInfoDictionaryKey: @"CFBundleShortVersionString"]], [[NSBundle mainBundle] objectForInfoDictionaryKey: (NSString *)kCFBundleVersionKey]];
}
Expand Down
6 changes: 6 additions & 0 deletions Simplenote/Classes/SPTracker.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,4 +90,10 @@
+ (void)trackUserSignedIn;
+ (void)trackUserSignedOut;

#pragma mark - Keychain Migration
+ (void)trackKeychainMigrationSucceeded;
+ (void)trackKeychainMigrationFailed;
+ (void)trackKeychainFailsafeSucceeded;
+ (void)trackKeychainFailsafeFailed;

@end
25 changes: 25 additions & 0 deletions Simplenote/Classes/SPTracker.m
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,31 @@ + (void)trackUserSignedOut
[self trackGoogleEventWithCategory:@"user" action:@"signed_out" label:nil value:nil];
}

#pragma mark - Keychain Migration

+ (void)trackKeychainMigrationSucceeded
{
[self trackAutomatticEventWithName:@"keychain_migration_succeeded" properties:nil];
[self trackGoogleEventWithCategory:@"keychain" action:@"migraiton" label:@"success" value:nil];
}

+ (void)trackKeychainMigrationFailed
{
[self trackAutomatticEventWithName:@"keychain_migration_failed" properties:nil];
[self trackGoogleEventWithCategory:@"keychain" action:@"migration" label:@"failure" value:nil];
}

+ (void)trackKeychainFailsafeSucceeded
{
[self trackAutomatticEventWithName:@"keychain_failsafe_succeeded" properties:nil];
[self trackGoogleEventWithCategory:@"keychain" action:@"failsafe" label:@"succeeded" value:nil];
}

+ (void)trackKeychainFailsafeFailed
{
[self trackAutomatticEventWithName:@"keychain_failsafe_failed" properties:nil];
[self trackGoogleEventWithCategory:@"keychain" action:@"failsafe" label:@"failure" value:nil];
}


#pragma mark - Google Analytics Helpers
Expand Down
205 changes: 205 additions & 0 deletions Simplenote/KeychainMigrator.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,205 @@
//
// MigrateKeychain.swift
// Simplenote
// Migrates keychain items from previous keychain group
//
import Foundation


// MARK: - KeychainMigrator
//
@objc
class KeychainMigrator: NSObject {

/// Keychain Constants
///
private struct Constants {
/// Legacy TeamID
///
static let oldPrefix = "4ESDVWK654."

/// New TeamID!
///
static let newPrefix = "PZYM8XX95Q."

/// Main App's Bundle ID
///
static let bundleId = "com.codality.NotationalFlow"

/// Share Extension's Bundle ID
///
static let shareBundleId = bundleId + ".Share"

/// Username's User Defaults Key
///
static let usernameKey = "SPUsername"

/// Legacy TeamID Access Group
///
static let legacyAccessGroup = oldPrefix + bundleId

/// New TeamID Access Group
///
static let newAccessGroup = newPrefix + bundleId
}

/// Migrates the Legacy Keychain Entry over to the new Access Group
///
@objc
func migrateIfNecessary() {
guard needsPasswordMigration() else {
return
}

migrateLegacyPassword()
}
}


// MARK: - Internal Helpers: This should actually be *private*, but for unit testing purposes, we're keeping them this way.
//
extension KeychainMigrator {

/// Indicates if the Migration should take place. This is true whenever:
///
/// - The Username is accessible
/// - There is no current password
///
func needsPasswordMigration() -> Bool {
guard let username = self.username else {
return false
}

let newKeychainEntry = try? loadKeychainEntry(accessGroup: .new, username: username)
return newKeychainEntry == nil
}

/// Migrates the Keychain Entry associated with the Old TeamID Prefix
///
func migrateLegacyPassword() {
guard let username = self.username,
let legacyPassword = try? loadKeychainEntry(accessGroup: .legacy, username: username)
else {
return
}

// Looks like we need to attempt a migration...
do {
try deleteKeychainEntry(accessGroup: .legacy, username: username)
try saveKeychainEntry(accessGroup: .new, username: username, password: legacyPassword)

SPTracker.trackKeychainMigrationSucceeded()
} catch {
// :(
NSLog("Keychain Migration Error: \(error)")

SPTracker.trackKeychainMigrationFailed()
self.restoreLegacyPassword(password: legacyPassword, for: username)
}
}

/// On error, we'll attempt to restore the legacy Password
///
func restoreLegacyPassword(password: String, for username: String) {
do {
try saveKeychainEntry(accessGroup: .legacy, username: username, password: password)
SPTracker.trackKeychainFailsafeSucceeded()
} catch {
NSLog("Keychain Failsafe Error: \(error)")

SPTracker.trackKeychainFailsafeFailed()
}
}
}


// MARK: - User Defaults Wrappers
//
extension KeychainMigrator {
/// Username
///
var username: String? {
set {
UserDefaults.standard.set(newValue, forKey: Constants.usernameKey)
UserDefaults.standard.synchronize()
}
get {
return UserDefaults.standard.string(forKey: Constants.usernameKey)
}
}
}


// MARK: - Keychain Wrappers: This should actually be *private*, but for unit testing purposes, we're keeping them this way.
//
extension KeychainMigrator {

enum AccessGroup {
case new
case legacy

var stringValue: String {
switch self {
case .new:
return Constants.newAccessGroup
case .legacy:
return Constants.legacyAccessGroup
}
}
}

func loadKeychainEntry(accessGroup: AccessGroup, username: String) throws -> String {
let passwordItem = KeychainPasswordItem(
service: SPCredentials.simperiumAppID(),
account: username,
accessGroup: accessGroup.stringValue
)

return try passwordItem.readPassword()
}

func deleteKeychainEntry(accessGroup: AccessGroup, username: String) throws {
let passwordItem = KeychainPasswordItem(
service: SPCredentials.simperiumAppID(),
account: username,
accessGroup: accessGroup.stringValue
)

try passwordItem.deleteItem()
}

func saveKeychainEntry(accessGroup: AccessGroup, username: String, password: String) throws {
let passwordItem = KeychainPasswordItem(
service: SPCredentials.simperiumAppID(),
account: username,
accessGroup: accessGroup.stringValue
)

try passwordItem.savePassword(password)
}

#if RELEASE
/// This method tests the Migration Flow. This should only be executed in the *Release* target, since the AppID's won't
/// match with the one(s) used in the other targets.
///
/// For testing purposes only.
///
@objc
func testMigration() {
let dummyUsername = "TestingUsername"
let dummyPassword = "TestingPassword"

// Recreate Pre-Migration Scenario
username = dummyUsername
try? deleteKeychainEntry(accessGroup: .new, username: dummyUsername)
try? saveKeychainEntry(accessGroup: .legacy, username: dummyUsername, password: dummyPassword)

// Migrate
migrateIfNecessary()

// Verify
let migrated = try? loadKeychainEntry(accessGroup: .new, username: dummyUsername)
assert(migrated == dummyPassword)
}
#endif
}
Loading

0 comments on commit 85a4511

Please sign in to comment.