[ELY-1745] The AvailableRealmsCallback should not result in a NPE if there is no mechanism configuration #1858
Conversation
| ServerAuthenticationContext sac = securityDomain.createNewAuthenticationContext(null); | ||
|
|
||
| try { | ||
| sac.setAuthenticationName("user"); |
There was a problem hiding this comment.
If you expect a Exception here, you should fail("Exception missing") in the next line.
There was a problem hiding this comment.
Hi @boris-unckel, thank you for your suggestion, I have updated PR accordingly.
lvydra
force-pushed
the
ELY-1745
branch
2 times, most recently
from
bed6480
to
78c91be
Compare
lvydra
requested review from
boris-unckel
and removed request for
fjuma and
Skyllarr
|
@lvydra I understand that this is an old issue, but can you please change the PR to be for 2.x branch. Thanks! |
|
Hi @ivassile, the base branch should be 2.x now. |
.../server/base/src/main/java/org/wildfly/security/auth/server/ServerAuthenticationContext.java
Outdated
Show resolved
Hide resolved
| @@ -1376,7 +1376,7 @@ public InactiveState(SecurityIdentity capturedIdentity, MechanismConfigurationSe | |||
| public InactiveState(SecurityIdentity capturedIdentity, MechanismConfigurationSelector mechanismConfigurationSelector, | |||
| MechanismInformation mechanismInformation, IdentityCredentials privateCredentials, IdentityCredentials publicCredentials, Attributes runtimeAttributes) { | |||
| this.capturedIdentity = capturedIdentity; | |||
| this.mechanismConfigurationSelector = mechanismConfigurationSelector; | |||
| this.mechanismConfigurationSelector = checkNotNullParam("mechanismConfigurationSelector", mechanismConfigurationSelector); | |||
There was a problem hiding this comment.
@lvydra changing this to:
this.mechanismConfigurationSelector = mechanismConfigurationSelector != null ? mechanismConfigurationSelector : MechanismConfigurationSelector.constantSelector(MechanismConfiguration.EMPTY);
seems safer to me. I am not sure if the mechanism configuration was always mandatory and am worried this might create backward incompatibilities. WDYT?
There was a problem hiding this comment.
Thanks @Skyllarr, it's definitely friendlier, updated.
|
@lvydra Thanks! |
|
@lvydra actually thinking about this some more. IIUC this is the part of the code that should be addressed as per the description of the ELY-1745 issue:
in the above, if the getMechanismConfiguration returns null, then it throws NPE. Your current solution addresses the case when the MechanismConfigurationSelector is null. But even if the selector is not null, the selector might not return any appropriate mechanism configuration. Notice the method selectConfiguration on the MechanismConfigurationSelector can return null. And this PR does not address this case. WDYT? You can take a look at the selectMechanismConfiguration method of the InitialState. It throws an exception if the selector was unable to select appropriate mechanism configuration. I wonder why this existing part of the code does not address this issue already |
…there is no mechanism configuration
|
@Skyllarr I have added a change that should address potential NPE as a result of null MechanismConfiguration. |
| @@ -1083,7 +1084,8 @@ private void handleOne(final Callback[] callbacks, final int idx) throws IOExcep | |||
| ((SecurityIdentityCallback) callback).setSecurityIdentity(identity); | |||
| handleOne(callbacks, idx + 1); | |||
| } else if (callback instanceof AvailableRealmsCallback) { | |||
| Collection<String> names = stateRef.get().getMechanismConfiguration().getMechanismRealmNames(); | |||
| MechanismConfiguration mechanismConfiguration = stateRef.get().getMechanismConfiguration(); | |||
| Collection<String> names = mechanismConfiguration != null ? mechanismConfiguration.getMechanismRealmNames() : Collections.emptyList(); | |||
There was a problem hiding this comment.
This part looks good to me to protect against the NPE
| @@ -1403,7 +1405,7 @@ public InactiveState(SecurityIdentity capturedIdentity, MechanismConfigurationSe | |||
| public InactiveState(SecurityIdentity capturedIdentity, MechanismConfigurationSelector mechanismConfigurationSelector, | |||
| MechanismInformation mechanismInformation, IdentityCredentials privateCredentials, IdentityCredentials publicCredentials, Attributes runtimeAttributes) { | |||
| this.capturedIdentity = capturedIdentity; | |||
| this.mechanismConfigurationSelector = mechanismConfigurationSelector; | |||
| this.mechanismConfigurationSelector = mechanismConfigurationSelector != null ? mechanismConfigurationSelector : MechanismConfigurationSelector.constantSelector(MechanismConfiguration.EMPTY); | |||
There was a problem hiding this comment.
I am just a little unusure myself the correct action here. In the Jira issue I was a bit vague as I was unsure of the correct behaviour.
"If mechanism configuration is mandatory this should report an appropriate error, if not it should fallback to specifying an empty list."
I do have a question if we think a mechanism configuration must be supplied for all mechanisms and most importantly if existing users are using this as a way to enable the mechanisms or that is a lot of overhead and it is cleaner to default to an empty configuration.
It is the risk that accidentally unexpectedly turning on a mechanism that makes me want to question this.
https://issues.redhat.com/browse/ELY-1745