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

(Version 4.3.0)TouchID.isSupported() returns "TouchID" even when there is no fingerprints available #171

Closed
ronilitman opened this issue Oct 28, 2018 · 30 comments

Comments

@ronilitman
Copy link
Contributor

Checked on IOS.
Till now it worked perfectly on the version I was using(4.1.0), but now on simulator and on a real device that has no fingerprints TouchID.isSupported() returns "TouchID", even though it should return false/undefined.

@zibs
Copy link
Collaborator

zibs commented Oct 28, 2018

Maybe take a look at this line - 51ede1d#diff-7b319858b5c7e6cc6467bc158c09176aR14 - could that have affected it?

@philolo1
Copy link

@zibs @ronilitman LAPolicyDeviceOwnerAuthentication will be also true if there is just a passcode support, i think we should delete this check. But how will passcodes be handled in generally. Maybe the api needs to be changed?

@zibs
Copy link
Collaborator

zibs commented Oct 30, 2018

Right, it's probably returning "TouchID" because the user has a passcode setup, and if they do, then you can use the authenticate method by passing the passwordFallback param as true

@philolo1
Copy link

@zibs i think in isSupported it should not return "TouchID" but rather "Passcode". What do you think? it should be an easy pr to change that line of code.

@zibs
Copy link
Collaborator

zibs commented Nov 4, 2018

Feel free to make a PR!

@davidmarinangeli
Copy link

Has anyone done that? If not, I could PR that. 😄

@philolo1
Copy link

philolo1 commented Nov 8, 2018

@davidmarinangeli i think it depends on what the behaviour should be? Should it be "Passcode" or should we rather just return false. Because really TouchID is not supported.

@davidmarinangeli
Copy link

I think both are okay: I just wanted to find a way to make it work and split the cases (the real TouchID supported and the fake one).

@ben-snaize
Copy link

ben-snaize commented Nov 9, 2018

Could do with this fix - it's also making devices that don't have Touch ID enabled return with a result of 'TouchID' when calling isSupported. This might be sim-only though.

@philolo1
Copy link

philolo1 commented Nov 9, 2018

@ben-snaize @davidmarinangeli i just made a pullrequest to return "Passcode", if passcode is available. #176.

@tlow92
Copy link

tlow92 commented Nov 9, 2018

I was using passcodes for a year with this library, without any problems. isSupported should still return LAErrorTouchIDNotEnrolled when no fingers are configured for Touch ID no matter if passcode is set or not. The recent changes mentioned above actually broke the code

@philolo1
Copy link

philolo1 commented Nov 9, 2018

ok @tlow92 i think it makes more sense to do it this way, i will update my pull request.

@davidmarinangeli
Copy link

davidmarinangeli commented Nov 9, 2018

I've found a bug for iPads too: the "isSupported()" always returns false. Is that for the same reason? 🤔

Edit: someone had the same problem on #146 but then he closed it don't know why

@philolo1
Copy link

philolo1 commented Nov 9, 2018

i dont believe so. i changed my pull request just to ignore the passcode for now.
#177

@tlow92
Copy link

tlow92 commented Nov 9, 2018

@philolo1 Thanks for the effort
In case someone is interested latest stable version is 4.1.0
So for production use reverting to that is probably safest bet

@zibs
Copy link
Collaborator

zibs commented Nov 9, 2018

@tlow92 @philolo1 Actually I think the direction we want to move in is to better support passcodes, and to do so, we should integrate your PR which returns "Passcode", if TouchId/FaceId are not supported, but passcode is.

We implemented #101 to support passcodes better. So it should actually return "Passcode" back, so that they dev can they use the passcodeFallback param to attempt to auth with PIN, instead of erroring out.

@davidmarinangeli
Copy link

Okay @zibs but can you confirm that for now the 4.1.0 is the best for production use?

@zibs
Copy link
Collaborator

zibs commented Nov 11, 2018

@ronilitman @tlow92 after thinking about it, I think you're correct. We will make it so that if TouchID/FaceID are not supported, isSupported will return an error.

However, since this library provides support for attempting to use passcodeFallback, the user can still attempt to call authenticate even if isSupported returns false.

Alternatively, we can add an option to isSupported that could be like checkForPasscodeToo (better name needed) and then if it's present, check to see if passcode is supported as well. That might be ideal for those wanting to always use the passcode fallback.

@tonmanayo
Copy link

I rolled back to 4.1.1, seem to get the correct behaviour there

@dgreenehy
Copy link

It's great to know if the device supports a particular biotype, but it also needs to have a way to let us know if the user has enrolled. We may need to roll back as well as 4.3 is a breaking change for us.

