-
Notifications
You must be signed in to change notification settings - Fork 110
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
Passwordless Email (Code/Link) and Social #395
Conversation
4d4ad09
to
7da20b3
Compare
Notes: Enabling Universal Links AppDelegatefunc application(_ application: UIApplication, continue userActivity: NSUserActivity, restorationHandler: @escaping ([Any]?) -> Void) -> Bool {
return Lock.continueAuth(using: userActivity)
} Setup
|
35abc97
to
203eb04
Compare
c85ded4
to
42dfb07
Compare
Although it will make the PR a little bigger, might make more sense to wait for SMS support as it has improvements and relies on this branch anyway as a base. So any change to this PR will need to be rebased anyway. |
Lock/Lock.swift
Outdated
*/ | ||
public static func passwordless() -> Lock { | ||
let newLock = Lock() | ||
return newLock.withOptions { $0.passwordless = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we do this in the init?
Lock/Lock.swift
Outdated
|
||
- returns: true if the link is of the appropriate format, false otherwise | ||
*/ | ||
public static func continueAuth(withActivity userActivity: NSUserActivity) -> Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
continueAuth(using userActivity: NSUserActivity) -> Bool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in next PR, will update in general public API bits.
Lock/LockOptions.swift
Outdated
var activeDirectoryEmailAsUsername: Bool = false | ||
var enterpriseConnectionUsingActiveAuth: [String] = [] | ||
|
||
var oidcConformant: Bool = false | ||
var audience: String? | ||
|
||
var passwordlessMethod: PasswordlessMethod = .code | ||
var passwordless: Bool = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either passwordlessOnly
or classicMode
to denote its one or the other
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k
constraintEqual(anchor: center.topAnchor, toAnchor: self.topAnchor) | ||
constraintEqual(anchor: center.rightAnchor, toAnchor: self.rightAnchor, constant: -20) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does the padding go? in the input?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to have vanished after rebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what vanished? I can still see the change being done in this branch that removes the padding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I miss interpreted you original comment. In the SingleInputView
component, it wasn't as reusable (or compressible) as it could be so tidied the usage up in general. I checked the presenters that use it. MFA, ForgotPassword, EnterpriseDomain and Passwordless.
Lock/ObserverStore.swift
Outdated
var onCancel: () -> Void = { } | ||
var onSignUp: (String, [String: Any]) -> Void = { _ in } | ||
var onForgotPassword: (String) -> Void = { _ in } | ||
var onAuth: (Credentials) -> () = { _ in } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't the change from ()
to Void
part of the linter warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is old, ran a Lint pass.
|
||
import Foundation | ||
|
||
class PasswordlessActivity: PasswordlessUserActivity { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we follow the same style as the Auth tx where the handler has all the code and dependencies and we avoid retaining the messagePresenter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, think I tried this initially then had an issue so swapped to this to get running. If okay I'll address in another PR as internal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool we can do it in another PR
var messagePresenter: MessagePresenter? | ||
|
||
var view: View { | ||
switch self.screen { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd have made two presenters and distinct screens to avoid polluting them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot cleaner in followup PR
Lock/Router.swift
Outdated
@@ -35,6 +35,7 @@ protocol Navigable { | |||
func resetScroll(_ animated: Bool) | |||
func scroll(toPosition: CGPoint, animated: Bool) | |||
|
|||
func onBack() -> () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we have this one? I remember we had an event to go back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a method, however it's not in the protocol so inaccessible in the presenters.
Lock/Router.swift
Outdated
@@ -35,6 +35,7 @@ protocol Navigable { | |||
func resetScroll(_ animated: Bool) | |||
func scroll(toPosition: CGPoint, animated: Bool) | |||
|
|||
func onBack() -> () | |||
} | |||
|
|||
struct Router: Navigable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldnt we have one router for classic and another for passwordless? The code seems more of a mess than it was previously
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly, it would be clearer in terms of internal separation, although it's 20 line block of code vs duplication of a lot more code. I don't really mind either way.
Lock/Lock.swift
Outdated
|
||
- returns: Lock itself for chaining | ||
*/ | ||
public func onPasswordless(callback: @escaping (String, PasswordlessMethod) -> ()) -> Lock { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why having the method here is useful? Also its missing docs on what the callback yields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may or may not be useful, I look at it that a developer may want to know when they are sending out Email/SMS and hook into their own analytics. I'll update docs although I feel it should just be the identifier now.
42dfb07
to
0d9b83e
Compare
|
||
- parameter name: name of the connection | ||
*/ | ||
mutating func passwordless(name: String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how you will tell if its sms or email?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see any options in dashboard to create custom email/sms connections? (Like you can with OAuth2) So in Passwordless SMS it simply selects by name, unless I am missing something?
CDN
{
connections = (
{
name = email;
}
);
name = email;
},
{
connections = (
{
name = sms;
}
);
name = sms;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create any connection with Auth0 API.
constraintEqual(anchor: center.topAnchor, toAnchor: self.topAnchor) | ||
constraintEqual(anchor: center.rightAnchor, toAnchor: self.rightAnchor, constant: -20) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what vanished? I can still see the change being done in this branch that removes the padding
Lock/OptionBuildable.swift
Outdated
} | ||
|
||
internal extension OptionBuildable { | ||
func validate() -> UnrecoverableError? { | ||
extension OptionBuildable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is not internal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My error.
} | ||
|
||
public enum PasswordlessMethod: Int, Equatable { | ||
case emailCode = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
magicLink
or code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this inline with Lock.Android which has emailCode, emailLink, smsCode, smsLink. Passwordless SMS is based on this so backported it so no breaking public change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok but lets only have magicLink
and code
in another PR
|
||
import Foundation | ||
|
||
enum PasswordlessScreen { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for Passwordless routing, naming inspired by DatabaseScreen
Stage 1 .request.
- Request a code/link
Stage 2a .code
- Passcode Auth
Stage 2b .linkSent
- Universal Link Confirmation
|
||
import Foundation | ||
|
||
class PasswordlessActivity: PasswordlessUserActivity { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool we can do it in another PR
Lock/Router.swift
Outdated
@@ -35,6 +35,7 @@ protocol Navigable { | |||
func resetScroll(_ animated: Bool) | |||
func scroll(toPosition: CGPoint, animated: Bool) | |||
|
|||
func onBack() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably was lost but why we added this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not accessible by the presenters as it's not in the protocol, I used it as a convenience in the Passwordless presenter for routing back if code/link has not been delivered. There is no real reason an onBack()
method shouldn't be in a Navigation
protocol.
Lock/Router.swift
Outdated
let presenter = DatabasePresenter(interactor: interactor, connection: database, navigator: self, options: self.lock.options) | ||
if !oauth2.isEmpty { | ||
|
||
if !self.lock.classicMode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this will be far cleaner using different routers to avoid the if-else nightmare. What are the parts that will be duplicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add one and see how it looks.
Renamed activity callback to continueAuth(withActivity: Added support for Social Connections in Passwordless mode PasswordlessView UI Improvements Refactored usage of SingleInputView across views Updated README Update Tests
Rebase Fixes
Split Options Validation into Classic / Passwordless Update PasswordlessMethod Update Public API naming Added classic flag to Lock init Added Lock passwordless init onPasswordless only has identifier now Fix Force wrap Update Tests
Router Split into RouterClassic and RouterPasswordless Added Tests
940d571
to
e9159de
Compare
Public API Methods:
Lock
static func continueAuth(using userActivity: NSUserActivity) -> Bool
: Support NSUserActivity callbacksfunc onPasswordless(callback: @escaping (String) -> Void) -> Lock
: Be informed of passwordless auth initiated with identifier.static func passwordless()
: Lock passwordless instance loading Auth0 client info fromAuth0.plist
file in main bundle.static func passwordless(clientId: String, domain: String) -> Lock
: Lock passwordless instance using clientId and domainNew Options
passwordlessMethod
: Specify to send a passcode.emailCode
or universal link.emailLink