Closed Bug 1079941 Opened 6 years ago Closed 6 years ago

Don't allow calls from my blocked contacts

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
normal
Points:
1

Tracking

(firefox33 unaffected, firefox34+ verified, firefox35+ verified, firefox36+ verified)

VERIFIED FIXED
mozilla36
Iteration:
36.1
Tracking Status
firefox33 --- unaffected
firefox34 + verified
firefox35 + verified
firefox36 + verified
Blocking Flags:
backlog Fx34+

People

(Reporter: pauly, Assigned: mikedeboer)

References

Details

Attachments

(1 file, 2 obsolete files)

I shouldn't be allowed to call a person who has me in its contacts list as 'blocked'
Mike -- Can you look into this?
Flags: needinfo?(mdeboer)
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 35.3
Points: --- → 1
Flags: qe-verify+
Flags: needinfo?(mdeboer)
Flags: firefox-backlog+
Flags: needinfo?(mmucci)
Added to IT 35.3
Flags: needinfo?(mmucci)
Mark, with direct calls, we identify callers by email address, right?
So, with an incoming direct call, when do we get more info about who's calling?

I *think* we're not getting that info atm. I would really like to have at least the email address be part of `callData` in `MozLoopService#_startCall()` for type 'incoming'.
Flags: needinfo?(standard8)
OS: Windows 7 → All
Hardware: x86_64 → All
Depends on: 1079950
Flags: needinfo?(standard8)
Depends on: 1082533
Unassigning for now, due to too many dependencies.
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Iteration: 35.3 → ---
No longer depends on: 1082533
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 35.3
Attached patch Patch v1 (obsolete) — Splinter Review
Adam, what do you think?
Attachment #8504804 - Flags: feedback?(adam)
Comment on attachment 8504804 [details] [diff] [review]
Patch v1

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

Headed in the right direction. Please make sure we have good test coverage here, especially with regard to phone numbers.

::: browser/components/loop/LoopContacts.jsm
@@ +818,5 @@
> +    }
> +
> +    const checkForMatch = function(fieldValue) {
> +      if (typeof fieldValue == "string") {
> +        return fieldValue.startsWith(query.q);

For phone numbers, this needs to be "ends with" rather than "starts with". We also want to make sure we trim out anything that's not /[0-9\+]/ for phone numbers. Which is to say that "tel" might need special handling here. It's inelegant, but I don't think there's an easy way around it.

For non-phone-numbers, I think we want an exact match. If you want anything fancier than that, I suggest allowing q: to be a regex.

@@ +841,5 @@
> +        // Many fields are defined as Arrays.
> +        if (Array.isArray(matchWith)) {
> +          for (let fieldValue of matchWith) {
> +            if (checkForMatch(fieldValue)) {
> +              foundContacts.push(contact);

break;

(We don't want a contact to appear twice in the results on the off chance that two entries in the array match)

::: browser/components/loop/MozLoopService.jsm
@@ +801,5 @@
> +        null,
> +        // No title, let the page set that, to avoid flickering.
> +        "",
> +        "about:loopconversation#" + conversationType + "/" + callData.callId);
> +    }

I think you're missing a semicolon here?

@@ +806,5 @@
> +
> +    if (conversationType == "incoming" && ("callerId" in callData)) {
> +      LoopContacts.search({
> +        q: callData.callerId,
> +        field: callData.callerId.contains("@") ? "email" : "tel"

This should be a bit more sophisticated. We can get three formats here:

  ^\w+@\w+$ is an email address

  ^\+\d+$ is a phone number

  Anything else is a display name (from a link clicker) that should not be looked up in the database.

::: browser/components/loop/test/mochitest/browser_LoopContacts.js
@@ +424,5 @@
>  
>    LoopStorage.switchDatabase();
>    Assert.equal(LoopStorage.databaseName, "default", "The active partition should have changed");
>  
> +  let deferred = Promise.defer();

Promise.defer() is deprecated; see https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Deferred

@@ +469,5 @@
> +        deferred.resolve();
> +      });
> +    });
> +  });
> +  yield deferred.promise;

Please add a test searching for phone number, where, e.g., the query string is "5551234" and the entry to match is "+12145551234".
Attachment #8504804 - Flags: feedback?(adam) → feedback+
Iteration: 35.3 → 36.1
Attached patch Patch v2 (obsolete) — Splinter Review
All comments addressed, I believe...
Attachment #8504804 - Attachment is obsolete: true
Attachment #8505430 - Flags: review?(adam)
Attached patch Patch v2.1Splinter Review
Made the regex look a bit more the way you like it.
Attachment #8505430 - Attachment is obsolete: true
Attachment #8505430 - Flags: review?(adam)
Attachment #8505450 - Flags: review?(adam)
backlog: --- → Fx34+
Comment on attachment 8505450 [details] [diff] [review]
Patch v2.1

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

I have a number of suggestions for improvements that you may take or leave; however, you must check for error at in your call to getAll before landing.

r+ conditional on the error-check fix.

::: browser/components/loop/LoopContacts.jsm
@@ +819,5 @@
> +    }
> +    if (!("field" in query)) {
> +      query.field = "email";
> +    }
> +