@zibs
Copy link
Collaborator

zibs commented Dec 10, 2018

Yeah I think we'll have to make some changes here. It sounds like we should keep existing functionality of isSupported to return what it always returned, and then add a separate method to check if passcode is enabled.

@dgreenehy
Copy link

The fix for this could potentially be exposing 2 methods, isSupported() and isEnrolled(). IsSupported() could return finger/face/passcode and isEnrolled() could simply return a boolean.

@zibs
Copy link
Collaborator

zibs commented Dec 12, 2018

I think that's the best solution @dgreenehy - can you make a PR? I probably can't get to anything until the holidays myself - sorry all!

@dgreenehy
Copy link

@zibs Thanks for the reply, I am going to fork your repo and make the changes we need, once everything is working as intended I will get a PR to you. Happy Holidays!

@zibs
Copy link
Collaborator

zibs commented Feb 15, 2019

4.4.0 should fix this regression by allowing a new config var passed into isSupported to return an error if faceid/touch id are not supported. please refer to the readme and the changelog!

@zibs zibs closed this as completed Feb 15, 2019
@abury
Copy link

abury commented Mar 4, 2019

Is this still the case? Testing on a simulator that isn't enrolled in touch ID, I'm calling isSupported and it's returning "TouchID" 🤔

  componentDidMount() {
    const optionalConfigObject = {
      unifiedErrors: false,
      passcodeFallback: true
    };
    TouchID.isSupported(optionalConfigObject)
      .then(biometryType => {
        console.log({ biometryType });
        // Success code
        if (biometryType === 'FaceID') {
          this.setState({ authenticationType: AUTHENTICATION_TYPES.FACE_ID });
        } else {
          this.setState({ authenticationType: AUTHENTICATION_TYPES.TOUCH_ID });
        }
      })
      .catch(error => {
        console.log({ error });
        this.setState({ authenticationType: AUTHENTICATION_TYPES.PASSCODE });
      });
  }

console.log:

Object {biometryType: "TouchID"}

Version: react-native-touch-id@^4.4.1:
Confirmed simulator is not enrolled in touch ID

@davidmarinangeli
Copy link

Have you tried to downgrade the version to the ones we recommended?

@abury
Copy link

abury commented Mar 4, 2019

@davidmarinangeli Thanks for the reply. I've tried it on:

  • 4.4.1
  • 4.1.1
  • 4.1.0

None of these versions seem to be working for me, they all report TouchID regardless of enrollment.

@abury
Copy link

abury commented Mar 4, 2019

I've found the issue. Looking at TouchID.m, the default value for authenticationType is true:

NSNumber *passcodeFallback = [NSNumber numberWithBool:true];
    if (RCTNilIfNull([options objectForKey:@"passcodeFallback"]) != nil) {
        passcodeFallback = [RCTConvert NSNumber:options[@"passcodeFallback"]];
    }

Also, it will only return an error if passcodeFallback is false:

    } else if ([passcodeFallback boolValue] && [context canEvaluatePolicy:LAPolicyDeviceOwnerAuthentication error:&error]) {
        // WE DONT WANT TO BE IN HERE
        // Attempt Authentification
        [context evaluatePolicy:LAPolicyDeviceOwnerAuthentication
                localizedReason:reason
                          reply:^(BOOL success, NSError *error)
         {
             [self handleAttemptToUseDeviceIDWithSuccess:success error:error callback:callback];
         }];
    }
    else {
        // THIS IS WHERE WE WANT TO BE
        if (error) {
            NSString *errorReason = [self getErrorReason:error];
            NSLog(@"Authentication failed: %@", errorReason);
            
            callback(@[RCTMakeError(errorReason, nil, nil), [self getBiometryType:context]]);
            return;
        }
        
        callback(@[RCTMakeError(@"RCTTouchIDNotSupported", nil, nil)]);
        return;
    }

The Readme says that false is the default value, and passing true will return an error, both of which are incorrect.

const optionalConfigObject = {
  unifiedErrors: false // use unified error messages (default false)
  passcodeFallback: false // if true is passed, itwill allow isSupported to return an error if the device is not enrolled in touch id/face id etc. Otherwise, it will just tell you what method is supported, even if the user is not enrolled.  (default false)
}

Once I passed false, it worked as expected

@zibs I'm happy to issue a PR to fix either the readme or the obj-c logic. 👍

@saadmutahar
Copy link

Thanks @abury for saving my day
and yes this is true that isSupported return an error if passcodeFallback is false

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

No branches or pull requests

10 participants