Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prohibit application tokens to access repositories with GUEST permission #1093

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Prohibit application tokens to access repositories with GUEST permission #1093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c95236
Prohibit application tokens to access repositories with GUEST permission
ikhoon Feb 5, 2025
85aec06
fix broken tests
ikhoon Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion .../src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import com.fasterxml.jackson.databind.node.JsonNodeFactory;
import com.fasterxml.jackson.databind.node.JsonNodeType;
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -1130,17 +1131,24 @@ private static <T> T handleErrorResponse(AggregatedHttpResponse res) {
final JsonNode node = toJson(res, JsonNodeType.OBJECT);
final JsonNode exceptionNode = node.get("exception");
final JsonNode messageNode = node.get("message");
String message = messageNode.textValue();

if (exceptionNode != null) {
final String typeName = exceptionNode.textValue();
if (typeName != null) {
final Function<String, CentralDogmaException> exceptionFactory =
EXCEPTION_FACTORIES.get(typeName);
if (exceptionFactory != null) {
throw exceptionFactory.apply(messageNode.textValue());
throw exceptionFactory.apply(message);
}
}
}
if (status == HttpStatus.FORBIDDEN) {
if (Strings.isNullOrEmpty(message)) {
message = "Access denied";
}
throw new PermissionException(message);
}
}

