search

velikanov / SwiftPasscodeLock

An iOS passcode lock with TouchID authentication written in Swift.
MIT License
199 stars 52 forks source link

Additions for more versatile use. Also includes view with black background now as default. #50

Open oskarirauta opened 7 years ago

oskarirauta commented 7 years ago

Nothing that my commits do - was impossible before I made these commits, but now it's just easier...

ziogaschr commented 7 years ago

@oskarirauta can you explain with more details what this PR is for?

oskarirauta commented 7 years ago

I made changes, and wanted you to let know about 'em. Maybe you get some ideas.

Lähetetty iPhonesta

On 7 Jun 2017, at 21.54, Chris Ziogas notifications@github.com wrote:

@oskarirauta can you explain with more details what this PR is for?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

ziogaschr commented 7 years ago

Nice ideas.

  • more callbacks

Might be nice to also follow @yankodimitrov way and instead of callbacks do the same my posting a notification. What do you think about this?

  • possibility to dismiss a cancellable view automatically after attempts exceed maximum

Can you describe the use case of this? What I think you want from a security perspective is to lock the user out of your App/View. I already see partly your use case, as you can say that this view is cancelable. Although, will it be better UX wise to handle this case in your code and let the user know why she got out of the flow he was trying to reach? So your code will here for a notification and in this case will a) show an alert or something to the user, b) close the Passcode view.

  • bug fix: reset attempts when view comes up

In the App we are using this library, this is not what we want, so might be easier that the App itself handles this case. Although I would like to know your use case, and if it fits better to what most need, then it might be better for this library. :)

  • possibility to use touchid when passcode is requested, but hide button used for that

Which button, the one in the PasscodeViewController? This would mean that once the user puts your App in background mode, and again back in foreground, there will be no way for the user to open TouchID. Of course, you can still open it for him programmatically. Again, what most people need from this library.

Thanks for your contribution @oskarirauta and for putting the above on discussion from all of us. It's all about setting this library defaults, as the each implementor can switch them in their App implementation, so let's discuss altogether what the library defaults we want to be.

oskarirauta commented 7 years ago

On 9 Jun 2017, at 16:52, Chris Ziogas notifications@github.com wrote:

Nice ideas.

more callbacks Might be nice to also follow @yankodimitrov https://github.com/yankodimitrov way and instead of callbacks do the same my posting a notification. What do you think about this?

I am developing a application which has multiple lockable view controllers, I thought notification approach, but decided that this way it’s easier to point notifications to correct view controller. My application also has multiple points that are locked in same view controller, therefore, it’s way easier to give each section their own callback instead of thinking which part is supposed to receive a notification.

But one could easily provide both, it won’t grow heap size too much to include these as well.

possibility to dismiss a cancellable view automatically after attempts exceed maximum Can you describe the use case of this? What I think you want from a security perspective is to lock the user out of your App/View. I already see partly your use case, as you can say that this view is cancelable. Although, will it be better UX wise to handle this case in your code and let the user know why she got out of the flow he was trying to reach? So your code will here for a notification and in this case will a) show an alert or something to the user, b) close the Passcode view.

It more or less follows the “protocol” set by Apple. When you want to open keypad lock, you can use either your finger or enter code - also, if you don’t have the code - or decide so, you may cancel. In application that I am developing, I have sections that are described as public and secure, public sections are available without authentication, but secure ones must be authenticated for viewing, therefore, without cancellation, user who wouldn’t have credentials, would be forced to restart the application when he reaches point where authentication is required. I wanted a way for going back.

bug fix: reset attempts when view comes up In the App we are using this library, this is not what we want, so might be easier that the App itself handles this case. Although I would like to know your use case, and if it fits better to what most need, then it might be better for this library. :)

Yes, but still - I don’t want to authenticate when starting application - I want to authenticate inside it, depending on user’s configuration - multiple times, user might want to give another use access to some parts, but not all, so he authenticates these parts and leaves other parts locked.. Making app handle this- would cause unnecessary replication of code.

possibility to use touchid when passcode is requested, but hide button used for that Which button, the one in the PasscodeViewController? This would mean that once the user puts your App in background mode, and again back in foreground, there will be no way for the user to open TouchID. Of course, you can still open it for him programmatically. Again, what most people need from this library.

True. But in my approach I don’t use it like this at all. One using it in the way you and example describes it - this shouldn’t be enabled. Idea behind this is simple: we use touchID and passcode is a fallback, and there’s no going back after. Thought I saw this approach somewhere else… And liked it :) But your mileage may vary, that’s why it’s optional.

Thanks for your contribution @oskarirauta https://github.com/oskarirauta and for putting the above on discussion from all of us. It's all about setting this library defaults, as the each implementor can switch them in their App implementation, so let's discuss altogether what the library defaults we want to be.