For efficiency, I'd move the query.q.replace() for telephone numbers up here, so you're not repeating it each time you check a contact.

@@ +828,5 @@
> +        }
> +        return fieldValue == query.q;
> +      }
> +      if (typeof fieldValue == "number" || typeof fieldValue == "boolean") {
> +        return fieldValue === query.q;

I would think we want coercion of the query.q to a boolean if the field is boolean. Perhaps "==" is more appropriate here?

@@ +837,5 @@
> +      return false;
> +    };
> +
> +    let foundContacts = [];
> +    this.getAll((err, contacts) => {

Check for err here.

::: browser/components/loop/test/mochitest/browser_LoopContacts.js
@@ +451,5 @@
> +add_task(function* () {
> +  yield promiseLoadContacts();
> +
> +  yield new Promise(resolve => {
> +    LoopContacts.search({

This sequence would be much cleaner, shorter, and easier to read if you used LoopContacts.promise("search",...) rather than repeated callbacks.
Attachment #8505450 - Flags: review?(adam) → review+
Hi Mike -- Can you update the patch and land it today?  I'm hoping to get this into tonight's Nightly so QA can verify it tomorrow (against the dev server).
Flags: needinfo?(mdeboer)
Pushed with all comments addressed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/b0843f9cb541

Thanks for those, Adam!
Flags: needinfo?(mdeboer)
Pauly -- Can you verify this in tomorrow's Nightly if it gets merged in time?  You will need to use the Loop dev-server like you did for caller ID (bug 1020449).
Flags: needinfo?(paul.silaghi)
sorry guys this was backed out in https://hg.mozilla.org/mozilla-central/rev/7fac025c8cdb for  mochitest-bc test failures
[Tracking Requested - why for this release]:

We need to handling blocking calls from blocked contacts.  It's a privacy requirement.  (This also requires a Loop server change which is currently being deployed to production.)
Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fe9325c1befa

This got backed out yesterday. Need to reland.
https://hg.mozilla.org/mozilla-central/rev/e8a01f5feb55
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8505450 [details] [diff] [review]
Patch v2.1

Approval Request Comment
[Feature/regressing bug #]: Loop
[User impact if declined]: When a user blocks a contact, currently that contact is still able to call that user, because the logic to block incoming calls from blocked contacts was simply not implemented. This patch fixes that.
[Describe test coverage new/current, TBPL]: Landed on m-c, the changes in the LoopContacts class has plenty unit test coverage.
[Risks and why]: minor.
[String/UUID change made/needed]: n/a.
Attachment #8505450 - Flags: approval-mozilla-beta?
Attachment #8505450 - Flags: approval-mozilla-aurora?
Tested on Windows and Linux nightly 10/18 (using dev server, see bug 1020449), works
Comment on attachment 8505450 [details] [diff] [review]
Patch v2.1

Thanks for testing jesup. Approved for Aurora. If everything goes well with your testing on Aurora on Sunday, we'll get this uplifted for beta2.
Attachment #8505450 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tested and good on Aurora nightly build; windows and linux.
Comment on attachment 8505450 [details] [diff] [review]
Patch v2.1

Previously approved offline. Adding approval to the bug.
Attachment #8505450 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Randell Jesup [:jesup] from comment #22)
> Tested and good on Aurora nightly build; windows and linux.
Verified fixed 36.0a1 (2014-10-20) Win 7
Status: RESOLVED → VERIFIED
Flags: needinfo?(paul.silaghi)
Paul, can you please test this with the next beta?
Flags: needinfo?(paul.silaghi)
QA Contact: anthony.s.hughes → paul.silaghi
Just a reminder: we need server version 0.12.5 to verify that this works.  Services is deploying server version 0.12.5 to production today and tomorrow.  If it's not available when you do this verification, you would need to use the dev server (as described in bug 1020449:https://bugzilla.mozilla.org/show_bug.cgi?id=1020449#c32).   Basically, we need caller ID from the server in order to test call blocking.
Verified fixed FF 34b2 OS X 10.9.5
Flags: needinfo?(paul.silaghi)
Note to testers (on production): for a callee to receive the caller id, the caller must have last signed into FxA after the deployment time of 0.12.5 (Mon 27 Oct).
You need to log in before you can comment on or make changes to this bug.