1162174 - [meta] Make sure third-party password managers show up as Action Extensions

Status: RESOLVED FIXED

Description

[Richard Eriksson]

Created attachment 8602230 [details]
Share sheet in Firefox while looking at the Letterboxd website

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_4) AppleWebKit/600.5.17 (KHTML, like Gecko) Version/8.0.5 Safari/600.5.17

Steps to reproduce:

1. Visited Letterboxd website http://letterboxd.com/ and clicked the lock icon to login.
2. Cancelled the keyboard.
3. Tapped the share icon in the bottom row.


Actual results:

The share sheet came up with LastPass listed but not 1Password.


Expected results:

1Password should be listed and supported as a password app. Attached screenshot shows this. Tapping "More" does not show the 1Password app listed. This is in build 10.

Comment 1

[Aaron Train [:aaronmt]]

Does Safari have 1Password in their share sheet?

Comment 2

[Richard Eriksson]

Created attachment 8602277 [details]
Not able to add 1Password to the action sheet

Comment 3

[Richard Eriksson]

Created attachment 8602278 [details]
1Password in action sheet in Safari

Comment 4

[Richard Eriksson]

Safari has it, yes. I just added a screenshot of Safari (8602278).

(I also added a screenshot of not being able to add 1Password in Firefox when tapping more, 8602277.)

Comment 5

[Stefan Arentz [:st3fan]]

The bug at https://code.google.com/p/chromium/issues/detail?id=405894 seems to suggest that there now is a workaround possible. Will contact 1Password folks to find out more.

(1Password people, do you like Timbits, Coffees, frappucinos? Let us know, half the Firefox for iOS team happens to be two blocks away from you on Adelaide St West. Would love to discuss this issue and get a workaround going.)

Comment 6

[Rad Azzouz]

Hey Stefan,

I was just working on it. I'll pushing a PR very very soon :-)

I do love them all Timbits, Coffees, frappucinos :-) I am located in Montreal, but I may be in Toronto this summer and I'd love to see you then :-D

Cheers!

Rad

Comment 7

[Stefan Arentz [:st3fan]]

Created attachment 8612286 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/510

Comment 8

[Stefan Arentz [:st3fan]]

I have conflicting thoughts about this pull request.

First, I love this. I use 1Password every day and many people who will use Firefox for iOS probably will too. So this brings immediate value to people using our application.

However, right now this is a 1Password specific integration and not really a workaround for app extensions in combination with the WKWebView in third-party apps. (Which is the real cause of why 1Password and LastPass only work in Safari). What I was really hoping for, was that the Chromium team was hinting, in that bug that I mentioned earlier, towards a non-specific workaround that would *any* password provider work.

On a technical level, this is a simple patch and I have no objections including it. Also, not-excluding similar patches of other password managers. It is all pretty nicely hidden behind the UIActivityViewController, and we could create a more structured way to accommodate other parties. 

However, that may not be just a technical decision.

Comment 9

[Karen Rudnitski [:kar]]

For the expediency of time and the high value of having this integrated, I'd like to see us move forward with the specific workaround.

However, when we take a deep breath after v1 goes out, I'd like to revisit to see about integrated the more generalized approach since I would hate to see having specific code to each provider in the future (from an efficiency / scalability perspective).

Comment 10

[Rad Azzouz]

Hey Karen and Stefan!

Thanks for reviewing my PR :-)

Any password manager which supports web view filling (UIWebView or WKWebView in the Firefox case) and which conforms to the App Extension API (https://github.com/AgileBits/onepassword-app-extension), simply needs to add their extension's bundle identifier to the "isPasswordManagerActivityType()" function. 

	private func isPasswordManagerActivityType(activityType: String?) -> Bool {
		var isOnePassword = OnePasswordExtension.sharedExtension().isOnePasswordExtensionActivityType(activityType);
		var isPasswordManager = (activityType == "bundle.identifier.for.another.password.manager")
		return isOnePassword || isPasswordManager
	}

Hope that this helps :-)

Best

Comment 11

[Stefan Arentz [:st3fan]]

Rad, our built-in Send Tab action has disappeared now. It is available from other apps but not from within Firefox anymore.

Comment 12

[Rad Azzouz]

Nice catch Stefan!

I found the fix! I'll push it later on today!

Cheers!

Comment 13

[Rad Azzouz]

I just pushed the fix :-)

The problem was caused by having both "url" and “self" in the "activityItems" array:

1) Now that our BrowserViewController implements the UIActivityItemSource, self can be either the 1Password extension item or the selected tab’s URL (see "func activityViewController(activityViewController: UIActivityViewController, itemForActivityType activityType: String) -> AnyObject?").

