TouchID support (FREEBIE) #1606

noahemmet · 2017-01-17T00:04:55Z

First time contributor checklist

I have read the README and CONTRIBUTING documents
I have signed the Contributor Licence Agreement

Contributor checklist

I'm following the code, UI and style conventions
My commits are rebased on the latest master branch
My commits are in nice logical chunks
My contribution is fully baked and is ready to be merged as is
I have tested my contribution on these devices:

iPhone SE, iOS 10.2.0 (sorry, this is the only device I have handy! It'd be nice to test it on an iOS 8 device, and a device without TouchID support.)

I have made the choice whether I want the BitHub reward or not by omitting or adding the word FREEBIE in my commit message

Description

This PR adds Touch ID support to the app. It builds off @fbartho's work from #663, with updated style conventions and rebased off master. Here's a video of it in action.

Summary:

Adds a TouchID security toggle to the Privacy settings (off by default).
Enabling it will require a TouchID authentication on first launch, and after 60 seconds in the background. (Note that there isn't UI informing the user of the 60 second cut-off; we may wish to add a label somewhere, or change the timeout to something shorter, like 1 second.)
Requires TouchID authentication when enabling/disabling the feature.
Enabling TouchID will automatically enable Screen Security.
Disables the TouchID switch if there is no hardware availability (untested).
Adds a setter and getter in PropertyListPreferences (I'm assuming this class is appropriately secure for this setting, as it's also responsible for the Screen Security feature, but if it's not, we'll want to use the keychain for storage).
Adds two localized strings (English only) for the prompt and the settings toggle.
Canceling TouchID will immediately re-present the prompt, as we don't currently have a password option to fallback on.

Tested:

User launches app for first time; is not prompted for TouchID.
User enables/disables TouchID in settings; is prompted for TouchID both times.
User cancels TouchID after enabling/disabling; TouchID setting is unchanged.
User launches app with TouchID enabled; is prompted for TouchID.
User authenticates with TouchID, backgrounds app, then foregrounds app within 60 seconds of backgrounding; is not prompted for TouchID.
User authenticates with TouchID, backgrounds app, then foregrounds app after 60 seconds of backgrounding; is prompted for TouchID.

charlesmchen · 2017-01-17T18:45:56Z

Thank you @noahemmet - this PR looks promising. The team is occupied with a forthcoming release. We'll circle back to this PR when that work is complete.

CC @michaelkirk

michaelkirk

Hey @noahemmet - thanks a lot for this well thought out PR.

I had some feedback, and unfortunately in the meanwhile we've made some conflicting changes - I think mostly in the PrivacySettingsTableViewController.m

If you're interested in getting this up to date, I'm interested in getting it merged =)

michaelkirk · 2017-03-14T00:46:21Z

Signal/src/AppDelegate.m


-    [self removeScreenProtection];
+    if (Environment.preferences.touchIDIsEnabled && !TouchIDManager.shared.isTouchIDUnlocked) {


We intentionally don't access any storage synchronously from didLaunch / didBecomeActive methods. Reason being that a long running-migration could cause this call to block - hanging the UI. Hence the TSAccountManager runAsync machinery below.

Can you do something similar?

michaelkirk · 2017-03-14T00:48:59Z

Signal/src/AppDelegate.m

+		if (!TouchIDManager.shared.userDidCancel) {
+			[self presentTouchID];
+		} else {
+			[self protectScreen];


So the user would just see the signal logo on a blue screen and be unable to use the app here?

Based on your comment below...

// User canceled; we don't want to unlock the phone, but we want to avoid an endless cycle of // TouchID prompts, which hijacks the homebutton and prevents the user from switching apps, // so just show the security screen until the user backgrounds and foregrounds the app.

...this limitation makes some sense, but expecting the user to figure out to relaunch the app seems a bit much.

Maybe tapping/swiping on the screen re-prompts? What do you think?

michaelkirk · 2017-03-14T00:53:14Z

Signal/src/util/TouchIDManager.h

+//  Copyright (c) 2015 Open Whisper Systems. All rights reserved.
+//
+
+#import <Foundation/Foundation.h>


Can you add nullability annotations?

e.g. NS_ASSUME_NONNULL_BEGIN here and in your .m file?

michaelkirk · 2017-03-14T00:54:27Z

Signal/src/util/TouchIDManager.m

+        completion(TouchIDAuthResultUnavailable);
+    } else {
+        [myContext evaluatePolicy:LAPolicyDeviceOwnerAuthenticationWithBiometrics
+                  localizedReason:NSLocalizedString(@"TOUCHID_SECURITY_PROMPT",@"")


please add a useful comment on all localized strings for our translation team describing what this string is for.

michaelkirk · 2017-03-14T00:58:40Z

Signal/src/util/TouchIDManager.m

+- (BOOL)isTouchIDAvailable {
+    LAContext *myContext = [[LAContext alloc] init];
+    NSError *authError = nil;
+    return [myContext canEvaluatePolicy:LAPolicyDeviceOwnerAuthenticationWithBiometrics error:&authError] && !authError;


please log any error

michaelkirk · 2017-03-14T00:59:50Z

Signal/src/util/TouchIDManager.h

+
+#import <Foundation/Foundation.h>
+
+typedef NS_ENUM(NSInteger, TouchIDAuthResult) {


Any reason not to use the existing error codes? https://developer.apple.com/reference/localauthentication/laerror.code

michaelkirk · 2017-03-14T01:00:14Z

Signal/src/util/TouchIDManager.m

+                                dispatch_async(dispatch_get_main_queue(), ^{
+                                    if (error || !success){
+                                        DDLogError(@"Error Accessing TouchID: %@",error);
+                                        if (error.code == -2) {


Can we compare to the constant rather than a magic number here?

https://developer.apple.com/reference/localauthentication/laerror.code

michaelkirk · 2017-03-14T01:02:28Z

Signal/src/util/TouchIDManager.m

+#import "TouchIDManager.h"
+
+/// The number of seconds the app must be backgrounded before we lock the screen.
+NSTimeInterval const TouchIDLockTimeoutDefault = 60;


nit: please prefix constants with their corresponding class name

e.g. TouchIDManagerLockTimeoutDefault

michaelkirk · 2017-03-14T01:05:57Z

Signal/src/view controllers/PrivacySettingsTableViewController.m

+        self.enableTouchIDSecuritySwitch.enabled = NO;
+    }
+
+#if DEBUG


I'd prefer not to - the more we diverge from RELEASE the more likely we are to break shit later and not notice =(

michaelkirk · 2017-03-14T01:07:28Z

Signal/src/view controllers/PrivacySettingsTableViewController.m

+}
+
+/// Ensures that security switches reflect the user's preferences.
+- (void)validateSecuritySwitches


I don't see anything inherently wrong with this, but IMO it muddies the waters a bit about responsibilities.

Can you elaborate on why this method exists?

hodgesmr · 2017-04-03T03:28:29Z

I came across this issue while Googling for PIN functionality. According to the feedback in #738, it seems that locking the app from an unlocked iPhone is undesirable. If that has changed, would it make sense to have PINs and TouchID, not just the latter?

bvadnai · 2017-11-26T09:02:04Z

Please make this happen.

philipcv · 2017-11-27T11:23:06Z

I've been waiting for this for a long time.
Guys, please implement this as soon as possible.

michaelkirk · 2017-11-27T17:31:05Z

Please do not bump issues or pull requests.

This has been sitting stagnant for a long time, so I'm closing it.

TouchID support (FREEBIE)

ab4b46b

michaelkirk suggested changes Mar 14, 2017

View reviewed changes

sigenc mentioned this pull request May 8, 2017

Additional password #2079

Closed

michaelkirk closed this Nov 27, 2017

signalapp locked and limited conversation to collaborators Nov 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TouchID support (FREEBIE) #1606

TouchID support (FREEBIE) #1606

noahemmet commented Jan 17, 2017 •

edited

charlesmchen commented Jan 17, 2017

michaelkirk left a comment

michaelkirk Mar 14, 2017

michaelkirk Mar 14, 2017 •

edited

michaelkirk Mar 14, 2017

michaelkirk Mar 14, 2017

michaelkirk Mar 14, 2017

michaelkirk Mar 14, 2017

michaelkirk Mar 14, 2017

michaelkirk Mar 14, 2017

michaelkirk Mar 14, 2017

michaelkirk Mar 14, 2017

hodgesmr commented Apr 3, 2017

bvadnai commented Nov 26, 2017

philipcv commented Nov 27, 2017

michaelkirk commented Nov 27, 2017


		[self removeScreenProtection];
		if (Environment.preferences.touchIDIsEnabled && !TouchIDManager.shared.isTouchIDUnlocked) {


		#import <Foundation/Foundation.h>

		typedef NS_ENUM(NSInteger, TouchIDAuthResult) {

TouchID support (FREEBIE) #1606

TouchID support (FREEBIE) #1606

Conversation

noahemmet commented Jan 17, 2017 • edited

First time contributor checklist

Contributor checklist

Description

charlesmchen commented Jan 17, 2017

michaelkirk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk Mar 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hodgesmr commented Apr 3, 2017

bvadnai commented Nov 26, 2017

philipcv commented Nov 27, 2017

michaelkirk commented Nov 27, 2017

noahemmet commented Jan 17, 2017 •

edited

michaelkirk Mar 14, 2017 •

edited