:Paolo Amadini Assignee
	Description • 11 years ago

The modifyLogin method should make the same validations as addLogin with regard to the properties of the login after modification.

	:Paolo Amadini Assignee
	Comment 1 • 11 years ago

This patch solves the issue by replicating the existing addLogin validation code.

I'll probably refactor all the validation code to be shared between the two back-ends as part of the work for bug 853549, but for now I've only modified the behavior with minimal code changes.

Doing this simpler change in a separate bug instead of all together in bug 853549 will help with bisection in case the new validation causes unexpected issues.

In fact, ideally we should get to a point where bug 853549 is able to change the back-end without affecting any of the existing tests.

	Justin Dolske [:Dolske]
	Comment 2 • 11 years ago

Comment on attachment 8355855 [details] [diff] [review]
The patch

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

::: toolkit/components/passwordmgr/storage-mozStorage.js
@@ +423,5 @@
>          } else {
>              throw "newLoginData needs an expected interface!";
>          }
>  
> +        // Sanity check the login

This should really be in nsLoginManager, where the addLogin check is.

Please move this there, and make addLogin() and modifyLogin() call a shared function. (Just make the exceptions generic, "Can't have a...").

The shared check will be a little ugly to deal with the case of modifyLogin being able to take a property bag. But should be simple enough to just clone oldLogin and copy over the 5 properties we care about. eg

function modifyLogin() {
  ...
  if (newLogin instanceof propbag)
    commonCheck(oldLogin, propBag);
  else
    commonCheck(newLogin);
  ...
}

function commonCheck(login, changes) {
  if (changes) {
    login = login.clone();

    for (let prop in ["hostname", "username", "password", ...];
      if (_bagHasProperty(prop))
        login[prop] = changes.getProperty(prop);
  }

  // ... existing checks ...
}

(or something like that, pick your favorite flavor)

@@ +458,5 @@
> +                                         newLogin.httpRealm);
> +
> +            if (logins.some(login => newLogin.matches(login, true)))
> +                throw "This login already exists.";
> +        }

I'm a little wary of this; there's a bunch of edge cases that might be hard to get right. I'll think about it later, worst case we just punt on doing this check. It's not been a problem to date...

	:Paolo Amadini Assignee
	Comment 3 • 11 years ago

Comment on attachment 8355855 [details] [diff] [review]
The patch

(In reply to Justin Dolske [:Dolske] from comment #2)
> This should really be in nsLoginManager, where the addLogin check is.

As noted earlier and as we discussed, work is already underway for sharing the validation code between both back-ends in bug 853549.

I'd like to use this bug to change the tests and the expectations on the API. I don't think there is much value in spending time to figure out a sharing scheme here when the code will be rewritten anyways in bug 853549.

If you prefer to wait until I finish the patch in bug 853549 before reviewing this, just tell me!

> > +            if (logins.some(login => newLogin.matches(login, true)))
> > +                throw "This login already exists.";
> > +        }
> 
> I'm a little wary of this; there's a bunch of edge cases that might be hard
> to get right. I'll think about it later, worst case we just punt on doing
> this check. It's not been a problem to date...

This is why I filed a separate bug, so that in case an add-on or some other consumer is regressed in an edge case, the regression range will point to this bug, making it clear what happened. (If you have specific real-world cases in mind that this would break, feel free to point them out.)

	Tracy Walker [:tracy]
	Comment 4 • 11 years ago

What are the steps for QA to verify this?

	:Paolo Amadini Assignee
	Comment 5 • 11 years ago

