From e8ff4273e3efc7e983c4de829e0394cd7e217645 Mon Sep 17 00:00:00 2001 From: Cellivar Date: Sun, 31 May 2020 20:07:50 -0700 Subject: [PATCH] Feature/quality of life improvements (#21) This bundles up a handful of small changes that makes the application easier to build and run. * Warning hunt to clean up some existing warnings that have crept in. * Adds AdminList configuration option, a comma-separated list of emails that are admin accounts * This partially implements #4 but the actual admin-y pages need to be added to complete it. * Adds unit tests for AdminList related functions * Extraneous forward slashes are dropped from the end of URL lookups, making #6 easier to implement * Fix the dockerfile to use the right sdk image for building. * Fix the default okta API urls. --- .editorconfig | 2 +- NetGoLynx.Tests/NetGoLynx.Tests.csproj | 2 + .../Configuration/AccountSettingsTest.cs | 30 +++++ .../Models/NewRedirectModelTests.cs | 10 +- .../Models/Services/AccountServiceTests.cs | 112 ++++++++++++++++++ NetGoLynx/Controllers/AccountController.cs | 8 +- NetGoLynx/Controllers/HomeController.cs | 3 + NetGoLynx/Controllers/RedirectController.cs | 4 +- NetGoLynx/Dockerfile | 2 +- NetGoLynx/Models/AccessType.cs | 6 + .../Models/Configuration/AccountSettings.cs | 37 ++++++ .../Configuration/Authentication/Okta.cs | 18 ++- NetGoLynx/Services/AccountService.cs | 34 +++++- NetGoLynx/Services/RedirectService.cs | 8 +- NetGoLynx/appsettings.json | 5 +- 15 files changed, 256 insertions(+), 25 deletions(-) create mode 100644 NetGoLynx.Tests/UnitTests/Models/Configuration/AccountSettingsTest.cs rename NetGoLynx.Tests/{ => UnitTests}/Models/NewRedirectModelTests.cs (90%) create mode 100644 NetGoLynx.Tests/UnitTests/Models/Services/AccountServiceTests.cs create mode 100644 NetGoLynx/Models/Configuration/AccountSettings.cs diff --git a/.editorconfig b/.editorconfig index 8a23a0f..327bf3a 100644 --- a/.editorconfig +++ b/.editorconfig @@ -37,7 +37,7 @@ indent_size = 2 # Dotnet diagnostic settings [*.{cs,vb}] -dotnet_diagnostic.xUnit2018.severity = suppress # "do not compare an object's exact type to the abstract class" is a valid assert, but very noisy right now +# dotnet_diagnostic.xUnit2018.severity = suppress # "do not compare an object's exact type to the abstract class" is a valid assert, but very noisy right now # Dotnet code style settings: [*.{cs,vb}] diff --git a/NetGoLynx.Tests/NetGoLynx.Tests.csproj b/NetGoLynx.Tests/NetGoLynx.Tests.csproj index c7fb62e..fb08b85 100644 --- a/NetGoLynx.Tests/NetGoLynx.Tests.csproj +++ b/NetGoLynx.Tests/NetGoLynx.Tests.csproj @@ -11,6 +11,8 @@ all runtime; build; native; contentfiles; analyzers; buildtransitive + + diff --git a/NetGoLynx.Tests/UnitTests/Models/Configuration/AccountSettingsTest.cs b/NetGoLynx.Tests/UnitTests/Models/Configuration/AccountSettingsTest.cs new file mode 100644 index 0000000..638e0e4 --- /dev/null +++ b/NetGoLynx.Tests/UnitTests/Models/Configuration/AccountSettingsTest.cs @@ -0,0 +1,30 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; +using NetGoLynx.Models; +using NetGoLynx.Models.Configuration; + +namespace NetGoLynx.Tests.UnitTests.Models.Configuration +{ + [TestClass] + public class AccountSettingsTest + { + [TestMethod] + public void AdminUsernameReturnsTrue() + { + var goodAccount = new Account() + { + Name = "someaccount@example.com" + }; + var badAccount = new Account() + { + Name = "somerando@example.com" + }; + var settings = new AccountSettings + { + AdminList = goodAccount.Name + }; + + Assert.IsTrue(settings.IsAdmin(goodAccount)); + Assert.IsFalse(settings.IsAdmin(badAccount)); + } + } +} diff --git a/NetGoLynx.Tests/Models/NewRedirectModelTests.cs b/NetGoLynx.Tests/UnitTests/Models/NewRedirectModelTests.cs similarity index 90% rename from NetGoLynx.Tests/Models/NewRedirectModelTests.cs rename to NetGoLynx.Tests/UnitTests/Models/NewRedirectModelTests.cs index 8cffdf9..7ad0479 100644 --- a/NetGoLynx.Tests/Models/NewRedirectModelTests.cs +++ b/NetGoLynx.Tests/UnitTests/Models/NewRedirectModelTests.cs @@ -3,7 +3,7 @@ using Microsoft.VisualStudio.TestTools.UnitTesting; using NetGoLynx.Models.RedirectModels; -namespace NetGoLynx.Tests.Models +namespace NetGoLynx.Tests.UnitTests.Models { [TestClass] public class NewRedirectModelTests @@ -27,9 +27,9 @@ public void LinkNameIsValid(string name) { Target = "https://valid.target" }; - var (IsValid, _) = TryValidate(model); + var (isValid, _) = TryValidate(model); - Assert.IsTrue(IsValid); + Assert.IsTrue(isValid); } [DataTestMethod] @@ -43,9 +43,9 @@ public void LinkNameIsNotValid(string name) { Target = "https://valid.target" }; - var (IsValid, _) = TryValidate(model); + var (isValid, _) = TryValidate(model); - Assert.IsFalse(IsValid); + Assert.IsFalse(isValid); } [TestMethod] diff --git a/NetGoLynx.Tests/UnitTests/Models/Services/AccountServiceTests.cs b/NetGoLynx.Tests/UnitTests/Models/Services/AccountServiceTests.cs new file mode 100644 index 0000000..b3dc387 --- /dev/null +++ b/NetGoLynx.Tests/UnitTests/Models/Services/AccountServiceTests.cs @@ -0,0 +1,112 @@ +using System.Collections.Generic; +using System.Security.Claims; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Configuration; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using NetGoLynx.Data; +using NetGoLynx.Models; +using NetGoLynx.Services; + +namespace NetGoLynx.Tests.UnitTests.Models.Services +{ + [TestClass] + public class AccountServiceTests + { + private static readonly DbContextOptions s_dbOps = + new DbContextOptionsBuilder() + .UseInMemoryDatabase(databaseName: "account_service_tests") + .Options; + + private static int s_accountCount; + + private static int AccountCount + { + get + { + s_accountCount++; + return s_accountCount; + } + } + + private static Account GetValidAccount() + { + var count = AccountCount; + return new Account() + { + Access = AccessType.Default, + AccountId = count, + Name = $"test_account_{count}@example.com", + }; + } + + [ClassInitialize] + public static void ClassInitialize(TestContext testContext) + { + testContext.WriteLine("Adding default database entries"); + // Add some reference stuff + using var context = new RedirectContext(s_dbOps); + context.Add(GetValidAccount()); + context.Add(GetValidAccount()); + context.Add(new Account() + { + AccountId = 12345, + Access = AccessType.Default, + Name = "GoodAccount@example.com" + }); + context.SaveChanges(); + } + + [TestMethod] + public void WhitelistedAccountReturnsAsAdminViaAnyMethod() + { + using var context = new RedirectContext(s_dbOps); + var configDict = new Dictionary + { + {"AccountSettings:AdminList", "GoodAccount@example.com,OtherGoodAccount@example.com,moregood@example.com"} + }; + var config = new ConfigurationBuilder() + .AddInMemoryCollection(configDict) + .Build(); + var accountService = new AccountService(context, null, config); + + // Standard get methods + Assert.AreEqual(accountService.Get("GoodAccount@example.com").Result.Access, AccessType.Admin); + Assert.AreEqual(accountService.Get(12345).Result.Access, AccessType.Admin); + + // ClaimsPrincipal overload + var claims = new ClaimsPrincipal(new ClaimsIdentity(new Claim[] + { + new Claim(ClaimTypes.Email, "GoodAccount@example.com") + }, "mock")); + Assert.AreEqual(accountService.Get(claims).Result.Access, AccessType.Admin); + + // Create operation + var newAccount = new Account() + { + Access = AccessType.Default, + Name = "OtherGoodAccount@example.com" + }; + var (account, created) = accountService.Create(newAccount).Result; + Assert.IsTrue(created); + Assert.AreEqual(account.Access, AccessType.Admin); + + Assert.AreEqual(accountService.GetOrCreate("moregood@example.com").Result.Access, AccessType.Admin); + } + + [TestMethod] + public void RegularAccountDoesntReturnAsAdmin() + { + using var context = new RedirectContext(s_dbOps); + var configDict = new Dictionary + { + {"AccountSettings:AdminList", "GoodAccount@example.com"} + }; + var config = new ConfigurationBuilder() + .AddInMemoryCollection(configDict) + .Build(); + var accountService = new AccountService(context, null, config); + + Assert.AreNotEqual(accountService.Get("test_account_1@example.com").Result.Access, AccessType.Admin); + } + } +} diff --git a/NetGoLynx/Controllers/AccountController.cs b/NetGoLynx/Controllers/AccountController.cs index 35b8b27..bbeb349 100644 --- a/NetGoLynx/Controllers/AccountController.cs +++ b/NetGoLynx/Controllers/AccountController.cs @@ -30,7 +30,7 @@ public AccountController(IAccountService service, IAuthenticationSchemeProvider /// /// The login selection view. [HttpGet("Login")] - public async Task LoginAsync() + public IActionResult LoginAsync() { return View(new LoginModel(_availableSchemes)); } @@ -40,7 +40,7 @@ public async Task LoginAsync() /// /// A redirect to the list view page. [HttpGet("Google")] - public async Task LoginGoogleAsync() + public IActionResult LoginGoogleAsync() { return Challenge( new AuthenticationProperties @@ -55,7 +55,7 @@ public async Task LoginGoogleAsync() /// /// A redirect to the list view page. [HttpGet("GitHub")] - public async Task LoginGitHubAsync() + public IActionResult LoginGitHubAsync() { return Challenge( new AuthenticationProperties @@ -70,7 +70,7 @@ public async Task LoginGitHubAsync() /// /// A redirect to the list view page. [HttpGet("Okta")] - public async Task LoginOktaAsync() + public IActionResult LoginOktaAsync() { return Challenge( new AuthenticationProperties diff --git a/NetGoLynx/Controllers/HomeController.cs b/NetGoLynx/Controllers/HomeController.cs index 101580a..8d6694f 100644 --- a/NetGoLynx/Controllers/HomeController.cs +++ b/NetGoLynx/Controllers/HomeController.cs @@ -41,6 +41,9 @@ public IActionResult Index() [HttpGet("{*name}", Order = int.MaxValue)] public async Task ResolveAsync(string name) { + // Trim extraneous forward slashes only off the end + name = name.TrimEnd('/'); + var target = await _redirectService.GetRedirectTargetAsync(name); if (!string.IsNullOrEmpty(target)) diff --git a/NetGoLynx/Controllers/RedirectController.cs b/NetGoLynx/Controllers/RedirectController.cs index f2c31ed..06b06a8 100644 --- a/NetGoLynx/Controllers/RedirectController.cs +++ b/NetGoLynx/Controllers/RedirectController.cs @@ -26,7 +26,7 @@ public RedirectController(IRedirectService redirectService) [HttpGet("notfound")] [AllowAnonymous] - public IActionResult NotFound() + public new IActionResult NotFound() { return View(); } @@ -49,7 +49,7 @@ public async Task ListAsync(int highlightId) /// An optional suggested link name to add. /// The add view [HttpGet("add")] - public async Task AddAsync(string suggestedLinkName = "") + public IActionResult Add(string suggestedLinkName = "") { return View("Add", new RedirectMetadata(suggestedLinkName)); } diff --git a/NetGoLynx/Dockerfile b/NetGoLynx/Dockerfile index 0f951ed..d2e44e0 100644 --- a/NetGoLynx/Dockerfile +++ b/NetGoLynx/Dockerfile @@ -3,7 +3,7 @@ WORKDIR /app EXPOSE 80 EXPOSE 443 -FROM mcr.microsoft.com/dotnet/core/aspnet:3.1-alpine AS build +FROM mcr.microsoft.com/dotnet/core/sdk:3.1-alpine AS build WORKDIR /src COPY ["NetGoLynx/NetGoLynx.csproj", "NetGoLynx/"] RUN dotnet restore "NetGoLynx/NetGoLynx.csproj" diff --git a/NetGoLynx/Models/AccessType.cs b/NetGoLynx/Models/AccessType.cs index 8314cbd..0d595a4 100644 --- a/NetGoLynx/Models/AccessType.cs +++ b/NetGoLynx/Models/AccessType.cs @@ -8,8 +8,14 @@ namespace NetGoLynx.Models [Flags] public enum AccessType { + /// + /// Standard user access level + /// Default = 0, + /// + /// Complete control access level + /// Admin = 1, } } diff --git a/NetGoLynx/Models/Configuration/AccountSettings.cs b/NetGoLynx/Models/Configuration/AccountSettings.cs new file mode 100644 index 0000000..a5eb440 --- /dev/null +++ b/NetGoLynx/Models/Configuration/AccountSettings.cs @@ -0,0 +1,37 @@ +using System.Collections.Generic; + +namespace NetGoLynx.Models.Configuration +{ + /// + /// Model for Account-related settings. + /// + public class AccountSettings + { + private HashSet _adminList; + + private string _whitelist; + + /// + /// Gets or sets the comma-separated list of accounts that have admin. + /// + public string AdminList + { + get => _whitelist; + set + { + _whitelist = value; + _adminList = new HashSet(_whitelist.Split(',')); + } + } + + /// + /// Determine if an account is an admin. + /// + /// The account to check + /// True if the username exactly matches an entry in the whitelist. + public bool IsAdmin(Account account) + { + return account != null && _adminList.Contains(account.Name); + } + } +} diff --git a/NetGoLynx/Models/Configuration/Authentication/Okta.cs b/NetGoLynx/Models/Configuration/Authentication/Okta.cs index d6b4511..d6e6c36 100644 --- a/NetGoLynx/Models/Configuration/Authentication/Okta.cs +++ b/NetGoLynx/Models/Configuration/Authentication/Okta.cs @@ -32,17 +32,29 @@ public class Okta /// /// Gets or sets the OAuth auth endpoint URL /// - public string AuthorizationEndpoint => $"{OktaDomain}/oauth2/default/v1/authorize"; + /// + /// No /default/ between oauth2 and v1 here because that only applies to + /// custom auth servers: https://developer.okta.com/docs/reference/api/oidc/ + /// + public string AuthorizationEndpoint => $"{OktaDomain}/oauth2/v1/authorize"; /// /// Gets or sets the OAuth token endpoint URL /// - public string TokenEndpoint => $"{OktaDomain}/oauth2/default/v1/token"; + /// + /// No /default/ between oauth2 and v1 here because that only applies to + /// custom auth servers: https://developer.okta.com/docs/reference/api/oidc/ + /// + public string TokenEndpoint => $"{OktaDomain}/oauth2/v1/token"; /// /// Gets or sets the user information URL /// - public string UserInformationEndpoint => $"{OktaDomain}/oauth2/default/v1/userinfo"; + /// + /// No /default/ between oauth2 and v1 here because that only applies to + /// custom auth servers: https://developer.okta.com/docs/reference/api/oidc/ + /// + public string UserInformationEndpoint => $"{OktaDomain}/oauth2/v1/userinfo"; /// /// Gets or sets the okta domain. diff --git a/NetGoLynx/Services/AccountService.cs b/NetGoLynx/Services/AccountService.cs index ce44414..84dd647 100644 --- a/NetGoLynx/Services/AccountService.cs +++ b/NetGoLynx/Services/AccountService.cs @@ -2,24 +2,34 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Configuration; using NetGoLynx.Data; using NetGoLynx.Models; +using NetGoLynx.Models.Configuration; namespace NetGoLynx.Services { public class AccountService : IAccountService { - private RedirectContext _context; + private readonly RedirectContext _context; private readonly IHttpContextAccessor _contextAccessor; + private readonly AccountSettings _accountSettings; public AccountService( RedirectContext context, - IHttpContextAccessor contextAccessor) + IHttpContextAccessor contextAccessor, + IConfiguration configuration) { _context = context; _contextAccessor = contextAccessor; + _accountSettings = configuration.GetSection("AccountSettings").Get(); } + /// + /// Get an account by username + /// + /// The name to get the account by. + /// The account with that username, or null. public async Task Get(string name) { if (string.IsNullOrEmpty(name)) @@ -27,7 +37,10 @@ public async Task Get(string name) return null; } - return await _context.Accounts.FirstOrDefaultAsync(a => a.Name == name); + var account = await _context.Accounts.FirstOrDefaultAsync(a => a.Name == name); + + SetAdminFlag(ref account); + return account; } public async Task Get(int id) @@ -37,7 +50,10 @@ public async Task Get(int id) return null; } - return await _context.Accounts.FindAsync(id); + var account = await _context.Accounts.FindAsync(id); + + SetAdminFlag(ref account); + return account; } public async Task Get(ClaimsPrincipal claimsPrincipal) @@ -70,6 +86,8 @@ public async Task GetFromCurrentUser() await _context.Accounts.AddAsync(account); await _context.SaveChangesAsync(); + SetAdminFlag(ref account); + return (account, true); } @@ -88,5 +106,13 @@ private string GetEmailFromClaimsPrincipal(ClaimsPrincipal claimsPrincipal) { return claimsPrincipal?.FindFirstValue(ClaimTypes.Email); } + + private void SetAdminFlag(ref Account account) + { + if (account != null) + { + account.Access = _accountSettings.IsAdmin(account) ? AccessType.Admin : account.Access; + } + } } } diff --git a/NetGoLynx/Services/RedirectService.cs b/NetGoLynx/Services/RedirectService.cs index a0656f3..43a574c 100644 --- a/NetGoLynx/Services/RedirectService.cs +++ b/NetGoLynx/Services/RedirectService.cs @@ -10,7 +10,7 @@ namespace NetGoLynx.Services { public class RedirectService : IRedirectService { - private RedirectContext _context; + private readonly RedirectContext _context; private readonly IAccountService _accountService; private readonly Lazy _lazyUser; @@ -54,7 +54,8 @@ public async Task> GetAsync() } else { - return _context.Redirects.Where(r => r.AccountId == User.AccountId); + return await _context.Redirects.Where(r => r.AccountId == User.AccountId) + .ToListAsync(); } } @@ -127,8 +128,7 @@ public async Task ExistsAsync(string name) return (false, null); } - var redirect = await GetAsync(id) as Redirect; - if (redirect == null || !User.MayView(redirect)) + if (!(await GetAsync(id) is Redirect redirect) || !User.MayView(redirect)) { return (false, null); } diff --git a/NetGoLynx/appsettings.json b/NetGoLynx/appsettings.json index ac42e34..0737146 100644 --- a/NetGoLynx/appsettings.json +++ b/NetGoLynx/appsettings.json @@ -1,6 +1,6 @@ { "ConnectionStrings": { - "DefaultConnection": "Data Source=redirects.db" + "DefaultConnection": "Data Source=data_path/redirects.db" }, "Logging": { "LogLevel": { @@ -8,6 +8,9 @@ } }, "AllowedHosts": "*", + "AccountSettings": { + "AdminList": "someaccount@example.com,anotheraccount@example.com" + }, "Authentication": { "GitHub": { "ClientId": "This is very not real",