launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/webservice-view-permission into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/webservice-view-permission/+merge/87997

Update webservice view_permission to LimitedView.

    Launchpad bug: https://bugs.launchpad.net/bugs/904296
    also fixes bug: https://bugs.launchpad.net/bugs/885524
    Pre-implementation: flacoste, wgrant

Users with limited view permission on private teams cannot access the
teams using the API. The webservice is hard coded to use launchpad.View
as the permission to check when working with collections. It is trivial
to change the webservice to use launchpad.LimitedView, but since most
objects  do not use it, additional work is needed to provide a default
LimitedView adapter.

--------------------------------------------------------------------

RULES

    * Change webservice/configuration.py to use launchpad.LimitedView.
    * Add a a LimitedView security checkers for Interface to act as the
      default checker. It should defer to the Launchpad.View checker.
    * ADDENDUM: The two email address checkers are checking account instead
      of users, which is incompatible with forwardCheckAuthenticated(). We
      removed personless accounts so we can update the checkers to use
      person instead of account.


QA

There are no exported collections for limited view, so this change cannot
be directly changed. We do want to test the change to email address to
ensure it's change is correct.

As a non-member of private-registry-test-team verify an AttributeError
is not raised with the team.confirmed_email_addresses attribute is accesses.
{{{
from launchpadlib.launchpad import Launchpad

def verify_team_visibility():
    lp = Launchpad.login_with(
        'testing', service_root='https://api.qastaging.launchpad.net',
        version='devel')
    team = lp.people['private-registry-test-team']
    print team.confirmed_email_addresses
}}}


LINT

    lib/lp/security.py
    lib/lp/services/webservice/configuration.py


TEST

    I ran the whole test suite, but all the failures in the spike were
    discovered in webservice/xx-person.txt.

    ./bin/test -vvc -t webservice/xx-person lp.registry.tests.test_doc


IMPLEMENTATION

Updated the webservice configuration to check if the user has LimitedView
on the collection attribute.
    lib/lp/services/webservice/configuration.py

Added A generic LimitedView checker for Interface. The non-authenticated
check uses check_permission because Lp often does not define permissions
on public attributes accessible to anonymous users. IHasMilestone is an
example where it is not possible to upcall to the View checker because
no checkers are defined. check_permission looks for all possible
permission adapters, not just the nearest adapter. The authenticated
check revealed a single test failure on email address. The email address
checkers check accounts instead of persons. We do not want to do this
anymore because personless-accounts have been impossible since Lp stopped
being an SSO provider. William already had a branch fixing this, so I
patched and cribbed his changes, which fixed the one failing test.
    lib/lp/security.py
-- 
https://code.launchpad.net/~sinzui/launchpad/webservice-view-permission/+merge/87997
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/webservice-view-permission into lp:launchpad.

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2011-12-30 06:14:56 +0000
+++ lib/lp/security.py	2012-01-09 23:41:31 +0000
@@ -147,7 +147,6 @@
 from lp.registry.interfaces.role import (
     IHasDrivers,
     IHasOwner,
-    IPersonRoles,
     )
 from lp.registry.interfaces.sourcepackage import ISourcePackage
 from lp.registry.interfaces.teammembership import ITeamMembership
@@ -165,6 +164,7 @@
     IOAuthAccessToken,
     IOAuthRequestToken,
     )
+from lp.services.webapp.authorization import check_permission
 from lp.services.openid.interfaces.openididentifier import IOpenIdIdentifier
 from lp.services.webapp.interfaces import ILaunchpadRoot
 from lp.services.worlddata.interfaces.country import ICountry
@@ -234,6 +234,30 @@
         return True
 
 
+class LimitedViewDeferredToView(AuthorizationBase):
+    """The default ruleset for the launchpad.LimitedView permission.
+
+    Few objects define LimitedView permission because it is only needed
+    in cases where a user may know something about a private object. The
+    default behaviour is to check if the user has launchpad.View permission;
+    private objects must define their own launchpad.LimitedView checker to
+    trully check the permission.
+    """
+    permission = 'launchpad.LimitedView'
+    usedfor = Interface
+
+    def checkUnauthenticated(self):
+        # The forward adapter approach is not reliable because the object
+        # might not define a permission checker for launchpad.View.
+        # eg. IHasMilestones is implicitly public to anonymous users,
+        #     there is no nearest adapter to call checkUnauthenticated.
+        return check_permission('launchpad.View', self.obj)
+
+    def checkAuthenticated(self, user):
+        return self.forwardCheckAuthenticated(
+            user, self.obj, 'launchpad.View')
+
+
 class AdminByAdminsTeam(AuthorizationBase):
     permission = 'launchpad.Admin'
     usedfor = Interface
@@ -2590,7 +2614,7 @@
         # Anonymous users can never see email addresses.
         return False
 
-    def checkAccountAuthenticated(self, account):
+    def checkAuthenticated(self, user):
         """Can the user see the details of this email address?
 
         If the email address' owner doesn't want his email addresses to be
@@ -2598,17 +2622,13 @@
         admins can see them.
         """
         # Always allow users to see their own email addresses.
-        if self.obj.account == account:
+        if self.obj.person == user:
             return True
 
         if not (self.obj.person is None or
                 self.obj.person.hide_email_addresses):
             return True
 
-        user = IPersonRoles(IPerson(account, None), None)
-        if user is None:
-            return False
-
         return (self.obj.person is not None and user.inTeam(self.obj.person)
                 or user.in_commercial_admin
                 or user.in_registry_experts
@@ -2619,12 +2639,11 @@
     permission = 'launchpad.Edit'
     usedfor = IEmailAddress
 
-    def checkAccountAuthenticated(self, account):
+    def checkAuthenticated(self, user):
         # Always allow users to see their own email addresses.
-        if self.obj.account == account:
+        if self.obj.person == user:
             return True
-        return super(EditEmailAddress, self).checkAccountAuthenticated(
-            account)
+        return super(EditEmailAddress, self).checkAuthenticated(user)
 
 
 class ViewGPGKey(AnonymousAuthorization):

=== modified file 'lib/lp/services/webservice/configuration.py'
--- lib/lp/services/webservice/configuration.py	2012-01-01 02:58:52 +0000
+++ lib/lp/services/webservice/configuration.py	2012-01-09 23:41:31 +0000
@@ -26,7 +26,7 @@
     active_versions = ["beta", "1.0", "devel"]
     last_version_with_mutator_named_operations = "beta"
     first_version_with_total_size_link = "devel"
-    view_permission = "launchpad.View"
+    view_permission = "launchpad.LimitedView"
     require_explicit_versions = True
     compensate_for_mod_compress_etag_modification = True