(In reply to [:tracy] Tracy Walker - QA Mentor from comment #4)
> What are the steps for QA to verify this?

This change is covered by the modification of regression tests. It is unlikely that this change will affect add-ons in the wild, though there is a potential in case invalid data or keys were provided by an add-on.

	Marco Mucci [:MarcoM]
	Comment 6 • 11 years ago

Project Deferred - Being Removed From Desktop Backlog.

	Justin Dolske [:Dolske]
	Comment 7 • 10 years ago

(In reply to :Paolo Amadini from comment #3)

> (In reply to Justin Dolske [:Dolske] from comment #2)
> > This should really be in nsLoginManager, where the addLogin check is.
> 
> As noted earlier and as we discussed, work is already underway for sharing
> the validation code between both back-ends in bug 853549.
> 
> I'd like to use this bug to change the tests and the expectations on the
> API. I don't think there is much value in spending time to figure out a
> sharing scheme here when the code will be rewritten anyways in bug 853549.

Alas I've forgotten the details of our jet-lagged Paris discussion. :( It still seems to me that this verification code belongs in nsLoginManager.js, where the other similar code is already at. The patch in bug 853549 doesn't seem to affect this stuff, other than duplicating it into storage-json.js (i.e., these checks become spread out over 3 separate files).

What was your argument for doing it this way?


> > > +            if (logins.some(login => newLogin.matches(login, true)))
> > > +                throw "This login already exists.";
> > > +        }
> > 
> > I'm a little wary of this; there's a bunch of edge cases that might be hard
> > to get right. I'll think about it later, worst case we just punt on doing
> > this check. It's not been a problem to date...
> 
> This is why I filed a separate bug, so that in case an add-on or some other
> consumer is regressed in an edge case, the regression range will point to
> this bug, making it clear what happened. (If you have specific real-world
> cases in mind that this would break, feel free to point them out.)

It's hard to pin down exactly, just because we've had (and might still have?) some subtle bugs that could result in ending up with duplicate or near-duplicate logins. The specific scenario that I was wary of is being unable to change your password on a site, at least via the usual UI, due to this code thinking it becomes a duplicate.

But I guess that can't happen, because the additional-context for this patch shows it wrapped in another check:

+        if (!newLogin.matches(oldLogin, true)) {
+            let logins = this.findLogins({}, newLogin.hostname,
+                                         newLogin.formSubmitURL,
+                                         newLogin.httpRealm);
+
+            if (logins.some(login => newLogin.matches(login, true)))
+                throw "This login already exists.";
+        }

So what I missed before is that the first .matches() check will only catch (certain) changes _other_ than the password, which Firefox doesn't do. And I don't _think_ Sync can trigger this. So this should be fairly safe to do.

	:Paolo Amadini Assignee
	Comment 8 • 10 years ago

(In reply to Justin Dolske [:Dolske] from comment #7)
> Alas I've forgotten the details of our jet-lagged Paris discussion. :( It
> still seems to me that this verification code belongs in nsLoginManager.js,
> where the other similar code is already at. The patch in bug 853549 doesn't
> seem to affect this stuff, other than duplicating it into storage-json.js
> (i.e., these checks become spread out over 3 separate files).

I was working on updating the patch in bug 853549 to move all this shared code to a single place, namely a LoginHelper.jsm module. This is still in progress, but I've just attached an updated patch there.

The choice of shared JSM over putting all the validations at the outer level was related to implementation details, like the exact order of the checks and some nsILoginManagerStorage interface requirements, if I still remember correctly. We can figure out if this is the right approach to consolidation in that bug.

> What was your argument for doing it this way?

Using this bug to update the logic, instead of doing it all at once ij the other one, is related to easier bisection in case of regressions. The other bug does not change the expected behavior. I'm fine if you want to review the final patch in bug 853549 first.

	:Paolo Amadini Assignee
	Comment 9 • 10 years ago

Comment on attachment 8355855 [details] [diff] [review]
The patch

Can we now land this in the context of the changes in bug 853549?

	Justin Dolske [:Dolske]
	Comment 10 • 10 years ago

(In reply to :Paolo Amadini from comment #9)

> Can we now land this in the context of the changes in bug 853549?

Is the intent to move all the verification code into LoginHelper? (And, I guess, have nsLoginManager not really do any of it, but instead defer the work to the storage module?) Can we just do that, instead of this odd half-measure? As far as I can tell this patch just makes an already-confusing situation worse, for the reasons I've already listed.

	:Paolo Amadini Assignee
	Comment 11 • 10 years ago

(In reply to Justin Dolske [:Dolske] from comment #10)
> Is the intent to move all the verification code into LoginHelper? (And, I
> guess, have nsLoginManager not really do any of it, but instead defer the
> work to the storage module?) Can we just do that, instead of this odd
> half-measure? As far as I can tell this patch just makes an
> already-confusing situation worse, for the reasons I've already listed.

I don't understand what you're asking. Are you saying the end state in bug 853549 is not satisfactory?

	Justin Dolske [:Dolske]
	Comment 12 • 10 years ago

I think we're talking past each other.

I sat down and poked through this all again, and now understand the issue I wasn't clear on. Bug 853549 builds on top of this patch. And so while this intermediate state is a bit unclean, 853549 fixes it by having both storage-mozStorage and storage-json use LoginHelper.checkLoginValues (i.e., it essentially reverts this patch and replaces it with that LoginHelper call).

	:Paolo Amadini Assignee
	Comment 13 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/931da6f0b522

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/931da6f0b522