minor edits

Author: kalyankrishna1

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

@@ -1,35 +1,47 @@
-# Registering the sample apps with Microsoft identity platform and updating the configuration files using PowerShell scripts
+# Registering the sample apps with the Microsoft identity platform and updating the configuration files using PowerShell
 
 ## Overview
 
 ### Quick summary
 
 1. On Windows run PowerShell and navigate to the root of the cloned directory
 1. In PowerShell run:
+
    ```PowerShell
    Set-ExecutionPolicy -ExecutionPolicy RemoteSigned -Scope Process -Force
    ```
+
 1. Run the script to create your Azure AD application and configure the code of the sample application accordingly. (Other ways of running the scripts are described below)
+
    ```PowerShell
    cd .\AppCreationScripts\
    .\Configure.ps1
    ```
+
 1. Open the Visual Studio solution and click start
 
 ### More details
 
 The following paragraphs:
 
-- [Present the scripts](#presentation-of-the-scripts) and explain their [usage patterns](#usage-pattern-for-tests-and-devops-scenarios) for test and DevOps scenarios.
-- Explain the [pre-requisites](#pre-requisites)
-- Explain [four ways of running the scripts](#four-ways-to-run-the-script):
-  - [Interactively](#option-1-interactive) to create the app in your home tenant
-  - [Passing credentials](#option-2-non-interactive) to create the app in your home tenant
-  - [Interactively in a specific tenant](#option-3-interactive-but-create-apps-in-a-specified-tenant)
-  - [Passing credentials in a specific tenant](#option-4-non-interactive-and-create-apps-in-a-specified-tenant)
-  - [Passing environment name, for Sovereign clouds](#running-the-script-on-azure-sovereign-clouds)
-
-## Goal of the scripts
+- [Registering the sample apps with the Microsoft identity platform and updating the configuration files using PowerShell](#Registering-the-sample-apps-with-the-Microsoft-identity-platform-and-updating-the-configuration-files-using-PowerShell)
+  - [Overview](#Overview)
+    - [Quick summary](#Quick-summary)
+    - [More details](#More-details)
+  - [Goal of the provided scripts](#Goal-of-the-provided-scripts)
+    - [Presentation of the scripts](#Presentation-of-the-scripts)
+    - [Usage pattern for tests and DevOps scenarios](#Usage-pattern-for-tests-and-DevOps-scenarios)
+  - [How to use the app creation scripts?](#How-to-use-the-app-creation-scripts)
+    - [Pre-requisites](#Pre-requisites)
+    - [Run the script and start running](#Run-the-script-and-start-running)
+    - [Four ways to run the script](#Four-ways-to-run-the-script)
+      - [Option 1 (interactive)](#Option-1-interactive)
+      - [Option 2 (non-interactive)](#Option-2-non-interactive)
+      - [Option 3 (Interactive, but create apps in a specified tenant)](#Option-3-Interactive-but-create-apps-in-a-specified-tenant)
+      - [Option 4 (non-interactive, and create apps in a specified tenant)](#Option-4-non-interactive-and-create-apps-in-a-specified-tenant)
+    - [Running the script on Azure Sovereign clouds](#Running-the-script-on-Azure-Sovereign-clouds)
+
+## Goal of the provided scripts
 
 ### Presentation of the scripts
 
@@ -56,36 +68,43 @@ The `Configure.ps1` will stop if it tries to create an Azure AD application whic
 ### Pre-requisites
 
 1. Open PowerShell (On Windows, press  `Windows-R` and type `PowerShell` in the search window)
-2. Navigate to the root directory of the project.
-3. Until you change it, the default [Execution Policy](https:/go.microsoft.com/fwlink/?LinkID=135170) for scripts is usually `Restricted`. In order to run the PowerShell script you need to set the Execution Policy to `RemoteSigned`. You can set this just for the current PowerShell process by running the command:
+1. Navigate to the root directory of the project.
+1. Until you change it, the default [Execution Policy](https:/go.microsoft.com/fwlink/?LinkID=135170) for scripts is usually `Restricted`. In order to run the PowerShell script you need to set the Execution Policy to `RemoteSigned`. You can set this just for the current PowerShell process by running the command:
+
     ```PowerShell
     Set-ExecutionPolicy -ExecutionPolicy RemoteSigned -Scope Process
     ```
-### (Optionally) install AzureAD PowerShell modules
+
+1. ### (Optionally) install AzureAD PowerShell modules
+2. 
 The scripts install the required PowerShell module (AzureAD) for the current user if needed. However, if you want to install if for all users on the machine, you can follow the following steps:
 
-4. If you have never done it already, in the PowerShell window, install the AzureAD PowerShell modules. For this:
+1. If you have never done it already, in the PowerShell window, install the AzureAD PowerShell modules. For this:
 
    1. Open PowerShell as admin (On Windows, Search Powershell in the search bar, right click on it and select Run as administrator).
    2. Type:
+ 
       ```PowerShell
       Install-Module AzureAD
       ```
 
       or if you cannot be administrator on your machine, run:
+ 
       ```PowerShell
       Install-Module AzureAD -Scope CurrentUser
       ```
 
 ### Run the script and start running
 
-5. Go to the `AppCreationScripts` sub-folder. From the folder where you cloned the repo,
+1. Go to the `AppCreationScripts` sub-folder. From the folder where you cloned the repo,
+
     ```PowerShell
     cd AppCreationScripts
     ```
-6. Run the scripts. See below for the [four options](#four-ways-to-run-the-script) to do that.
-7. Open the Visual Studio solution, and in the solution's context menu, choose **Set Startup Projects**.
-8. select **Start** for the projects
+
+1. Run the scripts. See below for the [four options](#four-ways-to-run-the-script) to do that.
+1. Open the Visual Studio solution, and in the solution's context menu, choose **Set Startup Projects**.
+1. select **Start** for the projects
 
 You're done. this just works!
 
@@ -123,6 +142,7 @@ Of course, in real life, you might already get the password as a `SecureString`.
 #### Option 3 (Interactive, but create apps in a specified tenant)
 
   if you want to create the apps in a particular tenant, you can use the following option:
+  
 - open the [Azure portal](https://portal.azure.com)
 - Select the Azure Active directory you are interested in (in the combo-box below your name on the top right of the browser window)
 - Find the "Active Directory" object in this tenant

5-WebApp-AuthZ/5-2-Groups/AppCreationScripts/Cleanup.ps1

@@ -7,7 +7,7 @@ param(
     [string] $azureEnvironmentName
 )
 
-#Requires -Modules AzureAD
+#Requires -Modules AzureAD -RunAsAdministrator
 
 
 if ($null -eq (Get-Module -ListAvailable -Name "AzureAD")) { 

5-WebApp-AuthZ/5-2-Groups/AppCreationScripts/Configure.ps1

@@ -7,7 +7,7 @@ param(
     [string] $azureEnvironmentName
 )
 
-#Requires -Modules AzureAD
+#Requires -Modules AzureAD -RunAsAdministrator
 
 <#
  This script creates the Azure AD applications needed for this sample and updates the configuration files
@@ -102,42 +102,6 @@ Function GetRequiredPermissions([string] $applicationDisplayName, [string] $requ
 }
 
 
-Function UpdateLine([string] $line, [string] $value)
-{
-    $index = $line.IndexOf('=')
-    $delimiter = ';'
-    if ($index -eq -1)
-    {
-        $index = $line.IndexOf(':')
-        $delimiter = ','
-    }
-    if ($index -ige 0)
-    {
-        $line = $line.Substring(0, $index+1) + " "+'"'+$value+'"'+$delimiter
-    }
-    return $line
-}
-
-Function UpdateTextFile([string] $configFilePath, [System.Collections.HashTable] $dictionary)
-{
-    $lines = Get-Content $configFilePath
-    $index = 0
-    while($index -lt $lines.Length)
-    {
-        $line = $lines[$index]
-        foreach($key in $dictionary.Keys)
-        {
-            if ($line.Contains($key))
-            {
-                $lines[$index] = UpdateLine $line $dictionary[$key]
-            }
-        }
-        $index++
-    }
-
-    Set-Content -Path $configFilePath -Value $lines -Force
-}
-
 Set-Content -Value "<html><body><table>" -Path createdApps.html
 Add-Content -Value "<thead><tr><th>Application</th><th>AppId</th><th>Url in the Azure portal</th></tr></thead><tbody>" -Path createdApps.html
 
@@ -233,7 +197,7 @@ Function ConfigureApplications
    # Add Required Resources Access (from 'webApp' to 'Microsoft Graph')
    Write-Host "Getting access from 'webApp' to 'Microsoft Graph'"
    $requiredPermissions = GetRequiredPermissions -applicationDisplayName "Microsoft Graph" `
-                                                -requiredDelegatedPermissions "GroupMember.Read.All" `
+                                                -requiredDelegatedPermissions "User.Read GroupMember.Read.All" `
 
    $requiredResourcesAccess.Add($requiredPermissions)
 
@@ -244,8 +208,6 @@ Function ConfigureApplications
    # Update config file for 'webApp'
    $configFile = $pwd.Path + "\..\appsettings.json"
    Write-Host "Updating the sample code ($configFile)"
-   $dictionary = @{ "ClientId" = $webAppAadApplication.AppId;"TenantId" = $tenantId;"Domain" = $tenantName;"ClientSecret" = $webAppAppKey };
-   UpdateTextFile -configFilePath $configFile -dictionary $dictionary
 
    Add-Content -Value "</tbody></table></body></html>" -Path createdApps.html  
 }

5-WebApp-AuthZ/5-2-Groups/AppCreationScripts/sample.json

@@ -25,7 +25,7 @@
       "RequiredResourcesAccess": [
         {
           "Resource": "Microsoft Graph",
-          "DelegatedPermissions": [ "GroupMember.Read.All" ]
+          "DelegatedPermissions": [ "User.Read GroupMember.Read.All" ]
         }
       ]
     }

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

@@ -1,51 +1,55 @@
-using System.Threading.Tasks;
+using Microsoft.AspNetCore.Authentication.OpenIdConnect;
+using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Graph;
 using System;
 using System.Collections.Generic;
-using Microsoft.AspNetCore.Authentication.OpenIdConnect;
-using Microsoft.Extensions.DependencyInjection;
 using System.IdentityModel.Tokens.Jwt;
-using System.Security.Claims;
 using System.Linq;
+using System.Security.Claims;
+using System.Threading.Tasks;
 
 namespace WebApp_OpenIDConnect_DotNet.Services.GroupProcessing
 {
     public class GraphHelper
     {
         /// <summary>
-        /// Adds groups claim for group overage
+        /// This method inspects the claims collection created from the ID or Access token and detects groups overage. If Groups overage is detected, the method then makes calls to
+        /// Microsoft Graph to fetch the group membership of the authenticated user.
         /// </summary>
         /// <param name="context">TokenValidatedContext</param>
-        public static async Task ProcessGroupsClaimforAccessToken(TokenValidatedContext context)
+        public static async Task ProcessClaimsForGroupsOverage(TokenValidatedContext context)
         {
             try
             {
-                //Checks if the token contains 'Group Overage' Claim.
+                // Checks if the incoming token contained a 'Group Overage' claim.
                 if (context.Principal.Claims.Any(x => x.Type == "hasgroups" || (x.Type == "_claim_names" && x.Value == "{\"groups\":\"src1\"}")))
                 {
-                    //This API should have permission set for Microsoft graph: 'GroupMember.Read.All'
+                    // For this API call to succeed, the app should have permission 'GroupMember.Read.All' granted.
                     var graph = context.HttpContext.RequestServices.GetService<GraphServiceClient>();
 
                     if (graph == null)
                     {
-                        Console.WriteLine("No service for type 'Microsoft.Graph.GraphServiceClient' has been registered.");
+                        Console.WriteLine("No service for type 'Microsoft.Graph.GraphServiceClient' has been registered in the Startup.");
                     }
                     else if (context.SecurityToken != null)
                     {
+                        // Check if an on-behalf-of all was made to a Web API
                         if (!context.HttpContext.Items.ContainsKey("JwtSecurityTokenUsedToCallWebAPI"))
                         {
-                            //Added current access token in below key to get Access Token on-behalf of user. 
+                            // extract the cached AT that was presented to the Web API
                             context.HttpContext.Items.Add("JwtSecurityTokenUsedToCallWebAPI", context.SecurityToken as JwtSecurityToken);
                         }
-                        //Specify the property names in the 'select' variable to get values for the specified properties.
+
+                        // We do not want to pull all attributes of a group from MS Graph, so we use a 'select' to just pick the ones we need.
                         string select = "id,displayName,onPremisesNetBiosName,onPremisesDomainName,onPremisesSamAccountNameonPremisesSecurityIdentifier";
 
-                        //Request to get groups and directory roles that the user is a direct member of.
+                        // TODO: this line needs a try-catch, with the exception error message being "A call to Microsoft Graph failed, the error is <whatever>"
+                        // Make a Graph call to get groups and directory roles that the user is a direct member of.
                         var memberPage = await graph.Me.MemberOf.Request().Select(select).GetAsync().ConfigureAwait(false);
 
                         if (memberPage?.Count > 0)
                         {
-                            //There is a limit to number of groups returned, below method make calls to Microsoft graph to get all the groups.
+                            // If the result is paginated, this method will process all the pages for us.
                             var allgroups = ProcessIGraphServiceMemberOfCollectionPage(memberPage);
 
                             if (allgroups?.Count > 0)
@@ -54,17 +58,19 @@ public static async Task ProcessGroupsClaimforAccessToken(TokenValidatedContext
 
                                 if (identity != null)
                                 {
-                                    //Remove existing groups claims
-                                    RemoveExistingClaims(context, identity);
+                                    // Remove any existing groups claim
+                                    RemoveExistingClaim(identity);
 
                                     List<Claim> groupClaims = new List<Claim>();
 
+                                    // Re-populate the `groups` claim with the complete list of groups fetched from MS Graph
                                     foreach (Group group in allgroups)
                                     {
-                                        //Claim is added in list and it can be used by saving the groups in session or as per project implementation.
-                                        //Adds group id as 'groups' claim. But it can be changed as per requirment. 
-                                        //For instance if the required format is 'NetBIOSDomain\sAMAccountName' then the code is as commented below:
-                                        //groupClaims.AddClaim(new Claim("groups", group.OnPremisesNetBiosName+"\\"+group.OnPremisesSamAccountName));
+                                        // The following code adds group ids to the 'groups' claim. But depending upon your reequirement and the format of the 'groups' claim selected in
+                                        // the app registration, you might want to add other attributes than id to the `groups` claim, examples being;
+
+                                        // For instance if the required format is 'NetBIOSDomain\sAMAccountName' then the code is as commented below:
+                                        // groupClaims.AddClaim(new Claim("groups", group.OnPremisesNetBiosName+"\\"+group.OnPremisesSamAccountName));
                                         groupClaims.Add(new Claim("groups", group.Id));
                                     }
                                 }
@@ -81,8 +87,9 @@ public static async Task ProcessGroupsClaimforAccessToken(TokenValidatedContext
             {
                 if (context.HttpContext.Items.ContainsKey("JwtSecurityTokenUsedToCallWebAPI"))
                 {
-                    //Remove the key as Microsoft.Identity.Web library utilizes this key. 
-                    //If not removed then it can cause failure to the application.
+                    // TODO: The following comment makes no sense !
+                    // Remove the key as Microsoft.Identity.Web library utilizes this key.
+                    // If not removed then it can cause failure to the application.
                     context.HttpContext.Items.Remove("JwtSecurityTokenUsedToCallWebAPI");
                 }
             }
@@ -93,10 +100,10 @@ public static async Task ProcessGroupsClaimforAccessToken(TokenValidatedContext
         /// </summary>
         /// <param name="context"></param>
         /// <param name="identity"></param>
-        private static void RemoveExistingClaims(TokenValidatedContext context, ClaimsIdentity identity)
+        private static void RemoveExistingClaim(ClaimsIdentity identity)
         {
-            //clear existing claim
-            List<Claim> existingGroupsClaims = context.Principal.Claims.Where(x => x.Type == "groups").ToList();
+            // clear an existing claim
+            List<Claim> existingGroupsClaims = identity.Claims.Where(x => x.Type == "groups").ToList();
             if (existingGroupsClaims?.Count > 0)
             {
                 foreach (Claim groupsClaim in existingGroupsClaims)
@@ -152,4 +159,4 @@ private static List<Group> ProcessIGraphServiceMemberOfCollectionPage(IUserMembe
             return allGroups;
         }
     }
-}
+}

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

@@ -45,17 +45,19 @@ public void ConfigureServices(IServiceCollection services)
                     options.Events.OnTokenValidated = async context =>
                     {
                         //Calls method to process groups overage claim.
-                        await GraphHelper.ProcessGroupsClaimforAccessToken(context);
+                        await GraphHelper.ProcessClaimsForGroupsOverage(context);
                     };
                 }, options => { Configuration.Bind("AzureAd", options); })
                     .EnableTokenAcquisitionToCallDownstreamApi(options => Configuration.Bind("AzureAd", options))
                     .AddMicrosoftGraph(Configuration.GetSection("GraphAPI"))
                     .AddInMemoryTokenCaches();
 
-            services.Configure<OpenIdConnectOptions>(OpenIdConnectDefaults.AuthenticationScheme, options => {
+            services.Configure<OpenIdConnectOptions>(OpenIdConnectDefaults.AuthenticationScheme, options =>
+            {
                 // The following code instructs the ASP.NET Core middleware to use the data in the "groups" claim in the [Authorize] attribute and for User.IsInRole()
+                // Uncomment the following if you wish to use groups for roles
                 // See https://docs.microsoft.com/en-us/aspnet/core/security/authorization/roles for more info.
-                options.TokenValidationParameters.RoleClaimType = "groups";
+                // options.TokenValidationParameters.RoleClaimType = "groups";
             });
 
             services.AddControllersWithViews(options =>