I think defaults are good as they were, I just wanted to give something more, which can be toggled when necessary. What I contributed… Is not finished/polished, but it works, so it could be considered as a stub - or a concept. But too many times in this world, we decide to re-invent the wheel, so I decided to make my contribution public and tell my ideas out loud :)

For sure, you could join my pull request as is, but one who wants features I brought, can easily use my fork - I just think you have great potential, so I shared my ideas :)

With best regards, Oskari Rauta.

ziogaschr commented 7 years ago

more callbacks

I see your point to this one.

possibility to dismiss a cancellable view automatically after attempts exceed maximum

So, it is indeed good to offer this easily to implementors. DOes it makes sense to change this line case .enterOptionalPasscode: return EnterOptionalPasscodeState() to case .enterOptionalPasscode: return EnterPasscodeState(allowCancellation: true)?

Then you can remove the whole EnterOptionalPasscodeState implementation. I wasn't able to check the diff in my work laptop, so I am checking the files in GitHub only, let me know if more changes are made.

bug fix: reset attempts when view comes up

I still can't see how this is a problem in any case. I will try to run some tests in XCode in order I see it in action.

possibility to use touchid when passcode is requested, but hide button used for that

Fair enough, do you think that hideTouchIDButton makes more sense as the option name instead of shouldDisableTouchIDButton?

Thanks a lot @oskarirauta and I think some from ideas above make sense.

I will try to test them and will come back with feedback if needed.

@yankodimitrov & @velikanov are you willing to give back your feedback, as you know the lib better than me :)

oskarirauta commented 7 years ago

On 10 Jun 2017, at 19:32, Chris Ziogas notifications@github.com wrote:

more callbacks

I see your point to this one.

possibility to dismiss a cancellable view automatically after attempts exceed maximum

So, it is indeed good to offer this easily to implementors. DOes it makes sense to change this line case .enterOptionalPasscode: return EnterOptionalPasscodeState() to case .enterOptionalPasscode: return EnterPasscodeState(allowCancellation: true)?

Then you can remove the whole EnterOptionalPasscodeState implementation. I wasn't able to check the diff in my work laptop, so I am checking the files in GitHub only, let me know if more changes are made.

Yes, this indeed is a better solution. No need to invent wheel twice ;)

bug fix: reset attempts when view comes up

I still can't see how this is a problem in any case. I will try to run some tests in XCode in order I see it in action.

While testing this, without resetting attempts, and when using dismiss option after maximum attempts, sometimes, it dismissed after one or two attempts, even though maximum attempts was set to 3. possibility to use touchid when passcode is requested, but hide button used for that

Fair enough, do you think that hideTouchIDButton makes more sense as the option name instead of shouldDisableTouchIDButton?

I indeed do think, it makes more sense. Thanks a lot @oskarirauta https://github.com/oskarirauta and I think some from ideas above make sense.

I will try to test them and will come back with feedback if needed.

@yankodimitrov https://github.com/yankodimitrov & @velikanov https://github.com/velikanov are you willing to give back your feedback, as you know the lib better than me :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/velikanov/SwiftPasscodeLock/pull/50#issuecomment-307575798, or mute the thread https://github.com/notifications/unsubscribe-auth/ABRSHt6Vq51MXRXpH4BP8O0_lxFO2uAaks5sCsUngaJpZM4Nwpwt.

ziogaschr commented 7 years ago

Hi @oskarirauta. This PR becomes very big, making it harder to review and merge it. I will try to check it again, but it will be helpful if you separate your future work in smaller, more pointed PRs. :)

p.s.: it's really cool, that you are willing to contribute to this library 👍

oskarirauta commented 7 years ago

Previous changes were a bug fix. What is your apple ID? I will let you beta test my app, so you can see how this performs.

Lähetetty iPhonesta

On 11 Jul 2017, at 13.37, Chris Ziogas notifications@github.com wrote:

Hi @oskarirauta. This PR becomes very big, making it harder to review and merge it. I will try to check it again, but it will be helpful if you separate your future work in smaller, more pointed PRs. :)

p.s.: it's really cool, that you are willing to contribute to this library 👍

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

ziogaschr commented 7 years ago

I can't find your email, in order I send my ID in a private channel. Can you try to email me?

oskarirauta commented 7 years ago

It is oskari.rauta@gmail.com mailto:oskari.rauta@gmail.com

Software although is in a beta stage, but partially gives idea, why I need authentication multiple times. (And partially hiding screen when backgrounding app does not work, but this is not because of SwiftPasscodeLock)

On 11 Jul 2017, at 17:38, Chris Ziogas notifications@github.com wrote:

I can't find your email, in order I send my ID in a private channel. Can you try to email me?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/velikanov/SwiftPasscodeLock/pull/50#issuecomment-314465505, or mute the thread https://github.com/notifications/unsubscribe-auth/ABRSHnPoOgFu7WEPKwSz-qUWqbhLppkKks5sM4kAgaJpZM4Nwpwt.