[GitHub] sijie closed pull request #1092: Dont log ledgermetadata which contains password.

[GitBox <gi...@apache.org>]

sijie closed pull request #1092: Dont log ledgermetadata which contains password.
URL: https://github.com/apache/bookkeeper/pull/1092
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerMetadata.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerMetadata.java
index d37e7fc80..753eebd27 100644
--- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerMetadata.java
+++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerMetadata.java
@@ -327,6 +327,10 @@ void setCustomMetadata(Map<String, byte[]> customMetadata) {
     }
 
     LedgerMetadataFormat buildProtoFormat() {
+        return buildProtoFormat(true);
+    }
+
+    LedgerMetadataFormat buildProtoFormat(boolean withPassword) {
         LedgerMetadataFormat.Builder builder = LedgerMetadataFormat.newBuilder();
         builder.setQuorumSize(writeQuorumSize).setAckQuorumSize(ackQuorumSize)
             .setEnsembleSize(ensembleSize).setLength(length)
@@ -337,7 +341,10 @@ LedgerMetadataFormat buildProtoFormat() {
         }
 
         if (hasPassword) {
-            builder.setDigestType(digestType).setPassword(ByteString.copyFrom(password));
+            builder.setDigestType(digestType);
+            if (withPassword) {
+                builder.setPassword(ByteString.copyFrom(password));
+            }
         }
 
         if (customMetadata != null) {
@@ -366,13 +373,17 @@ LedgerMetadataFormat buildProtoFormat() {
      * @return the metadata serialized into a byte array
      */
     public byte[] serialize() {
+        return serialize(true);
+    }
+
+    public byte[] serialize(boolean withPassword) {
         if (metadataFormatVersion == 1) {
             return serializeVersion1();
         }
 
         StringBuilder s = new StringBuilder();
         s.append(VERSION_KEY).append(tSplitter).append(CURRENT_METADATA_FORMAT_VERSION).append(lSplitter);
-        s.append(TextFormat.printToString(buildProtoFormat()));
+        s.append(TextFormat.printToString(buildProtoFormat(withPassword)));
         if (LOG.isDebugEnabled()) {
             LOG.debug("Serialized config: {}", s);
         }
@@ -671,8 +682,24 @@ boolean isConflictWith(LedgerMetadata newMeta) {
 
     @Override
     public String toString() {
+        return toStringRepresentation(true);
+    }
+
+    /**
+     * Returns a string representation of this LedgerMetadata object by
+     * filtering out the password field.
+     *
+     * @return a string representation of the object without password field in
+     *         it.
+     */
+    public String toSafeString() {
+        return toStringRepresentation(false);
+    }
+
+    private String toStringRepresentation(boolean withPassword) {
         StringBuilder sb = new StringBuilder();
-        sb.append("(meta:").append(new String(serialize(), UTF_8)).append(", version:").append(version).append(")");
+        sb.append("(meta:").append(new String(serialize(withPassword), UTF_8)).append(", version:").append(version)
+                .append(")");
         return sb.toString();
     }
 
diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadOnlyLedgerHandle.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadOnlyLedgerHandle.java
index c940d276d..d1ab61503 100644
--- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadOnlyLedgerHandle.java
+++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadOnlyLedgerHandle.java
@@ -55,7 +55,7 @@ public void safeRun() {
             Version.Occurred occurred =
                     ReadOnlyLedgerHandle.this.metadata.getVersion().compare(this.m.getVersion());
             if (Version.Occurred.BEFORE == occurred) {
-                LOG.info("Updated ledger metadata for ledger {} to {}.", ledgerId, this.m);
+                LOG.info("Updated ledger metadata for ledger {} to {}.", ledgerId, this.m.toSafeString());
                 synchronized (ReadOnlyLedgerHandle.this) {
                     if (this.m.isClosed()) {
                             ReadOnlyLedgerHandle.this.lastAddConfirmed = this.m.getLastEntryId();
diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/LedgerMetadataTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/LedgerMetadataTest.java
index 9570c58fe..dc947dde7 100644
--- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/LedgerMetadataTest.java
+++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/LedgerMetadataTest.java
@@ -36,7 +36,8 @@
  */
 public class LedgerMetadataTest {
 
-    private static final byte[] passwd = "testPasswd".getBytes(UTF_8);
+    private static final String passwdStr = "testPasswd";
+    private static final byte[] passwd = passwdStr.getBytes(UTF_8);
 
     @Test
     public void testGetters() {
@@ -154,4 +155,20 @@ public void testIsConflictWithDifferentStoreSystemtimeAsLedgerCtimeFlags() {
         assertTrue(lm1.isConflictWith(lm2));
     }
 
+    @Test
+    public void testToString() {
+        LedgerMetadata lm1 = new LedgerMetadata(
+            3,
+            3,
+            2,
+            DigestType.CRC32,
+            passwd,
+            Collections.emptyMap(),
+            true);
+
+        assertTrue("toString should contain 'password' field", lm1.toString().contains("password"));
+        assertTrue("toString should contain password value", lm1.toString().contains(passwdStr));
+        assertFalse("toSafeString should not contain 'password' field", lm1.toSafeString().contains("password"));
+        assertFalse("toSafeString should not contain password value", lm1.toSafeString().contains(passwdStr));
+    }
 }


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services