[Bug] DefaultCertificateLoaderByCriterium should not return a null cert.

[brentschmaltz]

Which version of Microsoft Identity Web are you using? master

Where is the issue? Certificate doesn't load, and user is not informed.

To Repo

1. put a certificate that has expired into the local cert store.
2. populate a CertificateDescription that has the coordinates to the certificate in the local store.
3. call DefaultCertificateLoader.LoadIfNeeded()

The load will fail because of this line. One could argue, why are you filtering by default. It would be helpful to have a property on CertificateDescription to filter. Anyone that calls 'LoadIfNeeded' would expect it to succeed, since there is no return value. Users that subsequently use the cert, could have a null ref, etc.

I think it would be good to throw here.

[jmprieur]

Thanks @brentschmaltz This would be a breaking change. And the whole retry model is based on this. I don't think we'll take this one.

cc; @jennyf19

[brentschmaltz]

@jmprieur how will retrying help, if the certificate can never be loaded with your api? This is difficult for users to catch, this resulted in a null-ref in SAL.

This API silently fails only for the application to fail later in an unrelated area, hard to diagnose. It is always better to fail fast.

If you choose to do nothing

1. log an error.
2. add documentation that the certificate can be null.