Merge pull request #182 from Azure-Samples/basher-3-1

Update 3.1 to follow BASHER and Zero Trust guidelines

Author: Doğan Erişen

.github/workflows/node.js.yml

@@ -14,7 +14,7 @@ jobs:
 
     strategy:
       matrix:
-        node-version: [12.x]
+        node-version: [16.x]
         # See supported Node.js release schedule at https://nodejs.org/en/about/releases/
 
     steps:
@@ -65,7 +65,7 @@ jobs:
         npm ci
         npm audit --production
         npm run test
-      
+
     - run: |
         cd 7-AdvancedScenarios/1-call-api-obo/SPA
         npm ci
@@ -76,4 +76,4 @@ jobs:
         cd 7-AdvancedScenarios/2-call-api-pop/SPA
         npm ci
         npm audit --production
-        npm run test
+        npm run test

.gitignore

@@ -111,4 +111,7 @@ obj/
 .vs
 
 # VS Code cache
-.vscode
+.vscode
+
+# Angular cache
+.angular

3-Authorization-II/1-call-api/API/TodoListAPI.Tests/ConfigurationTests.cs

@@ -1,6 +1,5 @@
 using System;
 using Xunit;
-using System.Text.RegularExpressions;
 using Microsoft.Extensions.Configuration;
 
 namespace TodoListAPI.Tests
@@ -17,37 +16,30 @@ public static IConfiguration InitConfiguration()
         }
 
         [Fact]
-        public void ShouldNotContainClientId()
+        public void ShouldContainClientId()
         {
             var myConfiguration = ConfigurationTests.InitConfiguration();
-            string clientId = myConfiguration.GetSection("AzureAd")["ClientId"];
+            var clientId = myConfiguration.GetSection("AzureAd")["ClientId"];
 
-            string pattern = @"(\{){0,1}[0-9a-fA-F]{8}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{12}(\}){0,1}";
-            var regex = new Regex(pattern);
-            Assert.DoesNotMatch(regex, clientId);
+            Assert.True(Guid.TryParse(clientId, out var theGuid));
         }
 
         [Fact]
-        public void ShouldNotContainTenantId()
+        public void ShouldContainTenantId()
         {
             var myConfiguration = ConfigurationTests.InitConfiguration();
-            string tenantId = myConfiguration.GetSection("AzureAd")["TenantId"];
+            var tenantId = myConfiguration.GetSection("AzureAd")["TenantId"];
 
-            string pattern = @"(\{){0,1}[0-9a-fA-F]{8}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{12}(\}){0,1}";
-            var regex = new Regex(pattern);
-            Assert.DoesNotMatch(regex, tenantId);
+            Assert.True(Guid.TryParse(tenantId, out var theGuid));
         }
 
         [Fact]
-        public void ShouldNotContainDomain()
+        public void ShouldContainDomain()
         {
             var myConfiguration = ConfigurationTests.InitConfiguration();
-            string domain = myConfiguration.GetSection("AzureAd")["Domain"];
+            var domain = $"https://{myConfiguration.GetSection("AzureAd")["Domain"]}";
 
-            string pattern = @"(^http[s]?:\/\/|[a-z]*\.[a-z]{3}\.[a-z]{2})|([a-z]*\.[a-z]{3}$)";
-            var regex = new Regex(pattern);
-
-            Assert.DoesNotMatch(regex, domain);
+            Assert.True(Uri.TryCreate(domain, UriKind.Absolute, out var uri));
         }
     }
 }

3-Authorization-II/1-call-api/API/TodoListAPI.Tests/TodoListAPI.Tests.csproj

@@ -1,7 +1,7 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">
 
   <PropertyGroup>
-    <TargetFramework>netcoreapp3.1</TargetFramework>
+    <TargetFramework>net6.0</TargetFramework>
 
     <IsPackable>false</IsPackable>
   </PropertyGroup>

3-Authorization-II/1-call-api/API/TodoListAPI/Controllers/TodoListController.cs

@@ -1,14 +1,13 @@
-using System;
+using System.Linq;
 using System.Collections.Generic;
-using System.Linq;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc;
 using Microsoft.AspNetCore.Authorization;
 using Microsoft.EntityFrameworkCore;
-using TodoListAPI.Models;
-using System.Security.Claims;
+using Microsoft.Identity.Web;
 using Microsoft.Identity.Web.Resource;
+using TodoListAPI.Models;
 
 namespace TodoListAPI.Controllers
 {
@@ -17,70 +16,139 @@ namespace TodoListAPI.Controllers
     [ApiController]
     public class TodoListController : ControllerBase
     {
-        // The Web API will only accept tokens 1) for users, and 
-        // 2) having the access_as_user scope for this API
-        static readonly string[] scopeRequiredByApi = new string[] { "access_as_user" };
-
         private readonly TodoContext _context;
 
+        private const string _todoListRead = "TodoList.Read";
+        private const string _todoListReadWrite = "TodoList.ReadWrite";
+        private const string _todoListReadAll = "TodoList.Read.All";
+        private const string _todoListReadWriteAll = "TodoList.ReadWrite.All";
+
         public TodoListController(TodoContext context)
         {
             _context = context;
         }
 
+        /// <summary>
+        /// Indicates if the AT presented has application or delegated permissions.
+        /// </summary>
+        /// <returns></returns>
+        private bool IsAppOnlyToken()
+        {
+            // Add in the optional 'idtyp' claim to check if the access token is coming from an application or user.
+            // See: https://docs.microsoft.com/en-us/azure/active-directory/develop/active-directory-optional-claims
+            if (HttpContext.User.Claims.Any(c => c.Type == "idtyp"))
+            {
+                return HttpContext.User.Claims.Any(c => c.Type == "idtyp" && c.Value == "app");
+            }
+            else
+            {
+                // alternatively, if an AT contains the roles claim but no scp claim, that indicates it's an app token
+                return HttpContext.User.Claims.Any(c => c.Type == "roles") && HttpContext.User.Claims.Any(c => c.Type != "scp");
+            }
+        }
+
         // GET: api/TodoItems
         [HttpGet]
+        /// <summary>
+        /// Access tokens that have neither the 'scp' (for delegated permissions) nor
+        /// 'roles' (for application permissions) claim are not to be honored.
+        ///
+        /// An access token issued by Azure AD will have at least one of the two claims. Access tokens
+        /// issued to a user will have the 'scp' claim. Access tokens issued to an application will have
+        /// the roles claim. Access tokens that contain both claims are issued only to users, where the scp
+        /// claim designates the delegated permissions, while the roles claim designates the user's role.
+        ///
+        /// To determine whether an access token was issued to a user (i.e delegated) or an application
+        /// more easily, we recommend enabling the optional claim 'idtyp'. For more information, see:
+        /// https://docs.microsoft.com/azure/active-directory/develop/access-tokens#user-and-application-tokens
+        /// </summary>
+        [RequiredScopeOrAppPermission(
+            AcceptedScope = new string[] { _todoListRead, _todoListReadWrite },
+            AcceptedAppPermission = new string[] { _todoListReadAll, _todoListReadWriteAll }
+        )]
         public async Task<ActionResult<IEnumerable<TodoItem>>> GetTodoItems()
         {
-            HttpContext.VerifyUserHasAnyAcceptedScope(scopeRequiredByApi);
-            string owner = User.FindFirst(ClaimTypes.NameIdentifier)?.Value;
-            return await _context.TodoItems.Where(item => item.Owner == owner).ToListAsync();
+            if (!IsAppOnlyToken())
+            {
+                /// <summary>
+                /// The 'oid' (object id) is the only claim that should be used to uniquely identify
+                /// a user in an Azure AD tenant. The token might have one or more of the following claim,
+                /// that might seem like a unique identifier, but is not and should not be used as such:
+                ///
+                /// - upn (user principal name): might be unique amongst the active set of users in a tenant
+                /// but tend to get reassigned to new employees as employees leave the organization and others
+                /// take their place or might change to reflect a personal change like marriage.
+                ///
+                /// - email: might be unique amongst the active set of users in a tenant but tend to get reassigned
+                /// to new employees as employees leave the organization and others take their place.
+                /// </summary>
+                return await _context.TodoItems.Where(x => x.Owner == HttpContext.User.GetObjectId()).ToListAsync();
+            }
+            else
+            {
+                return await _context.TodoItems.ToListAsync();
+            }
         }
 
         // GET: api/TodoItems/5
         [HttpGet("{id}")]
+        [RequiredScopeOrAppPermission(
+            AcceptedScope = new string[] { _todoListRead, _todoListReadWrite },
+            AcceptedAppPermission = new string[] { _todoListReadAll, _todoListReadWriteAll }
+        )]
         public async Task<ActionResult<TodoItem>> GetTodoItem(int id)
         {
-            HttpContext.VerifyUserHasAnyAcceptedScope(scopeRequiredByApi);
- 
-            var todoItem = await _context.TodoItems.FindAsync(id);
-
-            if (todoItem == null)
+            // if it only has delegated permissions, then it will be t.id==id && x.Owner == owner
+            // if it has app permissions the it will return t.id==id
+            if (!IsAppOnlyToken())
             {
-                return NotFound();
+                return await _context.TodoItems.FirstOrDefaultAsync(t => t.Id == id && t.Owner == HttpContext.User.GetObjectId());
+            }
+            else
+            {
+                return await _context.TodoItems.FirstOrDefaultAsync(t => t.Id == id);
             }
-
-            return todoItem;
         }
 
         // PUT: api/TodoItems/5
         // To protect from overposting attacks, please enable the specific properties you want to bind to, for
         // more details see https://aka.ms/RazorPagesCRUD.
         [HttpPut("{id}")]
+        [RequiredScopeOrAppPermission(
+            AcceptedScope = new string[] { _todoListReadWrite },
+            AcceptedAppPermission = new string[] { _todoListReadWriteAll }
+        )]
         public async Task<IActionResult> PutTodoItem(int id, TodoItem todoItem)
         {
-            HttpContext.VerifyUserHasAnyAcceptedScope(scopeRequiredByApi);
-
-            if (id != todoItem.Id)
+            if (id != todoItem.Id  || !_context.TodoItems.Any(x => x.Id == id))
             {
-                return BadRequest();
+                return NotFound();
             }
 
-            _context.Entry(todoItem).State = EntityState.Modified;
 
-            try
-            {
-                await _context.SaveChangesAsync();
-            }
-            catch (DbUpdateConcurrencyException)
+            if ((!IsAppOnlyToken() && _context.TodoItems.Any(x => x.Id == id && x.Owner == HttpContext.User.GetObjectId()))
+                ||
+                IsAppOnlyToken())
             {
-                if (!TodoItemExists(id))
-                {
-                    return NotFound();
-                }
-                else
+                if (_context.TodoItems.Any(x => x.Id == id && x.Owner == HttpContext.User.GetObjectId()))
                 {
-                    throw;
+                    _context.Entry(todoItem).State = EntityState.Modified;
+
+                    try
+                    {
+                        await _context.SaveChangesAsync();
+                    }
+                    catch (DbUpdateConcurrencyException)
+                    {
+                        if (!_context.TodoItems.Any(e => e.Id == id))
+                        {
+                            return NotFound();
+                        }
+                        else
+                        {
+                            throw;
+                        }
+                    }
                 }
             }
 
@@ -91,10 +159,20 @@ public async Task<IActionResult> PutTodoItem(int id, TodoItem todoItem)
         // To protect from overposting attacks, please enable the specific properties you want to bind to, for
         // more details see https://aka.ms/RazorPagesCRUD.
         [HttpPost]
+        [RequiredScopeOrAppPermission(
+            AcceptedScope = new string[] { _todoListReadWrite },
+            AcceptedAppPermission = new string[] { _todoListReadWriteAll }
+        )]
         public async Task<ActionResult<TodoItem>> PostTodoItem(TodoItem todoItem)
         {
-            HttpContext.VerifyUserHasAnyAcceptedScope(scopeRequiredByApi);
-            string owner = User.FindFirst(ClaimTypes.NameIdentifier)?.Value;
+            string owner = HttpContext.User.GetObjectId();
+
+            if (IsAppOnlyToken())
+            {
+                // with such a permission any owner name is accepted
+                owner = todoItem.Owner;
+            }
+
             todoItem.Owner = owner;
             todoItem.Status = false;
 
@@ -106,25 +184,28 @@ public async Task<ActionResult<TodoItem>> PostTodoItem(TodoItem todoItem)
 
         // DELETE: api/TodoItems/5
         [HttpDelete("{id}")]
+        [RequiredScopeOrAppPermission(
+            AcceptedScope = new string[] { _todoListReadWrite },
+            AcceptedAppPermission = new string[] { _todoListReadWriteAll }
+        )]
         public async Task<ActionResult<TodoItem>> DeleteTodoItem(int id)
         {
-            HttpContext.VerifyUserHasAnyAcceptedScope(scopeRequiredByApi);
+            TodoItem todoItem = await _context.TodoItems.FindAsync(id);
 
-            var todoItem = await _context.TodoItems.FindAsync(id);
             if (todoItem == null)
             {
                 return NotFound();
             }
 
-            _context.TodoItems.Remove(todoItem);
-            await _context.SaveChangesAsync();
-
-            return todoItem;
-        }
+            if ((!IsAppOnlyToken() && _context.TodoItems.Any(x => x.Id == id && x.Owner == HttpContext.User.GetObjectId()))
+                ||
+                IsAppOnlyToken())
+            {
+                _context.TodoItems.Remove(todoItem);
+                await _context.SaveChangesAsync();
+            }
 
-        private bool TodoItemExists(int id)
-        {
-            return _context.TodoItems.Any(e => e.Id == id);
+            return NoContent();
         }
     }
 }