Updates as per review comments

Author: Shama-K

5-WebApp-AuthZ/5-2-Groups/Infrastructure/CustomAuthorization.cs

@@ -10,9 +10,9 @@
 namespace WebApp_OpenIDConnect_DotNet.Infrastructure
 {
     /// <summary>
-    /// GroupPolicyHandler represents Policy-based authorization.
+    /// GroupPolicyHandler deals with custom Policy-based authorization.
     /// GroupPolicyHandler evaluates the GroupPolicyRequirement against AuthorizationHandlerContext 
-    /// to determine if authorization is allowed.
+    /// by calling CheckUsersGroupMembership method to determine if authorization is allowed.
     /// </summary>
     public class GroupPolicyHandler : AuthorizationHandler<GroupPolicyRequirement>
     {

5-WebApp-AuthZ/5-2-Groups/README.md

@@ -331,7 +331,7 @@ The following files have the code that would be of interest to you:
      ```
 
     - have been replaced by these lines:
-      
+
      ```CSharp
       services.AddAuthentication(OpenIdConnectDefaults.AuthenticationScheme)
               .AddMicrosoftIdentityWebApp(
@@ -353,56 +353,68 @@ The following files have the code that would be of interest to you:
 
     `AddMicrosoftGraph` registers the service for `GraphServiceClient`. The values for BaseUrl and Scopes defined in `GraphAPI` section of **appsettings.json**.
 
-1. In GraphHelper.cs, ProcessClaimsForGroupsOverage method uses `GraphServiceClient` to retrieve groups for the signed-in user from [/me/memberOf](https://docs.microsoft.com/graph/api/user-list-memberof) endpoint. All the groups are stored in list of claims and the data can be used in the application as per requirement.
-
-   ```csharp
-   public static async Task ProcessClaimsForGroupsOverage(TokenValidatedContext context)
-   {
-        if (context.Principal.Claims.Any(x => x.Type == "hasgroups" || (x.Type == "_claim_names" && x.Value == "{\"groups\":\"src1\"}")))
-        {
-            var graphClient = context.HttpContext.RequestServices.GetService<GraphServiceClient>();
-            if (graphClient == null)
-            {
-                Console.WriteLine("No service for type 'Microsoft.Graph.GraphServiceClient' has been registered.");
-            }
-            else if (context.SecurityToken != null)
-            {
-                if (!context.HttpContext.Items.ContainsKey("JwtSecurityTokenUsedToCallWebAPI"))
-                {
-                    context.HttpContext.Items.Add("JwtSecurityTokenUsedToCallWebAPI", context.SecurityToken as JwtSecurityToken);
-                }
-                string select = "id,displayName,onPremisesNetBiosName,onPremisesDomainName,onPremisesSamAccountNameonPremisesSecurityIdentifier";
-                IUserMemberOfCollectionWithReferencesPage memberPage = new UserMemberOfCollectionWithReferencesPage();
-                try
-                {
-                    memberPage = await graphClient.Me.MemberOf.Request().Select(select).GetAsync().ConfigureAwait(false);
-                }
-                catch(Exception graphEx)
-                {
-                    var exMsg = graphEx.InnerException != null ? graphEx.InnerException.Message : graphEx.Message;
-                    Console.WriteLine("Call to Microsoft Graph failed: "+ exMsg);
-                }
-                if (memberPage?.Count > 0)
-                {
-                    var allgroups = ProcessIGraphServiceMemberOfCollectionPage(memberPage);
-                    if (allgroups?.Count > 0)
-                    {
-                        var identity = (ClaimsIdentity)context.Principal.Identity;
-                        if (identity != null)
-                        {
-                            RemoveExistingClaims(identity);
-                            List<Claim> groupClaims = new List<Claim>();
-                            foreach (Group group in allgroups)
-                            {
-                                groupClaims.Add(new Claim("groups", group.Id));
-                            }
-                        }
-                    }
-                }
-            }
-        }
-    ....
-    }
+1. In GraphHelper.cs, ProcessClaimsForGroupsOverage method uses `GraphServiceClient` to retrieve groups for the signed-in user from [/me/memberOf](https://docs.microsoft.com/graph/api/user-list-memberof) endpoint. All the group ids are stored in list of claims and the data can be used in the application as per requirement.
+
+    ```csharp
+    public static async Task ProcessClaimsForGroupsOverage(TokenValidatedContext context)
+    {
+          if (context.Principal.Claims.Any(x => x.Type == "hasgroups" || (x.Type == "_claim_names" && x.Value == "{\"groups\":\"src1\"}")))
+          {
+              var graphClient = context.HttpContext.RequestServices.GetService<GraphServiceClient>();
+              if (graphClient == null)
+              {
+                  Console.WriteLine("No service for type 'Microsoft.Graph.GraphServiceClient' has been registered.");
+              }
+              else if (context.SecurityToken != null)
+              {
+                  if (!context.HttpContext.Items.ContainsKey("JwtSecurityTokenUsedToCallWebAPI"))
+                  {
+                      context.HttpContext.Items.Add("JwtSecurityTokenUsedToCallWebAPI", context.SecurityToken as JwtSecurityToken);
+                  }
+                  string select = "id,displayName,onPremisesNetBiosName,onPremisesDomainName,onPremisesSamAccountNameonPremisesSecurityIdentifier";
+                  IUserMemberOfCollectionWithReferencesPage memberPage = new UserMemberOfCollectionWithReferencesPage();
+                  try
+                  {
+                      memberPage = await graphClient.Me.MemberOf.Request().Select(select).GetAsync().ConfigureAwait(false);
+                  }
+                  catch(Exception graphEx)
+                  {
+                      var exMsg = graphEx.InnerException != null ? graphEx.InnerException.Message : graphEx.Message;
+                      Console.WriteLine("Call to Microsoft Graph failed: "+ exMsg);
+                  }
+                  if (memberPage?.Count > 0)
+                  {
+                      var allgroups = ProcessIGraphServiceMemberOfCollectionPage(memberPage);
+                      if (allgroups?.Count > 0)
+                      {
+                          var identity = (ClaimsIdentity)context.Principal.Identity;
+                          if (identity != null)
+                          {
+                              RemoveExistingClaims(identity);
+                              List<Claim> groupClaims = new List<Claim>();
+                              foreach (Group group in allgroups)
+                              {
+                                  groupClaims.Add(new Claim("groups", group.Id));
+                              }
+                          }
+                      }
+                  }
+              }
+          }
+      ....
+      }
+      ```
+
+    If application is using different group property type for instance, `NetBIOSDomain\sAMAccountName` then replace
+
+    ```csharp
+    groupClaims.Add(new Claim("groups", group.Id));
+    ```
+
+    with
+
+    ```csharp
+    groupClaims.Add(group.OnPremisesNetBiosName+"\\"+group.OnPremisesSamAccountName));
     ```
 
 1. UserProfile\Index.cshtml

5-WebApp-AuthZ/5-2-Groups/Services/MicrosoftGraph-Rest/GraphHelper.cs

@@ -25,16 +25,19 @@ public static async Task<List<string>> GetSignedInUsersGroups(TokenValidatedCont
         {
             List<string> groupClaims = new List<string>();
 
-            // Gets group values from session variable if exists.
-            groupClaims = GetSessionGroupList(context.HttpContext.Session);
-            if (groupClaims?.Count>0)
-            {
-                return groupClaims;
-            }
             // Checks if the incoming token contained a 'Group Overage' claim.
-            else if (HasOverageOccurred(context.Principal))
+            if (HasOverageOccurred(context.Principal))
             {
-                groupClaims= await ProcessUserGroupsForOverage(context);
+                // Gets group values from session variable if exists.
+                groupClaims = GetSessionGroupList(context.HttpContext.Session);
+                if (groupClaims?.Count > 0)
+                {
+                    return groupClaims;
+                }
+                else
+                {
+                    groupClaims = await ProcessUserGroupsForOverage(context);
+                }
             }
             return groupClaims;
         }
@@ -72,12 +75,11 @@ private static bool HasOverageOccurred(ClaimsPrincipal identity)
         /// </summary>
         /// <param name="context"></param>
         /// <returns></returns>
-        static async Task<List<string>> ProcessUserGroupsForOverage(TokenValidatedContext context)
+        private static async Task<List<string>> ProcessUserGroupsForOverage(TokenValidatedContext context)
         {
             List<string> groupClaims = new List<string>();
             try
             {
-               
                 // Before instatntiating GraphServiceClient, the app should have granted admin consent for 'GroupMember.Read.All' permission.
                 var graphClient = context.HttpContext.RequestServices.GetService<GraphServiceClient>();
 
@@ -226,7 +228,7 @@ public static bool CheckUsersGroupMembership(AuthorizationHandlerContext context
         {
             bool result = false;
             // Checks if groups claim exists in claims collection of signed-in User.
-            if(HasOverageOccurred(context.User))
+            if (HasOverageOccurred(context.User))
             {
                 // Calls method GetSessionGroupList to get groups from session.
                 var groups = GetSessionGroupList(_httpContextAccessor.HttpContext.Session);
@@ -239,7 +241,7 @@ public static bool CheckUsersGroupMembership(AuthorizationHandlerContext context
             }
             else if (context.User.Claims.Any(x => x.Type == "groups" && x.Value == GroupName))
             {
-                    result = true;
+                result = true;
             }
             return result;
         }

5-WebApp-AuthZ/5-2-Groups/Startup.cs

@@ -47,7 +47,7 @@ public void ConfigureServices(IServiceCollection services)
                     options.Events.OnTokenValidated = async context =>
                     {
                         //Calls method to process groups overage claim.
-                        var groupClaims = await GraphHelper.GetSignedInUsersGroups(context);
+                        var overageGroupClaims = await GraphHelper.GetSignedInUsersGroups(context);
                     };
                 }, options => { Configuration.Bind("AzureAd", options); })
                     .EnableTokenAcquisitionToCallDownstreamApi(options => Configuration.Bind("AzureAd", options), initialScopes)