launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/anonymous-api-access-emails-681815 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #681815 registered emails for all users available via anonymous api
  https://bugs.launchpad.net/bugs/681815


Summary
=======
The webservice currently has a hole through which a person can anonymously collect email addresses. This is because the security check employed for it allows email addresses to   be returned if they're attached to a user. To be consistent with the web interface, you shouldn't get email addresses unless you're logged in.

This branch changes the unauthenticated check so that it never allows emails to be returned.

Proposed fix 
============
Change the ViewEmailAddress checkUnauthenticated to always return false; there is never a circumstance in which the API should anonymously return emails.

Preimplementation talk
======================
Spoke with Curtis Hovey.

Implementation details
======================
As in Proposed.

Tests
=====
bin/test -t registry.*webservice.*xx-person\.txt

Demo & QA
=========
There are several scripts attached to the bug that should pass if this whole is patched.


Lint
====
make lint output:

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/security.py
  lib/lp/registry/stories/webservice/xx-person.txt

./lib/canonical/launchpad/security.py
     733: E302 expected 2 blank lines, found 1
    1266: E302 expected 2 blank lines, found 1
    1476: E302 expected 2 blank lines, found 1
./lib/lp/registry/stories/webservice/xx-person.txt
      14: want exceeds 78 characters.
      16: want exceeds 78 characters.
      20: want exceeds 78 characters.
      38: want exceeds 78 characters.
      41: want exceeds 78 characters.
      44: want exceeds 78 characters.
      61: want exceeds 78 characters.
      63: want exceeds 78 characters.
      67: want exceeds 78 characters.
      69: want exceeds 78 characters.
      72: want exceeds 78 characters.
      86: want exceeds 78 characters.
      87: want exceeds 78 characters.
      90: want exceeds 78 characters.
      93: want exceeds 78 characters.
      95: want exceeds 78 characters.
     161: source exceeds 78 characters.
     162: source exceeds 78 characters.
     163: want has trailing whitespace.
     191: narrative exceeds 78 characters.
     266: source exceeds 78 characters.
     349: narrative has trailing whitespace.
     359: narrative has trailing whitespace.
     482: source exceeds 78 characters.
     506: source exceeds 78 characters.
     569: source exceeds 78 characters.
     822: source has bad indentation.

The blank line error in security.py is because of lint's issues with comments and blanklines.

The wants over 78 lines don't appear to be something that can easily be moved about; they are arguably more readable in their current form.

-- 
https://code.launchpad.net/~jcsackett/launchpad/anonymous-api-access-emails-681815/+merge/42309
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/anonymous-api-access-emails-681815 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2010-11-26 18:12:16 +0000
+++ lib/canonical/launchpad/security.py	2010-11-30 21:44:54 +0000
@@ -2394,11 +2394,8 @@
 
     def checkUnauthenticated(self):
         """See `AuthorizationBase`."""
-        # Email addresses without an associated Person cannot be seen by
-        # anonymous users.
-        if self.obj.person is None:
-            return False
-        return not self.obj.person.hide_email_addresses
+        # Anonymous users can never see email addresses.
+        return False
 
     def checkAccountAuthenticated(self, account):
         """Can the user see the details of this email address?

=== modified file 'lib/lp/registry/stories/webservice/xx-person.txt'
--- lib/lp/registry/stories/webservice/xx-person.txt	2010-11-22 15:30:05 +0000
+++ lib/lp/registry/stories/webservice/xx-person.txt	2010-11-30 21:44:54 +0000
@@ -1,4 +1,5 @@
-= People and teams =
+People and teams
+================
 
 Since we use a single class (Person) to represent a person or a team,
 representations of people and teams are supposed to have nearly the
@@ -113,13 +114,15 @@
     []
 
 
-== Links to related things ==
+Links to related things
+-----------------------
 
 As seen above, many attributes of a person are actually links to other
 things (or collections).
 
 
-=== Email addresses ===
+Email addresses
+...............
 
 Apart from the link to the preferred email, there is a link to the
 collection of other confirmed email addresses of that person/team.
@@ -151,7 +154,17 @@
     HTTP/1.1 404 Not Found
     ...
 
-== SSH keys ===
+Emails are protected over the webservice--an anonymous connection cannot get
+email entries.
+
+    >>> person = webservice.get("/~salgado").jsonBody()
+    >>> confirmed_email_addresses = person['confirmed_email_addresses_collection_link']
+    >>> print anon_webservice.get(confirmed_email_addresses).jsonBody()['total_size']
+    0 
+
+
+SSH keys
+........
 
 People have SSH keys which we can manipulate over the API.
 
@@ -192,7 +205,8 @@
     resource_type_link: u'http://.../#ssh_key'
     self_link: u'http://.../~ssh-user/+ssh-keys/...'
 
-=== GPG keys ===
+GPG keys
+........
 
 People have GPG keys which we can manipulate over the API.
 
@@ -230,7 +244,8 @@
     self_link: u'http://.../~name12/+gpg-keys/...'
 
 
-=== Team memberships ===
+Team memberships
+................
 
 A person is linked to their team memberships.
 
@@ -357,7 +372,8 @@
     ...
 
 
-=== Members ===
+Members
+.......
 
 A list of team memberships is distinct from a list of a team's
 members. Members are people; memberships are TeamMemberships. You've
@@ -427,7 +443,8 @@
     http://.../~karl
 
 
-=== Sub-teams and super-teams ===
+Sub-teams and super-teams
+.........................
 
 Teams can be members of other teams, and sometimes it's useful to know
 which teams are members of any given team as well as the ones it is a
@@ -442,7 +459,8 @@
     http://.../~guadamen
 
 
-=== Wiki names ===
+Wiki names
+..........
 
 All wiki names associated to a person/team are also linked to that
 person/team.
@@ -503,7 +521,8 @@
     Only URIs with the following schemes may be used: http, https
 
 
-=== Jabber IDs ===
+Jabber IDs
+..........
 
 Jabber IDs of a person are also linked.
 
@@ -534,7 +553,8 @@
     ...
 
 
-=== IRC nicknames ===
+IRC nicknames
+.............
 
 The same for IRC nicknames
 
@@ -568,7 +588,8 @@
     ...
 
 
-=== PPAs ===
+PPAs
+....
 
 We can get to the person's default PPA via the 'archive' property:
 
@@ -634,7 +655,8 @@
     [u'http://mark:testtoken@xxxxxxxxxxxxxxxxxxxxxxxxx/mark/p3a/ubuntu']
 
 
-== Custom operations ==
+Custom operations
+-----------------
 
 IPerson supports a bunch of operations.
 
@@ -669,7 +691,8 @@
     ---
 
 
-=== Team membership operations ===
+Team membership operations
+..........................
 
 Joining and leaving teams:
 
@@ -791,7 +814,8 @@
     # http://.../~mailing-list-experts
 
 
-== Restrictions ==
+Restrictions
+------------
 
 A team can't be its own owner.