[gianpaolo-pestilli]

Problem

DefaultPasswordGenerator.merge(List<DefaultPasswordRuleConf>) always returns
a configuration with usernameAllowed == true, even when every input
configuration has usernameAllowed == false.

How to reproduce

The test added in this PR fails on current master without the fix:

@Test
void allForbid() {
    List<DefaultPasswordRuleConf> rules = new ArrayList();
    rules.add(configRule(false));
    rules.add(configRule(false));
    assertFalse(passwordGenerator.merge(rules).isUsernameAllowed());
}

Cause

In merge, result is initialized with result.setUsernameAllowed(true)
and is never set to false before the following check

syncope/core/spring/src/main/java/org/apache/syncope/core/spring/security/DefaultPasswordGenerator.java

Lines 153 to 155 in 193c94e

	if (!result.isUsernameAllowed()) {
	result.setUsernameAllowed(ruleConf.isUsernameAllowed());
	}

if (!result.isUsernameAllowed()) {
    result.setUsernameAllowed(ruleConf.isUsernameAllowed());
}

The condition can therefore never be true, and the value of
usernameAllowed in the input configurations is never read.

Proposed fix

Remove the negation, so that usernameAllowed stays true only if all the
configurations allow it (consistently with the other fields, where the most
restrictive value wins):

if (result.isUsernameAllowed()) {
    result.setUsernameAllowed(ruleConf.isUsernameAllowed());
}

Tests

Added PasswordGeneratorTest.allForbid(): with
two configurations both having usernameAllowed == false, merge() must
return usernameAllowed == false. It fails without the fix and passes with it.

Note

If the current behavior is intended, please let me know and I will adjust
or close this PR.

cc @gulyx

[ilgrosso]

Hi @gianpaolo-pestilli thanks for this PR.
Have you already submitted your ICLA?

[ilgrosso]

@gianpaolo-pestilli I was able to verify this issue.
Besides providing information about your ICLA (I wasn't able to find any match in the ASF records), please also go ahead and open an issue on JIRA to track this problem.

[ilgrosso]

I went ahead and created https://issues.apache.org/jira/browse/SYNCOPE-2010

[gianpaolo-pestilli]

Hi @ilgrosso, and sorry for the late reply.

I have just pushed a new commit with the requested changes:

I added tests for the other combinations: with the fix applied, all of them pass.
I replaced new ArrayList() with new ArrayList<>(), as you pointed out.

Thank you for verifying the issue. I was waiting to reply because I don’t have a JIRA account yet.

Regarding the ICLA, I haven’t submitted it yet: this is my first contribution to an open source project and I wasn’t sure about it. I’m going to look into it and will let you know here.

Thanks for your patience and your help!

[ilgrosso]

@gianpaolo-pestilli the PR looks good now.
We are awaiting only your ICLA submission to merge it.

[gianpaolo-pestilli]

Thanks @ilgrosso! I have just sent my signed ICLA: please let me know if there’s anything else I should do.

[ilgrosso]

Thank you very much @gianpaolo-pestilli !
As you can see, I have merged this PR into master and cherry-picked the resulting commit into the 4_2_X and 4_1_X branches.