Cleanup 'access-control' for Vespa 8

Always enable read/write protection. Warn on 'read'/'write' attributes.
Update TODOs.

6 files changed, 15 insertions, 61 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/AccessControl.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/AccessControl.java
index 4bbc5f8f990..d85f00a5bb2 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/AccessControl.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/AccessControl.java
@@ -3,9 +3,6 @@ package com.yahoo.vespa.model.container.http;
 
 import com.yahoo.component.ComponentId;
 import com.yahoo.component.ComponentSpecification;
-import com.yahoo.component.chain.dependencies.Dependencies;
-import com.yahoo.component.chain.model.ChainedComponentModel;
-import com.yahoo.container.bundle.BundleInstantiationSpecification;
 import com.yahoo.vespa.defaults.Defaults;
 import com.yahoo.vespa.model.container.ApplicationContainerCluster;
 import com.yahoo.vespa.model.container.ContainerCluster;
@@ -52,8 +49,6 @@ public class AccessControl {
     );
     public static class Builder {
         private final String domain;
-        private boolean readEnabled = false;
-        private boolean writeEnabled = true;
         private ClientAuthentication clientAuthentication = ClientAuthentication.need;
         private final Set<BindingPattern> excludeBindings = new LinkedHashSet<>();
         private Collection<Handler<?>> handlers = Collections.emptyList();
@@ -61,16 +56,6 @@ public class AccessControl {
             this.domain = domain;
         }
 
-        public Builder readEnabled(boolean readEnabled) {
-            this.readEnabled = readEnabled;
-            return this;
-        }
-
-        public Builder writeEnabled(boolean writeEnabled) {
-            this.writeEnabled = writeEnabled;
-            return this;
-        }
-
         public Builder excludeBinding(BindingPattern binding) {
             this.excludeBindings.add(binding);
             return this;
@@ -87,26 +72,20 @@ public class AccessControl {
         }
 
         public AccessControl build() {
-            return new AccessControl(domain, writeEnabled, readEnabled, clientAuthentication, excludeBindings, handlers);
+            return new AccessControl(domain, clientAuthentication, excludeBindings, handlers);
         }
     }
 
     public final String domain;
-    public final boolean readEnabled;
-    public final boolean writeEnabled;
     public final ClientAuthentication clientAuthentication;
     private final Set<BindingPattern> excludedBindings;
     private final Collection<Handler<?>> handlers;
 
     private AccessControl(String domain,
-                          boolean writeEnabled,
-                          boolean readEnabled,
                           ClientAuthentication clientAuthentication,
                           Set<BindingPattern> excludedBindings,
                           Collection<Handler<?>> handlers) {
         this.domain = domain;
-        this.readEnabled = readEnabled;
-        this.writeEnabled = writeEnabled;
         this.clientAuthentication = clientAuthentication;
         this.excludedBindings = Collections.unmodifiableSet(excludedBindings);
         this.handlers = handlers;
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java
index 13503906c04..a8fb486b979 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/http/xml/HttpBuilder.java
@@ -68,16 +68,22 @@ public class HttpBuilder extends VespaDomBuilder.DomConfigProducerBuilder<Http>
         return http;
     }
 
-    private AccessControl buildAccessControl(DeployState deployState, AbstractConfigProducer ancestor, Element accessControlElem) {
+    private AccessControl buildAccessControl(DeployState deployState, AbstractConfigProducer<?> ancestor, Element accessControlElem) {
         AthenzDomain domain = getAccessControlDomain(deployState, accessControlElem);
         AccessControl.Builder builder = new AccessControl.Builder(domain.value());
 
         getContainerCluster(ancestor).ifPresent(builder::setHandlers);
 
         XmlHelper.getOptionalAttribute(accessControlElem, "read").ifPresent(
-                readAttr -> builder.readEnabled(Boolean.valueOf(readAttr)));
+                readAttr -> deployState.getDeployLogger()
+                        .logApplicationPackage(Level.WARNING,
+                                "The 'read' attribute of the 'access-control' element has no effect and is deprecated. " +
+                                        "Please remove the attribute from services.xml"));
         XmlHelper.getOptionalAttribute(accessControlElem, "write").ifPresent(
-                writeAttr -> builder.writeEnabled(Boolean.valueOf(writeAttr)));
+                writeAttr -> deployState.getDeployLogger()
+                        .logApplicationPackage(Level.WARNING,
+                                "The 'write' attribute of the 'access-control' element has no effect and is deprecated. " +
+                                        "Please remove the attribute from services.xml"));
 
         AccessControl.ClientAuthentication clientAuth =
                 XmlHelper.getOptionalAttribute(accessControlElem, "tls-handshake-client-auth")
@@ -98,7 +104,7 @@ public class HttpBuilder extends VespaDomBuilder.DomConfigProducerBuilder<Http>
         return builder.build();
     }
 
