From 55c1e652ec0edff7b5450fed34d0744b98fcf69a Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Tue, 14 Jan 2020 12:43:51 +0100 Subject: [PATCH 1/3] Do not perform the workspace namespace validation if the current user is anonymous. This will skip the check in the scheduled jobs, where we are in control of the data and will not affect the user initiated creations or updates where an anonymous user would be rejected anyway. Signed-off-by: Lukas Krejci --- ...K8sInfraNamespaceWsAttributeValidator.java | 10 +++++- ...nfraNamespaceWsAttributeValidatorTest.java | 34 +++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/K8sInfraNamespaceWsAttributeValidator.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/K8sInfraNamespaceWsAttributeValidator.java index 2e987629cb2..49fe16464a6 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/K8sInfraNamespaceWsAttributeValidator.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/K8sInfraNamespaceWsAttributeValidator.java @@ -21,6 +21,7 @@ import javax.inject.Provider; import org.eclipse.che.api.core.ValidationException; import org.eclipse.che.api.workspace.server.WorkspaceAttributeValidator; +import org.eclipse.che.commons.env.EnvironmentContext; import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesNamespaceFactory; /** @@ -76,7 +77,14 @@ public void validate(Map attributes) throws ValidationException + "')"); } - namespaceFactoryProvider.get().checkIfNamespaceIsAllowed(namespace); + // the current user is going to be anonymous only during internal scheduled jobs. In those + // circumstances, we don't actually need to check the namespace validity, because those + // workflows are defined by our code and are therefore "safe". User-initiated workspace + // creation or update is never going to succeed using an anonymous user anyway, so again, + // skipping this check for anonymous user is safe. + if (!EnvironmentContext.getCurrent().getSubject().isAnonymous()) { + namespaceFactoryProvider.get().checkIfNamespaceIsAllowed(namespace); + } } } diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/K8sInfraNamespaceWsAttributeValidatorTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/K8sInfraNamespaceWsAttributeValidatorTest.java index 52cbd4663ef..4bbe6ad9167 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/K8sInfraNamespaceWsAttributeValidatorTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/K8sInfraNamespaceWsAttributeValidatorTest.java @@ -14,13 +14,20 @@ import static java.util.Collections.emptyMap; import static org.eclipse.che.api.workspace.shared.Constants.WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyZeroInteractions; +import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableMap; import javax.inject.Provider; import org.eclipse.che.api.core.ValidationException; +import org.eclipse.che.commons.env.EnvironmentContext; +import org.eclipse.che.commons.subject.Subject; import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesNamespaceFactory; import org.mockito.InjectMocks; import org.mockito.Mock; @@ -46,6 +53,7 @@ public class K8sInfraNamespaceWsAttributeValidatorTest { @BeforeMethod public void setUp() { lenient().when(namespaceFactoryProvider.get()).thenReturn(namespaceFactory); + EnvironmentContext.setCurrent(new EnvironmentContext()); } @Test( @@ -82,6 +90,7 @@ public void testWhenNamespaceFactoryThrowsErrorOnCheckingIfNamespaceIsAllowed() .when(namespaceFactory) .checkIfNamespaceIsAllowed(anyString()); + setNonAnonymousUserInContext(); validator.validate(ImmutableMap.of(WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE, "any")); } @@ -112,8 +121,33 @@ public void shouldDoNotAllowToRemoveNamespaceAttribute() throws ValidationExcept emptyMap()); } + @Test + public void shouldNotCheckIfNamespaceIsAllowedWhenUsingAnonymousUser() + throws ValidationException { + validator.validate(ImmutableMap.of(WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE, "ns")); + verify(namespaceFactory, never()).checkIfNamespaceIsAllowed(anyString()); + } + + @Test + public void shouldCheckIfNamespaceIsAllowedWhenUsingNonAnonymousUser() + throws ValidationException { + setNonAnonymousUserInContext(); + validator.validate(ImmutableMap.of(WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE, "ns")); + verify(namespaceFactory).checkIfNamespaceIsAllowed(eq("ns")); + } + @DataProvider public Object[][] invalidNamespaces() { return new String[][] {{"name!space"}, {"name@space"}, {"-namespace"}, {"namespace-"}}; } + + private void setNonAnonymousUserInContext() { + Subject subj = mock(Subject.class); + when(subj.isAnonymous()).thenReturn(false); + + EnvironmentContext ctx = new EnvironmentContext(); + ctx.setSubject(subj); + + EnvironmentContext.setCurrent(ctx); + } } From ebbcfc42ab618bc44ba16a11640bf9c01c5c1edd Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 16 Jan 2020 14:02:27 +0100 Subject: [PATCH 2/3] Rework the namespace validation to not use the anonymous user hack based on the suggestion by @sleshche. Also updated the javadocs to clarify the responsibilities of the validation methods. Signed-off-by: Lukas Krejci --- ...K8sInfraNamespaceWsAttributeValidator.java | 21 +++++----- ...nfraNamespaceWsAttributeValidatorTest.java | 38 +++++++------------ .../server/WorkspaceAttributeValidator.java | 3 ++ .../workspace/server/WorkspaceValidator.java | 5 +-- 4 files changed, 29 insertions(+), 38 deletions(-) diff --git a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/K8sInfraNamespaceWsAttributeValidator.java b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/K8sInfraNamespaceWsAttributeValidator.java index 49fe16464a6..ed0b0065395 100644 --- a/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/K8sInfraNamespaceWsAttributeValidator.java +++ b/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/K8sInfraNamespaceWsAttributeValidator.java @@ -21,7 +21,6 @@ import javax.inject.Provider; import org.eclipse.che.api.core.ValidationException; import org.eclipse.che.api.workspace.server.WorkspaceAttributeValidator; -import org.eclipse.che.commons.env.EnvironmentContext; import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesNamespaceFactory; /** @@ -77,14 +76,7 @@ public void validate(Map attributes) throws ValidationException + "')"); } - // the current user is going to be anonymous only during internal scheduled jobs. In those - // circumstances, we don't actually need to check the namespace validity, because those - // workflows are defined by our code and are therefore "safe". User-initiated workspace - // creation or update is never going to succeed using an anonymous user anyway, so again, - // skipping this check for anonymous user is safe. - if (!EnvironmentContext.getCurrent().getSubject().isAnonymous()) { - namespaceFactoryProvider.get().checkIfNamespaceIsAllowed(namespace); - } + namespaceFactoryProvider.get().checkIfNamespaceIsAllowed(namespace); } } @@ -117,6 +109,17 @@ public void validateUpdate(Map existing, Map upd WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE, existingNamespace)); } + if (isNullOrEmpty(existingNamespace)) { + // this would mean that the user made an update to the workspace without it having the + // namespace attribute stored. This is very, very unlikely, because the setting of attributes + // happens during the creation process. But let's just cover this case anyway, just to be + // sure. + validate(update); + + // everything is fine. We allow to change infra namespace in such case. + return; + } + if (!updateNamespace.equals(existingNamespace)) { throw new ValidationException( format( diff --git a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/K8sInfraNamespaceWsAttributeValidatorTest.java b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/K8sInfraNamespaceWsAttributeValidatorTest.java index 4bbe6ad9167..1c167c57ddf 100644 --- a/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/K8sInfraNamespaceWsAttributeValidatorTest.java +++ b/infrastructures/kubernetes/src/test/java/org/eclipse/che/workspace/infrastructure/kubernetes/K8sInfraNamespaceWsAttributeValidatorTest.java @@ -13,21 +13,18 @@ import static java.util.Collections.emptyMap; import static org.eclipse.che.api.workspace.shared.Constants.WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.lenient; -import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyZeroInteractions; -import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableMap; import javax.inject.Provider; import org.eclipse.che.api.core.ValidationException; -import org.eclipse.che.commons.env.EnvironmentContext; -import org.eclipse.che.commons.subject.Subject; import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesNamespaceFactory; import org.mockito.InjectMocks; import org.mockito.Mock; @@ -53,7 +50,6 @@ public class K8sInfraNamespaceWsAttributeValidatorTest { @BeforeMethod public void setUp() { lenient().when(namespaceFactoryProvider.get()).thenReturn(namespaceFactory); - EnvironmentContext.setCurrent(new EnvironmentContext()); } @Test( @@ -90,7 +86,6 @@ public void testWhenNamespaceFactoryThrowsErrorOnCheckingIfNamespaceIsAllowed() .when(namespaceFactory) .checkIfNamespaceIsAllowed(anyString()); - setNonAnonymousUserInContext(); validator.validate(ImmutableMap.of(WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE, "any")); } @@ -122,32 +117,25 @@ public void shouldDoNotAllowToRemoveNamespaceAttribute() throws ValidationExcept } @Test - public void shouldNotCheckIfNamespaceIsAllowedWhenUsingAnonymousUser() - throws ValidationException { - validator.validate(ImmutableMap.of(WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE, "ns")); - verify(namespaceFactory, never()).checkIfNamespaceIsAllowed(anyString()); + public void shouldValidateFullyIfExistingIsEmpty() throws ValidationException { + validator.validateUpdate( + emptyMap(), + ImmutableMap.of(WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE, "che-workspaces")); + + verify(namespaceFactory).checkIfNamespaceIsAllowed(eq("che-workspaces")); } @Test - public void shouldCheckIfNamespaceIsAllowedWhenUsingNonAnonymousUser() - throws ValidationException { - setNonAnonymousUserInContext(); - validator.validate(ImmutableMap.of(WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE, "ns")); - verify(namespaceFactory).checkIfNamespaceIsAllowed(eq("ns")); + public void shouldNotValidateFullyIfExistingIsNotEmpty() throws ValidationException { + validator.validateUpdate( + ImmutableMap.of(WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE, "che-workspaces"), + ImmutableMap.of(WORKSPACE_INFRASTRUCTURE_NAMESPACE_ATTRIBUTE, "che-workspaces")); + + verify(namespaceFactory, never()).checkIfNamespaceIsAllowed(any()); } @DataProvider public Object[][] invalidNamespaces() { return new String[][] {{"name!space"}, {"name@space"}, {"-namespace"}, {"namespace-"}}; } - - private void setNonAnonymousUserInContext() { - Subject subj = mock(Subject.class); - when(subj.isAnonymous()).thenReturn(false); - - EnvironmentContext ctx = new EnvironmentContext(); - ctx.setSubject(subj); - - EnvironmentContext.setCurrent(ctx); - } } diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceAttributeValidator.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceAttributeValidator.java index 1143c261e95..bd52d07de0a 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceAttributeValidator.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceAttributeValidator.java @@ -36,6 +36,9 @@ public interface WorkspaceAttributeValidator { /** * Validates if the specified workspace attributes can be updated with new values. * + * Note that this method must not allow updates that would not validate using the + * {@link #validate(Map)} method. + * *

This check includes: * *

    diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceValidator.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceValidator.java index d161be86003..2564ffd91c8 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceValidator.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceValidator.java @@ -123,8 +123,7 @@ public void validateAttributes(Map attributes) throws Validation } /** - * Checks whether workspace attributes are valid on updating. The attribute is valid if it's key - * is not null & not empty & is not prefixed with 'codenvy'. + * Checks whether workspace attributes are valid on updating. * * @param existing actual attributes * @param update new attributes that are going to be stored instead of existing @@ -132,8 +131,6 @@ public void validateAttributes(Map attributes) throws Validation */ public void validateUpdateAttributes(Map existing, Map update) throws ValidationException { - validateAttributes(update); - for (WorkspaceAttributeValidator attributeValidator : attributeValidators) { attributeValidator.validateUpdate(existing, update); } From 9452d1aa3b4f629f7ccbc09bc9cd5df416220074 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Thu, 16 Jan 2020 14:10:35 +0100 Subject: [PATCH 3/3] javadoc formatting Signed-off-by: Lukas Krejci --- .../che/api/workspace/server/WorkspaceAttributeValidator.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceAttributeValidator.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceAttributeValidator.java index bd52d07de0a..8181ce19469 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceAttributeValidator.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceAttributeValidator.java @@ -36,8 +36,8 @@ public interface WorkspaceAttributeValidator { /** * Validates if the specified workspace attributes can be updated with new values. * - * Note that this method must not allow updates that would not validate using the - * {@link #validate(Map)} method. + *

    Note that this method must not allow updates that would not validate using the {@link + * #validate(Map)} method. * *

    This check includes: *