Skip to content

Commit

Permalink
Greenfield: Rename API key redirect params; switch to POST body (#1898)
Browse files Browse the repository at this point in the history 
* 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 <evilkukka@gmail.com>

* Use DEBUG conditional for postredirect-callback-test route

* Remove unnecessary ChromeDriver references

* Add debug flag

* Remove debug flags

Co-authored-by: Andrew Camilleri <evilkukka@gmail.com>
  • Loading branch information
@dennisreimann @Kukks
dennisreimann and Kukks committed Sep 17, 2020
1 parent 8ba8520 commit 7e60328
Show file tree
Hide file tree
Showing 13 changed files with 181 additions and 111 deletions.
50 changes: 24 additions & 26 deletions BTCPayServer.Tests/ApiKeysTests.cs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Net.Http;
using System.Net.Http.Headers;
Expand All @@ -11,6 +10,7 @@
using BTCPayServer.Tests.Logging;
using BTCPayServer.Views.Manage;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;
using OpenQA.Selenium;
using Xunit;
using Xunit.Abstractions;
Expand Down Expand Up @@ -109,7 +109,6 @@ public async Task CanCreateApiKeys()
tester.PayTester.HttpClient);
});


//let's test the authorized screen now
//options for authorize are:
//applicationName
Expand All @@ -120,28 +119,27 @@ public async Task CanCreateApiKeys()
//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<KeyValuePair<string, string>> results = url.Split("?").Last().Split("&")
.Select(s1 => new KeyValuePair<string, string>(s1.Split("=")[0], s1.Split("=")[1]));
Assert.Equal(callbackUrl, s.Driver.Url);

var apiKeyRepo = s.Server.PayTester.GetService<APIKeyRepository>();
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);
Expand All @@ -154,34 +152,27 @@ public async Task CanCreateApiKeys()
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<string, string>(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"));
}
}

Expand Down Expand Up @@ -339,5 +330,12 @@ public async Task<T> TestApiAgainstAccessToken<T>(string apikey, string url, Htt

return JsonConvert.DeserializeObject<T>(rawJson);
}

private string GetAccessTokenFromCallbackResult(IWebDriver driver)
{
var source = driver.FindElement(By.TagName("body")).Text;
var json = JObject.Parse(source);
return json.GetValue("apiKey")!.Value<string>();
}
}
}
2 changes: 1 addition & 1 deletion BTCPayServer.Tests/BTCPayServer.Tests.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.6.1" />
<PackageReference Include="Newtonsoft.Json.Schema" Version="3.0.13" />
<PackageReference Include="Selenium.WebDriver" Version="3.141.0" />
<PackageReference Include="Selenium.WebDriver.ChromeDriver" Version="83.0.4103.3900" />
<PackageReference Include="Selenium.WebDriver.ChromeDriver" Version="85.0.4183.8700" />
<PackageReference Include="xunit" Version="2.4.1" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.4.2">
<PrivateAssets>all</PrivateAssets>
Expand Down
5 changes: 3 additions & 2 deletions BTCPayServer.Tests/SeleniumTester.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,9 @@ public string CreateInvoice(string storeName, decimal amount = 100, string curre
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];
Expand Down
2 changes: 1 addition & 1 deletion BTCPayServer/BTCPayServer.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
<Content Remove="Views\Shared\Ethereum\**\*" />
<Content Remove="Views\Shared\Monero\**\*" />
</ItemGroup>

<ItemGroup>
<PackageReference Include="BTCPayServer.Hwi" Version="1.1.3" />
<PackageReference Include="BTCPayServer.Lightning.All" Version="1.2.4" />
Expand Down
15 changes: 14 additions & 1 deletion BTCPayServer/Controllers/HomeController.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
Expand All @@ -12,6 +13,7 @@
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;
Expand Down Expand Up @@ -139,7 +141,6 @@ public IActionResult SwaggerDocs()
return View();
}


[HttpPost]
[Route("translate")]
public async Task<IActionResult> BitpayTranslator(BitpayTranslatorViewModel vm)
Expand Down Expand Up @@ -199,6 +200,18 @@ public IActionResult RecoverySeedBackup(RecoverySeedBackupViewModel vm)
return View("RecoverySeedBackup", vm);
}

