From 7e60328cff8539e33376a9d0a972f05da8fb90f0 Mon Sep 17 00:00:00 2001 From: Dennis Reimann Date: Thu, 17 Sep 2020 11:37:49 +0200 Subject: [PATCH] Greenfield: Rename API key redirect params; switch to POST body (#1898) * Rename user param to userId in API key redirect This way it is clearer what to expect and it also make the parameteer easier to consume. * Post redirect: Allow form url and prettify page - Form URL as alternative to controller/action for external URLs - Making it look nice and add explanation for non-JS case * APIKeys: Minor view updates fix * APIKeys: Use POST redirect for confirmation fix * UI: Minor update to confirm view Tidies it up and adapts to the newly added ConfirmAPIKeys view. * APIKeys: Update delete view Structures the information in title and description better. * APIKeys: Distinguish authorize and confirm (reuse) * Upgrade ChromeDriver * Test fixes * Clean up PostRedirect view By adding missing forgery token * Re-add tests for callback post values * Rename key param to apiKey in API key redirect * Update BTCPayServer/wwwroot/swagger/v1/swagger.template.authorization.json Co-authored-by: Andrew Camilleri * Use DEBUG conditional for postredirect-callback-test route * Remove unnecessary ChromeDriver references * Add debug flag * Remove debug flags Co-authored-by: Andrew Camilleri --- BTCPayServer.Tests/ApiKeysTests.cs | 50 ++++++------ BTCPayServer.Tests/BTCPayServer.Tests.csproj | 2 +- BTCPayServer.Tests/SeleniumTester.cs | 5 +- BTCPayServer/BTCPayServer.csproj | 2 +- BTCPayServer/Controllers/HomeController.cs | 15 +++- .../Controllers/ManageController.APIKeys.cs | 77 +++++++++++-------- BTCPayServer/Models/PostRedictViewModel.cs | 2 + BTCPayServer/Views/Manage/APIKeys.cshtml | 2 +- .../Views/Manage/AuthorizeAPIKey.cshtml | 16 ++-- .../Views/Manage/ConfirmAPIKey.cshtml | 41 ++++++++++ BTCPayServer/Views/Shared/Confirm.cshtml | 40 ++++------ BTCPayServer/Views/Shared/PostRedirect.cshtml | 38 +++++---- .../v1/swagger.template.authorization.json | 2 +- 13 files changed, 181 insertions(+), 111 deletions(-) create mode 100644 BTCPayServer/Views/Manage/ConfirmAPIKey.cshtml diff --git a/BTCPayServer.Tests/ApiKeysTests.cs b/BTCPayServer.Tests/ApiKeysTests.cs index 1a07cd740..a286f6243 100644 --- a/BTCPayServer.Tests/ApiKeysTests.cs +++ b/BTCPayServer.Tests/ApiKeysTests.cs @@ -1,5 +1,4 @@ using System; -using System.Collections.Generic; using System.Linq; using System.Net.Http; using System.Net.Http.Headers; @@ -11,6 +10,7 @@ using BTCPayServer.Security.GreenField; using BTCPayServer.Tests.Logging; using BTCPayServer.Views.Manage; using Newtonsoft.Json; +using Newtonsoft.Json.Linq; using OpenQA.Selenium; using Xunit; using Xunit.Abstractions; @@ -109,7 +109,6 @@ namespace BTCPayServer.Tests tester.PayTester.HttpClient); }); - //let's test the authorized screen now //options for authorize are: //applicationName @@ -120,28 +119,27 @@ namespace BTCPayServer.Tests //redirect //appidentifier var appidentifier = "testapp"; + var callbackUrl = tester.PayTester.ServerUri + "postredirect-callback-test"; var authUrl = BTCPayServerClient.GenerateAuthorizeUri(tester.PayTester.ServerUri, - new[] { Policies.CanModifyStoreSettings, Policies.CanModifyServerSettings }, applicationDetails: (appidentifier, new Uri("https://local.local/callback"))).ToString(); + new[] { Policies.CanModifyStoreSettings, Policies.CanModifyServerSettings }, applicationDetails: (appidentifier, new Uri(callbackUrl))).ToString(); s.Driver.Navigate().GoToUrl(authUrl); - Assert.True(s.Driver.PageSource.Contains(appidentifier)); + Assert.Contains(appidentifier, s.Driver.PageSource); Assert.Equal("hidden", s.Driver.FindElement(By.Id("btcpay.store.canmodifystoresettings")).GetAttribute("type").ToLowerInvariant()); Assert.Equal("true", s.Driver.FindElement(By.Id("btcpay.store.canmodifystoresettings")).GetAttribute("value").ToLowerInvariant()); Assert.Equal("hidden", s.Driver.FindElement(By.Id("btcpay.server.canmodifyserversettings")).GetAttribute("type").ToLowerInvariant()); Assert.Equal("true", s.Driver.FindElement(By.Id("btcpay.server.canmodifyserversettings")).GetAttribute("value").ToLowerInvariant()); Assert.DoesNotContain("change-store-mode", s.Driver.PageSource); s.Driver.FindElement(By.Id("consent-yes")).Click(); - var url = s.Driver.Url; - Assert.StartsWith("https://local.local/callback", url); - IEnumerable> results = url.Split("?").Last().Split("&") - .Select(s1 => new KeyValuePair(s1.Split("=")[0], s1.Split("=")[1])); + Assert.Equal(callbackUrl, s.Driver.Url); var apiKeyRepo = s.Server.PayTester.GetService(); + var accessToken = GetAccessTokenFromCallbackResult(s.Driver); - await TestApiAgainstAccessToken(results.Single(pair => pair.Key == "key").Value, tester, user, - (await apiKeyRepo.GetKey(results.Single(pair => pair.Key == "key").Value)).GetBlob().Permissions); + await TestApiAgainstAccessToken(accessToken, tester, user, + (await apiKeyRepo.GetKey(accessToken)).GetBlob().Permissions); authUrl = BTCPayServerClient.GenerateAuthorizeUri(tester.PayTester.ServerUri, - new[] { Policies.CanModifyStoreSettings, Policies.CanModifyServerSettings }, false, true, applicationDetails: (null, new Uri("https://local.local/callback"))).ToString(); + new[] { Policies.CanModifyStoreSettings, Policies.CanModifyServerSettings }, false, true, applicationDetails: (null, new Uri(callbackUrl))).ToString(); s.Driver.Navigate().GoToUrl(authUrl); Assert.DoesNotContain("kukksappname", s.Driver.PageSource); @@ -154,34 +152,27 @@ namespace BTCPayServer.Tests s.SetCheckbox(s, "btcpay.server.canmodifyserversettings", false); Assert.Contains("change-store-mode", s.Driver.PageSource); s.Driver.FindElement(By.Id("consent-yes")).Click(); - url = s.Driver.Url; - Assert.StartsWith("https://local.local/callback", url); - results = url.Split("?").Last().Split("&") - .Select(s1 => new KeyValuePair(s1.Split("=")[0], s1.Split("=")[1])); + Assert.Equal(callbackUrl, s.Driver.Url); + + accessToken = GetAccessTokenFromCallbackResult(s.Driver); + await TestApiAgainstAccessToken(accessToken, tester, user, + (await apiKeyRepo.GetKey(accessToken)).GetBlob().Permissions); - await TestApiAgainstAccessToken(results.Single(pair => pair.Key == "key").Value, tester, user, - (await apiKeyRepo.GetKey(results.Single(pair => pair.Key == "key").Value)).GetBlob().Permissions); - - //let's test the app identifier system authUrl = BTCPayServerClient.GenerateAuthorizeUri(tester.PayTester.ServerUri, - new[] { Policies.CanModifyStoreSettings, Policies.CanModifyServerSettings }, false, true, (appidentifier, new Uri("https://local.local/callback"))).ToString(); - + new[] { Policies.CanModifyStoreSettings, Policies.CanModifyServerSettings }, false, true, (appidentifier, new Uri(callbackUrl))).ToString(); //if it's the same, go to the confirm page s.Driver.Navigate().GoToUrl(authUrl); s.Driver.FindElement(By.Id("continue")).Click(); - url = s.Driver.Url; - Assert.StartsWith("https://local.local/callback", url); + Assert.Equal(callbackUrl, s.Driver.Url); //same app but different redirect = nono authUrl = BTCPayServerClient.GenerateAuthorizeUri(tester.PayTester.ServerUri, new[] { Policies.CanModifyStoreSettings, Policies.CanModifyServerSettings }, false, true, (appidentifier, new Uri("https://international.local/callback"))).ToString(); s.Driver.Navigate().GoToUrl(authUrl); - url = s.Driver.Url; - Assert.False(url.StartsWith("https://international.com/callback")); - + Assert.False(s.Driver.Url.StartsWith("https://international.com/callback")); } } @@ -339,5 +330,12 @@ namespace BTCPayServer.Tests return JsonConvert.DeserializeObject(rawJson); } + + private string GetAccessTokenFromCallbackResult(IWebDriver driver) + { + var source = driver.FindElement(By.TagName("body")).Text; + var json = JObject.Parse(source); + return json.GetValue("apiKey")!.Value(); + } } } diff --git a/BTCPayServer.Tests/BTCPayServer.Tests.csproj b/BTCPayServer.Tests/BTCPayServer.Tests.csproj index f7d08c634..d6ab7faa4 100644 --- a/BTCPayServer.Tests/BTCPayServer.Tests.csproj +++ b/BTCPayServer.Tests/BTCPayServer.Tests.csproj @@ -22,7 +22,7 @@ - + all diff --git a/BTCPayServer.Tests/SeleniumTester.cs b/BTCPayServer.Tests/SeleniumTester.cs index f4d02f370..4216f9841 100644 --- a/BTCPayServer.Tests/SeleniumTester.cs +++ b/BTCPayServer.Tests/SeleniumTester.cs @@ -308,8 +308,9 @@ namespace BTCPayServer.Tests currencyEl.Clear(); currencyEl.SendKeys(currency); Driver.FindElement(By.Id("BuyerEmail")).SendKeys(refundEmail); - Driver.FindElement(By.Name("StoreId")).SendKeys(storeName + Keys.Enter); - Driver.FindElement(By.Id("Create")).ForceClick(); + Driver.FindElement(By.Name("StoreId")).SendKeys(storeName); + Driver.FindElement(By.Id("Create")).Click(); + Assert.True(Driver.PageSource.Contains("just created!"), "Unable to create Invoice"); var statusElement = Driver.FindElement(By.ClassName("alert-success")); var id = statusElement.Text.Split(" ")[1]; diff --git a/BTCPayServer/BTCPayServer.csproj b/BTCPayServer/BTCPayServer.csproj index c18c150e9..49a852827 100644 --- a/BTCPayServer/BTCPayServer.csproj +++ b/BTCPayServer/BTCPayServer.csproj @@ -43,7 +43,7 @@ - + diff --git a/BTCPayServer/Controllers/HomeController.cs b/BTCPayServer/Controllers/HomeController.cs index 1b79ef93b..ead82b648 100644 --- a/BTCPayServer/Controllers/HomeController.cs +++ b/BTCPayServer/Controllers/HomeController.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Diagnostics; using System.IO; using System.Linq; @@ -12,6 +13,7 @@ using BTCPayServer.Security; using BTCPayServer.Services.Apps; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.FileProviders; @@ -139,7 +141,6 @@ namespace BTCPayServer.Controllers return View(); } - [HttpPost] [Route("translate")] public async Task BitpayTranslator(BitpayTranslatorViewModel vm) @@ -199,6 +200,18 @@ namespace BTCPayServer.Controllers return View("RecoverySeedBackup", vm); } + [HttpPost] + [Route("postredirect-callback-test")] + public ActionResult PostRedirectCallbackTestpage(IFormCollection data) + { + var list = data.Keys.Aggregate(new Dictionary(), (res, key) => + { + res.Add(key, data[key]); + return res; + }); + return Json(list); + } + public IActionResult Error() { return View(new ErrorViewModel { RequestId = Activity.Current?.Id ?? HttpContext.TraceIdentifier }); diff --git a/BTCPayServer/Controllers/ManageController.APIKeys.cs b/BTCPayServer/Controllers/ManageController.APIKeys.cs index d08f220c0..247d4202c 100644 --- a/BTCPayServer/Controllers/ManageController.APIKeys.cs +++ b/BTCPayServer/Controllers/ManageController.APIKeys.cs @@ -7,9 +7,7 @@ using BTCPayServer.Client; using BTCPayServer.Data; using BTCPayServer.Models; using BTCPayServer.Security.GreenField; -using ExchangeSharp; using Microsoft.AspNetCore.Authorization; -using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using NBitcoin; using NBitcoin.DataEncoders; @@ -40,10 +38,11 @@ namespace BTCPayServer.Controllers } return View("Confirm", new ConfirmModel() { - Title = "Delete API Key " + (string.IsNullOrEmpty(key.Label) ? string.Empty : key.Label) + "(" + key.Id + ")", - Description = "Any application using this api key will immediately lose access", + Title = $"Delete API Key {(string.IsNullOrEmpty(key.Label) ? string.Empty : key.Label)}", + DescriptionHtml = true, + Description = $"Any application using this API key will immediately lose access: {key.Id}", Action = "Delete", - ActionUrl = this.Url.ActionLink(nameof(RemoveAPIKeyPost), values: new { id = id }) + ActionUrl = Url.ActionLink(nameof(RemoveAPIKeyPost), values: new { id }) }); } @@ -81,7 +80,7 @@ namespace BTCPayServer.Controllers } [HttpGet("~/api-keys/authorize")] - public async Task AuthorizeAPIKey( string[] permissions, string applicationName = null, Uri redirect = null, + public async Task AuthorizeAPIKey(string[] permissions, string applicationName = null, Uri redirect = null, bool strict = true, bool selectiveStores = false, string applicationIdentifier = null) { if (!_btcPayServerEnvironment.IsSecure) @@ -157,16 +156,17 @@ namespace BTCPayServer.Controllers } //we have a key that is sufficient, redirect to a page to confirm that it's ok to provide this key to the app. - return View("Confirm", - new ConfirmModel() + return View("ConfirmAPIKey", + new AuthorizeApiKeysViewModel() { - Title = - $"Are you sure about exposing your API Key to {applicationName ?? applicationIdentifier}?", - Description = - $"You've previously generated this API Key ({key.Id}) specifically for {applicationName ?? applicationIdentifier} with the url {redirect}. ", - ActionUrl = GetRedirectToApplicationUrl(redirect, key), - ButtonClass = "btn-secondary", - Action = "Confirm" + ApiKey = key.Id, + RedirectUrl = redirect, + Label = applicationName, + ApplicationName = applicationName, + SelectiveStores = selectiveStores, + Strict = strict, + Permissions = string.Join(';', permissions), + ApplicationIdentifier = applicationIdentifier }); } } @@ -255,16 +255,37 @@ namespace BTCPayServer.Controllers return View(viewModel); } - switch (viewModel.Command.ToLowerInvariant()) + var command = viewModel.Command.ToLowerInvariant(); + switch (command) { - case "no": + case "cancel": return RedirectToAction("APIKeys"); - case "yes": - var key = await CreateKey(viewModel, (viewModel.ApplicationIdentifier, viewModel.RedirectUrl?.Authority)); + + case "authorize": + case "confirm": + var key = command == "authorize" + ? await CreateKey(viewModel, (viewModel.ApplicationIdentifier, viewModel.RedirectUrl?.Authority)) + : await _apiKeyRepository.GetKey(viewModel.ApiKey); if (viewModel.RedirectUrl != null) { - return Redirect(GetRedirectToApplicationUrl(viewModel.RedirectUrl, key)); + var permissions = key.GetBlob().Permissions; + var redirectVm = new PostRedirectViewModel() + { + FormUrl = viewModel.RedirectUrl.ToString(), + Parameters = + { + new KeyValuePair("apiKey", key.Id), + new KeyValuePair("userId", key.UserId) + } + }; + foreach (var permission in permissions) + { + redirectVm.Parameters.Add( + new KeyValuePair("permissions[]", permission)); + } + + return View("PostRedirect", redirectVm); } TempData.SetStatusMessageModel(new StatusMessageModel() @@ -272,21 +293,14 @@ namespace BTCPayServer.Controllers Severity = StatusMessageModel.StatusSeverity.Success, Html = $"API key generated! {key.Id}" }); + return RedirectToAction("APIKeys", new { key = key.Id }); + default: return View(viewModel); } } - private string GetRedirectToApplicationUrl(Uri redirect, APIKeyData key) - { - var uri = new UriBuilder(redirect); - var permissions = key.GetBlob().Permissions; - BTCPayServerClient.AppendPayloadToQuery(uri, - new Dictionary() {{"key", key.Id}, {"permissions", permissions}, {"user", key.UserId}}); - return uri.Uri.AbsoluteUri; - } - [HttpPost] public async Task AddApiKey(AddApiKeyViewModel viewModel) { @@ -313,6 +327,7 @@ namespace BTCPayServer.Controllers }); return RedirectToAction("APIKeys"); } + private IActionResult HandleCommands(AddApiKeyViewModel viewModel) { if (string.IsNullOrEmpty(viewModel.Command)) @@ -333,7 +348,6 @@ namespace BTCPayServer.Controllers switch (command) { case "change-store-mode": - permissionValueItem.StoreMode = permissionValueItem.StoreMode == AddApiKeyViewModel.ApiKeyStoreMode.Specific ? AddApiKeyViewModel.ApiKeyStoreMode.AllStores : AddApiKeyViewModel.ApiKeyStoreMode.Specific; @@ -344,6 +358,7 @@ namespace BTCPayServer.Controllers permissionValueItem.SpecificStores.Add(null); } return View(viewModel); + case "add-store": permissionValueItem.SpecificStores.Add(null); return View(viewModel); @@ -501,9 +516,9 @@ namespace BTCPayServer.Controllers public bool Strict { get; set; } public bool SelectiveStores { get; set; } public string Permissions { get; set; } + public string ApiKey { get; set; } } - public class ApiKeysViewModel { public List ApiKeyDatas { get; set; } diff --git a/BTCPayServer/Models/PostRedictViewModel.cs b/BTCPayServer/Models/PostRedictViewModel.cs index 2580d9b28..dccf065b9 100644 --- a/BTCPayServer/Models/PostRedictViewModel.cs +++ b/BTCPayServer/Models/PostRedictViewModel.cs @@ -6,6 +6,8 @@ namespace BTCPayServer.Models { public string AspAction { get; set; } public string AspController { get; set; } + public string FormUrl { get; set; } + public List> Parameters { get; set; } = new List>(); } } diff --git a/BTCPayServer/Views/Manage/APIKeys.cshtml b/BTCPayServer/Views/Manage/APIKeys.cshtml index aad4ae50d..de40d4c50 100644 --- a/BTCPayServer/Views/Manage/APIKeys.cshtml +++ b/BTCPayServer/Views/Manage/APIKeys.cshtml @@ -34,7 +34,7 @@
    @foreach (var permission in Permission.ToPermissions(permissions).Select(c => c.ToString()).Distinct().ToArray()) { -
  • @permission
  • +
  • @permission
  • }
} diff --git a/BTCPayServer/Views/Manage/AuthorizeAPIKey.cshtml b/BTCPayServer/Views/Manage/AuthorizeAPIKey.cshtml index 5d5b1e6d9..50248070d 100644 --- a/BTCPayServer/Views/Manage/AuthorizeAPIKey.cshtml +++ b/BTCPayServer/Views/Manage/AuthorizeAPIKey.cshtml @@ -18,16 +18,15 @@
-
+

