[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.
|
@fjuma I'm reviewing issues from SET members and trying to move them to resolution. Can you please review Darran's comment and post your opinion? Thanks! |
|
To unblock this I think we are going to need two tests, one for SASL and one for HTTP - the problem being we may need the HTTP part to be in elytron-web and I am not sure if SASL can be tested alone here or if we would need JBoss Remoting. Where mutiple mechanisms could be offered I would like the tests to confirm first what is our current behaviour regarding mechanism selection where mechanism configuration is not provided for the mechanisms including the mechanisms that need the AvailableRealmsCallback to work. I think it does feel like we don't take this configuration into account when there is no mechanism configuration for mechanism selection but that will have then been confirmed by the first part. The second part is then assuming we proceed with the empty configuration do the mechanisms still work with this callback unable to return a realm or does moving to an empty configuration move the problem forward - i.e. at the end the test would demonstrate we have a complete solution. |
https://issues.redhat.com/browse/ELY-1745