opendev/gerrit
Code Issues Proposed changes

Change the default permissions from READ on refs/* to refs/heads/*

Browse Source 
The default permissions are initialized on "All-Projects" creation. This
currently grants the following access permissions on refs/*:
[access "refs/*"]
  read = group Administrators
  read = group Anonymous Users
  revert = group Registered Users

This change modifies these default permissions and moves the second and
third rows to the refs/heads/* section instead.

[access "refs/*"]
  read = group Administrators
[access "refs/heads/*"]
  read = group Anonymous Users
  revert = group Registered Users

With this change, refs/draft-comments, refs/starred-changes and
refs/meta/version are not accessible anymore. For the first two, I
modified the code so that they are implicitly visible to all users. This
won't hurt since we only allow access to the refs belonging to the
calling user (e.g. a user cannot view drafts of another user). For
refs/meta/version, I added a section for it in All-Projects
initialization, such that anonymous users are granted read access.

In the future, we can remove visibility to refs/draft-comments and
refs/starred-changes since they are not actually needed. We keep them in
this change to preserve the current behavior for refs visibility.

Change-Id: I770cd11f10cfb369abe00d34fe9a3287bdf62cfa
This commit is contained in:
Youssef Elghareeb
2020-11-10 15:50:38 +01:00
parent 7164b9485c
commit 3889cceabe
9 changed files with 69 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
java/com/google/gerrit/entities/RefNames.java
View File

@@ -271,6 +271,16 @@ public class RefNames {
return ref.startsWith(REFS_DELETED_GROUPS);
}
/** Returns true if the provided ref is for draft comments. */
public static boolean isRefsDraftsComments(String ref) {
return ref.startsWith(REFS_DRAFT_COMMENTS);
}
/** Returns true if the provided ref is for starred changes. */
public static boolean isRefsStarredChanges(String ref) {
return ref.startsWith(REFS_STARRED_CHANGES);
}
/**
* Whether the ref is used for storing group data in NoteDb. Returns {@code true} for all group
* branches, refs/meta/group-names and deleted group branches.

11
java/com/google/gerrit/server/permissions/RefVisibilityControl.java
View File

@@ -118,9 +118,14 @@ class RefVisibilityControl {
Account.Id accountId;
if ((accountId = Account.Id.fromRef(refName)) != null) {
// Account ref is visible only to the corresponding account.
if (accountId.equals(currentUserAccountId)
&& projectControl.controlForRef(refName).hasReadPermissionOnRef(true)) {
return true;
if (accountId.equals(currentUserAccountId)) {
// Always allow visibility to refs/draft-comments and refs/starred-changes. For all other
// refs, check if the user has read permissions.
if (RefNames.isRefsDraftsComments(refName)
|| RefNames.isRefsStarredChanges(refName)
|| projectControl.controlForRef(refName).hasReadPermissionOnRef(true)) {
return true;
}
}
return false;
}

16
java/com/google/gerrit/server/schema/AllProjectsCreator.java
View File

@@ -176,14 +176,16 @@ public class AllProjectsCreator {
grant(config, refsFor, Permission.ADD_PATCH_SET, registered);
});
config.upsertAccessSection(
"refs/meta/version",
version -> {
grant(config, version, Permission.READ, anonymous);
});
grant(config, heads, codeReviewLabel, -1, 1, registered);
grant(config, heads, Permission.FORGE_AUTHOR, registered);
config.upsertAccessSection(
"refs/*",
all -> {
grant(config, all, Permission.REVERT, registered);
});
grant(config, heads, Permission.READ, anonymous);
grant(config, heads, Permission.REVERT, registered);
config.upsertAccessSection(
"refs/for/" + AccessSection.ALL,
@@ -213,7 +215,7 @@ public class AllProjectsCreator {
config.upsertAccessSection(
AccessSection.ALL,
all -> {
grant(config, all, Permission.READ, adminsGroup, anonymous);
grant(config, all, Permission.READ, adminsGroup);
});
config.upsertAccessSection(

6
java/com/google/gerrit/server/schema/testing/AllProjectsCreatorTestUtil.java
View File

@@ -54,14 +54,14 @@ public class AllProjectsCreatorTestUtil {
ImmutableList.of(
"[access \"refs/*\"]",
" read = group Administrators",
" read = group Anonymous Users",
" revert = group Registered Users",
"[access \"refs/for/*\"]",
" addPatchSet = group Registered Users",
"[access \"refs/for/refs/*\"]",
" push = group Registered Users",
" pushMerge = group Registered Users",
"[access \"refs/heads/*\"]",
" read = group Anonymous Users",
" revert = group Registered Users",
" create = group Administrators",
" create = group Project Owners",
" editTopicName = +force group Administrators",
@@ -88,6 +88,8 @@ public class AllProjectsCreatorTestUtil {
" read = group Project Owners",
" submit = group Administrators",
" submit = group Project Owners",
"[access \"refs/meta/version\"]",
" read = group Anonymous Users",
"[access \"refs/tags/*\"]",
" create = group Administrators",
" create = group Project Owners",

24
javatests/com/google/gerrit/acceptance/api/project/CheckAccessIT.java
View File

@@ -222,6 +222,7 @@ public class CheckAccessIT extends AbstractDaemonTest {
public void accessible() throws Exception {
List<TestCase> inputs =
ImmutableList.of(
// Test 1
TestCase.projectRefPerm(
user.email(),
normalProject.get(),
@@ -231,10 +232,11 @@ public class CheckAccessIT extends AbstractDaemonTest {
ImmutableList.of(
"'user' can perform 'read' with force=false on project '"
+ normalProject.get()
+ "' for ref 'refs/*'",
+ "' for ref 'refs/meta/version'",
"'user' cannot perform 'viewPrivateChanges' with force=false on project '"
+ normalProject.get()
+ "' for ref 'refs/heads/master'")),
// Test 2
TestCase.project(
user.email(),
normalProject.get(),
@@ -242,7 +244,8 @@ public class CheckAccessIT extends AbstractDaemonTest {
ImmutableList.of(
"'user' can perform 'read' with force=false on project '"
+ normalProject.get()
+ "' for ref 'refs/*'")),
+ "' for ref 'refs/meta/version'")),
// Test 3
TestCase.project(
user.email(),
secretProject.get(),
@@ -250,7 +253,11 @@ public class CheckAccessIT extends AbstractDaemonTest {
ImmutableList.of(
"'user' cannot perform 'read' with force=false on project '"
+ secretProject.get()
+ "' for ref 'refs/*' because this permission is blocked")),
+ "' for ref 'refs/meta/version' because this permission is blocked",
"'user' cannot perform 'read' with force=false on project '"
+ secretProject.get()
+ "' for ref 'refs/heads/*' because this permission is blocked")),
// Test 4
TestCase.projectRef(
user.email(),
secretRefProject.get(),
@@ -263,6 +270,7 @@ public class CheckAccessIT extends AbstractDaemonTest {
"'user' cannot perform 'read' with force=false on project '"
+ secretRefProject.get()
+ "' for ref 'refs/heads/secret/master' because this permission is blocked")),
// Test 5
TestCase.projectRef(
privilegedUser.email(),
secretRefProject.get(),
@@ -275,6 +283,7 @@ public class CheckAccessIT extends AbstractDaemonTest {
"'privilegedUser' can perform 'read' with force=false on project '"
+ secretRefProject.get()
+ "' for ref 'refs/heads/secret/master'")),
// Test 6
TestCase.projectRef(
privilegedUser.email(),
normalProject.get(),
@@ -283,7 +292,8 @@ public class CheckAccessIT extends AbstractDaemonTest {
ImmutableList.of(
"'privilegedUser' can perform 'read' with force=false on project '"
+ normalProject.get()
+ "' for ref 'refs/*'")),
+ "' for ref 'refs/meta/version'")),
// Test 7
TestCase.projectRef(
privilegedUser.email(),
secretProject.get(),
@@ -293,6 +303,7 @@ public class CheckAccessIT extends AbstractDaemonTest {
"'privilegedUser' can perform 'read' with force=false on project '"
+ secretProject.get()
+ "' for ref 'refs/*'")),
// Test 8
TestCase.projectRefPerm(
privilegedUser.email(),
normalProject.get(),
@@ -302,10 +313,11 @@ public class CheckAccessIT extends AbstractDaemonTest {
ImmutableList.of(
"'privilegedUser' can perform 'read' with force=false on project '"
+ normalProject.get()
+ "' for ref 'refs/*'",
+ "' for ref 'refs/meta/version'",
"'privilegedUser' can perform 'viewPrivateChanges' with force=false on project '"
+ normalProject.get()
+ "' for ref 'refs/heads/master'")),
// Test 9
TestCase.projectRefPerm(
privilegedUser.email(),
normalProject.get(),
@@ -315,7 +327,7 @@ public class CheckAccessIT extends AbstractDaemonTest {
ImmutableList.of(
"'privilegedUser' can perform 'read' with force=false on project '"
+ normalProject.get()
+ "' for ref 'refs/*'",
+ "' for ref 'refs/meta/version'",
"'privilegedUser' can perform 'forgeServerAsCommitter' with force=false on project '"
+ normalProject.get()
+ "' for ref 'refs/heads/master'")));

7
javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.rest.change;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.entities.Permission.READ;
import static com.google.gerrit.entities.RefNames.changeMetaRef;
@@ -708,6 +709,9 @@ public class CreateChangeIT extends AbstractDaemonTest {
projectOperations
.project(project)
.forUpdate()
// Allow reading for refs/meta/config so that the project is visible to the user. Otherwise
// the request will fail with an UnprocessableEntityException "Project not found:".
.add(allow(READ).ref("refs/meta/config").group(REGISTERED_USERS))
.add(block(READ).ref("refs/heads/*").group(REGISTERED_USERS))
.update();
requestScopeOperations.setApiUser(user.id());
@@ -731,6 +735,9 @@ public class CreateChangeIT extends AbstractDaemonTest {
projectOperations
.project(project)
.forUpdate()
// Allow reading for refs/meta/config so that the project is visible to the user. Otherwise
// the request will fail with an UnprocessableEntityException "Project not found:".
.add(allow(READ).ref("refs/meta/config").group(REGISTERED_USERS))
.add(block(READ).ref("refs/heads/*").group(REGISTERED_USERS))
.update();
requestScopeOperations.setApiUser(user.id());

13
javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java
View File

@@ -78,6 +78,9 @@ public class AccessIT extends AbstractDaemonTest {
private static final String REFS_ALL = Constants.R_REFS + "*";
private static final String REFS_HEADS = Constants.R_HEADS + "*";
private static final String REFS_META_VERSION = "refs/meta/version";
private static final String REFS_DRAFTS = "refs/draft-comments/*";
private static final String REFS_STARRED_CHANGES = "refs/starred-changes/*";
private static final String LABEL_CODE_REVIEW = "Code-Review";
@@ -496,7 +499,10 @@ public class AccessIT extends AbstractDaemonTest {
AccessSectionInfo accessSectionInfo = createAccessSectionInfoDenyAll();
// Disallow READ
accessInput.add.put(REFS_ALL, accessSectionInfo);
accessInput.add.put(REFS_META_VERSION, accessSectionInfo);
accessInput.add.put(REFS_DRAFTS, accessSectionInfo);
accessInput.add.put(REFS_STARRED_CHANGES, accessSectionInfo);
accessInput.add.put(REFS_HEADS, accessSectionInfo);
pApi().access(accessInput);
requestScopeOperations.setApiUser(user.id());
@@ -510,7 +516,10 @@ public class AccessIT extends AbstractDaemonTest {
AccessSectionInfo accessSectionInfo = createAccessSectionInfoDenyAll();
// Disallow READ
accessInput.add.put(REFS_ALL, accessSectionInfo);
accessInput.add.put(REFS_META_VERSION, accessSectionInfo);
accessInput.add.put(REFS_DRAFTS, accessSectionInfo);
accessInput.add.put(REFS_STARRED_CHANGES, accessSectionInfo);
accessInput.add.put(REFS_HEADS, accessSectionInfo);
pApi().access(accessInput);
// Create a change to apply

1
javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java
View File

@@ -128,6 +128,7 @@ public class DeleteBranchIT extends AbstractDaemonTest {
projectOperations
.project(project)
.forUpdate()
.add(allow(Permission.READ).ref(metaRef).group(REGISTERED_USERS))
.add(allow(Permission.CREATE).ref(metaRef).group(REGISTERED_USERS))
.add(allow(Permission.PUSH).ref(metaRef).group(REGISTERED_USERS))
.update();

2
javatests/com/google/gerrit/integration/git/GitProtocolV2IT.java
View File

@@ -91,7 +91,7 @@ public class GitProtocolV2IT extends StandaloneSiteTest {
projectOperations
.project(project)
.forUpdate()
.add(deny(Permission.READ).ref("refs/*").group(SystemGroupBackend.ANONYMOUS_USERS))
.add(deny(Permission.READ).ref("refs/heads/*").group(SystemGroupBackend.ANONYMOUS_USERS))
.add(
allow(Permission.READ)
.ref("refs/heads/master")