Authorization Request

-
-

@(Model.ApplicationName ?? "An application") is requesting access to your account.

+

@(Model.ApplicationName ?? "An application") is requesting access to your account.

@if (Model.RedirectUrl != null) {

- If authorized, the generated API key will be provided to @Model.RedirectUrl.AbsoluteUri + If authorized, the generated API key will be provided to @Model.RedirectUrl.AbsoluteUri

}
@@ -174,13 +173,10 @@ }
-
+
-
- -
- - + +
diff --git a/BTCPayServer/Views/Manage/ConfirmAPIKey.cshtml b/BTCPayServer/Views/Manage/ConfirmAPIKey.cshtml new file mode 100644 index 000000000..f5fa5ca7b --- /dev/null +++ b/BTCPayServer/Views/Manage/ConfirmAPIKey.cshtml @@ -0,0 +1,41 @@ +@model BTCPayServer.Controllers.ManageController.AuthorizeApiKeysViewModel + +@{ + var displayName = Model.ApplicationName ?? Model.ApplicationIdentifier; + ViewData["Title"] = $"Are you sure about exposing your API Key to {displayName}?"; + Layout = null; +} + + + + + + + + + + diff --git a/BTCPayServer/Views/Shared/Confirm.cshtml b/BTCPayServer/Views/Shared/Confirm.cshtml index 15f28f564..23aed1ae7 100644 --- a/BTCPayServer/Views/Shared/Confirm.cshtml +++ b/BTCPayServer/Views/Shared/Confirm.cshtml @@ -16,33 +16,25 @@ -
diff --git a/BTCPayServer/Views/Shared/PostRedirect.cshtml b/BTCPayServer/Views/Shared/PostRedirect.cshtml index b27abb221..30e33c1a4 100644 --- a/BTCPayServer/Views/Shared/PostRedirect.cshtml +++ b/BTCPayServer/Views/Shared/PostRedirect.cshtml @@ -8,22 +8,34 @@ { routeParams["walletId"] = routeData.Values["walletId"]?.ToString(); } + var action = Model.FormUrl ?? Url.Action(Model.AspAction, Model.AspController, routeParams); } - - + + + + Post Redirect + -
- @foreach(var o in Model.Parameters) { - - } -
+ diff --git a/BTCPayServer/wwwroot/swagger/v1/swagger.template.authorization.json b/BTCPayServer/wwwroot/swagger/v1/swagger.template.authorization.json index d8712e39b..c072abce8 100644 --- a/BTCPayServer/wwwroot/swagger/v1/swagger.template.authorization.json +++ b/BTCPayServer/wwwroot/swagger/v1/swagger.template.authorization.json @@ -86,7 +86,7 @@ } }, "307": { - "description": "Redirects to the specified url in `redirect` with query string values for `key` (the api key created or matched), `permissions` (the permissions the user consented to), and `user` (the id of the user that consented) upon consent" + "description": "Makes browser do an HTTP POST request to the specified url in `redirect` with a JSON body consisting of `apiKey` (the api key created or matched), `permissions` (the permissions the user consented to), and `userId` (the id of the user that consented) upon consent" } }, "security": []