1Password Support for Database Connections #422

cocojoe · 2017-04-05T12:20:52Z

All PasswordPolicy levels supported in Sign Up password generation
Added Lock options showPasswordManager, passwordManagerAppIdentifier
Embedded OnePassword ObjC Extension into Lock

cocojoe · 2017-04-05T12:28:31Z

cocojoe · 2017-04-05T13:11:17Z

Test Notes:

Must have 1Password app installed on device.

info.plist

Add following to the app's info.plist:

<key>LSApplicationQueriesSchemes</key>
<array>
    <string>org-appextension-feature-password-management</string>
</array>

Options

By default the bundler identifier will be used, If you want to share credentials with a website that uses Auth0, then use the name of the site e.g.

.withOptions {
    $0.passwordManagerAppIdentifier = "www.website.com"
}

cocojoe · 2017-04-06T13:19:44Z

Additional thoughts, currently the password recipe will match the DB password policy, although this feels incredibly weak for anything < fair. Given that if you are using a password manager you are mostly setting it and forgetting it. Perhaps the default should be fair+ ?

hzalaz

Also the 1password button is a bit bigger than I'd like

hzalaz · 2017-04-06T15:08:18Z

Lock/LockOnePasswordExtension.h

@@ -0,0 +1,210 @@
+//


I'd keep the name without the Lock prefix and also I hope this is no part of the public headers right?

No it's not, they recommend renaming it so I did that 'just in case'. I made it an isolated commit so easy to revert.

https://github.com/auth0/Lock.swift/blob/added-1password-support/Lock/LockOnePasswordExtension.h#L57

Probably makes sense for apps to avoid clashing

But here we don't need to rename it since it won't be a public header

hzalaz · 2017-04-06T16:02:10Z

Lock/OptionBuildable.swift

@@ -90,6 +90,9 @@ public protocol OptionBuildable: Options {

        /// Specify the passwordless method, send a passcode or send a magic link. By default is .emailCode
    var passwordlessMethod: PasswordlessMethod { get set }
+
+        /// Enable and specify the 1Password identifier to use, typically you will want to use the app's bundle identifier. However you may have a website and want to ensure 1Password works with both by using an identifier like `www.mysite.com`
+    var enableOnePasswordWithIdentifier: String? { get set }


Let's split this in two

/// Show password manager button when user credentials are needed, e.g. during Sign Up, and allow storing/retrieving credentials from the password manager var showPasswordManager: Bool { get set } /// Application identifier used by the password manager, if you also have a web app we recommend adding your site url. By default is the application bundle identifier var passwordManagerAppIdentifier: String { get set }

I merged it into one, as I look at it from the perspective of if I am going to enable it then I will want to use it, so why not in streamline into one option? However I have no strong feelings either way.

First I'd like it to be enabled by default and ideally some people won't set the app identifier and prefer to use the bundle identifier

I thought perhaps passwordManagerAppIdentifier should be optional as Bundle.main.bundleIdentifier is an optional. Although the user has bigger issues in their app if their bundleIdentifier is nil. So forced it.

We should probably add another option for the Password Title in 1Password, by default it will parse the identifier however it's designed to parse website urls so looks weird.

We could of course just pass the CFBundleDisplayName on the store method as it would also be strange not to use the same name as the app for 1Password.

I've set it to use display name as title for now.

We can have passwordManagerDisplayName(Default bundle display name) or improve the options and have a struct to configure the password manager with the three properties inside

hzalaz · 2017-04-06T16:43:01Z

Lock/PasswordPolicy.swift

+            return [ AppExtensionGeneratedPasswordMinLengthKey: "8",
+                     AppExtensionGeneratedPasswordRequireDigitsKey: true,
+                     AppExtensionGeneratedPasswordRequireSymbolsKey: true ]
+        case Auth0.excellent.rawValue:


There was a comment and yes use excellent for every rule even if there is no rule

hzalaz

Also the 1pass icon seems too big compared to the field. I'd try to leave around 3-4 pt of padding in the edges

hzalaz · 2017-04-18T21:37:20Z

Lock/OnePassword.swift

@@ -71,3 +71,19 @@ class OnePassword: PasswordManager {
        }
    }
 }
