Closed Bug 771915 Opened 8 years ago Closed 8 years ago

Show the origin of the page in the chrome of the app when it is different from webapp's origin

Categories

(Firefox for Android :: Web Apps (PWAs), defect, P1)

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 18
Tracking Status
firefox17 --- fixed
firefox18 --- verified

People

(Reporter: jsmith, Assigned: wesj)

References

Details

(Keywords: uiwanted, Whiteboard: [blocking-webrtandroid1+])

Attachments

(3 files, 7 obsolete files)

Flagged by security for desktop in bug 771395, so likely applies to Android as well. If an app goes off the origin of the app origin, it needs to show the current origin of the page in the chrome of the web app. An example use case:

1. Launch mozilla marketplace <-- nothing in the chrome yet
2. Click login <-- pop-up would have chrome with BID as the origin
See Also: → 771395
Vlad/Wes, thoughts?
It's all possible to do, but this needs UX input to figure out how to do it.
Keywords: uiwanted
Priority: -- → P1
Assignee: nobody → wjohnston
Idea? Show the Android titlebar in these cases, and use setTitle() (which will work here) to make it show the origin?
Priority: P1 → --
(In reply to Wesley Johnston (:wesj) from comment #3)
> Idea? Show the Android titlebar in these cases, and use setTitle() (which
> will work here) to make it show the origin?

Works for me.
Priority: -- → P1
Attached patch Patch (obsolete) — Splinter Review
This shows a simple titlebar at the top of the screen when you're on a third party and hides it when you're on the trusted app's origin. Tested with Facebook and Twitter links on Jauntly.

Its a bit ugly right now, but I can live with that.
Attachment #642152 - Flags: review?(mark.finkle)
Whiteboard: [blocking-webrtandroid1+]
Attached image Screenshot (obsolete) —
This is a screenshot of this (with the url moved to the bottom of the screen). Probably needs a shadow or a line dividing it from content and a little padding. Also, its shifting content up (i.e. we need to relayout).
Attached patch Patch (obsolete) — Splinter Review
This seems to work well. Very unobtrusive. Some amazing art by me. Shows the url at the bottom of the screen. Sadly, its spoofable if the app wants to pretend to be off-origin when its not. I'll post a screenshot.
Attachment #648112 - Flags: review?(mark.finkle)
Attached image Screenshot (obsolete) —
Attachment #642152 - Attachment is obsolete: true
Attachment #645603 - Attachment is obsolete: true
Attachment #642152 - Flags: review?(mark.finkle)
That's a bit hard to read. Looks better on my device, but its still light colored. Easy way to test is to use ed.agadak.net/app, install the ed agadak app which has links to google/youtube/etc.
(In reply to Wesley Johnston (:wesj) from comment #7)
> Created attachment 648112 [details] [diff] [review]
> Patch
> 
> This seems to work well. Very unobtrusive. Some amazing art by me. Shows the
> url at the bottom of the screen. Sadly, its spoofable if the app wants to
> pretend to be off-origin when its not. I'll post a screenshot.

Can you clarify how an app could do this? Could this be a security concern?
Comment on attachment 648112 [details] [diff] [review]
Patch

Overall this approach is fine, but I want Ian to take a look at the screenshot and offer some tweaks.

For example:
* Top or bottom of the screen? Bottom is more like a statusbar, but Android has no such concept.
* Should we mimic the normal Android title bar a bit more?
Attachment #648112 - Flags: review?(mark.finkle) → feedback+
Sounds good. I can put up a build as well as some instructions too. I found the appearance of the titlebar at the top distracting. It would appear and shove content down. Maybe a nice transition would help (but that would be a layout transition which Android kinda hates). I'd rather do that later though.
Btw, looking at the screenshot, are you showing the full URL or just the origin?
full url
(In reply to Wesley Johnston (:wesj) from comment #14)
> full url

Oh. For meeting the original security requirement, I believe you only need to show the origin (developer.mozilla.org if you were on developer.mozilla.org/en-US/Apps), not the full URL. Not sure of the tradeoffs of only showing the origin vs. the full URL.
I talked to ian on irc. He said he'd rather the url was at the top. I'll make that adjustment and file a follow up to animate it? He said he can do a mockup, but I'm fine moving that somewhere else for follow up prettifying.
Attached image Mockup
Attached is a mockup describing where we'd like to take the design of this bar. Basically, it should look like a micro version of the title bar. 

I have broken the design into two parts here. The first part takes what Wes has already done and adds some styling. The second part adds a few elements that might also be useful down the road, like a place for the Globe / Lock icons, and potentially a progress indicator while the page is loading. 

One possible approach might be to do the first part now for 16, and the second for 17.
(In reply to Ian Barlow (:ibarlow) from comment #17)
> Basically, it should look like a micro version of the title bar. 
> 

Sorry, I meant to say a micro version of the *Firefox* title bar there.
(In reply to Ian Barlow (:ibarlow) from comment #17)
> I have broken the design into two parts here. The first part takes what Wes
> has already done and adds some styling. The second part adds a few elements
> that might also be useful down the road, like a place for the Globe / Lock
> icons, and potentially a progress indicator while the page is loading. 

Over in bug 772680, we decided not to show a throbber when an app is doing network activity.  The rationale is not described in the bug, but the summary is that doing so should be the responsibility of the application, not the runtime, because such runtime behavior is conventional (cf. the Java and Adobe Air runtimes), and because the semantics of network activity are complex in modern web apps, and the runtime does not have enough context to present progress indicators on their apps' behalves.

The latter is especially true when the indication is of the loading of a page, since modern webapps are often constructed with lightweight pages that load content dynamically after the static page and its statically-declared resources have loaded.

So I don't think we should be showing a progress indicator for these pages.  Nor show other UI that is designed to emulate browsing affordances.  The goal here is for the normally-invisible runtime to intrude on the user's experience of apps to the minimum extent necessary to protect users from the consequences of malicious or insecure apps.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #19)

> So I don't think we should be showing a progress indicator for these pages. 
> Nor show other UI that is designed to emulate browsing affordances.  The
> goal here is for the normally-invisible runtime to intrude on the user's
> experience of apps to the minimum extent necessary to protect users from the
> consequences of malicious or insecure apps.

That's OK. We can move forward on the basic styling and perhaps the globe
(In reply to Mark Finkle (:mfinkle) from comment #20)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #19)
> 
> > So I don't think we should be showing a progress indicator for these pages. 
> > Nor show other UI that is designed to emulate browsing affordances.  The
> > goal here is for the normally-invisible runtime to intrude on the user's
> > experience of apps to the minimum extent necessary to protect users from the
> > consequences of malicious or insecure apps.
> 
> That's OK. We can move forward on the basic styling and perhaps the globe

Yup, sounds good.
Attached patch Patch (obsolete) — Splinter Review
This move this to the top and styles it using an Android gradient.
Attachment #648112 - Attachment is obsolete: true
Attachment #648114 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Patch v2 - This changes this to just show the protocol + host
Attachment #651861 - Attachment is obsolete: true
Attachment #651870 - Flags: review?(mark.finkle)
Attached patch Patch v2 (obsolete) — Splinter Review
Grrr. forgot to qref.
Attachment #651870 - Attachment is obsolete: true
Attachment #651870 - Flags: review?(mark.finkle)
Attachment #651872 - Flags: review?(mark.finkle)
Comment on attachment 651872 [details] [diff] [review]
Patch v2

* Is the webapp_titlebar_bg.xml Ian-approved? :)
* You add webapp_titlebar_bg.xml into the Makefile twice, but only have one actual file (I think). Is that right?
* Do we need to import AnimationUtils? I don't see it explicitly used.
* SharedPreferences prefs does not seem to be used
* Getting the "app" using the index created from a string seems weak. Do we have any other ways to look it up?
Attachment #651872 - Flags: review?(mark.finkle) → review-
Status: NEW → ASSIGNED
Attached patch Patch v3Splinter Review
Updated patch. I added a "getIndex" function that's defined for WebApp# in the fragment. Good idea. I'll attach a screenshot too and ping ibarlow for approval.
Attachment #651872 - Attachment is obsolete: true
Attachment #654718 - Flags: review?(mark.finkle)
Attached image Screenshot
irc approval from ian with a change in the text color to 666666
Comment on attachment 654718 [details] [diff] [review]
Patch v3

>diff --git a/mobile/android/base/WebAppsFragment.java.frag b/mobile/android/base/WebAppsFragment.java.frag

>+    protected int getIndex() { return @APPNUM@; }

Nice

>diff --git a/mobile/android/base/resources/drawable/webapp_titlebar_bg.xml b/mobile/android/base/resources/drawable/webapp_titlebar_bg.xml

>\ No newline at end of file

nit: add a newline
Attachment #654718 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/8264b7d01a5a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Comment on attachment 654718 [details] [diff] [review]
Patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Webapps
User impact if declined: Security concerns with sites that take people to an unexpected off-origin url
Testing completed (on m-c etc.): Landed on mc 8/27
Risk to taking this patch (and alternatives if risky): Low risk. Webapps only
String or UUID changes made by this patch: None.
Attachment #654718 - Flags: approval-mozilla-aurora?
Status: RESOLVED → VERIFIED
Comment on attachment 654718 [details] [diff] [review]
Patch v3

Low risk, webapps only (targeted for 17) approving.
Attachment #654718 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.