throw new CentralDogmaException("unexpected response: " + res.headers() + ", " + res.contentUtf8());
Expand Down
14 changes: 7 additions & 7 deletions ...rmission/src/test/java/com/linecorp/centraldogma/server/test/XdsMemberPermissionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@
import com.linecorp.centraldogma.client.CentralDogma;
import com.linecorp.centraldogma.client.CentralDogmaRepository;
import com.linecorp.centraldogma.client.armeria.ArmeriaCentralDogmaBuilder;
import com.linecorp.centraldogma.common.CentralDogmaException;
import com.linecorp.centraldogma.common.Change;
import com.linecorp.centraldogma.common.PermissionException;
import com.linecorp.centraldogma.server.CentralDogmaBuilder;
import com.linecorp.centraldogma.server.auth.shiro.ShiroAuthProviderFactory;
import com.linecorp.centraldogma.server.internal.credential.NoneCredential;
Expand Down Expand Up @@ -119,25 +119,25 @@ void shouldAllowMembersToAccessInternalProjects() throws Exception {
assertThatThrownBy(() -> {
userClient.createRepository("@xds", "test2").join();
}).isInstanceOf(CompletionException.class)
.hasCauseInstanceOf(CentralDogmaException.class)
.hasMessageContaining(":status=403");
.hasCauseInstanceOf(PermissionException.class)
.hasMessageContaining("You must have the OWNER project role to access the project '@xds'.");

final CentralDogmaRepository userRepo = userClient.forRepo("@xds", "test");
assertThatThrownBy(() -> {
userRepo.commit("Update test.txt", Change.ofTextUpsert("/text.txt", "bar"))
.push()
.join();
}).isInstanceOf(CompletionException.class)
.hasCauseInstanceOf(CentralDogmaException.class)
.hasMessageContaining(":status=403");
.hasCauseInstanceOf(PermissionException.class)
.hasMessageContaining("You must have the READ repository role to access the '@xds/test'.");

assertThatThrownBy(() -> {
userRepo.file("/text.txt")
.get()
.join();
}).isInstanceOf(CompletionException.class)
.hasCauseInstanceOf(CentralDogmaException.class)
.hasMessageContaining(":status=403");
.hasCauseInstanceOf(PermissionException.class)
.hasMessageContaining("You must have the READ repository role to access the '@xds/test'.");

assertThat(userWebClient.prepare()
.get("/api/v1/projects/@xds/credentials/test")
Expand Down
16 changes: 8 additions & 8 deletions ...er/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/TokenService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,14 @@

import com.linecorp.armeria.common.HttpHeaderNames;
import com.linecorp.armeria.common.HttpStatus;
import com.linecorp.armeria.common.ResponseEntity;
import com.linecorp.armeria.common.ResponseHeaders;
import com.linecorp.armeria.server.HttpStatusException;
import com.linecorp.armeria.server.ServiceRequestContext;
import com.linecorp.armeria.server.annotation.Consumes;
import com.linecorp.armeria.server.annotation.Default;
import com.linecorp.armeria.server.annotation.Delete;
import com.linecorp.armeria.server.annotation.Get;
import com.linecorp.armeria.server.annotation.HttpResult;
import com.linecorp.armeria.server.annotation.Param;
import com.linecorp.armeria.server.annotation.Patch;
import com.linecorp.armeria.server.annotation.Post;
Expand Down Expand Up @@ -106,12 +106,12 @@ public CompletableFuture<Collection<Token>> listTokens(User loginUser) {
@Post("/tokens")
@StatusCode(201)
@ResponseConverter(CreateApiResponseConverter.class)
public CompletableFuture<HttpResult<Token>> createToken(@Param String appId,
// TODO(minwoox): Remove isAdmin field.
@Param @Default("false") boolean isAdmin,
@Param @Default("false") boolean isSystemAdmin,
@Param @Nullable String secret,
Author author, User loginUser) {
public CompletableFuture<ResponseEntity<Token>> createToken(@Param String appId,
// TODO(minwoox): Remove isAdmin field.
@Param @Default("false") boolean isAdmin,
@Param @Default("false") boolean isSystemAdmin,
@Param @Nullable String secret,
Author author, User loginUser) {
final boolean isSystemAdminToken = isSystemAdmin || isAdmin;
checkArgument(!isSystemAdminToken || loginUser.isSystemAdmin(),
"Only system administrators are allowed to create a system admin-level token.");
Expand All @@ -132,7 +132,7 @@ public CompletableFuture<HttpResult<Token>> createToken(@Param String appId,
final ResponseHeaders headers = ResponseHeaders.of(HttpStatus.CREATED,
HttpHeaderNames.LOCATION,
"/tokens/" + appId);
return HttpResult.of(headers, token);
return ResponseEntity.of(headers, token);
});
}

Expand Down
85 changes: 51 additions & 34 deletions server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,30 +519,30 @@ private CompletableFuture<Revision> removeToken(String projectName, Author autho
final String commitSummary = "Remove the token '" + appId + "' from the project '" + projectName + '\'';
final ProjectMetadataTransformer transformer =
new ProjectMetadataTransformer((headRevision, projectMetadata) -> {
final Map<String, TokenRegistration> tokens = projectMetadata.tokens();
final Map<String, TokenRegistration> newTokens;
if (tokens.get(appId) == null) {
if (!quiet) {
throw new TokenNotFoundException(
"failed to find the token " + appId + " in project " + projectName);
}
newTokens = tokens;
} else {
newTokens = tokens.entrySet()
.stream()
.filter(entry -> !entry.getKey().equals(appId))
.collect(toImmutableMap(Entry::getKey, Entry::getValue));
}
final Map<String, TokenRegistration> tokens = projectMetadata.tokens();
final Map<String, TokenRegistration> newTokens;
if (tokens.get(appId) == null) {
if (!quiet) {
throw new TokenNotFoundException(
"failed to find the token " + appId + " in project " + projectName);
}
newTokens = tokens;
} else {
newTokens = tokens.entrySet()
.stream()
.filter(entry -> !entry.getKey().equals(appId))
.collect(toImmutableMap(Entry::getKey, Entry::getValue));
}

final ImmutableMap<String, RepositoryMetadata> newRepos =
removeTokenFromRepositories(appId, projectMetadata);
return new ProjectMetadata(projectMetadata.name(),
newRepos,
projectMetadata.members(),
newTokens,
projectMetadata.creation(),
projectMetadata.removal());
});
final ImmutableMap<String, RepositoryMetadata> newRepos =
removeTokenFromRepositories(appId, projectMetadata);
return new ProjectMetadata(projectMetadata.name(),
newRepos,
projectMetadata.members(),
newTokens,
projectMetadata.creation(),
projectMetadata.removal());
});
return metadataRepo.push(projectName, Project.REPO_DOGMA, author, commitSummary, transformer);
}

Expand Down Expand Up @@ -850,30 +850,42 @@ public CompletableFuture<RepositoryRole> findRepositoryRole(String projectName,
return CompletableFuture.completedFuture(RepositoryRole.ADMIN);
}
if (user instanceof UserWithToken) {
return findRepositoryRole(projectName, repoName, ((UserWithToken) user).token().appId());
return findRepositoryRole(projectName, repoName, ((UserWithToken) user).token());
}
return findRepositoryRole0(projectName, repoName, user);
}

