Rewrite Password setting class in Java (second attempt) (#18231)

Secoind attempt to translates Password setting class into plain Java, which exposes a logger used by the Ruby ValidatedPassword subclass. Fixes a bug happened in previous #18183.

Co-authored: @donoghuc

Author: andsel

logstash-core/lib/logstash/environment.rb

@@ -75,7 +75,7 @@ def self.as_java_range(r)
             Setting::StringSetting.new("api.environment", "production"),
             Setting::StringSetting.new("api.auth.type", "none", true, %w(none basic)),
             Setting::StringSetting.new("api.auth.basic.username", nil, false).nullable,
-          Setting::Password.new("api.auth.basic.password", nil, false).nullable,
+          Setting::PasswordSetting.new("api.auth.basic.password", nil, false).nullable,
             Setting::StringSetting.new("api.auth.basic.password_policy.mode", "WARN", true, %w[WARN ERROR]),
            Setting::NumericSetting.new("api.auth.basic.password_policy.length.minimum", 8),
             Setting::StringSetting.new("api.auth.basic.password_policy.include.upper", "REQUIRED", true, %w[REQUIRED OPTIONAL]),
@@ -84,7 +84,7 @@ def self.as_java_range(r)
             Setting::StringSetting.new("api.auth.basic.password_policy.include.symbol", "OPTIONAL", true, %w[REQUIRED OPTIONAL]),
            Setting::BooleanSetting.new("api.ssl.enabled", false),
   Setting::ExistingFilePath.new("api.ssl.keystore.path", nil, false).nullable,
-          Setting::Password.new("api.ssl.keystore.password", nil, false).nullable,
+          Setting::PasswordSetting.new("api.ssl.keystore.password", nil, false).nullable,
        Setting::StringArray.new("api.ssl.supported_protocols", nil, true, %w[TLSv1 TLSv1.1 TLSv1.2 TLSv1.3]),
            Setting::StringSetting.new("pipeline.batch.metrics.sampling_mode", "minimal", true, ["disabled", "minimal", "full"]),
             Setting::StringSetting.new("queue.type", "memory", true, ["persisted", "memory"]),

logstash-core/lib/logstash/settings.rb

@@ -439,27 +439,9 @@ def validate(value)
 
     java_import org.logstash.settings.NullableStringSetting
 
-    class Password < Coercible
-      def initialize(name, default = nil, strict = true)
-        super(name, LogStash::Util::Password, default, strict)
-      end
-
-      def coerce(value)
-        return value if value.kind_of?(LogStash::Util::Password)
-
-        if value && !value.kind_of?(::String)
-          raise(ArgumentError, "Setting `#{name}` could not coerce non-string value to password")
-        end
-
-        LogStash::Util::Password.new(value)
-      end
-
-      def validate(value)
-        super(value)
-      end
-    end
+    java_import org.logstash.settings.PasswordSetting
 
-    class ValidatedPassword < Setting::Password
+    class ValidatedPassword < Setting::PasswordSetting
       def initialize(name, value, password_policies)
         @password_policies = password_policies
         super(name, value, true)

logstash-core/spec/logstash/settings/password_spec.rb

@@ -18,7 +18,7 @@
 require "spec_helper"
 require "logstash/settings"
 
-describe LogStash::Setting::Password do
+describe LogStash::Setting::PasswordSetting do
   let(:setting_name) { "secure" }
   subject(:password_setting) { described_class.new(setting_name, nil, true) }
 
@@ -55,7 +55,7 @@
     context 'with an invalid non-string value' do
       let(:setting_value) { 867_5309 }
       it 'rejects the invalid value' do
-        expect { password_setting.set(setting_value) }.to raise_error(ArgumentError, "Setting `#{setting_name}` could not coerce non-string value to password")
+        expect { password_setting.set(setting_value) }.to raise_error(IllegalArgumentException, "Setting `#{setting_name}` could not coerce non-string value to password")
         expect(password_setting).to_not be_set
       end
     end

logstash-core/spec/logstash/settings_spec.rb

@@ -300,22 +300,22 @@
       end
     end
 
-    context "containing a mode WARN policy" do
-      before :each do
-        # Needs to mock the logger method at LogStash::Settings instead of LogStash::Setting::ValidatedPassword
-        # else the LOGGABLE_PROXY hide the mock itself.
-        allow(LogStash::Settings).to receive(:logger).at_least(:once).and_return(mock_logger)
-        allow(mock_logger).to receive(:warn)
-      end
-      let(:mock_logger) { double("logger") }
-      let(:password_policies) { super().merge({ "mode": "WARN" }) }
+    describe "mode WARN" do
+      let(:password_policies) { super().merge("mode": "WARN") }
+
+      context "when the password does not conform to the policy" do
+        let(:password) { LogStash::Util::Password.new("NoNumbers!") }
+        let(:mock_logger) { double("logger") }
+
+        before :each do
+          allow_any_instance_of(LogStash::Setting::ValidatedPassword).to receive(:logger).and_return(mock_logger)
+        end
+
+        it "logs a warning on validation failure" do
+          expect(mock_logger).to receive(:warn).with(a_string_including("Password must contain at least one digit between 0 and 9."))
 
-      it "logs a warning on validation failure" do
-        password = LogStash::Util::Password.new("Password!")
-        expect {
           LogStash::Setting::ValidatedPassword.new("test.validated.password", password, password_policies)
-        }.not_to raise_error
-        expect(mock_logger).to have_received(:warn).with(a_string_including("Password must contain at least one digit between 0 and 9."))
+        end
       end
     end
   end

logstash-core/src/main/java/org/logstash/settings/PasswordSetting.java

@@ -0,0 +1,53 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *	http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.logstash.settings;
+
+import co.elastic.logstash.api.Password;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+
+
+public class PasswordSetting extends Coercible<Object> {
+
+    private static final Logger LOG = LogManager.getLogger();
+
+    public PasswordSetting(String name, Object defaultValue) {
+        this(name, defaultValue, true);
+    }
+
+    public PasswordSetting(String name, Object defaultValue, boolean strict) {
+        super(name, defaultValue, strict, noValidator());
+    }
+
+    @Override
+    public Password coerce(Object obj) {
+        if (obj instanceof Password) {
+            return (Password) obj;
+        }
+        if (obj != null && !(obj instanceof String)) {
+            throw new IllegalArgumentException("Setting `" + getName() + "` could not coerce non-string value to password");
+        }
+        return new Password((String) obj);
+    }
+
+    public Logger getLogger() {
+        return LOG;
+    }
+}

logstash-core/src/test/java/org/logstash/settings/PasswordSettingTest.java

@@ -0,0 +1,82 @@
+/*
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *	http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.logstash.settings;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.*;
+
+public class PasswordSettingTest {
+
+    private final String SETTING_NAME = "setting_name";
+    private PasswordSetting sut;
+
+    @Before
+    public void setUp() {
+        sut = new PasswordSetting(SETTING_NAME, null, true);
+    }
+
+    @Test
+    public void  givenUnsetPasswordSetting_thenIsConsideredAsValid() {
+        assertNotThrown(() -> sut.validateValue());
+        assertThat(sut.value(), is(instanceOf(co.elastic.logstash.api.Password.class)));
+        assertNull(((co.elastic.logstash.api.Password) sut.value()).getValue());
+    }
+
+    @Test
+    public void  givenUnsetPasswordSetting_whenIsSetIsInvoked_thenReturnFalse() {
+        assertFalse(sut.isSet());
+    }
+
+    @Test
+    public void givenSetPasswordSetting_thenIsValid() {
+        sut.set("s3cUr3p4$$w0rd");
+
+        assertNotThrown(() -> sut.validateValue());
+        assertThat(sut.value(), is(instanceOf(co.elastic.logstash.api.Password.class)));
+        assertEquals("s3cUr3p4$$w0rd", ((co.elastic.logstash.api.Password) sut.value()).getValue());
+    }
+
+    @Test
+    public void  givenSetPasswordSetting_whenIsSetIsInvoked_thenReturnTrue() {
+        sut.set("s3cUr3p4$$w0rd");
+
+        assertTrue(sut.isSet());
+    }
+
+    @Test
+    public void  givenSetPasswordSettingWithInvalidNonStringValue_thenRejectsTheInvalidValue() {
+        Exception e = assertThrows(IllegalArgumentException.class, () -> sut.set(867_5309));
+        assertThat(e.getMessage(), is("Setting `" + SETTING_NAME + "` could not coerce non-string value to password"));
+    }
+
+    private void assertNotThrown(Runnable test) {
+        try {
+            test.run();
+        } catch (Exception e) {
+            fail("Exception should not be thrown");
+        }
+    }
+
+}