+
+public struct PasswordManagerConfig {


Why don't use the same PasswordManager instead of creating yet another type?

I do not see any requirement or need for PasswordManager Protocol to be a Public API. It also utilises internal types such as InputField...
It is debatable to use a struct vs properties for 2-3 values anyway. So can always revert to properties.
Although another type is not a big deal either.

We can use the OnePassword type and configure it, the protocol can still be private (and implemented by OnePassword) since we are not allowing any extra extensibility.

hzalaz · 2017-04-18T21:38:47Z

Lock/OptionBuildable.swift

-
-    /// Application identifier used by the password manager, if you also have a web app we recommend adding your site url. By default is the application bundle identifier
-    var passwordManagerAppIdentifier: String { get set }
+        /// Configuration to be used by the password manager.


Can we explain a bit better what it means in the code comment?

Also we need to explain its attributes

hzalaz · 2017-04-18T21:39:30Z

Lock/OnePassword.swift

@@ -71,3 +71,19 @@ class OnePassword: PasswordManager {
        }
    }
 }
+
+public struct PasswordManagerConfig {
+    public var isEnabled: Bool = true


I think enabled is fine

hzalaz · 2017-04-18T21:41:29Z

README.md

+
+```swift
+.withOptions {
+    $0.passwordManager = PasswordManagerConfig(appIdentifier: "www.myapp.com", displayName: "My App")


Having https://github.com/auth0/Lock.swift/pull/422/files#diff-04c6e90faac2675aa89e2176d2eec7d8R371 why not follow the same approach?

I did originally then changed it, will revert.

hzalaz · 2017-04-18T21:45:03Z

Lock/ClassicRouter.swift

@@ -52,6 +52,10 @@ struct ClassicRouter: Router {
            let authentication = self.lock.authentication
            let interactor = DatabaseInteractor(connection: database, authentication: authentication, user: self.user, options: self.lock.options, dispatcher: lock.observerStore)
            let presenter = DatabasePresenter(interactor: interactor, connection: database, navigator: self, options: self.lock.options)
+            if self.lock.options.passwordManager.isEnabled, OnePassword.isAvailable() {


Below I suggested to join the config with the object itself so we could just ask it if its enabled (user settings + app availability in the same query).

Ideally we could just assign it all the time and ask it if its enabled when trying to use it since we will do the same but with the optional

Not sure there is any real life advantage, if OnePassword is not available on device (still needs checked), there is not much point in initialising the manager just to perform a bool check vs an optional check.

Less code, encapsulation and simplicity. The presenter will always have a password manager and will always ask it if it should display the button. All the thing is done in the presenter and the router only needs to inject it.

hzalaz · 2017-04-18T21:45:33Z

Lock/LockOnePasswordExtension.h

@@ -0,0 +1,210 @@
+//


But here we don't need to rename it since it won't be a public header

hzalaz · 2017-04-18T21:46:04Z

Lock/OnePassword.swift

+    }
+}
+
+public struct PasswordManagerConfig {


Just merge the password manager and the config its not necessary to add an extra type

Also let's add code comments to the attributes

hzalaz · 2017-04-18T21:46:54Z

Lock/OnePassword.swift

+}
+
+public struct PasswordManagerConfig {
+    public var isEnabled: Bool = true


lets use enabled

hzalaz · 2017-04-18T21:47:35Z

Lock/OptionBuildable.swift

@@ -90,6 +90,9 @@ public protocol OptionBuildable: Options {

        /// Specify the passwordless method, send a passcode or magic link. By default is .code
    var passwordlessMethod: PasswordlessMethod { get set }
+
+        /// Configuration to be used by the password manager.


Lets explain it a bit more

hzalaz

@cocojoe also please update the screenshots showing the button, if needed, so I can see how it looks now

hzalaz · 2017-04-25T00:48:26Z

Lock/DatabaseOnlyView.swift

@@ -108,9 +109,15 @@ class DatabaseOnlyView: UIView, DatabaseView {
        self.layoutSecondaryButton(self.allowedModes.contains(.ResetPassword))
        self.form = form

+        if passwordManager.enabled, passwordManager.isAvailable() {


Probably I wasn't clear before but ideally the passwordManager.isAvaliable() could be encapsulated inside the passwordManager.enabled

hzalaz · 2017-04-25T00:51:36Z

Lock/DatabaseOnlyView.swift

@@ -85,7 +86,7 @@ class DatabaseOnlyView: UIView, DatabaseView {
    private let separatorIndex = 2
    private let socialIndex = 1

-    func showLogin(withIdentifierStyle style: DatabaseIdentifierStyle, identifier: String? = nil, authCollectionView: AuthCollectionView? = nil) {
+    func showLogin(withIdentifierStyle style: DatabaseIdentifierStyle, identifier: String? = nil, authCollectionView: AuthCollectionView? = nil, passwordManager: PasswordManager) {


From a design stand, passing the password manager is sort of breaching the separation of concerns of the view and presenter. Ideally the view should only know that it should show the button (just a simple true or false) then the rest of the work should be in the presenter (the wiring)

hzalaz · 2017-04-25T00:57:41Z

Lock/OnePassword.swift

+    public var displayName: String
+
+    weak var controller: UIViewController?
+    var fields: [String: InputField] = [:]


Not sure why I didnt spot this before but I am not too convinced about this mapping. AFAIK the dict will strong retain the field and thats a thing we want to avoid. Also mixing something from the view here is sort of a code smell, maybe we need to work this around by either having closures but maybe that is not enough either

hzalaz · 2017-04-25T00:57:54Z

Lock/OnePassword.swift

+
+    public init() {
+        self.appIdentifier = Bundle.main.bundleIdentifier!
+        self.displayName = Bundle.main.object(forInfoDictionaryKey: "CFBundleName") as! String


No force-wrap please

I'd rather use here a default like <No App>. This wont be ever execute since Xcode will fail without the bundle name but its better than a force-wrap

hzalaz · 2017-04-25T01:00:15Z

LockTests/Models/PasswordPolicySpec.swift

@@ -119,6 +119,36 @@ class PasswordPolicySpec: QuickSpec {

        }

+        describe("one password extension") {


Is there some way we can test this in a better way?

cocojoe · 2017-04-25T13:31:19Z

Will add styling to this once I can rebase style branch (WIP)

hzalaz · 2017-04-26T19:14:47Z

Lock/DatabasePresenter.swift

@@ -146,6 +149,23 @@ class DatabasePresenter: Presentable, Loggable {
        view.secondaryButton?.onPress = { button in
            self.navigator.navigate(.forgotPassword)
        }
+
+        if let identifyField = view.identityField, let passwordField = view.passwordField {
+            passwordManager.onUpdate = { identifier, password in


Good but in the end we end up having the same retain behavior with a different implementation

hzalaz · 2017-04-26T19:16:52Z

Lock/InputField.swift

+        button.color = color
+        container.addSubview(button)
+
+        self.textFieldRightPadding?.constant = -50


Being picky but can't we disable this constraint and add a new one between field and button?

hzalaz · 2017-04-26T19:17:17Z

Lock/OnePassword.swift

+
+    public init() {
+        self.appIdentifier = Bundle.main.bundleIdentifier.verbatim()
+        self.displayName = Bundle.main.object(forInfoDictionaryKey: "CFBundleName").verbatim()


LOL, nice trick

cocojoe · 2017-05-02T08:38:47Z

@hzalaz this can be merged

hzalaz · 2017-05-02T16:53:00Z

Lock/DatabasePresenter.swift

+            }
+        }
+        view.passwordManagerButton?.onPress = { _ in
+            self.passwordManager.login {


we are retaining the presenter here

hzalaz · 2017-05-02T16:53:13Z

Lock/DatabasePresenter.swift

+            }
+        }
+        view.passwordManagerButton?.onPress = { _ in
+            self.passwordManager.store(withPolicy: passwordPolicyValidator?.policy.onePasswordRules(), identifier: self.creator.identifier) {


we are retaining the presenter here

For both, there is no strong reference to the block which is why I didn't modify in the last pass. Quick checked in DatabasePresenter.swift with:

deinit { print("dealloc") }

cocojoe · 2017-05-03T12:06:35Z

@hzalaz I can't merge as the codecov is a bit weird, I set the 3rd Party OnePasswordExtension .h/.m to be ignored in CodeCov

Added PasswordManager Protocol Added 1Password Swift Facade Added IconButton component OnePassword Assets Update PodSpec

PasswordManager Added to Classic Router with Extension check PasswordPolicy extension to convert existing policy into 1Password password Recipe PasswordManager added to Database Signup, will store username when available

Update README PaswordManager Section

Update Mocks Tidied up UITests as got broke in rebase (Snapshot)

cocojoe force-pushed the added-1password-support branch from 5723f1b to 1379367 Compare April 5, 2017 14:00

cocojoe mentioned this pull request Apr 6, 2017

1Password extension support #212

Closed

cocojoe requested a review from hzalaz April 6, 2017 13:20

hzalaz suggested changes Apr 6, 2017

View reviewed changes

hzalaz added the CH: Added label Apr 6, 2017

hzalaz added this to the v2-Next milestone Apr 6, 2017

cocojoe force-pushed the added-1password-support branch 2 times, most recently from e4348a2 to d3468ad Compare April 7, 2017 13:25

cocojoe force-pushed the added-1password-support branch from 373a933 to c3ef57e Compare April 18, 2017 15:26

hzalaz suggested changes Apr 18, 2017

View reviewed changes

cocojoe force-pushed the added-1password-support branch 2 times, most recently from 0753496 to b4234ad Compare April 20, 2017 10:03

hzalaz suggested changes Apr 25, 2017

View reviewed changes

cocojoe force-pushed the added-1password-support branch from 7ac54e9 to 4243a0b Compare April 25, 2017 13:32

hzalaz suggested changes Apr 26, 2017

View reviewed changes

cocojoe force-pushed the added-1password-support branch 2 times, most recently from f9f42b4 to f8ce528 Compare April 27, 2017 15:11

hzalaz suggested changes May 2, 2017

View reviewed changes

hzalaz approved these changes May 3, 2017

View reviewed changes

cocojoe force-pushed the added-1password-support branch from f8ce528 to 127c358 Compare May 8, 2017 11:04

cocojoe added 2 commits May 8, 2017 13:54

Added 1Password Login Support for Database Connections

1c1f879

Added PasswordManager Protocol Added 1Password Swift Facade Added IconButton component OnePassword Assets Update PodSpec

Added Password Manager Options

48e6ea9

PasswordManager Added to Classic Router with Extension check PasswordPolicy extension to convert existing policy into 1Password password Recipe PasswordManager added to Database Signup, will store username when available

cocojoe force-pushed the added-1password-support branch from 127c358 to 5eb27eb Compare May 8, 2017 13:40

Added Password Manager Tests

28e7aec

Update README PaswordManager Section

Added 1Password Styling

8f862f5

Update Mocks Tidied up UITests as got broke in rebase (Snapshot)

cocojoe force-pushed the added-1password-support branch from 5eb27eb to 8f862f5 Compare May 8, 2017 14:24

cocojoe merged commit cb0a12d into master May 8, 2017

hzalaz deleted the added-1password-support branch May 8, 2017 23:55

cocojoe mentioned this pull request Jun 6, 2017

Release 2.3.0 #439

Merged

		@@ -119,6 +119,36 @@ class PasswordPolicySpec: QuickSpec {

		}

		describe("one password extension") {

1Password Support for Database Connections #422

1Password Support for Database Connections #422

Conversation

cocojoe commented Apr 5, 2017 • edited Loading

cocojoe commented Apr 5, 2017

cocojoe commented Apr 5, 2017 • edited Loading

Test Notes:

info.plist

Options

cocojoe commented Apr 6, 2017

hzalaz left a 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

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

hzalaz left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocojoe Apr 19, 2017 • edited Loading

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

hzalaz left a 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

cocojoe commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocojoe commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocojoe commented May 3, 2017

cocojoe commented Apr 5, 2017 •

edited

Loading

cocojoe commented Apr 5, 2017 •

edited

Loading

cocojoe Apr 19, 2017 •

edited

Loading