/**
* Finds {@link RepositoryRole} of the specified {@code appId} from the specified
* Finds {@link RepositoryRole} of the specified {@link Token} from the specified
* {@code repoName} in the specified {@code projectName}. If the {@code appId} is not found,
* it will return {@code null}.
*/
public CompletableFuture<RepositoryRole> findRepositoryRole(String projectName, String repoName,
String appId) {
Token token) {
requireNonNull(projectName, "projectName");
requireNonNull(repoName, "repoName");
requireNonNull(appId, "appId");
requireNonNull(token, "token");

return getProject(projectName).thenApply(metadata -> {
final RepositoryMetadata repositoryMetadata = metadata.repo(repoName);
final Roles roles = repositoryMetadata.roles();
final String appId = token.appId();
final RepositoryRole tokenRepositoryRole = roles.tokens().get(appId);

final TokenRegistration projectTokenRegistration = metadata.tokens().get(appId);
final ProjectRole projectRole = projectTokenRegistration != null ? projectTokenRegistration.role()
: ProjectRole.GUEST;
final ProjectRole projectRole;
if (projectTokenRegistration != null) {
projectRole = projectTokenRegistration.role();
} else {
// System admin tokens were checked before this method.
assert !token.isSystemAdmin();
if (token.allowGuestAccess()) {
projectRole = ProjectRole.GUEST;
} else {
// The token is not allowed with the GUEST permission.
return null;
}
}
return repositoryRole(roles, tokenRepositoryRole, projectRole);
});
}
Expand Down Expand Up @@ -989,7 +1001,10 @@ public CompletableFuture<Revision> createToken(Author author, String appId, Stri

checkArgument(secret.startsWith(SECRET_PREFIX), "secret must start with: " + SECRET_PREFIX);

final Token newToken = new Token(appId, secret, isSystemAdmin, UserAndTimestamp.of(author));
// Does not allow guest access for normal tokens.
final boolean allowGuestAccess = isSystemAdmin;
final Token newToken = new Token(appId, secret, isSystemAdmin, allowGuestAccess,
UserAndTimestamp.of(author));
final JsonPointer appIdPath = JsonPointer.compile("/appIds" + encodeSegment(newToken.id()));
final String newTokenSecret = newToken.secret();
assert newTokenSecret != null;
Expand Down Expand Up @@ -1024,8 +1039,8 @@ public CompletableFuture<Revision> destroyToken(Author author, String appId) {
final String secret = token.secret();
assert secret != null;
final Token newToken = new Token(token.appId(), secret, token.isSystemAdmin(),
token.isSystemAdmin(), token.creation(),
token.deactivation(), userAndTimestamp);
token.isSystemAdmin(), token.allowGuestAccess(),
token.creation(), token.deactivation(), userAndTimestamp);
return new Tokens(updateMap(tokens.appIds(), appId, newToken), tokens.secrets());
});
return tokenRepo.push(INTERNAL_PROJECT_DOGMA, Project.REPO_DOGMA, author, commitSummary, transformer);
Expand Down Expand Up @@ -1087,7 +1102,8 @@ public CompletableFuture<Revision> activateToken(Author author, String appId) {
assert secret != null;
final Map<String, String> newSecrets =
addToMap(tokens.secrets(), secret, appId); // Note that the key is secret not appId.
final Token newToken = new Token(token.appId(), secret, token.isSystemAdmin(), token.creation());
final Token newToken = new Token(token.appId(), secret, token.isSystemAdmin(),
token.allowGuestAccess(), token.creation());
return new Tokens(updateMap(tokens.appIds(), appId, newToken), newSecrets);
});
return tokenRepo.push(INTERNAL_PROJECT_DOGMA, Project.REPO_DOGMA, author, commitSummary, transformer);
Expand All @@ -1111,7 +1127,8 @@ public CompletableFuture<Revision> deactivateToken(Author author, String appId)
final String secret = token.secret();
assert secret != null;
final Token newToken = new Token(token.appId(), secret, token.isSystemAdmin(),
token.isSystemAdmin(), token.creation(), userAndTimestamp, null);
token.isSystemAdmin(), token.allowGuestAccess(), token.creation(),
userAndTimestamp, null);
final Map<String, Token> newAppIds = updateMap(tokens.appIds(), appId, newToken);
final Map<String, String> newSecrets =
removeFromMap(tokens.secrets(), secret); // Note that the key is secret not appId.
Expand Down
Loading