2) Having both url and self (being the URL) meant that we had two URLs which rendered the NSExtensionActivationRule condition (NSExtensionActivationSupportsWebURLWithMaxCount = 1)  false.

Removing the “url" "activityItems” array fixes the issue.

https://www.evernote.com/l/AVTYyr0aT3dEzpYkNYoBXTZQifb01h3UeRcB/image.png

Comment 14

[Stefan Arentz [:st3fan]]

Note to self, I think this only works with the following NSExtensionActivationRule:

SUBQUERY ( extensionItems, $extensionItem, SUBQUERY ( $extensionItem.attachments, $attachment, ANY $attachment.registeredTypeIdentifiers UTI-CONFORMS-TO "public.url").@count == 1 ).@count == 1

Comment 15

[Rad Azzouz]

Hi Stefan,

I was testing with the current beta version of 1Password. We fixed this on our end in the most recent betas. 

I just pushed the fix where I conform the 1Password custom activity type ("org.appextension.fill-browser-action") to "public.url".

BTW, your predicate looks good assuming that "Send To" extension requires only a URL to show up in the share sheet.

Best,

Comment 16

[Stefan Arentz [:st3fan]]

Rad, If the NSExtensionActivationRule change works then I don't see a reason to implement add the UTImportedTypeDeclarations. What do you think?

Comment 17

[Rad Azzouz]

Hi Stefan,

I like the NSExtensionActivationRule change as I have a soft spot for the NSPredicate :-) 

Adding the NSExtensionActivationRule predicate and removing the UTImportedTypeDeclarations works only when 1Password 5.4.2 is installed. It does not work when 1Password 5.5 beta is installed.

So I decided to leave the UTImportedTypeDeclarations change and to change the NSExtensionActivationRule to the predicate that you sent me, this way we have the same share sheet in Firefox regardless of the version of 1Password that you’re using.

Thoughts?

Comment 18

[Stefan Arentz [:st3fan]]

Rad, just a quick note that I am putting this PR on hold until 1Password 5.5 has been released.

Comment 19

[Rad Azzouz]

Stefan, this works for me too :-)

I will comment here as soon as 1Password 5.5 hits the shelves.

Thanks!

Comment 20

[Stefan Arentz [:st3fan]]

We most certainly want to include this but we are not blocking on it for our v1.0 release. Removing myself as the reviewer for now to clear my queue. We will most likely start looking at this bug again when the v1 storm has calmed down.

Comment 22

[Aaron Train [:aaronmt]]

PR has a milestone 1.1, should this track?

Comment 23

[Rad Azzouz]

As promised, I am letting everyone know that 1Password for iOS version 5.5 is out in the wild. The attached PR is also conflict free (I keep it in sync with master). Please feel free to merge it at your leisure.

Thanks!

Comment 24

[Stefan Arentz [:st3fan]]

Darrin asked about UX related issues for this patch. This is the only thing I can thing of:

I think we need a setting to disable our own password code. The reason is that if you are the kind of user that uses an external password manager then you are probably not interested in our dialogs that ask you to remember the password that your preferred password manager just filled in for you. They become an annoyance very quickly.

So I would like to suggest a "Remember passwords for sites [on/off]" setting that can globally disable our password code. It will default to on of course.

Comment 25

[Eric Shepherd [:sheppy]]

