Skip to content

Commit

Permalink
Remove Anonymous Role (#1052)
Browse files Browse the repository at this point in the history
Motivation:
The anonymous role poses a security risk, as described in [issue
#1048](#1048). Modifications:
- Removed the anonymous role, effectively reversing changes introduced
in [PR #917](#917).
- Added `CentralDogmaExtension.accessToken()` for adding the access
token to the testing Dogma client.
- There was a bug after we introduced the anonymous role, which is an
anonymous can create a project.
- Becuase the anonymous role is now removed, the testing dogma client
without an access token can't create a project if the auth provider is
set to dogma extension.

Result:
- The anonymous role has been removed.
  • Loading branch information
minwoox authored Nov 7, 2024
1 parent 8ca35bf commit 4d4af7e
Show file tree
Hide file tree
Showing 18 changed files with 74 additions and 153 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,20 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonNode;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;

import com.linecorp.armeria.client.WebClient;
import com.linecorp.armeria.common.auth.OAuth2Token;
import com.linecorp.armeria.server.ServerBuilder;
import com.linecorp.armeria.server.auth.AuthService;
import com.linecorp.armeria.server.auth.Authorizer;
import com.linecorp.armeria.server.grpc.GrpcService;
import com.linecorp.armeria.testing.junit5.server.ServerExtension;
import com.linecorp.centraldogma.client.CentralDogma;
import com.linecorp.centraldogma.client.armeria.ArmeriaCentralDogmaBuilder;
import com.linecorp.centraldogma.common.Change;
import com.linecorp.centraldogma.common.Entry;
import com.linecorp.centraldogma.common.Query;
Expand Down Expand Up @@ -66,6 +69,18 @@ protected void configure(CentralDogmaBuilder builder) {
builder.authProviderFactory(new TestAuthProviderFactory());
}

@Override
protected void configureClient(ArmeriaCentralDogmaBuilder builder) {
try {
final String accessToken = getAccessToken(
WebClient.of("http://127.0.0.1:" + dogma.serverAddress().getPort()),
TestAuthMessageUtil.USERNAME, TestAuthMessageUtil.PASSWORD);
builder.accessToken(accessToken);
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
}
}

@Override
protected void scaffold(CentralDogma client) {
client.createProject("foo").join();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@
public enum ProjectRole {
OWNER,
MEMBER,
GUEST,
ANONYMOUS;
GUEST;

/**
* Returns a {@link ProjectRole} matched with the specified {@code str}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.google.common.io.Resources;

import com.linecorp.armeria.client.BlockingWebClient;
Expand All @@ -35,6 +36,7 @@
import com.linecorp.armeria.common.ResponseEntity;
import com.linecorp.armeria.common.auth.AuthToken;
import com.linecorp.centraldogma.client.CentralDogma;
import com.linecorp.centraldogma.client.armeria.ArmeriaCentralDogmaBuilder;
import com.linecorp.centraldogma.internal.api.v1.MirrorDto;
import com.linecorp.centraldogma.internal.api.v1.PushResultDto;
import com.linecorp.centraldogma.server.CentralDogmaBuilder;
Expand All @@ -60,6 +62,18 @@ protected void configure(CentralDogmaBuilder builder) {
builder.administrators(USERNAME);
}

@Override
protected void configureClient(ArmeriaCentralDogmaBuilder builder) {
try {
final String accessToken = getAccessToken(
WebClient.of("http://127.0.0.1:" + dogma.serverAddress().getPort()),
USERNAME, PASSWORD);
builder.accessToken(accessToken);
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
}
}

@Override
protected void scaffold(CentralDogma client) {
client.createProject(FOO_PROJ).join();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import com.linecorp.armeria.common.ResponseEntity;
import com.linecorp.armeria.common.auth.AuthToken;
import com.linecorp.centraldogma.client.CentralDogma;
import com.linecorp.centraldogma.client.armeria.ArmeriaCentralDogmaBuilder;
import com.linecorp.centraldogma.common.Revision;
import com.linecorp.centraldogma.internal.api.v1.MirrorDto;
import com.linecorp.centraldogma.internal.api.v1.PushResultDto;
Expand All @@ -67,6 +68,18 @@ protected void configure(CentralDogmaBuilder builder) {
builder.administrators(USERNAME);
}

@Override
protected void configureClient(ArmeriaCentralDogmaBuilder builder) {
try {
final String accessToken = getAccessToken(
WebClient.of("http://127.0.0.1:" + dogma.serverAddress().getPort()),
USERNAME, PASSWORD);
builder.accessToken(accessToken);
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
}
}

@Override
protected void scaffold(CentralDogma client) {
client.createProject(FOO_PROJ).join();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,7 @@ private static ProjectRole getUserRole(Project project, User user) {
}

if (role == null) {
if (user.isAnonymous()) {
role = ProjectRole.ANONYMOUS;
} else {
role = ProjectRole.GUEST;
}
role = ProjectRole.GUEST;
}

return role;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,11 @@
import com.linecorp.armeria.server.ServiceRequestContext;
import com.linecorp.armeria.server.auth.AuthTokenExtractors;
import com.linecorp.armeria.server.auth.Authorizer;
import com.linecorp.centraldogma.internal.CsrfToken;
import com.linecorp.centraldogma.server.internal.admin.auth.AuthUtil;
import com.linecorp.centraldogma.server.internal.admin.service.TokenNotFoundException;
import com.linecorp.centraldogma.server.internal.api.HttpApiUtil;
import com.linecorp.centraldogma.server.metadata.Token;
import com.linecorp.centraldogma.server.metadata.Tokens;
import com.linecorp.centraldogma.server.metadata.User;
import com.linecorp.centraldogma.server.metadata.UserWithToken;

/**
Expand All @@ -62,12 +60,6 @@ public ApplicationTokenAuthorizer(Function<String, CompletionStage<Token>> token
@Override
public CompletionStage<Boolean> authorize(ServiceRequestContext ctx, HttpRequest data) {
final OAuth2Token token = AuthTokenExtractors.oAuth2().apply(data.headers());
if (token != null && token.accessToken().equals(CsrfToken.ANONYMOUS)) {
AuthUtil.setCurrentUser(ctx, User.ANONYMOUS);
HttpApiUtil.setVerboseResponses(ctx, User.ANONYMOUS);
return completedFuture(true);
}

if (token == null || !Tokens.isValidSecret(token.accessToken())) {
return completedFuture(false);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -412,17 +412,13 @@ public CompletableFuture<Revision> updatePerRolePermissions(Author author,
"Can't update the per role permission for internal repository: " + repoName);
}

final Set<Permission> anonymous = perRolePermissions.anonymous();
if (Project.REPO_META.equals(repoName)) {
final Set<Permission> guest = perRolePermissions.guest();
if (!guest.isEmpty() || !anonymous.isEmpty()) {
if (!guest.isEmpty()) {
throw new UnsupportedOperationException(
"Can't give a permission to guest or anonymous for internal repository: " + repoName);
"Can't give a permission to guest for internal repository: " + repoName);
}
}
if (anonymous.contains(Permission.WRITE)) {
throw new IllegalArgumentException("Anonymous users cannot have write permission.");
}

final JsonPointer path = JsonPointer.compile("/repos" + encodeSegment(repoName) +
"/perRolePermissions");
Expand Down Expand Up @@ -770,10 +766,9 @@ private CompletableFuture<Collection<Permission>> findPermissions0(String projec
final RepositoryMetadata repositoryMetadata = metadata.repo(repoName);
final Member member = metadata.memberOrDefault(user.id(), null);

// If the member is guest or using anonymous token.
// If the member is guest.
if (member == null) {
return !user.isAnonymous() ? repositoryMetadata.perRolePermissions().guest()
: repositoryMetadata.perRolePermissions().anonymous();
return repositoryMetadata.perRolePermissions().guest();
}
final Collection<Permission> p = repositoryMetadata.perUserPermissions().get(member.id());
if (p != null) {
Expand All @@ -790,10 +785,8 @@ private static Collection<Permission> findPerRolePermissions(RepositoryMetadata
return repositoryMetadata.perRolePermissions().owner();
case MEMBER:
return repositoryMetadata.perRolePermissions().member();
case GUEST:
return repositoryMetadata.perRolePermissions().guest();
default:
return repositoryMetadata.perRolePermissions().anonymous();
return repositoryMetadata.perRolePermissions().guest();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,20 @@

package com.linecorp.centraldogma.server.metadata;

import static com.google.common.base.MoreObjects.firstNonNull;
import static java.util.Objects.requireNonNull;

import java.util.Collection;
import java.util.EnumSet;
import java.util.Objects;
import java.util.Set;

import javax.annotation.Nullable;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;

import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.centraldogma.common.ProjectRole;
import com.linecorp.centraldogma.server.storage.repository.Repository;

Expand All @@ -55,12 +54,12 @@ public class PerRolePermissions {
*/
@Deprecated
public static final PerRolePermissions DEFAULT =
new PerRolePermissions(READ_WRITE, READ_WRITE, NO_PERMISSION, NO_PERMISSION);
new PerRolePermissions(READ_WRITE, READ_WRITE, NO_PERMISSION, null);
private static final PerRolePermissions internalPermissions =
new PerRolePermissions(READ_WRITE, NO_PERMISSION, NO_PERMISSION, NO_PERMISSION);
new PerRolePermissions(READ_WRITE, NO_PERMISSION, NO_PERMISSION, null);

/**
* Creates a {@link PerRolePermissions} which allows read/write a repository from a owner.
* Creates a {@link PerRolePermissions} which allows read/write a repository from an owner.
*/
public static PerRolePermissions ofInternal() {
return internalPermissions;
Expand All @@ -77,14 +76,14 @@ public static PerRolePermissions ofDefault() {
* Creates a {@link PerRolePermissions} which allows accessing a repository from everyone.
*/
public static PerRolePermissions ofPublic() {
return new PerRolePermissions(READ_WRITE, READ_WRITE, READ_WRITE, NO_PERMISSION);
return new PerRolePermissions(READ_WRITE, READ_WRITE, READ_WRITE, null);
}

/**
* Creates a {@link PerRolePermissions} which allows accessing a repository from a project member.
*/
public static PerRolePermissions ofPrivate() {
return new PerRolePermissions(READ_WRITE, READ_WRITE, NO_PERMISSION, NO_PERMISSION);
return new PerRolePermissions(READ_WRITE, READ_WRITE, NO_PERMISSION, null);
}

/**
Expand All @@ -102,23 +101,18 @@ public static PerRolePermissions ofPrivate() {
*/
private final Set<Permission> guest;

/**
* {@link Permission}s for a {@link ProjectRole#ANONYMOUS}.
*/
private final Set<Permission> anonymous;

/**
* Creates an instance.
*/
@JsonCreator
public PerRolePermissions(@JsonProperty("owner") Iterable<Permission> owner,
@JsonProperty("member") Iterable<Permission> member,
@JsonProperty("guest") Iterable<Permission> guest,
@JsonProperty("anonymous") @Nullable Iterable<Permission> anonymous) {
// TODO(minwoox): Remove anonymous field after the migration.
@JsonProperty("anonymous") @Nullable Iterable<Permission> unused) {
this.owner = Sets.immutableEnumSet(requireNonNull(owner, "owner"));
this.member = Sets.immutableEnumSet(requireNonNull(member, "member"));
this.guest = Sets.immutableEnumSet(requireNonNull(guest, "guest"));
this.anonymous = Sets.immutableEnumSet(firstNonNull(anonymous, ImmutableSet.of()));
}

/**
Expand All @@ -145,17 +139,9 @@ public Set<Permission> guest() {
return guest;
}

/**
* Returns the permissions granted to anonymous users.
*/
@JsonProperty
public Set<Permission> anonymous() {
return anonymous;
}

@Override
public int hashCode() {
return Objects.hash(owner, member, guest, anonymous);
return Objects.hash(owner, member, guest);
}

@Override
Expand All @@ -170,8 +156,7 @@ public boolean equals(Object o) {
final PerRolePermissions that = (PerRolePermissions) o;
return owner.equals(that.owner) &&
member.equals(that.member) &&
guest.equals(that.guest) &&
anonymous.equals(that.anonymous);
guest.equals(that.guest);
}

@Override
Expand All @@ -180,7 +165,6 @@ public String toString() {
.add("owner", owner())
.add("member", member())
.add("guest", guest())
.add("anonymous", anonymous())
.toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,13 @@ public class User implements Identifiable, Serializable {

private static final long serialVersionUID = -5429782019985526549L;

private static final String LEVEL_ANONYMOUS_STR = "LEVEL_ANONYMOUS";
private static final String LEVEL_USER_STR = "LEVEL_USER";
private static final String LEVEL_ADMIN_STR = "LEVEL_ADMIN";

// System-wide roles for a user. It is different from the role in a project.
public static final List<String> LEVEL_ANONYMOUS = ImmutableList.of(LEVEL_ANONYMOUS_STR);
public static final List<String> LEVEL_USER = ImmutableList.of(LEVEL_USER_STR);
public static final List<String> LEVEL_ADMIN = ImmutableList.of(LEVEL_ADMIN_STR, LEVEL_USER_STR);

public static final User ANONYMOUS = new User("[email protected]", LEVEL_ANONYMOUS);
public static final User DEFAULT = new User("[email protected]", LEVEL_USER);
public static final User ADMIN = new User("[email protected]", LEVEL_ADMIN);

Expand All @@ -61,8 +58,6 @@ public class User implements Identifiable, Serializable {

private final boolean isAdmin;

private final boolean isAnonymous;

/**
* Creates a new instance.
*/
Expand All @@ -76,7 +71,6 @@ public User(@JsonProperty("login") String login,
this.email = requireNonNull(email, "email");
this.roles = ImmutableList.copyOf(requireNonNull(roles, "roles"));
isAdmin = roles.stream().anyMatch(LEVEL_ADMIN_STR::equals);
isAnonymous = roles.stream().anyMatch(LEVEL_ANONYMOUS_STR::equals);
}

/**
Expand All @@ -101,7 +95,6 @@ public User(String login, List<String> roles) {

this.roles = ImmutableList.copyOf(roles);
isAdmin = roles.stream().anyMatch(LEVEL_ADMIN_STR::equals);
isAnonymous = roles.stream().anyMatch(LEVEL_ANONYMOUS_STR::equals);
}

/**
Expand Down Expand Up @@ -148,13 +141,6 @@ public boolean isAdmin() {
return isAdmin;
}

/**
* Returns {@code true} if this user is anonymous.
*/
public boolean isAnonymous() {
return isAnonymous;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,7 @@ void testValidProject() throws IOException {
" \"perRolePermissions\" : {\n" +
" \"owner\" : [ \"READ\", \"WRITE\" ],\n" +
" \"member\" : [ \"READ\", \"WRITE\" ],\n" +
" \"guest\" : [],\n" +
" \"anonymous\" : []\n" +
" \"guest\" : []\n" +
" },\n" +
" \"perUserPermissions\" : { },\n" +
" \"perTokenPermissions\" : { },\n" +
Expand Down Expand Up @@ -162,8 +161,7 @@ void testRemovedProject() throws IOException {
" \"perRolePermissions\" : {\n" +
" \"owner\" : [ \"READ\", \"WRITE\" ],\n" +
" \"member\" : [ \"READ\", \"WRITE\" ],\n" +
" \"guest\" : [],\n" +
" \"anonymous\" : []\n" +
" \"guest\" : []\n" +
" },\n" +
" \"perUserPermissions\" : { },\n" +
" \"perTokenPermissions\" : { },\n" +
Expand Down
Loading

0 comments on commit 4d4af7e

Please sign in to comment.