NestedCollection changes without .and()

CHANGELOG.md

@@ -1,5 +1,31 @@
 ## Release Notes
 
+### 0.12.5
+
+This patch release:
+
+* Ensures that builders' `NestedCollection` changes are applied to the collection immediately as mutation methods are called, no longer
+  requiring application developers to call `.and()` to 'commit' or apply a change.  For example, prior to this release,
+  the following code did not apply changes:
+  ```java
+  JwtBuilder builder = Jwts.builder();
+  builder.audience().add("an-audience"); // no .and() call
+  builder.compact(); // would not keep 'an-audience'
+  ```
+  Now this code works as expected and all other `NestedCollection` instances like it apply changes immediately (e.g. when calling
+  `.add(value)`).
+
+  However, standard fluent builder chains are still recommended for readability when feasible, e.g.
+
+  ```java
+  Jwts.builder()
+      .audience().add("an-audience").and() // allows fluent chaining
+      .subject("Joe")
+      // etc...
+      .compact()
+  ```
+  See [Issue 916](https://github.com/jwtk/jjwt/issues/916).
+
 ### 0.12.4
 
 This patch release includes various changes listed below.

README.md

@@ -993,7 +993,7 @@ String jws = Jwts.builder()
 
     .issuer("me")
     .subject("Bob")
-    .audience("you")
+    .audience().add("you").and()    
     .expiration(expiration) //a java.util.Date
     .notBefore(notBefore) //a java.util.Date 
     .issuedAt(new Date()) // for example, now

api/src/main/java/io/jsonwebtoken/ClaimsMutator.java

@@ -96,6 +96,17 @@ public interface ClaimsMutator<T extends ClaimsMutator<T>> {
      * <a href="https://www.rfc-editor.org/rfc/rfc7519.html#section-4.1.3"><code>aud</code></a> (audience) Claim
      * set, quietly ignoring any null, empty, whitespace-only, or existing value already in the set.
      *
+     * <p>When finished, the {@code audience} collection's {@link AudienceCollection#and() and()} method may be used
+     * to continue configuration. For example:</p>
+     * <blockquote><pre>
+     *  Jwts.builder() // or Jwts.claims()
+     *
+     *     .audience().add("anAudience")<b>.and() // return parent</b>
+     *
+     *  .subject("Joe") // resume configuration...
+     *  // etc...
+     * </pre></blockquote>
+     *
      * @return the {@link AudienceCollection AudienceCollection} to use for {@code aud} configuration.
      * @see AudienceCollection AudienceCollection
      * @see AudienceCollection#single(String) AudienceCollection.single(String)
@@ -221,6 +232,16 @@ public interface ClaimsMutator<T extends ClaimsMutator<T>> {
      * A {@code NestedCollection} for setting {@link #audience()} values that also allows overriding the collection
      * to be a {@link #single(String) single string value} for legacy JWT recipients if necessary.
      *
+     * <p>Because this interface extends {@link NestedCollection}, the {@link #and()} method may be used to continue
+     * parent configuration. For example:</p>
+     * <blockquote><pre>
+     *  Jwts.builder() // or Jwts.claims()
+     *
+     *     .audience().add("anAudience")<b>.and() // return parent</b>
+     *
+     *  .subject("Joe") // resume parent configuration...
+     *  // etc...</pre></blockquote>
+     *
      * @param <P> the type of ClaimsMutator to return for method chaining.
      * @see #single(String)
      * @since 0.12.0

api/src/main/java/io/jsonwebtoken/JwtParserBuilder.java

@@ -101,12 +101,25 @@ public interface JwtParserBuilder extends Builder<JwtParser> {
      * the parser encounters a Protected JWT that {@link ProtectedHeader#getCritical() requires} extensions, and
      * those extensions' header names are not specified via this method, the parser will reject that JWT.
      *
+     * <p>The collection's {@link Conjunctor#and() and()} method returns to the builder for continued parser
+     * configuration, for example:</p>
+     * <blockquote><pre>
+     * parserBuilder.critical().add("headerName")<b>.{@link Conjunctor#and() and()} // etc...</b></pre></blockquote>
+     *
      * <p><b>Extension Behavior</b></p>
      *
      * <p>The {@code critical} collection only identifies header parameter names that are used in extensions supported
      * by the application. <b>Application developers, <em>not JJWT</em>, MUST perform the associated extension behavior
      * using the parsed JWT</b>.</p>
      *
+     * <p><b>Continued Parser Configuration</b></p>
+     * <p>When finished, use the collection's
+     * {@link Conjunctor#and() and()} method to continue parser configuration, for example:
+     * <blockquote><pre>
+     * Jwts.parser()
+     *     .critical().add("headerName").<b>{@link Conjunctor#and() and()} // return parent</b>
+     * // resume parser configuration...</pre></blockquote>
+     *
      * @return the {@link NestedCollection} to use for {@code crit} configuration.
      * @see ProtectedHeader#getCritical()
      * @since 0.12.0
@@ -557,7 +570,7 @@ public interface JwtParserBuilder extends Builder<JwtParser> {
      * <p>The collection's {@link Conjunctor#and() and()} method returns to the builder for continued parser
      * configuration, for example:</p>
      * <blockquote><pre>
-     * parserBuilder.enc().add(anAeadAlgorithm).{@link Conjunctor#and() and()} // etc...</pre></blockquote>
+     * parserBuilder.enc().add(anAeadAlgorithm)<b>.{@link Conjunctor#and() and()} // etc...</b></pre></blockquote>
      *
      * <p><b>Standard Algorithms and Overrides</b></p>
      *
@@ -597,7 +610,7 @@ public interface JwtParserBuilder extends Builder<JwtParser> {
      * <p>The collection's {@link Conjunctor#and() and()} method returns to the builder for continued parser
      * configuration, for example:</p>
      * <blockquote><pre>
-     * parserBuilder.key().add(aKeyAlgorithm).{@link Conjunctor#and() and()} // etc...</pre></blockquote>
+     * parserBuilder.key().add(aKeyAlgorithm)<b>.{@link Conjunctor#and() and()} // etc...</b></pre></blockquote>
      *
      * <p><b>Standard Algorithms and Overrides</b></p>
      *
@@ -639,7 +652,7 @@ public interface JwtParserBuilder extends Builder<JwtParser> {
      * <p>The collection's {@link Conjunctor#and() and()} method returns to the builder for continued parser
      * configuration, for example:</p>
      * <blockquote><pre>
-     * parserBuilder.sig().add(aSignatureAlgorithm).{@link Conjunctor#and() and()} // etc...</pre></blockquote>
+     * parserBuilder.sig().add(aSignatureAlgorithm)<b>.{@link Conjunctor#and() and()} // etc...</b></pre></blockquote>
      *
      * <p><b>Standard Algorithms and Overrides</b></p>
      *
@@ -680,7 +693,7 @@ public interface JwtParserBuilder extends Builder<JwtParser> {
      * <p>The collection's {@link Conjunctor#and() and()} method returns to the builder for continued parser
      * configuration, for example:</p>
      * <blockquote><pre>
-     * parserBuilder.zip().add(aCompressionAlgorithm).{@link Conjunctor#and() and()} // etc...</pre></blockquote>
+     * parserBuilder.zip().add(aCompressionAlgorithm)<b>.{@link Conjunctor#and() and()} // etc...</b></pre></blockquote>
      *
      * <p><b>Standard Algorithms and Overrides</b></p>
      *

api/src/main/java/io/jsonwebtoken/ProtectedHeaderMutator.java

@@ -33,9 +33,11 @@ public interface ProtectedHeaderMutator<T extends ProtectedHeaderMutator<T>> ext
     /**
      * Configures names of header parameters used by JWT or JWA specification extensions that <em>MUST</em> be
      * understood and supported by the JWT recipient. When finished, use the collection's
-     * {@link Conjunctor#and() and()} method to return to the header builder, for example:
+     * {@link Conjunctor#and() and()} method to continue header configuration, for example:
      * <blockquote><pre>
-     * builder.critical().add("headerName").{@link Conjunctor#and() and()} // etc...</pre></blockquote>
+     * headerBuilder
+     *     .critical().add("headerName").<b>{@link Conjunctor#and() and()} // return parent</b>
+     * // resume header configuration...</pre></blockquote>
      *
      * @return the {@link NestedCollection} to use for {@code crit} configuration.
      * @see <a href="https://www.rfc-editor.org/rfc/rfc7515.html#section-4.1.11">JWS <code>crit</code> (Critical) Header Parameter</a>

api/src/main/java/io/jsonwebtoken/lang/NestedCollection.java

@@ -17,7 +17,12 @@
 
 /**
  * A {@link CollectionMutator} that can return access to its parent via the {@link Conjunctor#and() and()} method for
- * continued configuration.
+ * continued configuration.  For example:
+ * <blockquote><pre>
+ * builder
+ *     .aNestedCollection()// etc...
+ *     <b>.and() // return parent</b>
+ * // resume parent configuration...</pre></blockquote>
  *
  * @param <E> the type of elements in the collection
  * @param <P> the parent to return

api/src/main/java/io/jsonwebtoken/security/JwkBuilder.java

@@ -109,9 +109,9 @@ public interface JwkBuilder<K extends Key, J extends Jwk<K>, T extends JwkBuilde
      * the key is intended to be used. When finished, use the collection's {@link Conjunctor#and() and()} method to
      * return to the JWK builder, for example:
      * <blockquote><pre>
-     * jwkBuilder.operations().add(aKeyOperation).{@link Conjunctor#and() and()} // etc...</pre></blockquote>
+     * jwkBuilder.operations().add(aKeyOperation)<b>.{@link Conjunctor#and() and()} // etc...</b></pre></blockquote>
      *
-     * <p>The {@code and()} method will throw an {@link IllegalArgumentException} if any of the specified
+     * <p>The {@code add()} method(s) will throw an {@link IllegalArgumentException} if any of the specified
      * {@code KeyOperation}s are not permitted by the JWK's
      * {@link #operationPolicy(KeyOperationPolicy) operationPolicy}. See that documentation for more
      * information on security vulnerabilities when using the same key with multiple algorithms.</p>

impl/src/main/java/io/jsonwebtoken/impl/DefaultJweHeaderMutator.java

@@ -107,9 +107,8 @@ public T setCompressionAlgorithm(String zip) {
     public NestedCollection<String, T> critical() {
         return new DefaultNestedCollection<String, T>(self(), this.DELEGATE.get(DefaultProtectedHeader.CRIT)) {
             @Override
-            public T and() {
+            protected void changed() {
                 put(DefaultProtectedHeader.CRIT, Collections.asSet(getCollection()));
-                return super.and();
             }
         };
     }

impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtParserBuilder.java

@@ -220,9 +220,8 @@ public JwtParserBuilder clock(Clock clock) {
     public NestedCollection<String, JwtParserBuilder> critical() {
         return new DefaultNestedCollection<String, JwtParserBuilder>(this, this.critical) {
             @Override
-            public JwtParserBuilder and() {
+            protected void changed() {
                 critical = Collections.asSet(getCollection());
-                return super.and();
             }
         };
     }
@@ -304,9 +303,8 @@ private JwtParserBuilder decryptWith(final Key key) {
     public NestedCollection<CompressionAlgorithm, JwtParserBuilder> zip() {
         return new DefaultNestedCollection<CompressionAlgorithm, JwtParserBuilder>(this, this.zipAlgs.values()) {
             @Override
-            public JwtParserBuilder and() {
+            protected void changed() {
                 zipAlgs = new IdRegistry<>(StandardCompressionAlgorithms.NAME, getCollection());
-                return super.and();
             }
         };
     }
@@ -315,9 +313,8 @@ public JwtParserBuilder and() {
     public NestedCollection<AeadAlgorithm, JwtParserBuilder> enc() {
         return new DefaultNestedCollection<AeadAlgorithm, JwtParserBuilder>(this, this.encAlgs.values()) {
             @Override
-            public JwtParserBuilder and() {
+            public void changed() {
                 encAlgs = new IdRegistry<>(StandardEncryptionAlgorithms.NAME, getCollection());
-                return super.and();
             }
         };
     }
@@ -326,9 +323,8 @@ public JwtParserBuilder and() {
     public NestedCollection<SecureDigestAlgorithm<?, ?>, JwtParserBuilder> sig() {
         return new DefaultNestedCollection<SecureDigestAlgorithm<?, ?>, JwtParserBuilder>(this, this.sigAlgs.values()) {
             @Override
-            public JwtParserBuilder and() {
+            public void changed() {
                 sigAlgs = new IdRegistry<>(StandardSecureDigestAlgorithms.NAME, getCollection());
-                return super.and();
             }
         };
     }
@@ -337,9 +333,8 @@ public JwtParserBuilder and() {
     public NestedCollection<KeyAlgorithm<?, ?>, JwtParserBuilder> key() {
         return new DefaultNestedCollection<KeyAlgorithm<?, ?>, JwtParserBuilder>(this, this.keyAlgs.values()) {
             @Override
-            public JwtParserBuilder and() {
+            public void changed() {
                 keyAlgs = new IdRegistry<>(StandardKeyAlgorithms.NAME, getCollection());
-                return super.and();
             }
         };
     }

impl/src/main/java/io/jsonwebtoken/impl/DelegatingClaimsMutator.java

@@ -130,12 +130,12 @@ public AudienceCollection<T> audience() {
             @Override
             public T single(String audience) {
                 return audienceSingle(audience);
+                // DO NOT call changed() here - we don't want to replace the value with a collection
             }
 
             @Override
-            public T and() {
+            protected void changed() {
                 put(DefaultClaims.AUDIENCE, Collections.asSet(getCollection()));
-                return super.and();
             }
         };
     }

impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java

@@ -37,38 +37,52 @@ protected final M self() {
         return (M) this;
     }
 
-    @Override
-    public M add(E e) {
-        if (Objects.isEmpty(e)) return self();
+    private boolean doAdd(E e) {
+        if (Objects.isEmpty(e)) return false;
         if (e instanceof Identifiable && !Strings.hasText(((Identifiable) e).getId())) {
             String msg = e.getClass() + " getId() value cannot be null or empty.";
             throw new IllegalArgumentException(msg);
         }
-        this.collection.remove(e);
-        this.collection.add(e);
+        return this.collection.add(e);
+    }
+
+    @Override
+    public M add(E e) {
+        if (doAdd(e)) changed();
         return self();
     }
 
     @Override
     public M remove(E e) {
-        this.collection.remove(e);
+        if (this.collection.remove(e)) changed();
         return self();
     }
 
     @Override
     public M add(Collection<? extends E> c) {
+        boolean changed = false;
         for (E element : Collections.nullSafe(c)) {
-            add(element);
+            changed = doAdd(element) || changed;
         }
+        if (changed) changed();
         return self();
     }
 
     @Override
     public M clear() {
+        boolean changed = !Collections.isEmpty(this.collection);
         this.collection.clear();
+        if (changed) changed();
         return self();
     }
 
+    /**
+     * Callback for subclasses that wish to be notified if the internal collection has changed via builder mutation
+     * methods.
+     */
+    protected void changed() {
+    }
+
     protected Collection<E> getCollection() {
         return Collections.immutable(this.collection);
     }

impl/src/main/java/io/jsonwebtoken/impl/security/AbstractJwkBuilder.java

@@ -111,11 +111,10 @@ public T idFromThumbprint(HashAlgorithm alg) {
     public NestedCollection<KeyOperation, T> operations() {
         return new DefaultNestedCollection<KeyOperation, T>(self(), this.DELEGATE.getOperations()) {
             @Override
-            public T and() {
+            protected void changed() {
                 Collection<? extends KeyOperation> c = getCollection();
                 opsPolicy.validate(c);
                 DELEGATE.setOperations(c);
-                return super.and();
             }
         };
     }

impl/src/test/groovy/io/jsonwebtoken/impl/DefaultJwtBuilderTest.groovy

@@ -791,4 +791,47 @@ class DefaultJwtBuilderTest {
         assertEquals three, claims.aud
     }
 
+    /**
+     * Asserts that if a .audience() builder is used, and its .and() method is not called, the change to the
+     * audience is still applied when building the JWT.
+     * @see <a href="https://github.com/jwtk/jjwt/issues/916">JJWT Issue 916</a>
+     * @since JJWT_RELEASE_VERSION
+     */
+    @Test
+    void testAudienceWithoutConjunction() {
+        def aud = 'my-web'
+        def builder = Jwts.builder()
+        builder.audience().add(aud) // no .and() call
+        def jwt = builder.compact()
+
+        // assert that the resulting claims has the audience array set as expected:
+        def parsed = Jwts.parser().unsecured().build().parseUnsecuredClaims(jwt)
+        assertEquals aud, parsed.payload.getAudience()[0]
+    }
+
+    /**
+     * Asserts that if a .header().critical() builder is used, and its .and() method is not called, the change to the
+     * crit collection is still applied when building the header.
+     * @see <a href="https://github.com/jwtk/jjwt/issues/916">JJWT Issue 916</a>
+     * @since JJWT_RELEASE_VERSION
+     */
+    @Test
+    void testCritWithoutConjunction() {
+        def crit = 'test'
+        def builder = Jwts.builder().issuer('me')
+        def headerBuilder = builder.header()
+        headerBuilder.critical().add(crit) // no .and() method
+        headerBuilder.add(crit, 'foo') // no .and() method
+        builder.signWith(TestKeys.HS256)
+        def jwt = builder.compact()
+
+        def headerBytes = Decoders.BASE64URL.decode(jwt.split('\\.')[0])
+        def headerMap = Services.get(Deserializer).deserialize(Streams.reader(headerBytes)) as Map<String, ?>
+
+        def expected = [crit] as Set<String>
+        def val = headerMap.get('crit') as Set<String>
+        assertNotNull val
+        assertEquals expected, val
+    }
+
 }