(In reply to Stefan Arentz [:st3fan] from comment #24)
> Darrin asked about UX related issues for this patch. This is the only thing
> I can thing of:
> 
> I think we need a setting to disable our own password code. The reason is
> that if you are the kind of user that uses an external password manager then
> you are probably not interested in our dialogs that ask you to remember the
> password that your preferred password manager just filled in for you. They
> become an annoyance very quickly.
> 
> So I would like to suggest a "Remember passwords for sites [on/off]" setting
> that can globally disable our password code. It will default to on of course.

That's actually something that should be true for desktop Firefox too.

/me goes looking to see if that's an extant bug, and to file it if it's not.

Comment 26

[Eric Shepherd [:sheppy]]

Just in case it comes up, I've filed a bug for disabling the password management stuff in Firefox desktop as bug 1205010.

Comment 27

[Darrin Henein [:darrin]]

To keep consistency with the proposed changes in https://bugzilla.mozilla.org/show_bug.cgi?id=1205047, I will propose this option read:

"Save logins" [on/off]

Comment 28

[Eric Shepherd [:sheppy]]

(In reply to Darrin Henein [:darrin] from comment #27)
> To keep consistency with the proposed changes in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1205047, I will propose this
> option read:
> 
> "Save logins" [on/off]

One wording trick is that turning this on doesn't mean that logins are automatically saved; it just means users will be asked to save them. Might be better worded as "Offer to save logins" or something like that (or some label text below explaining what it means, but that might just be clutter).

Comment 29

[Richard Newman [:rnewman]]

Should we also not *fill* logins?

Comment 30

[Aaron Train [:aaronmt]]

Please reset the 2.0+ flag.

Comment 31

[Maria Popova]

Bulk changes to Aha cards. Filter on 'mpopova-aha-20151008' to find all matching messages.

Comment 32

[Richard Newman [:rnewman]]

I split out the settings work for this meta into Bug 1215877.

Comment 33

[Stefan Arentz [:st3fan]]

(In reply to Richard Newman [:rnewman] from comment #32)
> I split out the settings work for this meta into Bug 1215877.

Note that we did expect to do this for this bug and even landed the string already.

Comment 34

[James Hugman [:jhugman] [@jhugman]]

Created attachment 8678519 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/1191

Work in progress rebasing onto master.

Fails linking, even after clean and cleaning DerivedData.

Comment 35

[James Hugman [:jhugman] [@jhugman]]

Fixed build, and now functioning well with OnePassword but not at all with LastPass.

The Swift is communicating with the extension, though LastPass is not recognizing what it's being handed.

I have moved the ActivityViewController construction and behaviour into a ShareExtensionHelper, then put all the OnePasswordExtension specific code separated into a private extension – this sets us up well for experimenting with other Password Manager Extensions. 

Steph – could you give some feedback re: BVC-refactoring-style?

Comment 36

[Stephan Leroux [:sleroux]]

This looks good! I like the separation of concerns and loose coupling between the properties/method invocations on the BVC from the helper by only passing in the tab and delegating the actual update BVC calls to a callback block. The rest of the code that remains in the BVC should definitely be part of the BVC since it handles presenting the pop over view controller.

Comment 37

[James Hugman [:jhugman] [@jhugman]]

Created attachment 8681283 [details]
OnePassword JSON payload

OnePassword JSON payload from OPWebViewCollectFieldsScript and passed to the app extension.

Comment 38

[James Hugman [:jhugman] [@jhugman]]

Created attachment 8681284 [details]
GenericPasswordExtension JSON payload

Collected by OPWebViewCollectFieldsScript before being passed to the app extension.

This the same page as the OnePasswordExtension JSON payload.

Comment 39

[James Hugman [:jhugman] [@jhugman]]

Created attachment 8681287 [details]
OnePasswordExtension's OPWebViewCollectFieldsScript; prettified.

Comment 40

[James Hugman [:jhugman] [@jhugman]]

Created attachment 8681288 [details]
GenericPasswordExtension's OPWebViewCollectFieldsScript; prettified.

Comment 41

[James Hugman [:jhugman] [@jhugman]]

I've dug out the JS for both GenericPassword and OnePassword extensions to collect the file, in an effort to get OnePasswordExtension to work with LastPass.

OnePassword works with the OnePasswordExtension with GenericPasswordExtension's js, but LastPass doesn't.

Javascript of this size needs to be editable/inspectable. Are their any build instructions and sources for OPWebViewCollectFieldsScript and OPWebViewFillScript?

Comment 42

[James Hugman [:jhugman] [@jhugman]]

Created attachment 8681366 [details] [review]
PR for onepassword-app-extension.

Comment 43

[James Hugman [:jhugman] [@jhugman]]

Contacted both LastPass and OnePassword to try and resolve this. 

The short term solution is either using `GenericPasswordExtension` or repackaging the NSExtensionItem for both LastPass (legacy API) and OnePassword & pwSafe (uses current API).

Comment 44

[James Hugman [:jhugman] [@jhugman]]

Comment on attachment 8678519 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/1191

Currently waiting on LastPass support. This will determine the next, finishing steps. 

Please can you review this but I'm not ready to merge.

Comment 45

[Brian Nicholson (:bnicholson)]

Planning on testing and looking at this next week once I get my Find In Page branch up. Sounds like there's no rush since we're waiting on LastPass, but let me know if you need it reviewed sooner!

Comment 46

[Stefan Arentz [:st3fan]]

Comment on attachment 8678519 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/1191

Looks good. My only comment is about pinning the carthage dependency to a specific version. (1.6.4?) If that is possible.

Comment 47

[James Hugman [:jhugman] [@jhugman]]

Fixed nits and merged.