opendev/gerrit
Code Issues Proposed changes

Use MODIFY_ACCOUNT instead of ACCESS_DATABASE for external id endpoints

Browse Source 
With this change, we use MODIFY_ACCOUNT instead of ACCESS_DATABASE to
allow users to get/delete someone else's external IDs. The rational is
that this is the more narrow, better fitting permission.

We also document this possibility.

Change-Id: I90b5547326590ea9d3e5bcf5afcc9c803066598d
This commit is contained in:
Patrick Hiesel
2020-02-26 13:02:19 +01:00
parent 0137f8a497
commit db627ece31
4 changed files with 16 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
Documentation/rest-api-accounts.txt
View File

@@ -1728,6 +1728,10 @@ can contain a list of link:#project-watch-info[ProjectWatchInfo] entities.
Retrieves the external ids of a user account.
Only external ids belonging to the caller may be requested. Users that have
link:access-control.html#capability_modifyAccount[Modify Account] can request
external ids that belong to other accounts.
.Request
----
GET /a/accounts/self/external.ids HTTP/1.0
@@ -1761,7 +1765,9 @@ link:#account-external-id-info[AccountExternalIdInfo] entities.
Delete a list of external ids for a user account. The target external ids must
be provided as a list in the request body.
Only external ids belonging to the caller may be deleted.
Only external ids belonging to the caller may be deleted. Users that have
link:access-control.html#capability_modifyAccount[Modify Account] can delete
external ids that belong to other accounts.
.Request
----

2
java/com/google/gerrit/server/restapi/account/DeleteExternalIds.java
View File

@@ -73,7 +73,7 @@ public class DeleteExternalIds implements RestModifyView<AccountResource, List<S
public Response<?> apply(AccountResource resource, List<String> extIds)
throws RestApiException, IOException, ConfigInvalidException, PermissionBackendException {
if (!self.get().hasSameAccountId(resource.getUser())) {
permissionBackend.currentUser().check(GlobalPermission.ACCESS_DATABASE);
permissionBackend.currentUser().check(GlobalPermission.MODIFY_ACCOUNT);
}
if (extIds == null || extIds.isEmpty()) {

2
java/com/google/gerrit/server/restapi/account/GetExternalIds.java
View File

@@ -67,7 +67,7 @@ public class GetExternalIds implements RestReadView<AccountResource> {
public Response<List<AccountExternalIdInfo>> apply(AccountResource resource)
throws RestApiException, IOException, PermissionBackendException {
if (!self.get().hasSameAccountId(resource.getUser())) {
permissionBackend.currentUser().check(GlobalPermission.ACCESS_DATABASE);
permissionBackend.currentUser().check(GlobalPermission.MODIFY_ACCOUNT);
}
Collection<ExternalId> ids = externalIds.byAccount(resource.getUser().getAccountId());

14
javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java
View File

@@ -132,14 +132,14 @@ public class ExternalIdIT extends AbstractDaemonTest {
AuthException thrown =
assertThrows(
AuthException.class, () -> gApi.accounts().id(admin.id().get()).getExternalIds());
assertThat(thrown).hasMessageThat().contains("access database not permitted");
assertThat(thrown).hasMessageThat().contains("modify account not permitted");
}
@Test
public void getExternalIdsOfOtherUserWithAccessDatabase() throws Exception {
public void getExternalIdsOfOtherUserWithModifyAccount() throws Exception {
projectOperations
.allProjectsForUpdate()
.add(allowCapability(GlobalCapability.ACCESS_DATABASE).group(REGISTERED_USERS))
.add(allowCapability(GlobalCapability.MODIFY_ACCOUNT).group(REGISTERED_USERS))
.update();
Collection<ExternalId> expectedIds = getAccountState(admin.id()).externalIds();
@@ -193,7 +193,7 @@ public class ExternalIdIT extends AbstractDaemonTest {
gApi.accounts()
.id(admin.id().get())
.deleteExternalIds(extIds.stream().map(e -> e.identity).collect(toList())));
assertThat(thrown).hasMessageThat().contains("access database not permitted");
assertThat(thrown).hasMessageThat().contains("modify account not permitted");
}
@Test
@@ -213,10 +213,10 @@ public class ExternalIdIT extends AbstractDaemonTest {
}
@Test
public void deleteExternalIdsOfOtherUserWithAccessDatabase() throws Exception {
public void deleteExternalIdsOfOtherUserWithModifyAccount() throws Exception {
projectOperations
.allProjectsForUpdate()
.add(allowCapability(GlobalCapability.ACCESS_DATABASE).group(REGISTERED_USERS))
.add(allowCapability(GlobalCapability.MODIFY_ACCOUNT).group(REGISTERED_USERS))
.update();
List<AccountExternalIdInfo> externalIds = gApi.accounts().self().getExternalIds();
@@ -464,7 +464,7 @@ public class ExternalIdIT extends AbstractDaemonTest {
public void readExternalIdsWhenInvalidExternalIdsExist() throws Exception {
projectOperations
.allProjectsForUpdate()
.add(allowCapability(GlobalCapability.ACCESS_DATABASE).group(REGISTERED_USERS))
.add(allowCapability(GlobalCapability.MODIFY_ACCOUNT).group(REGISTERED_USERS))
.update();
requestScopeOperations.resetCurrentApiUser();