Implemented C# 8 Nullable Reference Type standard. #231

pmaytak · 2020-06-23T07:33:36Z

Issue #15
Original branch #183

Implemented C# 8 Nullable Reference Type standard.
Includes additional null checks for public method parameters.
Few cases where I had to refactor a little to prevent null exceptions.
Misc warning and comment fixes.

Note 1:
Created a separate branch because rebasing #183 had some conflicts. Work from #183 is included here.

Note 2:
Analyzers had some funky behavior. Some code didn't have squiggly underlines. Error list had some weird warning types. Perhaps something to do with preview .NET build?

References:
Nullable reference types
Reserved attributes contribute to the compiler's null state static analysis
Try out Nullable Reference Types
Update libraries to use nullable reference types and communicate nullable rules to callers

.editorconfig

pmaytak · 2020-06-23T07:40:10Z

src/Microsoft.Identity.Web/AuthorityHelpers.cs

@@ -10,8 +10,8 @@ internal static class AuthorityHelpers
    {
        internal static string BuildAuthority(MicrosoftIdentityOptions options)
        {
-            var baseUri = new Uri(options.Instance);
-            var pathBase = baseUri.PathAndQuery.TrimEnd('/');
+            Uri baseUri = new Uri(options.Instance);


By default, var becomes the nullable reference type (Uri? in this case). So in some cases I used explicit type instead.

pmaytak · 2020-06-23T07:46:06Z

src/Microsoft.Identity.Web/CertificateManagement/DefaultCertificateLoader.cs

@@ -27,19 +27,19 @@ public void LoadIfNeeded(CertificateDescription certificateDescription)
                switch (certificateDescription.SourceType)
                {
                    case CertificateSource.KeyVault:
-                        certificateDescription.Certificate = LoadFromKeyVault(certificateDescription.Container, certificateDescription.ReferenceOrValue);
+                        certificateDescription.Certificate = LoadFromKeyVault(certificateDescription.Container!, certificateDescription.ReferenceOrValue!);


! overrides the conversion from nullable to non-nullable type, which otherwise would throw an exception. In this case, the properties shouldn't be null.

src/Microsoft.Identity.Web/CertificateManagement/DefaultCertificateLoader.cs

pmaytak · 2020-06-23T07:53:47Z

src/Microsoft.Identity.Web/CertificateManagement/DefaultCertificateLoader.cs

@@ -203,7 +199,7 @@ private static void ParseStoreLocationAndName(string storeDescription, ref Store
            DefaultCertificateLoader defaultCertificateLoader = new DefaultCertificateLoader();
            CertificateDescription certDescription = certificateDescription.First();
            defaultCertificateLoader.LoadIfNeeded(certDescription);
-            return certDescription?.Certificate;
+            return certDescription.Certificate!;


Certificate here, I assume, should never be null. But if it's not found in the cert store, it will be null. I think this will result in passing null to MSAL and to Wilson.

MSAL will handle the cert being null and throw an ArgumentNullException, we catch the exception

In reply to: 444031657 [](ancestors = 444031657)

We do get an uncaught NPE when trying to set token decryption cert.

microsoft-identity-web/src/Microsoft.Identity.Web/WebApiAuthenticationBuilderExtensions.cs

Lines 117 to 118 in 6fe02db

options.TokenValidationParameters.TokenDecryptionKey =

new X509SecurityKey(DefaultCertificateLoader.LoadFirstCertificate(microsoftIdentityOptions.TokenDecryptionCertificates));

Yes, we do catch the NPE from MSAL. But the exception is kind of general. Also, FromBase64 gives Cannot find the requested object.; FromDisk gives The system cannot find the file specified. Similar to my other comment, maybe we can update this to give better messaging and user experience when we implement cert rotation stuff.

That's fine, but maybe something for a separate PR w/a new issue.

src/Microsoft.Identity.Web/InstanceDiscovery/IssuerConfigurationRetriever.cs

pmaytak · 2020-06-23T08:00:30Z

src/Microsoft.Identity.Web/MicrosoftIdentityOptions.cs

@@ -16,17 +16,17 @@ public class MicrosoftIdentityOptions : OpenIdConnectOptions
        /// <summary>
        /// Gets or sets the Azure Active Directory instance, e.g. "https://login.microsoftonline.com".
        /// </summary>
-        public string Instance { get; set; }
+        public string Instance { get; set; } = null!;


This is a workaround for properties that are really non-nullable, except in once instance when the Data Transfer Objects are not yet initialized.

pmaytak · 2020-06-23T08:07:34Z

tests/.editorconfig

+# SA0001: XML comment analysis disabled
+dotnet_diagnostic.SA0001.severity = none


This removes 4 warnings from the build pipeline related to disabled comment analysis.

src/Microsoft.Identity.Web/MicrosoftIdentityOptions.cs

src/Microsoft.Identity.Web/TokenAcquisition.cs

src/Microsoft.Identity.Web/WebAppCallsWebApiAuthenticationBuilderExtensions.cs

jennyf19 · 2020-06-25T04:08:03Z

src/Microsoft.Identity.Web/Resource/JwtBearerMiddlewareDiagnostics.cs

-            {
-                events = new JwtBearerEvents();
-            }
+            events ??= new JwtBearerEvents();


nit: space?

Where? Looks right.

jennyf19 · 2020-06-25T04:08:23Z

src/Microsoft.Identity.Web/Resource/OpenIdConnectMiddlewareDiagnostics.cs

@@ -80,6 +80,8 @@ public OpenIdConnectMiddlewareDiagnostics(ILogger<OpenIdConnectMiddlewareDiagnos
        /// <param name="events">Events to subscribe to.</param>
        public void Subscribe(OpenIdConnectEvents events)
        {
+            events ??= new OpenIdConnectEvents();


nit: space?

jennyf19 · 2020-06-25T04:10:17Z

src/Microsoft.Identity.Web/Resource/RegisterValidAudience.cs

+            if (token == null)
+            {
+                throw new SecurityTokenValidationException("Token is not JWT token.");
+            }


is that the right error message? when would the casting fail in this case and result in a null token value? the message should have some customer value, so how can they resolve or understand the issue to fix it.

I am not sure if it's always JwtSecurityToken. Is it? If it is, I can just explicitly cast it.

might be something to look into. the error message is misleading.

jennyf19 · 2020-06-25T04:12:34Z

some comments. it's hard to review w/so many xml and spelling changes. in the future, can you do those in a separate PR, especially when you have so many? it tends to create noise and slows down the review. thank you.

pmaytak · 2020-06-25T07:13:48Z

some comments. it's hard to review w/so many xml and spelling changes. in the future, can you do those in a separate PR, especially when you have so many? it tends to create noise and slows down the review. thank you.

@jennyf19 Will do.

jmprieur

LGTM.
Thanks @pmaytak
A few questions

.editorconfig

src/Microsoft.Identity.Web/AuthorizeForScopesAttribute.cs

src/Microsoft.Identity.Web/ITokenAcquisition.cs

jennyf19

on my end, only pending things are making a separate PR for throwing exceptions w/the certs and checking the token type.

…l null checks for public method parameters. Minor comment fixes.

pmaytak commented Jun 23, 2020

View reviewed changes

.editorconfig Show resolved Hide resolved

pmaytak commented Jun 23, 2020

View reviewed changes

src/Microsoft.Identity.Web/CertificateManagement/DefaultCertificateLoader.cs Show resolved Hide resolved

pmaytak commented Jun 23, 2020

View reviewed changes

src/Microsoft.Identity.Web/InstanceDiscovery/IssuerConfigurationRetriever.cs Outdated Show resolved Hide resolved

pmaytak commented Jun 23, 2020

View reviewed changes

pmaytak requested review from jennyf19, jmprieur and bgavrilMS June 23, 2020 08:12

pmaytak marked this pull request as ready for review June 23, 2020 08:12