SLING-12396: ACL restrictions with no values are mistakenly considered equivalent regardless of their name

[sdumitriu]

No description provided.

[rombert]

Thanks for the PR @sdumitriu and sorry for taking so long to review.

It's not immediately obvious to me why the change is needed, can you please explain that? I suspect that the scenario that you are hitting is when oldValues is empty ( declared restriction with no values ) and newValues is null ( restriction is not declared ).

I wonder then if from a JCR point of view it makes sense to treat restrictions with no value as different from missing restrictions.

@anchela - any thoughts on this?

[anchela]

@rombert , i vaguely remember that there was some trouble in the past on how to reflect empty values for restrictions in the past. for some like the glob-style or rep:current restrictions an empty value (or empty values for mv restrictions) has a meaning while null values for properties in JCR default writing are equivalent to removal.

i guess some additional tests would be good to illustrate the problem and the intended fix. they should also over the empty value scenario (unless that already exists). hope that helps.

[rombert]

@rombert , i vaguely remember that there was some trouble in the past on how to reflect empty values for restrictions in the past. for some like the glob-style or rep:current restrictions an empty value (or empty values for mv restrictions) has a meaning while null values for properties in JCR default writing are equivalent to removal.

i guess some additional tests would be good to illustrate the problem and the intended fix. they should also over the empty value scenario (unless that already exists). hope that helps.

Thanks @anchela , so the distinction is indeed relevant. I agree that additional tests would help.

[sonarqubecloud]

Quality Gate failed

Failed conditions
33.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

[sonarqubecloud]

Quality Gate failed

Failed conditions
33.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

[anchela]

@sdumitriu , please provide additional test-coverage for the proposed changes

33.3% Coverage on New Code (required ≥ 80%)

i would actually go for 100% coverage for the new changes.
it's about permission setup so any glitch will negatively impact customers and there is no easy way to fix broken setup.

[sonarqubecloud]

Quality Gate failed

Failed conditions
33.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud