Use window instead of pushing the lock onto the view controller by ayanonagon · Pull Request #65 · venmo/VENTouchLock

ayanonagon · 2015-09-30T20:53:42Z

No description provided.

hierarchy This is cleaner and also lets us show the lock when app switching too so the user can hide sensitive information behind the lock when in the app switch viewer.

eliperkins · 2015-09-30T21:44:15Z

Hm... Looks like Travis isn't too happy about Xcode 6.3... Any idea why we ended up with a box with 6.3 instead of 6.4?

eliperkins · 2015-09-30T21:47:48Z

Code looks good though!

@zinthemoney you wanna take a look at this change too, for a second set of 👀? I know you mentioned at DubDub this year that you're using this in Chime, so I just wanted to make sure these changes don't break too much for y'all!

zinthemoney · 2015-10-01T00:31:23Z

@ayanonagon, can you give a little background / problems on start using the window?

@eliperkins, by replacing VENTouchLock 1.11.0 with branch an/fix on my project, it doesn't work as before. I will take a deeper look later.

ayanonagon · 2015-10-01T00:35:17Z

@zinthemoney Sure! Two main reasons:

It’s not cool that the current VENTouchLock mucks around with your app’s view controller hierarchy. By having a separate window, it’s a lot cleaner to maintain, and doesn’t need to care about stuff going on in the app’s main window.
We can more reliably take a snapshot of the app when it goes into background mode, so we can cover sensitive data when the app is show in the app switcher.

Let me know if you have any more questions. Thanks for trying it out :D

eliperkins · 2015-10-01T13:05:55Z

VENTouchLock/VENTouchLock.m

            else {
-                displayController = splashViewController;
+                self.displayController = splashViewController;
            }


What do you think about adding:

self.lockWindow.windowLevel = UIWindowLevelAlert; // or UIWindowLevelAlert + 1

?

I think this will give us the benefit of overlaying other windows which reside at lower, more normal levels.

Good idea! Done.

Up the UIWindowLevel doesn't help in the situation i mentioned, by the same token that UIApplication is a shared instance. Basically, we want to make sure that we call [self.lockWindow makeKeyAndVisible]; last with respect to whatever the host app might be doing. Hope this makes sense :)

By introducing a delay helps, but not ideal

dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(.5 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ [self.lockWindow makeKeyAndVisible]; });

The documentation for -makeKeyAndVisible says that the method makes sure the window is displayed above all other windows with the same window level or lower.

This is a convenience method to make the receiver the main window and displays it in front of other windows at the same window level or lower.

To address your concerns, @zinthemoney, I propose doing a few things to guard against becoming obscured and to alert developers to the possibility that their app may be doing something that collides with how VENTouchLock operates.

On the invocation of -lock, check if -[UIApplication windows] has more than 1 window and -[UIApplication keyWindow] is not equal to -[id<UIApplicationDelegate> window]. If this is the case, we should set lockWindow's windowLevel to be equal to the windowLevel of the currently key window.

Make our lock window a UIWindow subclass that overrides -setRootViewController: and raises an assertion if the argument is any type of view controller that we don't expect. This won't crash in release builds but will crash in debug builds.

Observe the UIWindowDidBecomeKeyNotification and UIWindowDidBecomeVisibleNotification notifications. If we're currently locked and we receive these messages, raise an assertion that informs the developer that they've attempted to display a window on top of VENTouchLock and that this is undefined behavior.

@hyperspacemark I agree with your points, my only concern on having this obscured behavior that (potentially) breaks existing VENTouchLock integrations is adding integration complexity. We should solve the issue within the framework, and not pass on the responsibility to the developers. @ayanonagon, @eliperkins what do you think?

eliperkins · 2015-10-01T13:08:20Z

@zinthemoney ah, certainly true. The idea spurned out of some information from this blog post: http://www.thecave.com/2015/09/28/how-to-present-an-alert-view-using-uialertcontroller-when-you-dont-have-a-view-controller/, specifically:

At WWDC I stopped in at one of the labs and asked an Apple Engineer this same question: “What was the best practice for displaying a UIAlertController?” And he said they had been getting this question a lot and we joked that they should have had a session on it. He said that internally Apple is creating a UIWindow with a transparent UIViewController and then presenting the UIAlertController on it.

