launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06118
[Merge] lp:~wgrant/launchpad/no-checkAccountAuthenticated into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/no-checkAccountAuthenticated into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/no-checkAccountAuthenticated/+merge/89030
With personless accounts no longer being a thing, checkAccountAuthenticated provides no benefit over checkAuthenticated. This branch replaces all remaining checkAccountAuthenticated stuff with checkAuthenticated, bringing us closer to the abolition of Account ands its coconspirators.
--
https://code.launchpad.net/~wgrant/launchpad/no-checkAccountAuthenticated/+merge/89030
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/no-checkAccountAuthenticated into lp:launchpad.
=== modified file 'lib/lp/app/interfaces/security.py'
--- lib/lp/app/interfaces/security.py 2011-12-05 16:04:09 +0000
+++ lib/lp/app/interfaces/security.py 2012-01-18 13:29:09 +0000
@@ -26,10 +26,10 @@
the security policy's job of checking authorization of those pairs.
"""
- def checkAccountAuthenticated(account):
+ def checkAuthenticated(person):
"""Whether an authenticated user has `permission` on `obj`.
- Returns `True` if the account has that permission on the adapted
+ Returns `True` if the person has that permission on the adapted
object. Otherwise returns `False`.
If the check must be delegated to other objects, this method can
@@ -40,5 +40,5 @@
top-level authorization to be allowed, but this is dependent on the
security policy in force.
- :param account: The account that is authenticated.
+ :param person: The person that is authenticated.
"""
=== modified file 'lib/lp/app/security.py'
--- lib/lp/app/security.py 2011-12-05 11:57:59 +0000
+++ lib/lp/app/security.py 2012-01-18 13:29:09 +0000
@@ -21,8 +21,6 @@
from zope.security.permission import checkPermission
from lp.app.interfaces.security import IAuthorization
-from lp.registry.interfaces.person import IPerson
-from lp.registry.interfaces.role import IPersonRoles
class AuthorizationBase:
@@ -85,19 +83,6 @@
else:
return next_adapter.checkAuthenticated(user)
- def checkAccountAuthenticated(self, account):
- """See `IAuthorization.checkAccountAuthenticated`.
-
- :return: True or False.
- """
- # For backward compatibility, delegate to one of
- # checkAuthenticated() or checkUnauthenticated().
- person = IPerson(account, None)
- if person is None:
- return self.checkUnauthenticated()
- else:
- return self.checkAuthenticated(IPersonRoles(person))
-
class AnonymousAuthorization(AuthorizationBase):
"""Allow any authenticated and unauthenticated user access."""
=== modified file 'lib/lp/app/tests/test_security.py'
--- lib/lp/app/tests/test_security.py 2012-01-06 17:35:25 +0000
+++ lib/lp/app/tests/test_security.py 2012-01-18 13:29:09 +0000
@@ -17,6 +17,7 @@
AuthorizationBase,
DelegatedAuthorization,
)
+from lp.registry.interfaces.person import IPerson
from lp.testing import (
person_logged_in,
TestCase,
@@ -74,28 +75,17 @@
layer = ZopelessDatabaseLayer
- def test_checkAccountAuthenticated_for_full_fledged_account(self):
- # AuthorizationBase.checkAccountAuthenticated should delegate to
- # checkAuthenticated() when the given account can be adapted into an
- # IPerson.
+ def test_checkAuthenticated_for_full_fledged_account(self):
+ # AuthorizationBase.checkAuthenticated should delegate to
+ # checkAuthenticated() when the given account can be adapted
+ # into an IPerson.
full_fledged_account = self.factory.makePerson().account
adapter = FakeSecurityAdapter()
- adapter.checkAccountAuthenticated(full_fledged_account)
+ adapter.checkAuthenticated(IPerson(full_fledged_account))
self.assertVectorEqual(
(1, adapter.checkAuthenticated.call_count),
(0, adapter.checkUnauthenticated.call_count))
- def test_checkAccountAuthenticated_for_personless_account(self):
- # AuthorizationBase.checkAccountAuthenticated should delegate to
- # checkUnauthenticated() when the given account can't be adapted into
- # an IPerson.
- personless_account = self.factory.makeAccount('Test account')
- adapter = FakeSecurityAdapter()
- adapter.checkAccountAuthenticated(personless_account)
- self.assertVectorEqual(
- (0, adapter.checkAuthenticated.call_count),
- (1, adapter.checkUnauthenticated.call_count))
-
def test_forwardCheckAuthenticated_object_changes(self):
# Requesting a check for the same permission on a different object.
permission = self.factory.getUniqueString()
=== modified file 'lib/lp/code/model/tests/test_branchvisibility.py'
--- lib/lp/code/model/tests/test_branchvisibility.py 2012-01-01 02:58:52 +0000
+++ lib/lp/code/model/tests/test_branchvisibility.py 2012-01-18 13:29:09 +0000
@@ -26,6 +26,7 @@
CodeReviewNotificationLevel,
)
from lp.code.interfaces.branch import IBranchSet
+from lp.registry.interfaces.role import IPersonRoles
from lp.security import AccessBranch
from lp.services.webapp.authorization import (
check_permission,
@@ -69,7 +70,7 @@
self.assertTrue(access.checkUnauthenticated())
person = self.factory.makePerson()
self.assertTrue(
- access.checkAccountAuthenticated(person.account))
+ access.checkAuthenticated(IPersonRoles(person)))
def test_visible_to_owner(self):
# The owners of a branch always have visibility of their own branches.
@@ -82,7 +83,7 @@
access = AccessBranch(naked_branch)
self.assertFalse(access.checkUnauthenticated())
self.assertTrue(
- access.checkAccountAuthenticated(owner.account))
+ access.checkAuthenticated(IPersonRoles(owner)))
self.assertFalse(check_permission('launchpad.View', branch))
def test_visible_to_administrator(self):
@@ -92,7 +93,7 @@
naked_branch = removeSecurityProxy(branch)
admin = getUtility(ILaunchpadCelebrities).admin.teamowner
access = AccessBranch(naked_branch)
- self.assertTrue(access.checkAccountAuthenticated(admin.account))
+ self.assertTrue(access.checkAuthenticated(IPersonRoles(admin)))
def test_visible_to_subscribers(self):
# Branches that are not public are viewable by members of the
@@ -105,7 +106,7 @@
# Not visible to an unsubscribed person.
access = AccessBranch(naked_branch)
- self.assertFalse(access.checkAccountAuthenticated(person.account))
+ self.assertFalse(access.checkAuthenticated(IPersonRoles(person)))
# Subscribing the team to the branch will allow access to the branch.
naked_branch.subscribe(
@@ -113,7 +114,7 @@
BranchSubscriptionNotificationLevel.NOEMAIL,
BranchSubscriptionDiffSize.NODIFF,
CodeReviewNotificationLevel.NOEMAIL, teamowner)
- self.assertTrue(access.checkAccountAuthenticated(person.account))
+ self.assertTrue(access.checkAuthenticated(IPersonRoles(person)))
def test_branchset_restricted_queries(self):
# All of the BranchSet queries that are used to populate user viewable
=== modified file 'lib/lp/registry/doc/hasowner-authorization.txt'
--- lib/lp/registry/doc/hasowner-authorization.txt 2011-05-27 18:27:59 +0000
+++ lib/lp/registry/doc/hasowner-authorization.txt 2012-01-18 13:29:09 +0000
@@ -6,7 +6,10 @@
# First we define a class which only provides IHasOwner.
>>> from zope.interface import implements
>>> from lp.registry.interfaces.person import IPersonSet
- >>> from lp.registry.interfaces.role import IHasOwner
+ >>> from lp.registry.interfaces.role import (
+ ... IHasOwner,
+ ... IPersonRoles,
+ ... )
>>> salgado = getUtility(IPersonSet).getByName('salgado')
>>> class FooObject:
... implements(IHasOwner)
@@ -18,7 +21,7 @@
>>> from zope.component import queryAdapter
>>> from lp.app.interfaces.security import IAuthorization
>>> authorization = queryAdapter(foo, IAuthorization, 'launchpad.Edit')
- >>> print authorization.checkAccountAuthenticated(salgado.account)
+ >>> print authorization.checkAuthenticated(IPersonRoles(salgado))
True
So can a member of the Launchpad admins team.
@@ -27,7 +30,7 @@
>>> admins = getUtility(IPersonSet).getByName('admins')
>>> print mark.inTeam(admins)
True
- >>> print authorization.checkAccountAuthenticated(mark.account)
+ >>> print authorization.checkAuthenticated(IPersonRoles(mark))
True
But someone who's not salgado nor a member of the admins team won't be
@@ -36,5 +39,5 @@
>>> sample_person = getUtility(IPersonSet).getByName('name12')
>>> print sample_person.inTeam(admins)
False
- >>> print authorization.checkAccountAuthenticated(sample_person)
+ >>> print authorization.checkAuthenticated(IPersonRoles(sample_person))
False
=== modified file 'lib/lp/security.py'
--- lib/lp/security.py 2012-01-16 00:10:41 +0000
+++ lib/lp/security.py 2012-01-18 13:29:09 +0000
@@ -340,14 +340,8 @@
permission = 'launchpad.Edit'
usedfor = IAccount
- def checkAccountAuthenticated(self, account):
- if account == self.obj:
- return True
- return super(
- EditAccountBySelfOrAdmin, self).checkAccountAuthenticated(account)
-
def checkAuthenticated(self, user):
- return user.in_admin
+ return user.in_admin or user.person.accountID == self.obj.id
class ViewAccount(EditAccountBySelfOrAdmin):
@@ -358,12 +352,8 @@
permission = 'launchpad.View'
usedfor = IOpenIdIdentifier
- def checkAccountAuthenticated(self, account):
- if account == self.obj.account:
- return True
- return super(
- ViewOpenIdIdentifierBySelfOrAdmin,
- self).checkAccountAuthenticated(account)
+ def checkAuthenticated(self, user):
+ return user.in_admin or user.person.accountID == self.obj.accountID
class SpecialAccount(EditAccountBySelfOrAdmin):
@@ -371,7 +361,9 @@
def checkAuthenticated(self, user):
"""Extend permission to registry experts."""
- return user.in_admin or user.in_registry_experts
+ return (
+ super(SpecialAccount, self).checkAuthenticated(user)
+ or user.in_registry_experts)
class ModerateAccountByRegistryExpert(AuthorizationBase):
@@ -768,7 +760,7 @@
The admin team can also edit any Person.
"""
- return self.obj.id == user.person.id or user.in_admin
+ return self.obj.id == user.id or user.in_admin
class EditTranslationsPersonByPerson(AuthorizationBase):
@@ -913,7 +905,8 @@
# Do comparison by ids because they may be needed for comparison
# to membership.team.ids later.
- user_teams = [team.id for team in user.person.teams_participated_in]
+ user_teams = [
+ team.id for team in user.person.teams_participated_in]
super_teams = [team.id for team in self.obj.super_teams]
intersection_teams = set(user_teams) & set(super_teams)
=== modified file 'lib/lp/services/webapp/authorization.py'
--- lib/lp/services/webapp/authorization.py 2011-12-30 08:13:14 +0000
+++ lib/lp/services/webapp/authorization.py 2012-01-18 13:29:09 +0000
@@ -36,6 +36,7 @@
)
from lp.app.interfaces.security import IAuthorization
+from lp.registry.interfaces.role import IPersonRoles
from lp.services.database.readonly import is_read_only
from lp.services.database.sqlbase import block_implicit_flushes
from lp.services.privacy.interfaces import IObjectPrivacy
@@ -254,12 +255,11 @@
yield cache[objecttoauthorize][permission]
return
- # Create a check_auth function to call checkAccountAuthenticated or
+ # Create a check_auth function to call checkAuthenticated or
# checkUnauthenticated as appropriate.
if ILaunchpadPrincipal.providedBy(principal):
- account = principal.account
check_auth = lambda authorization: (
- authorization.checkAccountAuthenticated(account))
+ authorization.checkAuthenticated(IPersonRoles(principal.person)))
else:
check_auth = lambda authorization: (
authorization.checkUnauthenticated())
=== modified file 'lib/lp/services/webapp/tests/test_authorization.py'
--- lib/lp/services/webapp/tests/test_authorization.py 2011-12-30 08:03:42 +0000
+++ lib/lp/services/webapp/tests/test_authorization.py 2012-01-18 13:29:09 +0000
@@ -22,7 +22,8 @@
from lp.app.interfaces.security import IAuthorization
from lp.app.security import AuthorizationBase
-from lp.services.identity.interfaces.account import IAccount
+from lp.registry.interfaces.person import IPerson
+from lp.registry.interfaces.role import IPersonRoles
from lp.services.privacy.interfaces import IObjectPrivacy
from lp.services.webapp.authentication import LaunchpadPrincipal
from lp.services.webapp.authorization import (
@@ -62,7 +63,7 @@
def checkUnauthenticated(self):
return True
- def checkAccountAuthenticated(self, account):
+ def checkAuthenticated(self, user):
return True
@@ -72,7 +73,7 @@
def checkUnauthenticated(self):
return False
- def checkAccountAuthenticated(self, account):
+ def checkAuthenticated(self, user):
return False
@@ -82,7 +83,7 @@
def checkUnauthenticated(self):
raise NotImplementedError()
- def checkAccountAuthenticated(self, account):
+ def checkAuthenticated(self, user):
raise NotImplementedError()
@@ -105,13 +106,13 @@
self.calls.append('checkUnauthenticated')
return False
- def checkAccountAuthenticated(self, account):
- """See `IAuthorization.checkAccountAuthenticated`.
+ def checkAuthenticated(self, user):
+ """See `IAuthorization.checkAuthenticated`.
We record the call and then return False, arbitrarily chosen, to keep
the policy from complaining.
"""
- self.calls.append(('checkAccountAuthenticated', account))
+ self.calls.append(('checkAuthenticated', user))
return False
@@ -159,7 +160,7 @@
yield self.object_one, self.permission
yield self.object_two, self.permission
- def checkAccountAuthenticated(self, account):
+ def checkAuthenticated(self, user):
yield self.object_one, self.permission
yield self.object_two, self.permission
@@ -170,15 +171,15 @@
access_level = 'read'
-class FakeAccount:
- """A minimal object to represent an account."""
- implements(IAccount)
+class FakePerson:
+ """A minimal object to represent a person."""
+ implements(IPerson, IPersonRoles)
class FakeLaunchpadPrincipal:
"""A minimal principal implementing `ILaunchpadPrincipal`"""
implements(ILaunchpadPrincipal)
- account = FakeAccount()
+ person = FakePerson()
scope = None
access_level = ''
@@ -302,12 +303,12 @@
# calls the checker.
policy.checkPermission(permission, obj)
self.assertEqual(
- [('checkAccountAuthenticated', principal.account)],
+ [('checkAuthenticated', principal.person)],
checker_factory.calls)
# A subsequent identical call does not call the checker.
policy.checkPermission(permission, obj)
self.assertEqual(
- [('checkAccountAuthenticated', principal.account)],
+ [('checkAuthenticated', principal.person)],
checker_factory.calls)
def test_checkPermission_clearSecurityPolicyCache_resets_cache(self):
@@ -347,8 +348,8 @@
# rather than finding a value in the cache.
policy.checkPermission(permission, obj)
self.assertEqual(
- ['checkUnauthenticated', ('checkAccountAuthenticated',
- principal.account)],
+ ['checkUnauthenticated', ('checkAuthenticated',
+ principal.person)],
checker_factory.calls)
def test_checkPermission_commit_clears_cache(self):