[HttpPost]
[Route("postredirect-callback-test")]
public ActionResult PostRedirectCallbackTestpage(IFormCollection data)
{
var list = data.Keys.Aggregate(new Dictionary<string, string>(), (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 });
Expand Down
77 changes: 46 additions & 31 deletions BTCPayServer/Controllers/ManageController.APIKeys.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@
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;
Expand Down Expand Up @@ -40,10 +38,11 @@ public async Task<IActionResult> RemoveAPIKey(string id)
}
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: <code>{key.Id}</code>",
Action = "Delete",
ActionUrl = this.Url.ActionLink(nameof(RemoveAPIKeyPost), values: new { id = id })
ActionUrl = Url.ActionLink(nameof(RemoveAPIKeyPost), values: new { id })
});
}

Expand Down Expand Up @@ -81,7 +80,7 @@ public async Task<IActionResult> AddApiKey()
}

[HttpGet("~/api-keys/authorize")]
public async Task<IActionResult> AuthorizeAPIKey( string[] permissions, string applicationName = null, Uri redirect = null,
public async Task<IActionResult> AuthorizeAPIKey(string[] permissions, string applicationName = null, Uri redirect = null,
bool strict = true, bool selectiveStores = false, string applicationIdentifier = null)
{
if (!_btcPayServerEnvironment.IsSecure)
Expand Down Expand Up @@ -157,16 +156,17 @@ public async Task<IActionResult> AddApiKey()
}

//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
});
}
}
Expand Down Expand Up @@ -255,38 +255,52 @@ public async Task<IActionResult> AuthorizeAPIKey([FromForm] AuthorizeApiKeysView
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<string, string>("apiKey", key.Id),
new KeyValuePair<string, string>("userId", key.UserId)
}
};
foreach (var permission in permissions)
{
redirectVm.Parameters.Add(
new KeyValuePair<string, string>("permissions[]", permission));
}

return View("PostRedirect", redirectVm);
}

TempData.SetStatusMessageModel(new StatusMessageModel()
{
Severity = StatusMessageModel.StatusSeverity.Success,
Html = $"API key generated! <code class='alert-link'>{key.Id}</code>"
});

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<string, object>() {{"key", key.Id}, {"permissions", permissions}, {"user", key.UserId}});
return uri.Uri.AbsoluteUri;
}

[HttpPost]
public async Task<IActionResult> AddApiKey(AddApiKeyViewModel viewModel)
{
Expand All @@ -313,6 +327,7 @@ public async Task<IActionResult> AddApiKey(AddApiKeyViewModel viewModel)
});
return RedirectToAction("APIKeys");
}

private IActionResult HandleCommands(AddApiKeyViewModel viewModel)
{
if (string.IsNullOrEmpty(viewModel.Command))
Expand All @@ -333,7 +348,6 @@ private IActionResult HandleCommands(AddApiKeyViewModel viewModel)
switch (command)
{
case "change-store-mode":

permissionValueItem.StoreMode = permissionValueItem.StoreMode == AddApiKeyViewModel.ApiKeyStoreMode.Specific
? AddApiKeyViewModel.ApiKeyStoreMode.AllStores
: AddApiKeyViewModel.ApiKeyStoreMode.Specific;
Expand All @@ -344,6 +358,7 @@ private IActionResult HandleCommands(AddApiKeyViewModel viewModel)
permissionValueItem.SpecificStores.Add(null);
}
return View(viewModel);

case "add-store":
permissionValueItem.SpecificStores.Add(null);
return View(viewModel);
Expand Down Expand Up @@ -501,9 +516,9 @@ public class AuthorizeApiKeysViewModel : AddApiKeyViewModel
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<APIKeyData> ApiKeyDatas { get; set; }
Expand Down
2 changes: 2 additions & 0 deletions BTCPayServer/Models/PostRedictViewModel.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ public class PostRedirectViewModel
{
public string AspAction { get; set; }
public string AspController { get; set; }
public string FormUrl { get; set; }

public List<KeyValuePair<string, string>> Parameters { get; set; } = new List<KeyValuePair<string, string>>();
}
}
2 changes: 1 addition & 1 deletion BTCPayServer/Views/Manage/APIKeys.cshtml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
<ul>
@foreach (var permission in Permission.ToPermissions(permissions).Select(c => c.ToString()).Distinct().ToArray())
{
<li>@permission</li>
<li><code>@permission</code></li>
}
</ul>
}
Expand Down

0 comments on commit 7e60328

Please sign in to comment.