-    // TODO Fail if domain is not provided through deploy properties
+    // TODO(tokle,bjorncs) After Vespa 8 fail if domain is not provided through deploy properties
     private static AthenzDomain getAccessControlDomain(DeployState deployState, Element accessControlElem) {
         AthenzDomain tenantDomain = deployState.getProperties().athenzDomain().orElse(null);
         AthenzDomain explicitDomain = XmlHelper.getOptionalAttribute(accessControlElem, "domain")
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
index d1a176e37d5..084124e0ecb 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
@@ -526,8 +526,6 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
         if (tenantDomain == null) return; // tenant domain not present, cannot add access control. this should eventually be a failure.
         new AccessControl.Builder(tenantDomain.value())
                 .setHandlers(cluster)
-                .readEnabled(false)
-                .writeEnabled(false)
                 .clientAuthentication(AccessControl.ClientAuthentication.need)
                 .build()
                 .configureHttpFilterChains(http);
diff --git a/config-model/src/main/resources/schema/container.rnc b/config-model/src/main/resources/schema/container.rnc
index c16a5c4e3a5..4284dccc7c5 100644
--- a/config-model/src/main/resources/schema/container.rnc
+++ b/config-model/src/main/resources/schema/container.rnc
@@ -23,11 +23,10 @@ Server = element server {
 }
 
 AccessControl = element access-control {
-    attribute domain { xsd:NCName }? &  # TODO Vespa 8 Remove
-    attribute read { string "true" | string "false" }? & # TODO Vespa 8 Remove
-    attribute write { string "true" | string "false" }? & # TODO Vespa 8 Remove
+    attribute domain { xsd:NCName }? & # TODO(tokle,bjorncs) Remove after Vespa 8
+    attribute read { string "true" | string "false" }? &  # TODO(tokle,bjorncs) Remove after Vespa 8
+    attribute write { string "true" | string "false" }? &  # TODO(tokle,bjorncs) Remove after Vespa 8
     attribute tls-handshake-client-auth {string "want" | string "need"}? &
-    element vespa-domain { xsd:NCName }? & # TODO Remove after end of March 2020
     element exclude {
         Binding+
     }?
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/AccessControlTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/AccessControlTest.java
index f6a5bc14ab5..d676dc29c94 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/AccessControlTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/AccessControlTest.java
@@ -62,31 +62,6 @@ public class AccessControlTest extends ContainerModelBuilderTestBase {
         assertEquals("Wrong domain.", "my-tenant-domain", accessControl.domain);
     }
 
-    @Test
-    public void read_is_disabled_and_write_is_enabled_by_default() {
-        Http http = createModelAndGetHttp(
-                "  <http>",
-                "    <filtering>",
-                "      <access-control domain='my-tenant-domain'/>",
-                "    </filtering>",
-                "  </http>");
-
-        assertFalse("Wrong default value for read.", http.getAccessControl().get().readEnabled);
-        assertTrue("Wrong default value for write.", http.getAccessControl().get().writeEnabled);
-    }
-
-    @Test
-    public void read_and_write_can_be_overridden() {
-        Http http = createModelAndGetHttp(
-                "  <http>",
-                "    <filtering>",
-                "      <access-control domain='my-tenant-domain' read='true' write='false'/>",
-                "    </filtering>",
-                "  </http>");
-
-        assertTrue("Given read value not honoured.", http.getAccessControl().get().readEnabled);
-        assertFalse("Given write value not honoured.", http.getAccessControl().get().writeEnabled);
-    }
 
     @Test
     public void access_control_excluded_filter_chain_has_all_bindings_from_excluded_handlers() {
@@ -176,8 +151,6 @@ public class AccessControlTest extends ContainerModelBuilderTestBase {
         Optional<AccessControl> maybeAccessControl = http.getAccessControl();
         assertTrue(maybeAccessControl.isPresent());
         AccessControl accessControl = maybeAccessControl.get();
-        assertFalse(accessControl.writeEnabled);
-        assertFalse(accessControl.readEnabled);
         assertEquals(AccessControl.ClientAuthentication.need, accessControl.clientAuthentication);
         assertEquals("my-tenant-domain", accessControl.domain);
     }
diff --git a/config-model/src/test/schema-test-files/services.xml b/config-model/src/test/schema-test-files/services.xml
index 5d7e17a6e63..8ebcd549a03 100644
--- a/config-model/src/test/schema-test-files/services.xml
+++ b/config-model/src/test/schema-test-files/services.xml
@@ -60,11 +60,10 @@
 
     <http>
       <filtering strict-mode="true">
-        <access-control domain="my.athens-domain" read="true">
+        <access-control>
           <exclude>
             <binding>http//*/foo/*</binding>
           </exclude>
-          <vespa-domain>vespa.vespa.cd</vespa-domain>
         </access-control>
 
         <filter id="com.yahoo.YcaFilter" bundle="mybundle">