Fix usage of IDX13300 and IDX13107 #1458
Conversation
| @@ -122,7 +122,7 @@ public Uri DeclarationReference | |||
| set | |||
| { | |||
| if (value != null && !value.IsAbsoluteUri) | |||
| throw LogExceptionMessage(new ArgumentException(FormatInvariant(LogMessages.IDX13300, nameof(value), value))); | |||
| throw LogExceptionMessage(new ArgumentException(FormatInvariant(LogMessages.IDX13300, nameof(DeclarationReference), value))); | |||
There was a problem hiding this comment.
@keegan-caruso could you fix up the difference between ClassReference and DeclarationReference where ClassReference throws ArgumentNullException when value == null. I would vote for differentiating null.
There was a problem hiding this comment.
@brentschmaltz I'm happy to normalize the two. I am wondering if this goes back to the Saml2Core spec. Section 2.7.2.2 calls out both as being optional. That seems to imply neither throw on null. This looks safe as all access to this field checks for null conditions.
There was a problem hiding this comment.
@keegan-caruso in rethinking this, the ctors seem to be fine. Given the spec.
The issue seems to be that the ctors try and set a null value to the property.
It's OK that the value is null, but the reason we throw when setting the property is that seems to indicate a programming error.
So, perhaps the error is the constructor trying to set a null value.
There was a problem hiding this comment.
This is a good idea. It also fixes #1457
| @@ -73,8 +73,14 @@ public Saml2AuthenticationContext(Uri classReference) | |||
| /// <param name="declarationReference">The declaration reference of the authentication context.</param> | |||
| public Saml2AuthenticationContext(Uri classReference, Uri declarationReference) | |||
| { | |||
| ClassReference = classReference; | |||
| DeclarationReference = declarationReference; | |||
| // Class and declaration reference are optional, so don't try and set the respective properties if | |||
There was a problem hiding this comment.
I would recommend a slightly different pattern.
We have three ctors.
Check the parameters as we did before, but call a private method to initialize where we know the parameters were checked.
Otherwise we are not helping devs by helping them catch programming errors.
Here is an example here where we know in the private method we checked parameters in the public methods.
There was a problem hiding this comment.
I took a slightly different approach, but hopefully captured the spirit of what your feedback was addressing.
keegan-caruso
force-pushed
the
keegancaruso/fix-idx13300-idx13107-usage
branch
from
cd3f50f to
17b0655
Compare
| @@ -659,7 +659,10 @@ protected virtual Saml2AuthenticationContext ReadAuthenticationContext(XmlDictio | |||
| reader.ReadStartElement(Saml2Constants.Elements.AuthnContextDeclRef, Saml2Constants.Namespace); | |||
|
|
|||
| // Now we have enough data to create the object | |||
| var authnContext = new Saml2AuthenticationContext(classRef, declRef); | |||
| var authnContext = new Saml2AuthenticationContext(classRef); | |||
There was a problem hiding this comment.
This is to keep the behavior the same. Previously, classRef was always passed in and always validated as non-null. declRef was allowed to be null.
|
|
fixes #1455