launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #30497
[Merge] ~ines-almeida/launchpad:project-tokens/add-new-access-token-tables-to-view into launchpad:master
Ines Almeida has proposed merging ~ines-almeida/launchpad:project-tokens/add-new-access-token-tables-to-view into launchpad:master.
Commit message:
Add 'project tokens' table to git repo access tokens view
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/452109
This is a small update but I think it will improve the UX for these tokens, since we will be able to have a better view of the tokens that grant access to a repository within that repository "+access-tokens" view.
Regarding the CSS change, it is quite small and in line with Vanilla. It improves slightly the tables look. You can see a diff in the Jira ticket: https://warthogs.atlassian.net/browse/LP-1315. This bit is in a separate commit - happy to move it into a another new MP if you prefer.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:project-tokens/add-new-access-token-tables-to-view into launchpad:master.
diff --git a/lib/canonical/launchpad/icing/css/base.scss b/lib/canonical/launchpad/icing/css/base.scss
index f86bbf0..99ef8c4 100644
--- a/lib/canonical/launchpad/icing/css/base.scss
+++ b/lib/canonical/launchpad/icing/css/base.scss
@@ -366,9 +366,15 @@ body {
table {
&.listing {
margin: 0;
+ margin-bottom: 1.5rem;
width: 100%;
border-bottom: 1px solid #d2d2d2;
+ form {
+ /* Remove margin from forms when they are embeded within a table */
+ margin-bottom: 0;
+ }
+
tbody, td.end-of-section {
border-bottom: 1px solid #d2d2d2;
}
@@ -440,7 +446,8 @@ body {
}
th, td {
- padding: 0.25em;
+ padding: 0.5em;
+ padding-top: calc(.5rem - 1px);
}
table {
diff --git a/lib/lp/code/xmlrpc/git.py b/lib/lp/code/xmlrpc/git.py
index 3452fcd..7b450c0 100644
--- a/lib/lp/code/xmlrpc/git.py
+++ b/lib/lp/code/xmlrpc/git.py
@@ -195,7 +195,10 @@ class GitAPI(LaunchpadXMLRPCView):
or access_token.owner.account_status != AccountStatus.ACTIVE
):
raise faults.Unauthorized()
- if repository is not None and access_token.target != repository:
+ if repository is not None and (
+ access_token.target != repository
+ and access_token.target != repository.target
+ ):
raise faults.Unauthorized()
access_token.updateLastUsed()
return AccessTokenVerificationResult(access_token)
diff --git a/lib/lp/code/xmlrpc/tests/test_git.py b/lib/lp/code/xmlrpc/tests/test_git.py
index 15e4f09..add6348 100644
--- a/lib/lp/code/xmlrpc/tests/test_git.py
+++ b/lib/lp/code/xmlrpc/tests/test_git.py
@@ -1407,6 +1407,27 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
access_token_id=removeSecurityProxy(token).id,
)
+ def test_confirm_git_repository_with_project_access_token(self):
+ # Similarly to git repository tokens, a project access token cannot be
+ # used to authorize confirming repository creation.
+ requester = self.factory.makePerson()
+ project = self.factory.makeProduct(owner=requester)
+ repository = self.factory.makeGitRepository(
+ target=project,
+ owner=requester,
+ status=GitRepositoryStatus.CREATING,
+ )
+ _, token = self.factory.makeAccessToken(
+ owner=requester,
+ target=project,
+ scopes=[AccessTokenScope.REPOSITORY_PUSH],
+ )
+ self.assertConfirmRepoCreationUnauthorized(
+ requester,
+ repository,
+ access_token_id=removeSecurityProxy(token).id,
+ )
+
def test_abort_repo_creation(self):
requester = self.factory.makePerson()
repo = self.factory.makeGitRepository(owner=requester)
@@ -1636,6 +1657,27 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
access_token_id=removeSecurityProxy(token).id,
)
+ def test_abort_git_repository_with_project_access_token(self):
+ # Similarly to git repository tokens, a project access token cannot be
+ # used to authorize aborting repository creation.
+ requester = self.factory.makePerson()
+ project = self.factory.makeProduct(owner=requester)
+ repository = self.factory.makeGitRepository(
+ target=project,
+ owner=requester,
+ status=GitRepositoryStatus.CREATING,
+ )
+ _, token = self.factory.makeAccessToken(
+ owner=requester,
+ target=project,
+ scopes=[AccessTokenScope.REPOSITORY_PUSH],
+ )
+ self.assertAbortRepoCreationUnauthorized(
+ requester,
+ repository,
+ access_token_id=removeSecurityProxy(token).id,
+ )
+
def test_abort_git_repository_creation_of_non_existing_repository(self):
owner = self.factory.makePerson()
repo = removeSecurityProxy(self.factory.makeGitRepository(owner=owner))
@@ -2634,6 +2676,110 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
macaroon_raw=macaroon.serialize(),
)
+ def _assert_translatePath_expected(
+ self, requester, token, repository, expected_success=True, scope="read"
+ ):
+ with person_logged_in(requester):
+ path = "/%s" % repository.unique_name
+ private = (
+ repository.information_type == InformationType.PRIVATESECURITY
+ )
+
+ login(ANONYMOUS)
+ if expected_success:
+ self.assertTranslates(
+ requester,
+ path,
+ repository,
+ permission=scope,
+ readable=(scope == "read"),
+ writable=(scope == "write"),
+ private=private,
+ access_token_id=removeSecurityProxy(token).id,
+ )
+ else:
+ self.assertUnauthorized(
+ requester,
+ path,
+ permission=scope,
+ access_token_id=removeSecurityProxy(token).id,
+ )
+
+ def test_translatePath_user_project_access_token_pull(self):
+ # A user with a suitable project access token can pull from a
+ # repository that belongs to that project, but not others, even if they
+ # own them.
+ requester = self.factory.makePerson()
+ project = self.factory.makeProduct(owner=requester)
+ with person_logged_in(requester):
+ _, token = self.factory.makeAccessToken(
+ owner=requester,
+ target=project,
+ scopes=[AccessTokenScope.REPOSITORY_PULL],
+ )
+
+ repositories_access = [
+ (self.factory.makeGitRepository(), False),
+ (self.factory.makeGitRepository(owner=requester), False),
+ (self.factory.makeGitRepository(target=project), True),
+ (
+ self.factory.makeGitRepository(
+ owner=requester, target=project
+ ),
+ True,
+ ),
+ (
+ self.factory.makeGitRepository(
+ owner=requester,
+ target=project,
+ information_type=InformationType.PRIVATESECURITY,
+ ),
+ True,
+ ),
+ ]
+
+ for repository, expected_success in repositories_access:
+ self._assert_translatePath_expected(
+ requester, token, repository, expected_success
+ )
+
+ def test_translatePath_user_project_access_token_push(self):
+ # A user with a suitable project access token can push from a
+ # repository that belongs to that project, but not others, even if they
+ # own them.
+ requester = self.factory.makePerson()
+ project = self.factory.makeProduct(owner=requester)
+ with person_logged_in(requester):
+ _, token = self.factory.makeAccessToken(
+ owner=requester,
+ target=project,
+ scopes=[AccessTokenScope.REPOSITORY_PUSH],
+ )
+
+ repositories_access = [
+ (self.factory.makeGitRepository(), False),
+ (self.factory.makeGitRepository(owner=requester), False),
+ (
+ self.factory.makeGitRepository(
+ owner=requester, target=project
+ ),
+ True,
+ ),
+ (
+ self.factory.makeGitRepository(
+ owner=requester,
+ target=project,
+ information_type=InformationType.PRIVATESECURITY,
+ ),
+ True,
+ ),
+ ]
+
+ for repository, expected_success in repositories_access:
+ self._assert_translatePath_expected(
+ requester, token, repository, expected_success, "write"
+ )
+
def test_translatePath_user_access_token_pull(self):
# A user with a suitable access token can pull from the
# corresponding repository, but not others, even if they own them.
@@ -2679,23 +2825,6 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
access_token_id=removeSecurityProxy(token).id,
)
- def test_translatePath_user_access_token_pull_wrong_scope(self):
- # A user with an access token that does not have the repository:pull
- # scope cannot pull from the corresponding repository.
- requester = self.factory.makePerson()
- repository = self.factory.makeGitRepository(owner=requester)
- _, token = self.factory.makeAccessToken(
- owner=requester,
- target=repository,
- scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS],
- )
- self.assertPermissionDenied(
- requester,
- "/%s" % repository.unique_name,
- permission="read",
- access_token_id=removeSecurityProxy(token).id,
- )
-
def test_translatePath_user_access_token_push(self):
# A user with a suitable access token can push to the corresponding
# repository, but not others, even if they own them.
@@ -2743,23 +2872,66 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
access_token_id=removeSecurityProxy(token).id,
)
- def test_translatePath_user_access_token_push_wrong_scope(self):
- # A user with an access token that does not have the repository:push
- # scope cannot push to the corresponding repository.
- requester = self.factory.makePerson()
- repository = self.factory.makeGitRepository(owner=requester)
+ def _assert_translatePath_permission_denied_wrong_scope(
+ self, requester, repository, token_target, scope
+ ):
+ wrong_scope = (
+ AccessTokenScope.REPOSITORY_PUSH
+ if scope == "read"
+ else AccessTokenScope.REPOSITORY_PULL
+ )
_, token = self.factory.makeAccessToken(
owner=requester,
- target=repository,
- scopes=[AccessTokenScope.REPOSITORY_PULL],
+ target=token_target,
+ scopes=[AccessTokenScope.REPOSITORY_BUILD_STATUS, wrong_scope],
)
self.assertPermissionDenied(
requester,
"/%s" % repository.unique_name,
- permission="write",
+ permission=scope,
access_token_id=removeSecurityProxy(token).id,
)
+ def test_translatePath_user_git_access_token_pull_wrong_scope(self):
+ # A user with a git repository access token that does not have the
+ # repository:pull scope cannot pull from the corresponding repository.
+ requester = self.factory.makePerson()
+ repository = self.factory.makeGitRepository(owner=requester)
+ self._assert_translatePath_permission_denied_wrong_scope(
+ requester, repository, token_target=repository, scope="read"
+ )
+
+ def test_translatePath_user_project_access_token_pull_wrong_scope(self):
+ # A user with a project access token that does not have the
+ # repository:pull scope cannot pull from the corresponding repository.
+ requester = self.factory.makePerson()
+ project = self.factory.makeProduct(owner=requester)
+ repository = self.factory.makeGitRepository(
+ owner=requester, target=project
+ )
+ self._assert_translatePath_permission_denied_wrong_scope(
+ requester, repository, token_target=project, scope="read"
+ )
+
+ def test_translatePath_user_git_access_token_push_wrong_scope(self):
+ # A user with a git repository access token that does not have the
+ # repository:push scope cannot push to the corresponding repository.
+ requester = self.factory.makePerson()
+ repository = self.factory.makeGitRepository(owner=requester)
+ self._assert_translatePath_permission_denied_wrong_scope(
+ requester, repository, token_target=repository, scope="write"
+ )
+
+ def test_translatePath_user_project_access_token_push_wrong_scope(self):
+ # A user with a project access token that does not have the
+ # repository:push scope cannot push to the corresponding repository.
+ requester = self.factory.makePerson()
+ project = self.factory.makeProduct(owner=requester)
+ repository = self.factory.makeGitRepository(target=project)
+ self._assert_translatePath_permission_denied_wrong_scope(
+ requester, repository, token_target=project, scope="write"
+ )
+
def test_translatePath_user_access_token_nonexistent(self):
# Attempting to pass a nonexistent access token ID returns
# Unauthorized.
@@ -2927,15 +3099,12 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
auth_params,
)
- def test_getMergeProposalURL_user_access_token(self):
- # The merge proposal URL is returned by LP for a non-default branch
- # pushed by a user with a suitable access token that has their
- # ordinary privileges on the corresponding repository.
- requester = self.factory.makePerson()
- repository = self._makeGitRepositoryWithRefs(owner=requester)
+ def _assert_getMergeProposalURL_user_access_token(
+ self, requester, repository, token_target
+ ):
_, token = self.factory.makeAccessToken(
owner=requester,
- target=repository,
+ target=token_target,
scopes=[AccessTokenScope.REPOSITORY_PUSH],
)
auth_params = _make_auth_params(
@@ -2943,13 +3112,35 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
)
self.assertHasMergeProposalURL(repository, "branch", auth_params)
- def test_getMergeProposalURL_user_access_token_wrong_repository(self):
- # getMergeProposalURL refuses access tokens for a different
- # repository.
+ def test_getMergeProposalURL_user_git_access_token(self):
+ # The merge proposal URL is returned by LP for a non-default branch
+ # pushed by a user with a suitable git repository access token that has
+ # their ordinary privileges on a repository in that project.
requester = self.factory.makePerson()
repository = self._makeGitRepositoryWithRefs(owner=requester)
+ self._assert_getMergeProposalURL_user_access_token(
+ requester, repository, token_target=repository
+ )
+
+ def test_getMergeProposalURL_user_project_access_token(self):
+ # The merge proposal URL is returned by LP for a non-default branch
+ # pushed by a user with a suitable project access token that has
+ # their ordinary privileges on a repository in that project.
+ requester = self.factory.makePerson()
+ project = self.factory.makeProduct(owner=requester)
+ repository = self._makeGitRepositoryWithRefs(target=project)
+ self._assert_getMergeProposalURL_user_access_token(
+ requester, repository, token_target=repository
+ )
+
+ def _assert_getMergeProposalURL_user_access_token_wrong_repository(
+ self, requester, token_target
+ ):
+ repository = self._makeGitRepositoryWithRefs(owner=requester)
_, token = self.factory.makeAccessToken(
- owner=requester, scopes=[AccessTokenScope.REPOSITORY_PUSH]
+ target=token_target,
+ owner=requester,
+ scopes=[AccessTokenScope.REPOSITORY_PUSH],
)
auth_params = _make_auth_params(
requester, access_token_id=removeSecurityProxy(token).id
@@ -2963,6 +3154,26 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
auth_params,
)
+ def test_getMergeProposalURL_user_git_access_token_wrong_repository(self):
+ # getMergeProposalURL refuses access tokens for a different
+ # repository.
+ requester = self.factory.makePerson()
+ repository = self.factory.makeGitRepository(owner=requester)
+ self._assert_getMergeProposalURL_user_access_token_wrong_repository(
+ requester, token_target=repository
+ )
+
+ def test_getMergeProposalURL_user_project_access_token_wrong_repository(
+ self,
+ ):
+ # getMergeProposalURL refuses access tokens for repository from a
+ # different project.
+ requester = self.factory.makePerson()
+ project = self.factory.makeProduct(owner=requester)
+ self._assert_getMergeProposalURL_user_access_token_wrong_repository(
+ requester, token_target=project
+ )
+
def test_getMergeProposalURL_code_import(self):
# A merge proposal URL from LP to Turnip is not returned for
# code import job as there is no User at the other end.
@@ -3464,12 +3675,15 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
macaroon.serialize(),
)
- def test_authenticateWithPassword_user_access_token(self):
+ def _assert_authenticateWithPassword_user_access_token(
+ self,
+ requester,
+ secret,
+ token,
+ ):
# A user with a suitable access token can authenticate using it, in
# which case we return both the access token and the uid for use by
# later calls.
- requester = self.factory.makePerson()
- secret, token = self.factory.makeAccessToken(owner=requester)
self.assertIsNone(removeSecurityProxy(token).date_last_used)
self.assertEqual(
{
@@ -3490,6 +3704,30 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
secret,
)
+ def test_authenticateWithPassword_user_git_access_token(self):
+ # A user with a suitable git repository access token can authenticate
+ # using it
+ requester = self.factory.makePerson()
+ secret, token = self.factory.makeAccessToken(owner=requester)
+ self._assert_authenticateWithPassword_user_access_token(
+ requester,
+ secret,
+ token,
+ )
+
+ def test_authenticateWithPassword_user_project_access_token(self):
+ # A user with a suitable project access token can authenticate using it
+ requester = self.factory.makePerson()
+ project = self.factory.makeProduct(owner=requester)
+ secret, token = self.factory.makeAccessToken(
+ owner=requester, target=project
+ )
+ self._assert_authenticateWithPassword_user_access_token(
+ requester,
+ secret,
+ token,
+ )
+
def test_authenticateWithPassword_user_access_token_expired(self):
# An expired access token is rejected.
requester = self.factory.makePerson()
@@ -3855,21 +4093,76 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
macaroon_raw=macaroon.serialize(),
)
- def test_checkRefPermissions_user_access_token(self):
+ def _assert_checkRefPermissions_permissions(
+ self, requester, token, repository, expected_success
+ ):
+ ref_path = b"refs/heads/main"
+ login(ANONYMOUS)
+ if expected_success:
+ # This expects that since we don't add any permission rules and the
+ # requester is the owner of the repository, then the call returns
+ # all 3 permissions
+ expected_permissions = ["create", "push", "force_push"]
+ else:
+ expected_permissions = []
+ self.assertHasRefPermissions(
+ requester,
+ repository,
+ [ref_path],
+ {ref_path: expected_permissions},
+ access_token_id=removeSecurityProxy(token).id,
+ )
+
+ def test_checkRefPermissions_user_project_access_token(self):
+ # A user with a suitable access token targetted at a project, has their
+ # ordinary privileges on repositories from the same project, but not
+ # others, even if they own them.
+ requester = self.factory.makePerson()
+ project = self.factory.makeProduct(owner=requester)
+ repositories = [
+ (self.factory.makeGitRepository(), False),
+ (self.factory.makeGitRepository(owner=requester), False),
+ (self.factory.makeGitRepository(target=project), False),
+ (
+ self.factory.makeGitRepository(
+ owner=requester, target=project
+ ),
+ True,
+ ),
+ (
+ self.factory.makeGitRepository(
+ target=project,
+ owner=requester,
+ information_type=InformationType.PRIVATESECURITY,
+ ),
+ True,
+ ),
+ ]
+ with person_logged_in(requester):
+ _, token = self.factory.makeAccessToken(
+ owner=requester,
+ target=project,
+ scopes=[AccessTokenScope.REPOSITORY_PUSH],
+ )
+
+ for repository, expected_success in repositories:
+ self._assert_checkRefPermissions_permissions(
+ requester, token, repository, expected_success
+ )
+
+ def test_checkRefPermissions_user_git_access_token(self):
# A user with a suitable access token has their ordinary privileges
# on the corresponding repository, but not others, even if they own
# them.
requester = self.factory.makePerson()
repositories = [
- self.factory.makeGitRepository(owner=requester) for _ in range(2)
- ]
- repositories.append(
+ self.factory.makeGitRepository(owner=requester),
+ self.factory.makeGitRepository(owner=requester),
self.factory.makeGitRepository(
owner=requester,
information_type=InformationType.PRIVATESECURITY,
- )
- )
- ref_path = b"refs/heads/main"
+ ),
+ ]
tokens = []
with person_logged_in(requester):
for repository in repositories:
@@ -3879,29 +4172,21 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
scopes=[AccessTokenScope.REPOSITORY_PUSH],
)
tokens.append(token)
+
for i, repository in enumerate(repositories):
for j, token in enumerate(tokens):
- login(ANONYMOUS)
- if i == j:
- expected_permissions = ["create", "push", "force_push"]
- else:
- expected_permissions = []
- self.assertHasRefPermissions(
- requester,
- repository,
- [ref_path],
- {ref_path: expected_permissions},
- access_token_id=removeSecurityProxy(token).id,
+ self._assert_checkRefPermissions_permissions(
+ requester, token, repository, expected_success=(i == j)
)
- def test_checkRefPermissions_user_access_token_wrong_scope(self):
+ def _assert_checkRefPermissions_user_access_token_wrong_scope(
+ self, requester, repository, token_target
+ ):
# A user with an access token that does not have the repository:push
# scope cannot push to any branch in the corresponding repository.
- requester = self.factory.makePerson()
- repository = self.factory.makeGitRepository(owner=requester)
_, token = self.factory.makeAccessToken(
owner=requester,
- target=repository,
+ target=token_target,
scopes=[AccessTokenScope.REPOSITORY_PULL],
)
ref_path = b"refs/heads/main"
@@ -3913,6 +4198,25 @@ class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
access_token_id=removeSecurityProxy(token).id,
)
+ def test_checkRefPermissions_user_git_access_token_wrong_scope(self):
+ # A user with an access token that does not have the repository:push
+ # scope cannot push to any branch in the corresponding repository.
+ requester = self.factory.makePerson()
+ repository = self.factory.makeGitRepository(owner=requester)
+ self._assert_checkRefPermissions_user_access_token_wrong_scope(
+ requester, repository, token_target=repository
+ )
+
+ def test_checkRefPermissions_user_project_access_token_wrong_scope(self):
+ # A user with an access token that does not have the repository:push
+ # scope cannot push to any branch in the corresponding repository.
+ requester = self.factory.makePerson()
+ project = self.factory.makeProduct(owner=requester)
+ repository = self.factory.makeGitRepository(target=project)
+ self._assert_checkRefPermissions_user_access_token_wrong_scope(
+ requester, repository, token_target=project
+ )
+
def assertUpdatesRepackStats(self, repo):
start_time = datetime.now(timezone.utc)
self.assertIsNone(
diff --git a/lib/lp/registry/browser/product.py b/lib/lp/registry/browser/product.py
index bc77dd9..c6dc562 100644
--- a/lib/lp/registry/browser/product.py
+++ b/lib/lp/registry/browser/product.py
@@ -469,6 +469,11 @@ class ProductEditLinksMixin(StructuralSubscriptionMenuMixin):
return Link("+configure-blueprints", text, summary, icon="edit")
@enabled_with_permission("launchpad.Edit")
+ def access_tokens(self):
+ text = "Manage access tokens"
+ return Link("+access-tokens", text, icon="edit")
+
+ @enabled_with_permission("launchpad.Edit")
def branding(self):
text = "Change branding"
return Link("+branding", text, icon="edit")
@@ -546,6 +551,7 @@ class ProductActionNavigationMenu(NavigationMenu, ProductEditLinksMixin):
"sharing",
"search_oci_project",
"new_oci_project",
+ "access_tokens",
"webhooks",
]
add_subscribe_link(links)
diff --git a/lib/lp/registry/interfaces/product.py b/lib/lp/registry/interfaces/product.py
index 790f77d..af127f0 100644
--- a/lib/lp/registry/interfaces/product.py
+++ b/lib/lp/registry/interfaces/product.py
@@ -117,6 +117,10 @@ from lp.registry.interfaces.role import (
IHasDrivers,
IHasOwner,
)
+from lp.services.auth.interfaces import (
+ IAccessTokenTarget,
+ IAccessTokenTargetEdit,
+)
from lp.services.fields import (
Description,
IconImageUpload,
@@ -606,6 +610,7 @@ class IProductView(
IHasCodeImports,
IServiceUsage,
IHasGitRepositories,
+ IAccessTokenTarget,
):
"""Public IProduct properties."""
@@ -1099,7 +1104,11 @@ class IProductView(
"""
-class IProductEditRestricted(IOfficialBugTagTargetRestricted, IWebhookTarget):
+class IProductEditRestricted(
+ IOfficialBugTagTargetRestricted,
+ IWebhookTarget,
+ IAccessTokenTargetEdit,
+):
"""`IProduct` properties which require launchpad.Edit permission."""
@mutator_for(IProductView["bug_sharing_policy"])
diff --git a/lib/lp/registry/model/person.py b/lib/lp/registry/model/person.py
index 788333b..1bc9a03 100644
--- a/lib/lp/registry/model/person.py
+++ b/lib/lp/registry/model/person.py
@@ -1206,13 +1206,10 @@ class Person(
]
if transitive:
- # getProductPrivacyFilter may also use TeamParticipation, so
- # ensure we use a different one.
- ownership_participation = ClassAlias(TeamParticipation)
clauses.extend(
[
- Product._owner_id == ownership_participation.team_id,
- ownership_participation.person_id == self.id,
+ Product._owner_id == TeamParticipation.team_id,
+ TeamParticipation.person_id == self.id,
]
)
else:
diff --git a/lib/lp/registry/model/product.py b/lib/lp/registry/model/product.py
index f3bb669..1ebe91c 100644
--- a/lib/lp/registry/model/product.py
+++ b/lib/lp/registry/model/product.py
@@ -31,6 +31,7 @@ from storm.expr import (
Or,
Select,
)
+from storm.info import ClassAlias
from storm.locals import (
Bool,
DateTime,
@@ -150,6 +151,7 @@ from lp.registry.model.series import ACTIVE_STATUSES
from lp.registry.model.sharingpolicy import SharingPolicyMixin
from lp.registry.model.sourcepackagename import SourcePackageName
from lp.registry.model.teammembership import TeamParticipation
+from lp.services.auth.model import AccessTokenTargetMixin
from lp.services.database import bulk
from lp.services.database.constants import UTC_NOW
from lp.services.database.decoratedresultset import DecoratedResultSet
@@ -248,6 +250,7 @@ specification_policy_default = {
@implementer(IBugSummaryDimension, IHasCustomLanguageCodes, IProduct)
class Product(
StormBase,
+ AccessTokenTargetMixin,
BugTargetBase,
HasDriversMixin,
OfficialBugTagTargetMixin,
@@ -1891,6 +1894,10 @@ class ProductSet:
if roles.in_admin or roles.in_commercial_admin:
return True
+ # In places where this method is used, they might want to use
+ # TeamParticipation. This ensures that we use a different one.
+ ownership_participation = ClassAlias(TeamParticipation)
+
# Normal users can see any project for which they can see either
# an entire policy or an artifact.
# XXX wgrant 2015-06-26: This is slower than ideal for people in
@@ -1905,12 +1912,12 @@ class ProductSet:
tables=(
AccessPolicyGrantFlat,
Join(
- TeamParticipation,
- TeamParticipation.team_id
+ ownership_participation,
+ ownership_participation.team_id
== AccessPolicyGrantFlat.grantee_id,
),
),
- where=(TeamParticipation.person == user),
+ where=(ownership_participation.person_id == user.id),
),
),
False,
diff --git a/lib/lp/services/auth/browser.py b/lib/lp/services/auth/browser.py
index b10d64b..b73f83f 100644
--- a/lib/lp/services/auth/browser.py
+++ b/lib/lp/services/auth/browser.py
@@ -16,6 +16,8 @@ from lp.app.browser.launchpadform import LaunchpadFormView, action
from lp.app.errors import UnexpectedFormData
from lp.app.widgets.date import DateTimeWidget
from lp.app.widgets.itemswidgets import LabeledMultiCheckBoxWidget
+from lp.code.interfaces.gitrepository import IGitRepository
+from lp.registry.interfaces.product import IProduct
from lp.services.auth.interfaces import IAccessToken, IAccessTokenSet
from lp.services.propertycache import cachedproperty
from lp.services.webapp.publisher import canonical_url
@@ -49,6 +51,14 @@ class AccessTokensView(LaunchpadFormView):
page_title = "Personal access tokens"
+ @property
+ def context_type(self):
+ if IGitRepository.providedBy(self.context):
+ return "Git Repository"
+ elif IProduct.providedBy(self.context):
+ return "Project"
+ return "Target"
+
@cachedproperty
def access_tokens(self):
return list(
@@ -57,6 +67,18 @@ class AccessTokensView(LaunchpadFormView):
)
)
+ @cachedproperty
+ def parent_project_tokens(self):
+ if IGitRepository.providedBy(self.context) and IProduct.providedBy(
+ self.context.target
+ ):
+ return list(
+ getUtility(IAccessTokenSet).findByTarget(
+ self.context.target, visible_by_user=self.user
+ )
+ )
+ return []
+
@action("Revoke", name="revoke")
def revoke_action(self, action, data):
form = self.request.form
diff --git a/lib/lp/services/auth/interfaces.py b/lib/lp/services/auth/interfaces.py
index 6081830..10c3c58 100644
--- a/lib/lp/services/auth/interfaces.py
+++ b/lib/lp/services/auth/interfaces.py
@@ -73,7 +73,15 @@ class IAccessToken(Interface):
description=_("The Git repository for which the token was issued."),
# Really IGitRepository, patched in lp.services.auth.webservice.
schema=Interface,
- required=True,
+ required=False,
+ readonly=True,
+ )
+
+ project = Reference(
+ title=_("Project"),
+ description=_("The project for which the token was issued."),
+ schema=Interface,
+ required=False,
readonly=True,
)
diff --git a/lib/lp/services/auth/model.py b/lib/lp/services/auth/model.py
index f595637..2b4fe2b 100644
--- a/lib/lp/services/auth/model.py
+++ b/lib/lp/services/auth/model.py
@@ -21,6 +21,7 @@ from zope.security.proxy import removeSecurityProxy
from lp.code.interfaces.gitcollection import IAllGitRepositories
from lp.code.interfaces.gitrepository import IGitRepository
+from lp.registry.interfaces.product import IProduct
from lp.registry.model.teammembership import TeamParticipation
from lp.services.auth.enums import AccessTokenScope
from lp.services.auth.interfaces import IAccessToken, IAccessTokenSet
@@ -50,9 +51,12 @@ class AccessToken(StormBase):
description = Unicode(name="description", allow_none=False)
- git_repository_id = Int(name="git_repository", allow_none=False)
+ git_repository_id = Int(name="git_repository", allow_none=True)
git_repository = Reference(git_repository_id, "GitRepository.id")
+ project_id = Int(name="project", allow_none=True)
+ project = Reference(project_id, "Product.id")
+
_scopes = JSON(name="scopes", allow_none=False)
date_last_used = DateTime(
@@ -76,6 +80,8 @@ class AccessToken(StormBase):
self.description = description
if IGitRepository.providedBy(target):
self.git_repository = target
+ elif IProduct.providedBy(target):
+ self.project = target
else:
raise TypeError("Unsupported target: {!r}".format(target))
self.scopes = scopes
@@ -85,7 +91,7 @@ class AccessToken(StormBase):
@property
def target(self):
"""See `IAccessToken`."""
- return self.git_repository
+ return self.git_repository or self.project
@property
def scopes(self):
@@ -181,32 +187,57 @@ class AccessTokenSet:
self, target, visible_by_user=None, include_expired=False
):
"""See `IAccessTokenSet`."""
+
clauses = []
if IGitRepository.providedBy(target):
clauses.append(AccessToken.git_repository == target)
- if visible_by_user is not None:
- collection = (
- getUtility(IAllGitRepositories)
- .visibleByUser(visible_by_user)
- .ownedByTeamMember(visible_by_user)
+ elif IProduct.providedBy(target):
+ clauses.append(AccessToken.project == target)
+ else:
+ raise TypeError("Unsupported target: {!r}".format(target))
+
+ if visible_by_user is not None:
+ # Evaluate if user owns the target (directly or indirectly).
+ # The queries below might look more complex than they should to
+ # evaluate ownership, but they are more performant as this will run
+ # in one single query in the end.
+ if IGitRepository.providedBy(target):
+ user_owns_target = AccessToken.git_repository_id.is_in(
+ removeSecurityProxy(
+ (
+ getUtility(IAllGitRepositories)
+ .visibleByUser(visible_by_user)
+ .ownedByTeamMember(visible_by_user)
+ ).getRepositoryIds()
+ )._get_select()
)
- ids = collection.getRepositoryIds()
- clauses.append(
- Or(
- AccessToken.owner_id.is_in(
- Select(
- TeamParticipation.team_id,
- where=TeamParticipation.person
- == visible_by_user,
- )
- ),
- AccessToken.git_repository_id.is_in(
- removeSecurityProxy(ids)._get_select()
+ else:
+ # Avoid circular import
+ from lp.registry.model.product import Product, ProductSet
+
+ clauses.extend([Product.active, Product.id == target.id])
+ user_owns_target = AccessToken.project_id.is_in(
+ Select(
+ Product.id,
+ where=And(
+ Product._owner_id == TeamParticipation.team_id,
+ TeamParticipation.person_id == visible_by_user.id,
+ ProductSet.getProductPrivacyFilter(
+ visible_by_user
+ ),
),
)
)
- else:
- raise TypeError("Unsupported target: {!r}".format(target))
+
+ user_owns_token = AccessToken.owner_id.is_in(
+ Select(
+ TeamParticipation.team_id,
+ where=TeamParticipation.person_id == visible_by_user.id,
+ )
+ )
+
+ clauses.append(Or(user_owns_target, user_owns_token))
+
if not include_expired:
clauses.append(
Or(
diff --git a/lib/lp/services/auth/templates/accesstokentarget-access-tokens.pt b/lib/lp/services/auth/templates/accesstokentarget-access-tokens.pt
index 6d85df0..6274a68 100644
--- a/lib/lp/services/auth/templates/accesstokentarget-access-tokens.pt
+++ b/lib/lp/services/auth/templates/accesstokentarget-access-tokens.pt
@@ -74,6 +74,10 @@
</div>
<h2>Active tokens</h2>
+ <p>Active personal access tokens for
+ <a tal:replace="structure context/fmt:link" />
+ <span tal:content="view/context_type" />.
+ </p>
<table class="listing access-tokens-table"
style="max-width: 80em;">
<thead>
@@ -116,6 +120,56 @@
</tr>
</tbody>
</table>
+ <div tal:condition="view/parent_project_tokens">
+ <h2>Project tokens</h2>
+ <p>Your project access tokens for this git repository's project:
+ <a tal:replace="structure context/target/fmt:link" />.
+ </p>
+ <p>Project tokens grant access to all the project's git repositories
+ accessible to a user.</p>
+ <table class="listing project-access-tokens-table"
+ style="max-width: 80em;">
+ <thead>
+ <tr>
+ <th>Description</th>
+ <th>Scopes</th>
+ <th>Created</th>
+ <th>Last used</th>
+ <th>Expires</th>
+ <th><tal:comment condition="nothing">Revoke button</tal:comment></th>
+ </tr>
+ </thead>
+ <tbody id="access-tokens-tbody">
+ <tr tal:repeat="token view/parent_project_tokens"
+ tal:attributes="
+ class python: 'yui3-lazr-even' if repeat['token'].even()
+ else 'yui3-lazr-odd';
+ token-id token/id">
+ <td tal:content="token/description" />
+ <td tal:content="
+ python: ', '.join(scope.title for scope in token.scopes)" />
+ <td tal:content="
+ structure token/date_created/fmt:approximatedatetitle" />
+ <td tal:content="
+ structure token/date_last_used/fmt:approximatedatetitle" />
+ <td>
+ <tal:expires
+ condition="token/date_expires"
+ replace="
+ structure token/date_expires/fmt:approximatedatetitle" />
+ <span tal:condition="not: token/date_expires">Never</span>
+ </td>
+ <td>
+ <form method="post" tal:attributes="name string:revoke-${token/id}">
+ <input type="hidden" name="token_id"
+ tal:attributes="value token/id" />
+ <input type="submit" name="field.actions.revoke" value="Revoke" />
+ </form>
+ </td>
+ </tr>
+ </tbody>
+ </table>
+ </div>
</div>
</body>
diff --git a/lib/lp/services/auth/tests/test_browser.py b/lib/lp/services/auth/tests/test_browser.py
index a1aba38..9da268e 100644
--- a/lib/lp/services/auth/tests/test_browser.py
+++ b/lib/lp/services/auth/tests/test_browser.py
@@ -17,6 +17,7 @@ from testtools.matchers import (
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
+from lp.code.interfaces.gitrepository import IGitRepository
from lp.services.webapp.interfaces import IPlacelessAuthUtility
from lp.services.webapp.publisher import canonical_url
from lp.testing import TestCaseWithFactory, login_person
@@ -33,7 +34,13 @@ token_listing_constants = soupmatchers.HTMLContains(
soupmatchers.Within(breadcrumbs_tag, tokens_page_crumb_tag)
)
token_listing_tag = soupmatchers.Tag(
- "tokens table", "table", attrs={"class": "listing"}
+ "tokens table", "table", attrs={"class": "listing access-tokens-table"}
+)
+
+project_token_listing_tag = soupmatchers.Tag(
+ "project tokens table",
+ "table",
+ attrs={"class": "listing project-access-tokens-table"},
)
@@ -74,17 +81,19 @@ class TestAccessTokenViewBase:
return view
def test_access_tokens_link(self):
- target_url = canonical_url(self.target, rootsite="code")
+ target_url = canonical_url(self.target, rootsite=self.rootsite)
expected_tokens_url = canonical_url(
- self.target, view_name="+access-tokens", rootsite="code"
+ self.target, view_name="+access-tokens", rootsite=self.rootsite
)
browser = self.getUserBrowser(target_url, user=self.owner)
tokens_link = browser.getLink("Manage access tokens")
self.assertEqual(expected_tokens_url, tokens_link.url)
- def makeTokensAndMatchers(self, count):
+ def makeTokensAndMatchers(self, count, target=None):
+ if target is None:
+ target = self.target
tokens = [
- self.factory.makeAccessToken(target=self.target)[1]
+ self.factory.makeAccessToken(target=target)[1]
for _ in range(count)
]
# There is a row for each token.
@@ -114,19 +123,30 @@ class TestAccessTokenViewBase:
def test_empty(self):
self.assertThat(
self.makeView("+access-tokens")(),
- MatchesAll(
- token_listing_constants,
- soupmatchers.HTMLContains(token_listing_tag),
- ),
+ MatchesAll(*self.getPageContent(token_matchers=[])),
)
def test_existing_tokens(self):
token_matchers = self.makeTokensAndMatchers(10)
self.assertThat(
self.makeView("+access-tokens")(),
+ MatchesAll(*self.getPageContent(token_matchers)),
+ )
+
+ def test_extra_tokens(self):
+ if not IGitRepository.providedBy(self.target):
+ self.skipTest("Test only relevant for Git Repository tests.")
+
+ project = self.factory.makeProduct(owner=self.owner)
+ self.target.setTarget(target=project, user=self.owner)
+ token_matchers = self.makeTokensAndMatchers(5)
+ extra_matchers = self.makeTokensAndMatchers(5, target=project)
+ self.assertThat(
+ self.makeView("+access-tokens")(),
MatchesAll(
- token_listing_constants,
- soupmatchers.HTMLContains(token_listing_tag, *token_matchers),
+ *self.getPageContent(
+ token_matchers, extra_matchers=extra_matchers
+ )
),
)
@@ -190,8 +210,37 @@ class TestAccessTokenViewBase:
class TestAccessTokenViewGitRepository(
TestAccessTokenViewBase, TestCaseWithFactory
):
+ rootsite = "code"
+
def makeTarget(self):
return self.factory.makeGitRepository()
def getTraversalStack(self, obj):
return [obj.target, obj]
+
+ def getPageContent(self, token_matchers, extra_matchers=None):
+ contents = [
+ token_listing_constants,
+ soupmatchers.HTMLContains(token_listing_tag, *token_matchers),
+ ]
+
+ if extra_matchers is not None:
+ contents.append(
+ soupmatchers.HTMLContains(
+ project_token_listing_tag, *extra_matchers
+ )
+ )
+ return contents
+
+
+class TestAccessTokenViewProject(TestAccessTokenViewBase, TestCaseWithFactory):
+ rootsite = None
+
+ def makeTarget(self):
+ return self.factory.makeProduct()
+
+ def getTraversalStack(self, obj):
+ return [obj]
+
+ def getPageContent(self, token_matchers):
+ return [soupmatchers.HTMLContains(token_listing_tag, *token_matchers)]
diff --git a/lib/lp/services/auth/tests/test_model.py b/lib/lp/services/auth/tests/test_model.py
index ca33eec..80dda03 100644
--- a/lib/lp/services/auth/tests/test_model.py
+++ b/lib/lp/services/auth/tests/test_model.py
@@ -22,6 +22,7 @@ from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
from lp.code.interfaces.gitrepository import IGitRepository
+from lp.registry.interfaces.person import TeamMembershipPolicy
from lp.services.auth.enums import AccessTokenScope
from lp.services.auth.interfaces import IAccessTokenSet
from lp.services.auth.utils import create_access_token_secret
@@ -197,6 +198,11 @@ class TestAccessTokenGitRepository(TestAccessTokenBase, TestCaseWithFactory):
return self.factory.makeGitRepository(owner=owner)
+class TestAccessTokenProduct(TestAccessTokenBase, TestCaseWithFactory):
+ def makeTarget(self, owner=None):
+ return self.factory.makeProduct(owner=owner)
+
+
class TestAccessTokenSetBase:
layer = DatabaseFunctionalLayer
@@ -226,7 +232,7 @@ class TestAccessTokenSetBase:
)
def test_getByID(self):
- secret, token = self.factory.makeAccessToken(target=self.makeTarget())
+ _, token = self.factory.makeAccessToken(target=self.makeTarget())
token_id = removeSecurityProxy(token).id
self.assertEqual(token, getUtility(IAccessTokenSet).getByID(token_id))
self.assertIsNone(getUtility(IAccessTokenSet).getByID(token_id + 1))
@@ -315,6 +321,94 @@ class TestAccessTokenSetBase:
),
)
+ def test_findByTarget_visible_by_owner(self):
+ # If a user owns a target, they can see all the tokens for that target
+ # regadless if they own the tokens.
+ owner = self.factory.makePerson()
+ owned_target = self.makeTarget(owner=owner)
+ owned_target_tokens = [
+ self.factory.makeAccessToken(target=owned_target)[1],
+ self.factory.makeAccessToken(target=owned_target)[1],
+ ]
+ self.factory.makeAccessToken(target=self.makeTarget())[1]
+ self.factory.makeAccessToken(target=self.makeTarget())[1]
+
+ self.assertContentEqual(
+ owned_target_tokens,
+ getUtility(IAccessTokenSet).findByTarget(
+ owned_target, visible_by_user=owner
+ ),
+ )
+
+ def test_findByTarget_visible_by_team_member(self):
+ # If a user belongs to a team that owns a target, they can see all the
+ # tokens for that target regardless if they own the tokens.
+
+ # Only restricted or moderated teams can own certain targets (Product)
+ team = self.factory.makeTeam(
+ membership_policy=TeamMembershipPolicy.RESTRICTED
+ )
+ team_member = self.factory.makePerson()
+ with person_logged_in(team.teamowner):
+ team.addMember(team_member, team)
+
+ owned_target = self.makeTarget(owner=team)
+ owned_target_tokens = [
+ self.factory.makeAccessToken(target=owned_target)[1],
+ self.factory.makeAccessToken(target=owned_target)[1],
+ ]
+
+ self.factory.makeAccessToken(target=self.makeTarget())[1]
+ self.factory.makeAccessToken(target=self.makeTarget())[1]
+
+ self.assertContentEqual(
+ owned_target_tokens,
+ getUtility(IAccessTokenSet).findByTarget(
+ owned_target, visible_by_user=team_member
+ ),
+ )
+
+ def test_findByTarget_visible_by_user_inactive_target(self):
+ # User cannot see tokens for inactive targets.
+
+ owner = self.factory.makePerson()
+ target = self.makeTarget(owner=owner)
+
+ if IGitRepository.providedBy(target):
+ self.skipTest(
+ "Currently, Git repositories allow requests without scopes."
+ )
+
+ removeSecurityProxy(target).active = False
+ self.factory.makeAccessToken(target=target, owner=owner)[1]
+
+ self.assertContentEqual(
+ [],
+ getUtility(IAccessTokenSet).findByTarget(
+ target, visible_by_user=owner
+ ),
+ )
+
+ def test_findByTarget_visible_by_user_from_a_team_that_owns_token(self):
+ # Method with a visible_by_user parameter, returns only tokens that
+ # are owned by the `visible_by_user` Person or by a team that the
+ # `visible_by_user` Person belongs to.
+ target = self.makeTarget()
+ team = self.factory.makeTeam()
+ team_member = self.factory.makePerson()
+ with person_logged_in(team.teamowner):
+ team.addMember(team_member, team)
+ _, token = self.factory.makeAccessToken(owner=team, target=target)
+ self.factory.makeAccessToken(target=target)
+
+ for user in [team.teamowner, team_member]:
+ self.assertContentEqual(
+ [token],
+ getUtility(IAccessTokenSet).findByTarget(
+ target, visible_by_user=user
+ ),
+ )
+
def test_findByTarget_excludes_expired(self):
target = self.makeTarget()
_, current_token = self.factory.makeAccessToken(target=target)
@@ -432,8 +526,13 @@ class TestAccessTokenSetBase:
class TestGitRepositoryAccessTokenSet(
TestAccessTokenSetBase, TestCaseWithFactory
):
- def makeTarget(self):
- return self.factory.makeGitRepository()
+ def makeTarget(self, **kwargs):
+ return self.factory.makeGitRepository(**kwargs)
+
+
+class TestProjectAccessTokenSet(TestAccessTokenSetBase, TestCaseWithFactory):
+ def makeTarget(self, **kwargs):
+ return self.factory.makeProduct(**kwargs)
class TestAccessTokenTargetBase:
@@ -720,3 +819,10 @@ class TestAccessTokenTargetGitRepository(
b"user.",
response.body,
)
+
+
+class TestAccessTokenTargetProject(
+ TestAccessTokenTargetBase, TestCaseWithFactory
+):
+ def makeTarget(self):
+ return self.factory.makeProduct()
diff --git a/lib/lp/services/webapp/tests/test_servers.py b/lib/lp/services/webapp/tests/test_servers.py
index e777ac2..491fed7 100644
--- a/lib/lp/services/webapp/tests/test_servers.py
+++ b/lib/lp/services/webapp/tests/test_servers.py
@@ -968,6 +968,13 @@ class TestWebServiceAccessTokensGitRepository(
return self.factory.makeGitRepository(owner=owner)
+class TestWebServiceAccessTokensProject(
+ TestWebServiceAccessTokensBase, TestCaseWithFactory
+):
+ def makeTarget(self, owner=None):
+ return self.factory.makeProduct(owner=owner)
+
+
def test_suite():
suite = unittest.TestSuite()
suite.addTest(