I think setting the window level might help us mitigate some of those concerns: https://github.com/venmo/VENTouchLock/pull/65/files#r40910236 by at least placing our window over the rest. I'll have to check to see if this accomplishes what you're discussing, however, that touching our application's window will definitely have some interesting side-effects.

else including alerts

might not necessarily hide the window if the other window’s level is lower

hyperspacemark · 2015-10-02T16:13:19Z

VENTouchLock/VENTouchLock.m

+        [notificationCenter addObserver:self selector:@selector(applicationDidBecomeActive:) name:UIApplicationDidBecomeActiveNotification object:nil];
+        [notificationCenter addObserver:self selector:@selector(applicationWillResignActive:) name:UIApplicationWillResignActiveNotification object:nil];
+
+        self.mainWindow = [[UIApplication sharedApplication].windows firstObject];


Can we be sure this is the application's main window? It might be better to directly ask UIApplication's delegate for its window?

…th Touch ID

dasmer · 2016-04-05T00:19:52Z

@ayanonagon what's the status of this PR?

ghost · 2017-10-10T06:56:34Z

What's the status on this PR?

ericlewis · 2017-10-10T07:14:13Z

Not to speak for everyone, but as an active maintainer we haven’t been focused on implementing this. If you’d like to chat with me or test or suggest things we’d love to hear it. But as it stands this implementation doesn’t function for everyone.

ayanonagon added 2 commits September 30, 2015 13:51

Use window instead of pushing the lock onto the view controller

4ca404b

hierarchy This is cleaner and also lets us show the lock when app switching too so the user can hide sensitive information behind the lock when in the app switch viewer.

Remove backgroundLockVisible statefulness

2101e4f

eliperkins self-assigned this Sep 30, 2015

ayanonagon added 2 commits September 30, 2015 15:27

Update KIF to 3.3 and fix tests

9ffea95

[.travis.yml] Use Xcode 7

20ce671

ayanonagon force-pushed the an/fix branch from e743418 to bcb42da Compare October 1, 2015 00:26

ayanonagon force-pushed the an/fix branch from bcb42da to 3869b21 Compare October 1, 2015 00:48

eliperkins reviewed Oct 1, 2015
View reviewed changes

ayanonagon force-pushed the an/fix branch from 3869b21 to 20ce671 Compare October 1, 2015 17:00

ayanonagon added 2 commits October 1, 2015 10:23

Use windowLevel UIWindowLevelAlert + 1 to show the lock above everything

f6befe3

else including alerts

Unlock when passcode is deleted

787ed31

ayanonagon force-pushed the an/fix branch from 1bf51c9 to 787ed31 Compare October 1, 2015 19:24

ayanonagon added 2 commits October 1, 2015 12:35

Add isPasscodePresented

6ef390d

Hide lock window explicitly, because makeKeyAndVisible on another window

9bcc7e0

might not necessarily hide the window if the other window’s level is lower

hyperspacemark reviewed Oct 2, 2015
View reviewed changes

Fix bug where app switching on iOS 9 via double-tap was impossible wi…

e235c84

…th Touch ID

eliperkins removed their assignment Apr 5, 2017

Conversation

ayanonagon commented Sep 30, 2015

Uh oh!

eliperkins commented Sep 30, 2015

Uh oh!

eliperkins commented Sep 30, 2015

Uh oh!

zinthemoney commented Oct 1, 2015

Uh oh!

ayanonagon commented Oct 1, 2015

Uh oh!

eliperkins Oct 1, 2015

Choose a reason for hiding this comment

Uh oh!

eliperkins Oct 1, 2015

Choose a reason for hiding this comment

Uh oh!

ayanonagon Oct 1, 2015

Choose a reason for hiding this comment

Uh oh!

zinthemoney Oct 1, 2015

Choose a reason for hiding this comment

Uh oh!

hyperspacemark Oct 2, 2015

Choose a reason for hiding this comment

Uh oh!

zinthemoney Oct 2, 2015

Choose a reason for hiding this comment

Uh oh!

eliperkins commented Oct 1, 2015

Uh oh!

hyperspacemark Oct 2, 2015

Choose a reason for hiding this comment

Uh oh!

dasmer commented Apr 5, 2016

Uh oh!

ghost commented Oct 10, 2017

Uh oh!

ericlewis commented Oct 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments