Ensured various builder collection argument methods were semantically the same (append operations, not one append, one replace) (#830)

README.md

@@ -157,7 +157,7 @@ enforcement.
  * Convenient and readable [fluent](http://en.wikipedia.org/wiki/Fluent_interface) interfaces, great for IDE 
    auto-completion to write code quickly
  * Fully RFC specification compliant on all implemented functionality, tested against RFC-specified test vectors
- * Stable implementation with over 1,300+ tests and enforced 100% test code coverage.  Every single method, statement 
+ * Stable implementation with over 1,400+ tests and enforced 100% test code coverage.  Every single method, statement 
    and conditional branch variant in the entire codebase is tested and required to pass on every build.
  * Creating, parsing and verifying digitally signed compact JWTs (aka JWSs) with all standard JWS algorithms:
    

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

@@ -97,7 +97,7 @@ public interface ClaimsMutator<T extends ClaimsMutator<T>> {
      * data type for recipients.
      *
      * @param aud the value to use as the {@code aud} Claim single-String value (and not an array of Strings), or
-     *            {@code null} to remove the property from the JSON map.
+     *            {@code null}, empty or whitespace to remove the property from the JSON map.
      * @return the instance for method chaining
      * @since JJWT_RELEASE_VERSION
      * @deprecated This is technically not deprecated because the JWT RFC mandates support for single string values,
@@ -108,22 +108,26 @@ public interface ClaimsMutator<T extends ClaimsMutator<T>> {
     T audienceSingle(String aud);
 
     /**
-     * Adds the specified {@code aud} value to the {@link #audience(Collection) audience} Claim set (JSON Array). This
+     * Adds (appends) the specified {@code aud} value to the {@link #audience(Collection) audience} Claim set
-     * method may be called multiple times.
+     * (JSON Array) unless it is {@code null}, empty, whitespace-only or already exists in the set.
+     *
+     * <p>This method may be called multiple times.</p>
      *
      * @param aud a JWT {@code aud} value to add to the {@link #audience(Collection) audience} Claim set.
      * @return the {@code Claims} instance for method chaining.
      * @throws IllegalArgumentException if the {@code aud} argument is null or empty.
      * @since JJWT_RELEASE_VERSION
      */
-    T audience(String aud) throws IllegalArgumentException;
+    T audience(String aud);
 
     /**
-     * Sets the JWT <a href="https://www.rfc-editor.org/rfc/rfc7519.html#section-4.1.3"><code>aud</code></a> (audience)
+     * Adds (appends) the specified values to the JWT
-     * Claim set, replacing any previous value(s).
+     * <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.
      *
-     * @param aud the values to set as the {@code aud} Claim set (JSON Array), or {@code null}/empty to remove the
+     * <p>This method may be called multiple times.</p>
-     *            {@code aud} claim from the JSON map entirely.
+     *
+     * @param aud the values to add to the {@code aud} Claim set (JSON Array)
      * @return the instance for method chaining
      * @since JJWT_RELEASE_VERSION
      */

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

@@ -102,8 +102,8 @@ public interface JwtParserBuilder extends Builder<JwtParser> {
      *
      * <p><b>Extension Behavior</b></p>
      *
-     * <p>The {@code crit} set only identifies header parameter names that are used in extensions supported by the
+     * <p>The {@code crit} set argument only identifies header parameter names that are used in extensions supported
-     * application. <b>Application developers, <em>not JJWT</em>, MUST perform the associated extension behavior
+     * by the application. <b>Application developers, <em>not JJWT</em>, MUST perform the associated extension behavior
      * using the parsed JWT</b>.</p>
      *
      * @param crit the header parameter names used in JWT extensions supported by the application.

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

@@ -19,7 +19,7 @@ import io.jsonwebtoken.security.PublicJwk;
 import io.jsonwebtoken.security.X509Mutator;
 
 import java.net.URI;
-import java.util.Set;
+import java.util.Collection;
 
 /**
  * Mutation (modifications) to a {@link ProtectedHeader Header} instance.
@@ -30,17 +30,29 @@ import java.util.Set;
 public interface ProtectedHeaderMutator<T extends ProtectedHeaderMutator<T>> extends HeaderMutator<T>, X509Mutator<T> {
 
     /**
-     * Sets the header parameter names that use extensions to the JWT or JWA specification that <em>MUST</em>
+     * Adds the name of a header parameter used by a JWT or JWA specification extension that <em>MUST</em> be understood
-     * be understood and supported by the JWT recipient. A {@code null} value will remove the
+     * and supported by the JWT recipient.  A {@code null}, empty, whitespace-only or already existing value is ignored.
-     * property from the JSON map.
      *
-     * @param crit the header parameter names that use extensions to the JWT or JWA specification that <em>MUST</em>
+     * @param crit the name of a header parameter used by a JWT or JWA specification extension that <em>MUST</em> be
-     *             be understood and supported by the JWT recipient.
+     *             understood and supported by the JWT recipient.
      * @return the header for method chaining.
      * @see <a href="https://www.rfc-editor.org/rfc/rfc7515.html#section-4.1.11">JWS <code>crit</code> (Critical) Header Parameter</a>
      * @see <a href="https://www.rfc-editor.org/rfc/rfc7516.html#section-4.1.13">JWS <code>crit</code> (Critical) Header Parameter</a>
      */
-    T critical(Set<String> crit);
+    T critical(String crit);
+
+    /**
+     * Adds names of header parameters used by JWT or JWA specification extensions that <em>MUST</em> be
+     * understood and supported by the JWT recipient. {@code null}, empty, whitespace-only or already existing
+     * values are ignored.
+     *
+     * @param crit names of header parameters used by JWT or JWA specification extensions that <em>MUST</em> be
+     *             understood and supported by the JWT recipient
+     * @return the header for method chaining.
+     * @see <a href="https://www.rfc-editor.org/rfc/rfc7515.html#section-4.1.11">JWS <code>crit</code> (Critical) Header Parameter</a>
+     * @see <a href="https://www.rfc-editor.org/rfc/rfc7516.html#section-4.1.13">JWS <code>crit</code> (Critical) Header Parameter</a>
+     */
+    T critical(Collection<String> crit);
 
     /**
      * Sets the {@code jwk} (JSON Web Key) associated with the JWT.  When set for a {@link JwsHeader}, the

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

@@ -180,7 +180,6 @@ public final class Collections {
 
     /**
      * Returns a non-null set, either {@code s} if it is not null, or {@link #emptySet()} otherwise.
-     * Returns the specified set if not null or {@link #emptySet()} otherwise.
      *
      * @param s   the set to check for null
      * @param <T> type of elements in the set
@@ -191,6 +190,18 @@ public final class Collections {
         return s == null ? Collections.<T>emptySet() : s;
     }
 
+    /**
+     * Returns a non-null collection, either {@code c} if it is not null, or {@link #emptyList()} otherwise.
+     *
+     * @param c   the collection to check for null
+     * @param <T> type of elements in the collection
+     * @return a non-null collection, either {@code c} if it is not null, or {@link #emptyList()} otherwise.
+     * @since JJWT_RELEASE_VERSION
+     */
+    public static <T> Collection<T> nullSafe(Collection<T> c) {
+        return c == null ? Collections.<T>emptyList() : c;
+    }
+
     /**
      * Return <code>true</code> if the supplied Collection is <code>null</code>
      * or empty. Otherwise, return <code>false</code>.

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

@@ -138,14 +138,14 @@ public interface JwkBuilder<K extends Key, J extends Jwk<K>, T extends JwkBuilde
      *
      * @param operation the value to add to the JWK {@code key_ops} value set
      * @return the builder for method chaining.
-     * @throws IllegalArgumentException if {@code op} is {@code null} or if the operation is not permitted
+     * @throws IllegalArgumentException if the {@code op} is not permitted by the operations
-     *                                  by the operations {@link #operationPolicy(KeyOperationPolicy) policy}.
+     *                                  {@link #operationPolicy(KeyOperationPolicy) policy}.
      * @see Jwks.OP
      */
     T operation(KeyOperation operation) throws IllegalArgumentException;
 
     /**
-     * Sets the JWK <a href="https://www.rfc-editor.org/rfc/rfc7517.html#section-4.3">{@code key_ops}
+     * Adds {@code ops} to the JWK <a href="https://www.rfc-editor.org/rfc/rfc7517.html#section-4.3">{@code key_ops}
      * (Key Operations) Parameter</a> values.
      *
      * <p>The {@code key_ops} (key operations) parameter identifies the operation(s) for which the key is
@@ -176,10 +176,10 @@ public interface JwkBuilder<K extends Key, J extends Jwk<K>, T extends JwkBuilde
      * <p>For best interoperability with other applications however, it is recommended to use only the {@link Jwks.OP}
      * constants.</p>
      *
-     * @param ops the JWK {@code key_ops} value set, or {@code null} if not present.
+     * @param ops the {@code KeyOperation} values to add to the JWK {@code key_ops} value set
      * @return the builder for method chaining.
-     * @throws IllegalArgumentException {@code ops} is {@code null} or empty, or if any of the operations are not
+     * @throws IllegalArgumentException if any of the operations are not permitted by the operations
-     *                                  permitted by the operations {@link #operationPolicy(KeyOperationPolicy) policy}.
+     *                                  {@link #operationPolicy(KeyOperationPolicy) policy}.
      * @see Jwks.OP
      */
     T operations(Collection<KeyOperation> ops) throws IllegalArgumentException;

api/src/test/groovy/io/jsonwebtoken/lang/CollectionsTest.groovy

@@ -52,7 +52,7 @@ class CollectionsTest {
 
     @Test
     void testNullSafeSetWithNullArgument() {
-        def set = Collections.nullSafe(null)
+        def set = Collections.nullSafe((Set)null)
         assertNotNull set
         assertTrue set.isEmpty()
     }
@@ -70,4 +70,25 @@ class CollectionsTest {
         def b = Collections.nullSafe(a)
         assertSame a, b
     }
+
+    @Test
+    void testNullSafeCollectionWithNullArgument() {
+        Collection c = Collections.nullSafe((Collection)null)
+        assertNotNull c
+        assertTrue c.isEmpty()
+    }
+
+    @Test
+    void testNullSafeCollectionWithEmptyArgument() {
+        Collection a = new LinkedHashSet() as Collection
+        def b = Collections.nullSafe(a)
+        assertSame a, b
+    }
+
+    @Test
+    void testNullSafeCollectionWithNonEmptyArgument() {
+        Collection a = ["hello"] as Collection<String>
+        def b = Collections.nullSafe(a)
+        assertSame a, b
+    }
 }

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

@@ -19,11 +19,14 @@ import io.jsonwebtoken.JweHeaderMutator;
 import io.jsonwebtoken.impl.lang.DelegatingMapMutator;
 import io.jsonwebtoken.impl.lang.Parameter;
 import io.jsonwebtoken.impl.security.X509BuilderSupport;
+import io.jsonwebtoken.lang.Collections;
 import io.jsonwebtoken.lang.Strings;
 import io.jsonwebtoken.security.PublicJwk;
 
 import java.net.URI;
 import java.security.cert.X509Certificate;
+import java.util.Collection;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
 
@@ -102,8 +105,29 @@ public class DefaultJweHeaderMutator<T extends JweHeaderMutator<T>>
     // =============================================================
 
     @Override
-    public T critical(Set<String> crit) {
+    public T critical(String crit) {
-        return put(DefaultProtectedHeader.CRIT, crit);
+        crit = Strings.clean(crit);
+        if (Strings.hasText(crit)) {
+            critical(Collections.setOf(crit));
+        }
+        return self();
+    }
+
+    @Override
+    public T critical(Collection<String> crit) {
+        if (!Collections.isEmpty(crit)) {
+            Set<String> existing = Collections.nullSafe(this.DELEGATE.get(DefaultProtectedHeader.CRIT));
+            Set<String> set = new LinkedHashSet<>(existing.size() + crit.size());
+            set.addAll(existing);
+            for (String s : crit) {
+                s = Strings.clean(s);
+                if (s != null) {
+                    set.add(s);
+                }
+            }
+            put(DefaultProtectedHeader.CRIT, set);
+        }
+        return self();
     }
 
     @Override

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

@@ -19,7 +19,6 @@ import io.jsonwebtoken.ClaimsMutator;
 import io.jsonwebtoken.impl.lang.DelegatingMapMutator;
 import io.jsonwebtoken.impl.lang.Parameter;
 import io.jsonwebtoken.impl.lang.Parameters;
-import io.jsonwebtoken.lang.Assert;
 import io.jsonwebtoken.lang.Collections;
 import io.jsonwebtoken.lang.MapMutator;
 import io.jsonwebtoken.lang.Strings;
@@ -88,8 +87,7 @@ public class DelegatingClaimsMutator<T extends MapMutator<String, Object, T> & C
                 put(DefaultClaims.AUDIENCE, Collections.setOf(existing)); // replace as Set
             }
         }
-        Set<String> aud = get(DefaultClaims.AUDIENCE);
+        return get(DefaultClaims.AUDIENCE);
-        return aud != null ? aud : Collections.<String>emptySet();
     }
 
     @Override
@@ -108,16 +106,28 @@ public class DelegatingClaimsMutator<T extends MapMutator<String, Object, T> & C
 
     @Override
     public T audience(String aud) {
-        aud = Assert.hasText(Strings.clean(aud), "Audience string value cannot be null or empty.");
+        aud = Strings.clean(aud);
-        Set<String> set = new LinkedHashSet<>(getAudience());
+        if (Strings.hasText(aud)) {
-        set.add(aud);
+            audience(java.util.Collections.singleton(aud));
-        return audience(set);
+        }
+        return self();
     }
 
     @Override
     public T audience(Collection<String> aud) {
-        getAudience(); //coerce to Set<String> if necessary
+        if (!Collections.isEmpty(aud)) {
-        return put(DefaultClaims.AUDIENCE, Collections.asSet(aud));
+            Set<String> existing = Collections.nullSafe(getAudience());
+            Set<String> set = new LinkedHashSet<>(existing.size() + aud.size());
+            set.addAll(existing);
+            for (String s : aud) {
+                s = Strings.clean(s);
+                if (s != null) {
+                    set.add(s);
+                }
+            }
+            put(DefaultClaims.AUDIENCE, set);
+        }
+        return self();
     }
 
     @Override

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

@@ -109,18 +109,14 @@ abstract class AbstractJwkBuilder<K extends Key, J extends Jwk<K>, T extends Jwk
 
     @Override
     public T operation(KeyOperation operation) throws IllegalArgumentException {
-        Assert.notNull(operation, "KeyOperation cannot be null.");
+        return operation != null ? operations(Collections.setOf(operation)) : self();
-        return operations(Collections.setOf(operation));
     }
 
     @Override
     public T operations(Collection<KeyOperation> ops) {
-        Assert.notEmpty(ops, "KeyOperations collection argument cannot be null or empty.");
+        Set<KeyOperation> set = new LinkedHashSet<>(Collections.nullSafe(ops)); // new ones override existing ones
-        Set<KeyOperation> set = new LinkedHashSet<>(ops); // new ones override existing ones
+        Set<KeyOperation> existing = Collections.nullSafe(this.DELEGATE.getOperations());
-        Set<KeyOperation> existing = this.DELEGATE.getOperations();
-        if (!Collections.isEmpty(existing)) {
         set.addAll(existing);
-        }
         this.opsPolicy.validate(set);
         this.DELEGATE.setOperations(set);
         return self();

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

@@ -647,6 +647,18 @@ class DefaultJwtBuilderTest {
         assertEquals aud, Jwts.parser().enableUnsecured().build().parseClaimsJwt(jwt).payload.getAudience().iterator().next()
     }
 
+    @Test
+    void testAudienceNullString() {
+        def jwt = Jwts.builder().subject('me').audience(null).compact()
+        assertNull Jwts.parser().enableUnsecured().build().parseClaimsJwt(jwt).payload.getAudience()
+    }
+
+    @Test
+    void testAudienceEmptyString() {
+        def jwt = Jwts.builder().subject('me').audience('  ').compact()
+        assertNull Jwts.parser().enableUnsecured().build().parseClaimsJwt(jwt).payload.getAudience()
+    }
+
     @Test
     void testAudienceMultipleTimes() {
         def one = 'one'
@@ -657,6 +669,28 @@ class DefaultJwtBuilderTest {
         assertTrue aud.contains(two)
     }
 
+    @Test
+    void testAudienceNullCollection() {
+        Collection c = null
+        def jwt = Jwts.builder().subject('me').audience(c).compact()
+        assertNull Jwts.parser().enableUnsecured().build().parseClaimsJwt(jwt).payload.getAudience()
+    }
+
+    @Test
+    void testAudienceEmptyCollection() {
+        Collection c = new ArrayList()
+        def jwt = Jwts.builder().subject('me').audience(c).compact()
+        assertNull Jwts.parser().enableUnsecured().build().parseClaimsJwt(jwt).payload.getAudience()
+    }
+
+    @Test
+    void testAudienceCollectionWithNullElement() {
+        Collection c = new ArrayList()
+        c.add(null)
+        def jwt = Jwts.builder().subject('me').audience(c).compact()
+        assertNull Jwts.parser().enableUnsecured().build().parseClaimsJwt(jwt).payload.getAudience()
+    }
+
     /**
      * Asserts that if someone calls builder.audienceSingle and then audience(String), that the audience value
      * will automatically be coerced from a String to a Set<String> and contain both elements.
@@ -693,17 +727,17 @@ class DefaultJwtBuilderTest {
 
     /**
      * Asserts that if someone calls builder.audienceSingle and then audience(Collection), the builder coerces the
-     * aud to a Set<String> and only the elements in the Collection will be applied since audience(Collection) is a
+     * aud to a Set<String> and all elements will be applied since audience(Collection).
-     * full-replacement operation.
      */
     @Test
     void testAudienceSingleThenAudienceCollection() {
         def single = 'one'
         def collection = ['two', 'three'] as Set<String>
+        def expected = ['one', 'two', 'three'] as Set<String>
         def jwt = Jwts.builder().audienceSingle(single).audience(collection).compact()
         def aud = Jwts.parser().enableUnsecured().build().parseClaimsJwt(jwt).payload.getAudience()
-        assertEquals collection.size(), aud.size()
+        assertEquals expected.size(), aud.size()
-        assertTrue aud.containsAll(collection)
+        assertTrue aud.contains(single) && aud.containsAll(collection)
     }
 
     /**

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

@@ -496,21 +496,46 @@ class DefaultJwtHeaderBuilderTest {
     }
 
     @Test
-    void testCritNullRemovesValues() {
+    void testCritSingle() {
-        def crit = ['test'] as Set<String>
+        def crit = 'test'
-        def header = jws().add('test', 'foo').critical(crit).build() as ProtectedHeader
+        def header = jws().add(crit, 'foo').critical(crit).build() as ProtectedHeader
-        assertEquals crit, header.getCritical()
+        def expected = [crit] as Set<String>
-        header = builder.critical(null).build()
+        assertEquals expected, header.getCritical()
-        assertFalse header.containsKey('crit') // removed
     }
 
     @Test
-    void testCritEmptyRemovesValues() {
+    void testCritSingleNullIgnored() {
+        def crit = 'test'
+        def expected = [crit] as Set<String>
+        def header = jws().add(crit, 'foo').critical(crit).build() as ProtectedHeader
+        assertEquals expected, header.getCritical()
+        header = builder.critical((String) null).build() as ProtectedHeader // ignored
+        assertEquals expected, header.getCritical() // nothing changed
+    }
+
+    @Test
+    void testCritNullCollectionIgnored() {
+        def crit = ['test'] as Set<String>
+        def header = jws().add('test', 'foo').critical(crit).build() as ProtectedHeader
+        assertEquals crit, header.getCritical()
+        header = builder.critical((Collection) null).build() as ProtectedHeader
+        assertEquals crit, header.getCritical() // nothing changed
+    }
+
+    @Test
+    void testCritCollectionWithNullElement() {
+        def crit = [null] as Set<String>
+        def header = jws().add('test', 'foo').critical(crit).build() as ProtectedHeader
+        assertNull header.getCritical()
+    }
+
+    @Test
+    void testCritEmptyIgnored() {
         def crit = ['test'] as Set<String>
         def header = jws().add('test', 'foo').critical(crit).build() as ProtectedHeader
         assertEquals crit, header.getCritical()
         header = builder.critical([] as Set<String>).build()
-        assertFalse header.containsKey('crit') // removed
+        assertEquals crit, header.getCritical() // ignored
     }
 
     /**

impl/src/test/groovy/io/jsonwebtoken/impl/security/JwksTest.groovy

@@ -142,11 +142,50 @@ class JwksTest {
         testProperty('id', 'kid', srandom())
     }
 
+    @Test
+    void testSingleOperation() {
+        def op = Jwks.OP.ENCRYPT
+        def expected = [op] as Set<KeyOperation>
+        def canonical = [op.getId()] as Set<String>
+        def jwk = Jwks.builder().key(TestKeys.A128GCM).operation(op).build()
+        assertEquals expected, jwk.getOperations()
+        assertEquals canonical, jwk.get(AbstractJwk.KEY_OPS.id)
+    }
+
+    @Test
+    void testSingleOperationNull() {
+        def jwk = Jwks.builder().key(TestKeys.A128GCM).operation(null).build() //ignored null
+        assertNull jwk.getOperations() //nothing added
+        assertFalse jwk.containsKey(AbstractJwk.KEY_OPS.id)
+    }
+
+    @Test
+    void testSingleOperationAppends() {
+        def expected = [Jwks.OP.ENCRYPT, Jwks.OP.DECRYPT] as Set<KeyOperation>
+        def jwk = Jwks.builder().key(TestKeys.A128GCM)
+                .operation(Jwks.OP.ENCRYPT).operation(Jwks.OP.DECRYPT)
+                .build()
+        assertEquals expected, jwk.getOperations()
+    }
+
     @Test
     void testOperations() {
         def val = [Jwks.OP.SIGN, Jwks.OP.VERIFY] as Set<KeyOperation>
-        def canonical = Collections.setOf('sign', 'verify')
+        def jwk = Jwks.builder().key(TestKeys.A128GCM).operations(val).build()
-        testProperty('operations', 'key_ops', val, canonical)
+        assertEquals val, jwk.getOperations()
+    }
+
+    @Test
+    void testOperationsNull() {
+        def jwk = Jwks.builder().key(TestKeys.A128GCM).operations(null).build()
+        assertNull jwk.getOperations()
+        assertFalse jwk.containsKey(AbstractJwk.KEY_OPS.id)
+    }
+
+    @Test
+    void testOperationsEmpty() {
+        def jwk = Jwks.builder().key(TestKeys.A128GCM).operations(Collections.emptyList()).build()
+        assertNull jwk.getOperations()
     }
 
     @Test