Closed Bug 1162174 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for iOS :: General, defect)

Other
iOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fxios 2.0+ ---

People

(Reporter: richard, Assigned: jhugman)

References

(Depends on 1 open bug)

Details

(Whiteboard: [nicetohave1.1][needs strings])

Attachments

(9 files, 1 obsolete file)

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.
Does Safari have 1Password in their share sheet?
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.)
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.)
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
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(krudnitski)
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).
Flags: needinfo?(krudnitski)
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
Summary: 1Password not in extension sheet → Add support for 1Password
Rad, our built-in Send Tab action has disappeared now. It is available from other apps but not from within Firefox anymore.
Flags: needinfo?(rad)
Nice catch Stefan!

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

Cheers!
Flags: needinfo?(rad)
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
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
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,
Rad, If the NSExtensionActivationRule change works then I don't see a reason to implement add the UTImportedTypeDeclarations. What do you think?
Summary: Add support for 1Password → Make sure third-party password managers show up as Action Extensions
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?
Rad, just a quick note that I am putting this PR on hold until 1Password 5.5 has been released.
Stefan, this works for me too :-)

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

Thanks!
Assignee: nobody → sarentz
tracking-fxios: --- → ?
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.
Duplicate of this bug: 1198718
PR has a milestone 1.1, should this track?
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!
Whiteboard: [nicetohave1.1]
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.
Flags: needinfo?(dhenein)
Whiteboard: [nicetohave1.1] → [nicetohave1.1][needs strings]
(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.
Just in case it comes up, I've filed a bug for disabling the password management stuff in Firefox desktop as bug 1205010.
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]
Flags: needinfo?(dhenein)
(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).
Summary: Make sure third-party password managers show up as Action Extensions → [meta] Make sure third-party password managers show up as Action Extensions
Should we also not *fill* logins?
Please reset the 2.0+ flag.
Bulk changes to Aha cards. Filter on 'mpopova-aha-20151008' to find all matching messages.
Blocks: 1213016
Depends on: 1215877
I split out the settings work for this meta into Bug 1215877.
(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.
Assignee: sarentz → nobody
Assignee: nobody → jhugman
Status: NEW → ASSIGNED
Work in progress rebasing onto master.

Fails linking, even after clean and cleaning DerivedData.
Attachment #8612286 - Attachment is obsolete: true
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?
Flags: needinfo?(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.
Flags: needinfo?(sleroux)
OnePassword JSON payload from OPWebViewCollectFieldsScript and passed to the app extension.
Collected by OPWebViewCollectFieldsScript before being passed to the app extension.

This the same page as the OnePasswordExtension JSON payload.
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?
Flags: needinfo?(rad)
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 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.
Flags: needinfo?(rad)
Attachment #8678519 - Flags: review?(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!
Attachment #8678519 - Flags: review?(bnicholson) → review?(sarentz)
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.
Attachment #8678519 - Flags: review?(sarentz) → review+
Fixed nits and merged.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 1227592
You need to log in before you can comment on or make changes to this bug.