All users were logged out of Bugzilla on October 13th, 2018

Encapsulate identity data/security mode behind a type-safe API

RESOLVED FIXED in Firefox 29

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

unspecified
Firefox 29
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Instead of passing JSON and strings around.
(Assignee)

Comment 1

5 years ago
Created attachment 8341029 [details] [diff] [review]
Encapsulate identity data/security mode behind a type-safe API (r=margaret)
(Assignee)

Comment 2

5 years ago
Comment on attachment 8341029 [details] [diff] [review]
Encapsulate identity data/security mode behind a type-safe API (r=margaret)

Part of some of the major refactorings being worked on in bug 942862, bug 944533, and bug 943915.
Attachment #8341029 - Flags: review?(margaret.leibovic)

Comment 3

5 years ago
Comment on attachment 8341029 [details] [diff] [review]
Encapsulate identity data/security mode behind a type-safe API (r=margaret)

Review of attachment 8341029 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a good improvement, but I think this patch breaks the loaded mixed content notification. I tried applying this patch locally to test, but it didn't apply.

::: mobile/android/base/SiteIdentity.java
@@ +2,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko;

I wonder if this should go in the toolbar package, since that's where SiteIdentityPopup is.

::: mobile/android/base/toolbar/SiteIdentityPopup.java
@@ -124,5 @@
> -            // Hide the identity data if there isn't valid site identity data.
> -            // Set some top padding on the popup content to create a of light blue
> -            // between the popup arrow and the mixed content notification.
> -            mContent.setPadding(0, (int) mResources.getDimension(R.dimen.identity_padding_top), 0, 0);
> -            mIdentity.setVisibility(View.GONE);

This logic was added to handle pages that have loaded mixed content. In this case, we don't want to show the site identity because we've loaded unencrypted content, but we still want to show the popup so that we can include a notification that allows the user to re-enable mixed content blocking. See the patch where this logic was added:
http://hg.mozilla.org/mozilla-central/diff/17523d827afa/mobile/android/base/SiteIdentityPopup.java

In retrospect, this seems pretty hacky, and we should just be checking the security mode in here to do this.

We need to make sure this still works. You can test by visiting this page, then disabling mixed content blocking, then tapping on the yellow triangle to open the popup:
https://people.mozilla.org/~tvyas/mixedcontent.html

@@ +133,5 @@
>       * @param identityData A JSONObject that holds the current tab's identity data.
>       */
> +    void updateIdentity(SiteIdentity siteIdentity) {
> +        final SecurityMode mode = siteIdentity.getSecurityMode();
> +        if (mode == SecurityMode.UNKNOWN) {

We now never call updateIdentity in this case, so you could get rid of this check.
Attachment #8341029 - Flags: review?(margaret.leibovic) → review-
(Assignee)

Comment 4

5 years ago
(In reply to :Margaret Leibovic from comment #3)
> Comment on attachment 8341029 [details] [diff] [review]
> Encapsulate identity data/security mode behind a type-safe API (r=margaret)
> 
> Review of attachment 8341029 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like a good improvement, but I think this patch breaks the loaded
> mixed content notification. I tried applying this patch locally to test, but
> it didn't apply.

This is part of a big patch queue I have. You'd need to apply this patch on top of the ones from bug 943915. 

> ::: mobile/android/base/SiteIdentity.java
> @@ +2,5 @@
> > + * This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +package org.mozilla.gecko;
> 
> I wonder if this should go in the toolbar package, since that's where
> SiteIdentityPopup is.

I considered doing that but SiteIdentity is something meant to be owned by the Tab. It just happens to be used by SiteIdentityPopup now but it could be used in a different type of UI in the future.

> ::: mobile/android/base/toolbar/SiteIdentityPopup.java
> @@ -124,5 @@
> > -            // Hide the identity data if there isn't valid site identity data.
> > -            // Set some top padding on the popup content to create a of light blue
> > -            // between the popup arrow and the mixed content notification.
> > -            mContent.setPadding(0, (int) mResources.getDimension(R.dimen.identity_padding_top), 0, 0);
> > -            mIdentity.setVisibility(View.GONE);
> 
> This logic was added to handle pages that have loaded mixed content. In this
> case, we don't want to show the site identity because we've loaded
> unencrypted content, but we still want to show the popup so that we can
> include a notification that allows the user to re-enable mixed content
> blocking. See the patch where this logic was added:
> http://hg.mozilla.org/mozilla-central/diff/17523d827afa/mobile/android/base/
> SiteIdentityPopup.java
> 
> In retrospect, this seems pretty hacky, and we should just be checking the
> security mode in here to do this.

Oh, that wasn't obvious at all :-) I'll change the patch to handle this case based on the security mode instead.

> We need to make sure this still works. You can test by visiting this page,
> then disabling mixed content blocking, then tapping on the yellow triangle
> to open the popup:
> https://people.mozilla.org/~tvyas/mixedcontent.html

Thanks, I'll double-check this case works before submitting the new patch.

> @@ +133,5 @@
> >       * @param identityData A JSONObject that holds the current tab's identity data.
> >       */
> > +    void updateIdentity(SiteIdentity siteIdentity) {
> > +        final SecurityMode mode = siteIdentity.getSecurityMode();
> > +        if (mode == SecurityMode.UNKNOWN) {
> 
> We now never call updateIdentity in this case, so you could get rid of this
> check.

Yeah, I've changing the SiteIdentityPopup API a bit in bug 944533 (which covers your point here).
(Assignee)

Comment 5

5 years ago
Created attachment 8341690 [details] [diff] [review]
Encapsulate identity data/security mode behind a type-safe API (r=margaret)
(Assignee)

Updated

5 years ago
Attachment #8341029 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Comment on attachment 8341690 [details] [diff] [review]
Encapsulate identity data/security mode behind a type-safe API (r=margaret)

I confirm this works with the mixed content blocking as expected.
Attachment #8341690 - Flags: review?(margaret.leibovic)

Comment 7

5 years ago
Comment on attachment 8341690 [details] [diff] [review]
Encapsulate identity data/security mode behind a type-safe API (r=margaret)

Review of attachment 8341690 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks. Sorry about that confusing bit, but at least it's much clearer now!
Attachment #8341690 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/e61d36bd385f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29

Updated

5 years ago
Depends on: 951605

Updated

5 years ago
Depends on: 966866
You need to log in before you can comment on or make changes to this bug.