From d03763671b3ce912db4a15a4e534a6f7e3eb00b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Tue, 6 Oct 2020 15:42:26 +0200 Subject: [PATCH] Mind review remarks --- .../users/components/apiKeys/AddApiKey.tsx | 2 +- .../components/apiKeys/ApiKeyCreatedModal.tsx | 9 +++- .../users/components/apiKeys/ApiKeyEntry.tsx | 10 ++-- .../scm/api/v2/resources/ApiKeyResource.java | 4 +- .../BrowserResultToFileObjectDtoMapper.java | 8 +-- .../sonia/scm/security/ApiKeyCollection.java | 8 +-- .../scm/security/ScmWildcardPermission.java | 53 ++++++++++++++++++- .../scm/security/ApiKeyTokenHandlerTest.java | 10 ++-- .../java/sonia/scm/security/ScopesTest.java | 2 +- 9 files changed, 79 insertions(+), 27 deletions(-) diff --git a/scm-ui/ui-webapp/src/users/components/apiKeys/AddApiKey.tsx b/scm-ui/ui-webapp/src/users/components/apiKeys/AddApiKey.tsx index 9698a3af6e..216f33d425 100644 --- a/scm-ui/ui-webapp/src/users/components/apiKeys/AddApiKey.tsx +++ b/scm-ui/ui-webapp/src/users/components/apiKeys/AddApiKey.tsx @@ -64,7 +64,7 @@ const AddApiKey: FC = ({ if (!availableRepositoryRoles) { fetchAvailablePermissionsIfNeeded(repositoryRolesLink, repositoryVerbsLink); } - }); + }, [repositoryRolesLink, repositoryVerbsLink]); const isValid = () => { return !!displayName && !!permissionRole; diff --git a/scm-ui/ui-webapp/src/users/components/apiKeys/ApiKeyCreatedModal.tsx b/scm-ui/ui-webapp/src/users/components/apiKeys/ApiKeyCreatedModal.tsx index 68faef8697..2dfdcc0c01 100644 --- a/scm-ui/ui-webapp/src/users/components/apiKeys/ApiKeyCreatedModal.tsx +++ b/scm-ui/ui-webapp/src/users/components/apiKeys/ApiKeyCreatedModal.tsx @@ -36,6 +36,11 @@ const KeyArea = styled.textarea` white-space: nowrap; overflow: auto; font-family: "Courier New", Monaco, Menlo, "Ubuntu Mono", "source-code-pro", monospace; + height: 3rem; +`; + +const NoLeftMargin = styled.div` + margin-left: -1rem; `; const ApiKeyCreatedModal: FC = ({ addedKey, close }) => { @@ -60,9 +65,9 @@ const ApiKeyCreatedModal: FC = ({ addedKey, close }) => {
-
+ -
+ ); diff --git a/scm-ui/ui-webapp/src/users/components/apiKeys/ApiKeyEntry.tsx b/scm-ui/ui-webapp/src/users/components/apiKeys/ApiKeyEntry.tsx index ff349d11bf..89feeade1a 100644 --- a/scm-ui/ui-webapp/src/users/components/apiKeys/ApiKeyEntry.tsx +++ b/scm-ui/ui-webapp/src/users/components/apiKeys/ApiKeyEntry.tsx @@ -23,7 +23,7 @@ */ import React, { FC } from "react"; -import { DateFromNow, Icon } from "@scm-manager/ui-components"; +import { DateFromNow, DeleteButton } from "@scm-manager/ui-components"; import { ApiKey } from "./SetApiKeys"; import { Link } from "@scm-manager/ui-types"; import { useTranslation } from "react-i18next"; @@ -38,11 +38,7 @@ export const ApiKeyEntry: FC = ({ apiKey, onDelete }) => { let deleteButton; if (apiKey?._links?.delete) { deleteButton = ( - onDelete((apiKey._links.delete as Link).href)}> - - - - + onDelete((apiKey._links.delete as Link).href)}/> ); } @@ -54,7 +50,7 @@ export const ApiKeyEntry: FC = ({ apiKey, onDelete }) => { - {deleteButton} + {deleteButton} ); diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ApiKeyResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ApiKeyResource.java index f30382bc3b..e2faac3e37 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ApiKeyResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ApiKeyResource.java @@ -93,13 +93,13 @@ public class ApiKeyResource { @GET @Path("{id}") - @Produces(VndMediaType.API_KEY_COLLECTION) + @Produces(VndMediaType.API_KEY) @Operation(summary = "Get one api key for the current user", description = "Returns the registered api key with the given id for the logged in user.", tags = "User") @ApiResponse( responseCode = "200", description = "success", content = @Content( - mediaType = VndMediaType.API_KEY_COLLECTION, + mediaType = VndMediaType.API_KEY, schema = @Schema(implementation = HalRepresentation.class) ) ) diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapper.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapper.java index f289bc917e..752c7dfbd4 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapper.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapper.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.api.v2.resources; import de.otto.edison.hal.Embedded; @@ -61,8 +61,10 @@ public abstract class BrowserResultToFileObjectDtoMapper extends BaseFileObjectD @Override void applyEnrichers(Links.Builder links, Embedded.Builder embeddedBuilder, NamespaceAndName namespaceAndName, BrowserResult browserResult, FileObject fileObject) { EdisonHalAppender appender = new EdisonHalAppender(links, embeddedBuilder); - // we call enrichers, which are only responsible for top level browseresults - applyEnrichers(appender, browserResult, namespaceAndName); + if (browserResult.getFile().equals(fileObject)) { + // we call enrichers, which are only responsible for top level browseresults + applyEnrichers(appender, browserResult, namespaceAndName); + } // we call enrichers, which are responsible for all file object top level browse result and its children applyEnrichers(appender, fileObject, namespaceAndName, browserResult, browserResult.getRevision()); } diff --git a/scm-webapp/src/main/java/sonia/scm/security/ApiKeyCollection.java b/scm-webapp/src/main/java/sonia/scm/security/ApiKeyCollection.java index 6f21a55e03..1441e8af21 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/ApiKeyCollection.java +++ b/scm-webapp/src/main/java/sonia/scm/security/ApiKeyCollection.java @@ -28,6 +28,7 @@ import lombok.AccessLevel; import lombok.AllArgsConstructor; import lombok.Getter; import lombok.NoArgsConstructor; +import org.apache.commons.collections.CollectionUtils; import javax.xml.bind.annotation.XmlAccessType; import javax.xml.bind.annotation.XmlAccessorType; @@ -37,6 +38,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.function.Predicate; +import static java.util.Collections.singletonList; import static java.util.stream.Collectors.toList; @AllArgsConstructor @@ -50,13 +52,13 @@ class ApiKeyCollection { public ApiKeyCollection add(ApiKeyWithPassphrase key) { Collection newKeys; - if (keys == null) { - newKeys = new ArrayList<>(); + if (CollectionUtils.isEmpty(keys)) { + newKeys = singletonList(key); } else { newKeys = new ArrayList<>(keys.size() + 1); newKeys.addAll(keys); + newKeys.add(key); } - newKeys.add(key); return new ApiKeyCollection(newKeys); } diff --git a/scm-webapp/src/main/java/sonia/scm/security/ScmWildcardPermission.java b/scm-webapp/src/main/java/sonia/scm/security/ScmWildcardPermission.java index 2b9193e14e..6b46cd15a0 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/ScmWildcardPermission.java +++ b/scm-webapp/src/main/java/sonia/scm/security/ScmWildcardPermission.java @@ -42,6 +42,34 @@ public class ScmWildcardPermission extends WildcardPermission { super(permissionString); } + /** + * Limits this permission to the given scope. This will result in a collection of new permissions. This + * collection can be empty (but this will not return null). Three examples: + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + * + *
This permissionScopeResulting permission(s)
repository:*:42repository:read,pull:*repository:read,pull:42
repository:read:*repository:*:42, repository:*:1337repository:read:42, repository:read:1337
user:*:*repository:read,pull:*empty
+ * @param scope The scope this permission should be limited to. + * @return A collection with the resulting permissions (mind that this can be empty, but not null). + */ Collection limit(Scope scope) { Collection result = new ArrayList<>(); for (String s : scope) { @@ -50,11 +78,24 @@ public class ScmWildcardPermission extends WildcardPermission { return result; } + /** + * Limits this permission to a scope with a single permission. For examples see {@link #limit(String)}. + * @param scope The single scope. + * @return An {@link Optional} with the resulting permission if there was a overlap between this and the scope, or + * an empty {@link Optional} otherwise. + */ Optional limit(String scope) { return limit(new ScmWildcardPermission(scope)); } + /** + * Limits this permission to a scope with a single permission. For examples see {@link #limit(String)}. + * @param scope The single scope. + * @return An {@link Optional} with the resulting permission if there was a overlap between this and the scope, or + * an empty {@link Optional} otherwise. + */ Optional limit(ScmWildcardPermission scope) { + // if one permission is a subset of the other, we can return the smaller one. if (this.implies(scope)) { return of(scope); } @@ -62,6 +103,8 @@ public class ScmWildcardPermission extends WildcardPermission { return of(this); } + // First we check, whether the subjects are the same. We do not use permissions with different subjects, so we + // either have both this the same subject, or we have no overlap. final List> theseParts = getParts(); final List> scopeParts = scope.getParts(); @@ -69,7 +112,10 @@ public class ScmWildcardPermission extends WildcardPermission { return empty(); } - String type = getEntries(scopeParts, 0).iterator().next(); + String subject = getEntries(scopeParts, 0).iterator().next(); + + // Now we create the intersections of verbs and ids to create the resulting permission + // (if not one of the resulting sets is empty) Collection verbs = intersect(theseParts, scopeParts, 1); Collection ids = intersect(theseParts, scopeParts, 2); @@ -77,7 +123,7 @@ public class ScmWildcardPermission extends WildcardPermission { return empty(); } - return of(new ScmWildcardPermission(type + ":" + String.join(",", verbs) + ":" + String.join(",", ids))); + return of(new ScmWildcardPermission(subject + ":" + String.join(",", verbs) + ":" + String.join(",", ids))); } private Collection intersect(List> theseParts, List> scopeParts, int position) { @@ -92,6 +138,9 @@ public class ScmWildcardPermission extends WildcardPermission { return CollectionUtils.intersection(theseEntries, scopeEntries); } + /** + * Handles "shortened" permissions like repository:read that should be repository:read:*. + */ private Set getEntries(List> theseParts, int position) { if (position >= theseParts.size()) { return singleton(WILDCARD_TOKEN); diff --git a/scm-webapp/src/test/java/sonia/scm/security/ApiKeyTokenHandlerTest.java b/scm-webapp/src/test/java/sonia/scm/security/ApiKeyTokenHandlerTest.java index 198335b286..390d5e6ba2 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/ApiKeyTokenHandlerTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/ApiKeyTokenHandlerTest.java @@ -38,11 +38,9 @@ class ApiKeyTokenHandlerTest { @Test void shouldSerializeAndDeserializeToken() { - final String tokenString = handler.createToken("dent", new ApiKey("42", "hg2g", "READ", now()), "some secret"); + String tokenString = handler.createToken("dent", new ApiKey("42", "hg2g", "READ", now()), "some secret"); - System.out.println(tokenString); - - final Optional token = handler.readToken(tokenString); + Optional token = handler.readToken(tokenString); assertThat(token).isNotEmpty(); assertThat(token).get().extracting("user").isEqualTo("dent"); @@ -52,14 +50,14 @@ class ApiKeyTokenHandlerTest { @Test void shouldNotFailWithInvalidTokenEncoding() { - final Optional token = handler.readToken("invalid token"); + Optional token = handler.readToken("invalid token"); assertThat(token).isEmpty(); } @Test void shouldNotFailWithInvalidTokenContent() { - final Optional token = handler.readToken(Encoders.BASE64URL.encode("{\"invalid\":\"token\"}".getBytes())); + Optional token = handler.readToken(Encoders.BASE64URL.encode("{\"invalid\":\"token\"}".getBytes())); assertThat(token).isEmpty(); } diff --git a/scm-webapp/src/test/java/sonia/scm/security/ScopesTest.java b/scm-webapp/src/test/java/sonia/scm/security/ScopesTest.java index 695dd46641..22d09e4fc5 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/ScopesTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/ScopesTest.java @@ -71,7 +71,7 @@ public class ScopesTest { } @Test - public void testFilterX() { + public void testFilterIntersectingPermissions() { Scope scope = Scope.valueOf("repository:read,write:*"); AuthorizationInfo authz = authz("repository:*:123");