Comment 1 Akara Sucharitakul 2015-06-07 02:55:11 EDT

The issue is in the following code in ChainingCredentialsProvider:

111	@Override
112	public boolean get(URIish uri, CredentialItem... items)
113			throws UnsupportedCredentialItem {
114		for (CredentialsProvider p : credentialProviders) {
115			if (p.supports(items)) {
116				p.get(uri, items);
117				if (isAnyNull(items))
118					continue;
119				return true;
120			}
121		}
122		return false;
123	}
124
125	private boolean isAnyNull(CredentialItem... items) {
126		for (CredentialItem i : items)
127			if (i == null)
128				return true;
129		return false;
130	}

The loop on line 114 will keep looping into a proper provider can be found. On line 117, it checks whether the provider leaves any nulls (by calling isAnyNull) and if so will continue using the next provider.

Now lets look at isAnyNull on line 127. For CredentialItem i to be null, it means the list of CredentialItems (varargs) passed in is supposed to have null values. This same list was received through line 112 as varargs of get. Assuming the CredentialItems are never requested as null values, isAnyNull will never return true and therefore would never allow for proper chaining.

One very odd possibility is that p.supports(items) on line 115 or p.get(items) on line 116 would change requested CredentialItem in the array to null. This is very unlikely. All the providers would do is not set the value for the CredentialItem, leaving their content null, but not setting the CredentialItem in the array to null itself.

To make this at least work, although not perfectly, the isAnyNull method needs to be modified as follows:

    private boolean isAnyNull(CredentialItem... items) {
        for (CredentialItem i : items) {
            if (i == null)
                return true;
            if (i instanceof CredentialItem.StringType &&
                    ((CredentialItem.StringType) i).getValue() == null)
                return true;
            if (i instanceof CredentialItem.CharArrayType &&
                    ((CredentialItem.CharArrayType) i).getValue() == null)
                return true;
        }
        return false;
    }

This will check the content of the CredentialItem whether they are null or not. The null check of i itself is just defensive and should not be needed. This solve is still not perfect as it does not cover all subtypes of CredentialItem. Ones that contain primitives such as boolean values cannot be detected as a boolean will never be null. At least it checks for the state of the CredentialItems that should be set not being set properly. But a proper solution is to add a isSet() method to CredentialItem superclass. The subclass implementation could then just check, in some cases having null content is considered not set. In other cases touching a boolean property will consider it set, no matter what the resulting state is.

If we do so, isAnyNull will just have to loop through the CredentialItems and check isSet() on each. Then the method name isAnyNull() should also be renamed to isCompleted(). This would be one of the better solves.

On a separate note, there is also another, less serious issue in this file, here:

70	public ChainingCredentialsProvider(CredentialsProvider... providers) {
71		this.credentialProviders = new ArrayList<CredentialsProvider>(
72				Arrays.asList(providers));
73		for (CredentialsProvider p : providers)
74			credentialProviders.add(p);
75	}

This constructor is used to populate the providers into the this.credentialProviders list. On line 71, the whole array is added to the list already. On line 73, the code loops through the array once more and adds the providers to the list, again. This causes providers to be added in duplicate. The symptoms would be normally hidden, assuming one of the providers will be able to authenticate. Still, it is a bug in the code and perhaps worth cleaning up at the same time.