Fix: Create store could be called with a scoped store's modify apikey (#1696)

BTCPayServer.Client/BTCPayServerClient.cs

@@ -18,6 +18,8 @@ namespace BTCPayServer.Client
         private readonly string _password;
         private readonly HttpClient _httpClient;
 
+        public Uri Host => _btcpayHost;
+
         public string APIKey => _apiKey;
 
         public BTCPayServerClient(Uri btcpayHost, HttpClient httpClient = null)

BTCPayServer.Client/Permissions.cs

@@ -12,6 +12,7 @@ namespace BTCPayServer.Client
         public const string CanUseLightningNodeInStore = "btcpay.store.canuselightningnode";
         public const string CanModifyServerSettings = "btcpay.server.canmodifyserversettings";
         public const string CanModifyStoreSettings = "btcpay.store.canmodifystoresettings";
+        public const string CanModifyStoreSettingsUnscoped = "btcpay.store.canmodifystoresettings:";
         public const string CanViewStoreSettings = "btcpay.store.canviewstoresettings";
         public const string CanCreateInvoice = "btcpay.store.cancreateinvoice";
         public const string CanViewPaymentRequests = "btcpay.store.canviewpaymentrequests";

BTCPayServer.Tests/GreenfieldAPITests.cs

@@ -36,7 +36,7 @@ namespace BTCPayServer.Tests
 
         public GreenfieldAPITests(ITestOutputHelper helper)
         {
-            Logs.Tester = new XUnitLog(helper) {Name = "Tests"};
+            Logs.Tester = new XUnitLog(helper) { Name = "Tests" };
             Logs.LogProvider = new XUnitLogProvider(helper);
         }
 
@@ -69,6 +69,32 @@ namespace BTCPayServer.Tests
             }
         }
 
+
+        [Fact(Timeout = TestTimeout)]
+        [Trait("Integration", "Integration")]
+        public async Task SpecificCanModifyStoreCantCreateNewStore()
+        {
+            using (var tester = ServerTester.Create())
+            {
+                await tester.StartAsync();
+                var acc = tester.NewAccount();
+                await acc.GrantAccessAsync();
+                var unrestricted = await acc.CreateClient();
+                var response = await unrestricted.CreateStore(new CreateStoreRequest() { Name = "mystore" });
+                var apiKey = (await unrestricted.CreateAPIKey(new CreateApiKeyRequest() { Permissions = new[] { Permission.Create("btcpay.store.canmodifystoresettings", response.Id) } })).ApiKey;
+                var restricted = new BTCPayServerClient(unrestricted.Host, apiKey);
+
+                // Unscoped permission should be required for create store
+                await this.AssertHttpError(403, async () => await restricted.CreateStore(new CreateStoreRequest() { Name = "store2" }));
+                // Unrestricted should work fine
+                await unrestricted.CreateStore(new CreateStoreRequest() { Name = "store2" });
+                // Restricted but unscoped should work fine
+                apiKey = (await unrestricted.CreateAPIKey(new CreateApiKeyRequest() { Permissions = new[] { Permission.Create("btcpay.store.canmodifystoresettings") } })).ApiKey;
+                restricted = new BTCPayServerClient(unrestricted.Host, apiKey);
+                await restricted.CreateStore(new CreateStoreRequest() { Name = "store2" });
+            }
+        }
+
         [Fact(Timeout = TestTimeout)]
         [Trait("Integration", "Integration")]
         public async Task CanCreateAndDeleteAPIKeyViaAPI()
@@ -82,7 +108,7 @@ namespace BTCPayServer.Tests
                 var apiKey = await unrestricted.CreateAPIKey(new CreateApiKeyRequest()
                 {
                     Label = "Hello world",
-                    Permissions = new Permission[] {Permission.Create(Policies.CanViewProfile)}
+                    Permissions = new Permission[] { Permission.Create(Policies.CanViewProfile) }
                 });
                 Assert.Equal("Hello world", apiKey.Label);
                 var p = Assert.Single(apiKey.Permissions);
@@ -93,7 +119,7 @@ namespace BTCPayServer.Tests
                     async () => await restricted.CreateAPIKey(new CreateApiKeyRequest()
                     {
                         Label = "Hello world2",
-                        Permissions = new Permission[] {Permission.Create(Policies.CanViewProfile)}
+                        Permissions = new Permission[] { Permission.Create(Policies.CanViewProfile) }
                     }));
 
                 await unrestricted.RevokeAPIKey(apiKey.ApiKey);
@@ -114,50 +140,54 @@ namespace BTCPayServer.Tests
                     async () => await unauthClient.CreateUser(new CreateApplicationUserRequest()));
                 await AssertValidationError(new[] { "Password" },
                     async () => await unauthClient.CreateUser(
-                        new CreateApplicationUserRequest() {Email = "test@gmail.com"}));
+                        new CreateApplicationUserRequest() { Email = "test@gmail.com" }));
                 // Pass too simple
                 await AssertValidationError(new[] { "Password" },
                     async () => await unauthClient.CreateUser(
-                        new CreateApplicationUserRequest() {Email = "test3@gmail.com", Password = "a"}));
+                        new CreateApplicationUserRequest() { Email = "test3@gmail.com", Password = "a" }));
 
                 // We have no admin, so it should work
                 var user1 = await unauthClient.CreateUser(
-                    new CreateApplicationUserRequest() {Email = "test@gmail.com", Password = "abceudhqw"});
+                    new CreateApplicationUserRequest() { Email = "test@gmail.com", Password = "abceudhqw" });
                 // We have no admin, so it should work
                 var user2 = await unauthClient.CreateUser(
-                    new CreateApplicationUserRequest() {Email = "test2@gmail.com", Password = "abceudhqw"});
+                    new CreateApplicationUserRequest() { Email = "test2@gmail.com", Password = "abceudhqw" });
 
                 // Duplicate email
                 await AssertValidationError(new[] { "Email" },
                     async () => await unauthClient.CreateUser(
-                        new CreateApplicationUserRequest() {Email = "test2@gmail.com", Password = "abceudhqw"}));
+                        new CreateApplicationUserRequest() { Email = "test2@gmail.com", Password = "abceudhqw" }));
 
                 // Let's make an admin
                 var admin = await unauthClient.CreateUser(new CreateApplicationUserRequest()
                 {
-                    Email = "admin@gmail.com", Password = "abceudhqw", IsAdministrator = true
+                    Email = "admin@gmail.com",
+                    Password = "abceudhqw",
+                    IsAdministrator = true
                 });
 
                 // Creating a new user without proper creds is now impossible (unauthorized) 
                 // Because if registration are locked and that an admin exists, we don't accept unauthenticated connection
                 await AssertHttpError(401,
                     async () => await unauthClient.CreateUser(
-                        new CreateApplicationUserRequest() {Email = "test3@gmail.com", Password = "afewfoiewiou"}));
+                        new CreateApplicationUserRequest() { Email = "test3@gmail.com", Password = "afewfoiewiou" }));
 
 
                 // But should be ok with subscriptions unlocked
                 var settings = tester.PayTester.GetService<SettingsRepository>();
-                await settings.UpdateSetting<PoliciesSettings>(new PoliciesSettings() {LockSubscription = false});
+                await settings.UpdateSetting<PoliciesSettings>(new PoliciesSettings() { LockSubscription = false });
                 await unauthClient.CreateUser(
-                    new CreateApplicationUserRequest() {Email = "test3@gmail.com", Password = "afewfoiewiou"});
+                    new CreateApplicationUserRequest() { Email = "test3@gmail.com", Password = "afewfoiewiou" });
 
                 // But it should be forbidden to create an admin without being authenticated
                 await AssertHttpError(403,
                     async () => await unauthClient.CreateUser(new CreateApplicationUserRequest()
                     {
-                        Email = "admin2@gmail.com", Password = "afewfoiewiou", IsAdministrator = true
+                        Email = "admin2@gmail.com",
+                        Password = "afewfoiewiou",
+                        IsAdministrator = true
                     }));
-                await settings.UpdateSetting<PoliciesSettings>(new PoliciesSettings() {LockSubscription = true});
+                await settings.UpdateSetting<PoliciesSettings>(new PoliciesSettings() { LockSubscription = true });
 
                 var adminAcc = tester.NewAccount();
                 adminAcc.UserId = admin.Id;
@@ -167,21 +197,25 @@ namespace BTCPayServer.Tests
                 // We should be forbidden to create a new user without proper admin permissions
                 await AssertHttpError(403,
                     async () => await adminClient.CreateUser(
-                        new CreateApplicationUserRequest() {Email = "test4@gmail.com", Password = "afewfoiewiou"}));
+                        new CreateApplicationUserRequest() { Email = "test4@gmail.com", Password = "afewfoiewiou" }));
                 await AssertHttpError(403,
                     async () => await adminClient.CreateUser(new CreateApplicationUserRequest()
                     {
-                        Email = "test4@gmail.com", Password = "afewfoiewiou", IsAdministrator = true
+                        Email = "test4@gmail.com",
+                        Password = "afewfoiewiou",
+                        IsAdministrator = true
                     }));
 
                 // However, should be ok with the unrestricted permissions of an admin
                 adminClient = await adminAcc.CreateClient(Policies.Unrestricted);
                 await adminClient.CreateUser(
-                    new CreateApplicationUserRequest() {Email = "test4@gmail.com", Password = "afewfoiewiou"});
+                    new CreateApplicationUserRequest() { Email = "test4@gmail.com", Password = "afewfoiewiou" });
                 // Even creating new admin should be ok
                 await adminClient.CreateUser(new CreateApplicationUserRequest()
                 {
-                    Email = "admin4@gmail.com", Password = "afewfoiewiou", IsAdministrator = true
+                    Email = "admin4@gmail.com",
+                    Password = "afewfoiewiou",
+                    IsAdministrator = true
                 });
 
                 var user1Acc = tester.NewAccount();
@@ -192,18 +226,20 @@ namespace BTCPayServer.Tests
                 // User1 trying to get server management would still fail to create user
                 await AssertHttpError(403,
                     async () => await user1Client.CreateUser(
-                        new CreateApplicationUserRequest() {Email = "test8@gmail.com", Password = "afewfoiewiou"}));
+                        new CreateApplicationUserRequest() { Email = "test8@gmail.com", Password = "afewfoiewiou" }));
 
                 // User1 should be able to create user if subscription unlocked
-                await settings.UpdateSetting<PoliciesSettings>(new PoliciesSettings() {LockSubscription = false});
+                await settings.UpdateSetting<PoliciesSettings>(new PoliciesSettings() { LockSubscription = false });
                 await user1Client.CreateUser(
-                    new CreateApplicationUserRequest() {Email = "test8@gmail.com", Password = "afewfoiewiou"});
+                    new CreateApplicationUserRequest() { Email = "test8@gmail.com", Password = "afewfoiewiou" });
 
                 // But not an admin
                 await AssertHttpError(403,
                     async () => await user1Client.CreateUser(new CreateApplicationUserRequest()
                     {
-                        Email = "admin8@gmail.com", Password = "afewfoiewiou", IsAdministrator = true
+                        Email = "admin8@gmail.com",
+                        Password = "afewfoiewiou",
+                        IsAdministrator = true
                     }));
             }
         }
@@ -381,7 +417,7 @@ namespace BTCPayServer.Tests
 
 
                 Logs.Tester.LogInformation("Create a pull payment with USD");
-                var pp  = await client.CreatePullPayment(storeId, new Client.Models.CreatePullPaymentRequest()
+                var pp = await client.CreatePullPayment(storeId, new Client.Models.CreatePullPaymentRequest()
                 {
                     Name = "Test USD",
                     Amount = 5000m,
@@ -442,10 +478,10 @@ namespace BTCPayServer.Tests
                 var client = await user.CreateClient(Policies.Unrestricted);
 
                 //create store
-                var newStore = await client.CreateStore(new CreateStoreRequest() {Name = "A"});
+                var newStore = await client.CreateStore(new CreateStoreRequest() { Name = "A" });
 
                 //update store
-                var updatedStore = await client.UpdateStore(newStore.Id, new UpdateStoreRequest() {Name = "B"});
+                var updatedStore = await client.UpdateStore(newStore.Id, new UpdateStoreRequest() { Name = "B" });
                 Assert.Equal("B", updatedStore.Name);
                 Assert.Equal("B", (await client.GetStore(newStore.Id)).Name);
 
@@ -471,7 +507,7 @@ namespace BTCPayServer.Tests
                 });
                 Assert.Single(await client.GetStores());
 
-                newStore = await client.CreateStore(new CreateStoreRequest() {Name = "A"});
+                newStore = await client.CreateStore(new CreateStoreRequest() { Name = "A" });
                 var scopedClient =
                     await user.CreateClient(Permission.Create(Policies.CanViewStoreSettings, user.StoreId).ToString());
                 Assert.Single(await scopedClient.GetStores());
@@ -534,34 +570,38 @@ namespace BTCPayServer.Tests
                 await Assert.ThrowsAsync<HttpRequestException>(async () =>
                     await clientInsufficient.CreateUser(new CreateApplicationUserRequest()
                     {
-                        Email = $"{Guid.NewGuid()}@g.com", Password = Guid.NewGuid().ToString()
+                        Email = $"{Guid.NewGuid()}@g.com",
+                        Password = Guid.NewGuid().ToString()
                     }));
 
                 var newUser = await clientServer.CreateUser(new CreateApplicationUserRequest()
                 {
-                    Email = $"{Guid.NewGuid()}@g.com", Password = Guid.NewGuid().ToString()
+                    Email = $"{Guid.NewGuid()}@g.com",
+                    Password = Guid.NewGuid().ToString()
                 });
                 Assert.NotNull(newUser);
 
                 var newUser2 = await clientBasic.CreateUser(new CreateApplicationUserRequest()
                 {
-                    Email = $"{Guid.NewGuid()}@g.com", Password = Guid.NewGuid().ToString()
+                    Email = $"{Guid.NewGuid()}@g.com",
+                    Password = Guid.NewGuid().ToString()
                 });
                 Assert.NotNull(newUser2);
 
                 await AssertValidationError(new[] { "Email" }, async () =>
                     await clientServer.CreateUser(new CreateApplicationUserRequest()
                     {
-                        Email = $"{Guid.NewGuid()}", Password = Guid.NewGuid().ToString()
+                        Email = $"{Guid.NewGuid()}",
+                        Password = Guid.NewGuid().ToString()
                     }));
 
                 await AssertValidationError(new[] { "Password" }, async () =>
                     await clientServer.CreateUser(
-                        new CreateApplicationUserRequest() {Email = $"{Guid.NewGuid()}@g.com",}));
+                        new CreateApplicationUserRequest() { Email = $"{Guid.NewGuid()}@g.com", }));
 
                 await AssertValidationError(new[] { "Email" }, async () =>
                     await clientServer.CreateUser(
-                        new CreateApplicationUserRequest() {Password = Guid.NewGuid().ToString()}));
+                        new CreateApplicationUserRequest() { Password = Guid.NewGuid().ToString() }));
             }
         }
 
@@ -623,25 +663,25 @@ namespace BTCPayServer.Tests
                 //validation errors
                 await AssertValidationError(new[] { "Amount", "Currency" }, async () =>
                 {
-                    await client.CreatePaymentRequest(user.StoreId, new CreatePaymentRequestRequest() {Title = "A"});
+                    await client.CreatePaymentRequest(user.StoreId, new CreatePaymentRequestRequest() { Title = "A" });
                 });
                 await AssertValidationError(new[] { "Amount" }, async () =>
                 {
                     await client.CreatePaymentRequest(user.StoreId,
-                        new CreatePaymentRequestRequest() {Title = "A", Currency = "BTC", Amount = 0});
+                        new CreatePaymentRequestRequest() { Title = "A", Currency = "BTC", Amount = 0 });
                 });
                 await AssertValidationError(new[] { "Currency" }, async () =>
                 {
                     await client.CreatePaymentRequest(user.StoreId,
-                        new CreatePaymentRequestRequest() {Title = "A", Currency = "helloinvalid", Amount = 1});
+                        new CreatePaymentRequestRequest() { Title = "A", Currency = "helloinvalid", Amount = 1 });
                 });
                 await AssertHttpError(403, async () =>
                 {
                     await viewOnly.CreatePaymentRequest(user.StoreId,
-                        new CreatePaymentRequestRequest() {Title = "A", Currency = "helloinvalid", Amount = 1});
+                        new CreatePaymentRequestRequest() { Title = "A", Currency = "helloinvalid", Amount = 1 });
                 });
                 var newPaymentRequest = await client.CreatePaymentRequest(user.StoreId,
-                    new CreatePaymentRequestRequest() {Title = "A", Currency = "USD", Amount = 1});
+                    new CreatePaymentRequestRequest() { Title = "A", Currency = "USD", Amount = 1 });
 
                 //list payment request
                 var paymentRequests = await viewOnly.GetPaymentRequests(user.StoreId);
@@ -674,11 +714,11 @@ namespace BTCPayServer.Tests
                 await client.ArchivePaymentRequest(user.StoreId, paymentRequest.Id);
                 Assert.DoesNotContain(paymentRequest.Id,
                     (await client.GetPaymentRequests(user.StoreId)).Select(data => data.Id));
-                
+
                 //let's test some payment stuff
                 await user.RegisterDerivationSchemeAsync("BTC");
                 var paymentTestPaymentRequest = await client.CreatePaymentRequest(user.StoreId,
-                    new CreatePaymentRequestRequest() {Amount = 0.1m, Currency = "BTC", Title = "Payment test title"});
+                    new CreatePaymentRequestRequest() { Amount = 0.1m, Currency = "BTC", Title = "Payment test title" });
 
                 var invoiceId = Assert.IsType<string>(Assert.IsType<OkObjectResult>(await user.GetController<PaymentRequestController>()
                     .PayPaymentRequest(paymentTestPaymentRequest.Id, false)).Value);
@@ -688,11 +728,11 @@ namespace BTCPayServer.Tests
                     await tester.ExplorerNode.SendToAddressAsync(
                         BitcoinAddress.Create(invoice.BitcoinAddress, tester.ExplorerNode.Network), invoice.BtcDue);
                 });
-               await TestUtils.EventuallyAsync(async () =>
+                await TestUtils.EventuallyAsync(async () =>
-                {
+                 {
-                    Assert.Equal(Invoice.STATUS_PAID, user.BitPay.GetInvoice(invoiceId).Status);
+                     Assert.Equal(Invoice.STATUS_PAID, user.BitPay.GetInvoice(invoiceId).Status);
-                    Assert.Equal(PaymentRequestData.PaymentRequestStatus.Completed, (await client.GetPaymentRequest(user.StoreId, paymentTestPaymentRequest.Id)).Status);
+                     Assert.Equal(PaymentRequestData.PaymentRequestStatus.Completed, (await client.GetPaymentRequest(user.StoreId, paymentTestPaymentRequest.Id)).Status);
-                });
+                 });
             }
         }
 

BTCPayServer/Controllers/GreenField/StoresController.cs

@@ -65,7 +65,7 @@ namespace BTCPayServer.Controllers.GreenField
         }
         
         [HttpPost("~/api/v1/stores")]
-        [Authorize(Policy = Policies.CanModifyStoreSettings, AuthenticationSchemes = AuthenticationSchemes.Greenfield)]
+        [Authorize(Policy = Policies.CanModifyStoreSettingsUnscoped, AuthenticationSchemes = AuthenticationSchemes.Greenfield)]
         public async Task<IActionResult> CreateStore(CreateStoreRequest request)
         {
             var validationResult = Validate(request);

BTCPayServer/Security/GreenField/APIKeyExtensions.cs

@@ -46,14 +46,19 @@ namespace BTCPayServer.Security.GreenField
                     c.Type.Equals(GreenFieldConstants.ClaimTypes.Permission, StringComparison.InvariantCultureIgnoreCase))
                 .Select(claim => claim.Value).ToArray();
         }
-
         public static bool HasPermission(this AuthorizationHandlerContext context, Permission permission)
+        {
+            return HasPermission(context, permission, false);
+        }
+        public static bool HasPermission(this AuthorizationHandlerContext context, Permission permission, bool requireUnscoped)
         {
             foreach (var claim in context.User.Claims.Where(c =>
                 c.Type.Equals(GreenFieldConstants.ClaimTypes.Permission, StringComparison.InvariantCultureIgnoreCase)))
             {
                 if (Permission.TryParse(claim.Value, out var claimPermission))
                 {
+                    if (requireUnscoped && claimPermission.Scope is string)
+                        continue;
                     if (claimPermission.Contains(permission))
                     {
                         return true;

BTCPayServer/Security/GreenField/GreenFieldAuthorizationHandler.cs

@@ -35,14 +35,22 @@ namespace BTCPayServer.Security.GreenField
                 return;
             var userid = _userManager.GetUserId(context.User);
             bool success = false;
-            switch (requirement.Policy)
+            var policy = requirement.Policy;
+            var requiredUnscoped = false;
+            if (policy.EndsWith(':'))
             {
-                case { } policy when Policies.IsStorePolicy(policy):
+                policy = policy.Substring(0, policy.Length - 1);
+                requiredUnscoped = true;
+            }
+
+            switch (policy)
+            {
+                case { } when Policies.IsStorePolicy(policy):
                     var storeId = _HttpContext.GetImplicitStoreId();
                     // Specific store action
                     if (storeId != null)
                     {
-                        if (context.HasPermission(Permission.Create(requirement.Policy, storeId)))
+                        if (context.HasPermission(Permission.Create(policy, storeId), requiredUnscoped))
                         {
                             if (string.IsNullOrEmpty(userid))
                                 break;
@@ -60,19 +68,21 @@ namespace BTCPayServer.Security.GreenField
                     }
                     else
                     {
+                        if (requiredUnscoped && !context.HasPermission(Permission.Create(policy)))
+                            break;
                         var stores = await _storeRepository.GetStoresByUserId(userid);
                         List<StoreData> permissionedStores = new List<StoreData>();
                         foreach (var store in stores)
                         {
-                            if (context.HasPermission(Permission.Create(requirement.Policy, store.Id)))
+                            if (context.HasPermission(Permission.Create(policy, store.Id), requiredUnscoped))
                                 permissionedStores.Add(store);
                         }
                         _HttpContext.SetStoresData(permissionedStores.ToArray());
                         success = true;
                     }
                     break;
-                case { } policy when Policies.IsServerPolicy(policy):
+                case { } when Policies.IsServerPolicy(policy):
-                    if (context.HasPermission(Permission.Create(requirement.Policy)))
+                    if (context.HasPermission(Permission.Create(policy)))
                     {
                         var user = await _userManager.GetUserAsync(context.User);
                         if (user == null)
@@ -85,7 +95,7 @@ namespace BTCPayServer.Security.GreenField
                 case Policies.CanModifyProfile:
                 case Policies.CanViewProfile:
                 case Policies.Unrestricted:
-                    success = context.HasPermission(Permission.Create(requirement.Policy));
+                    success = context.HasPermission(Permission.Create(policy), requiredUnscoped);
                     break;
             }
 

BTCPayServer/Security/ServerPolicies.cs

@@ -11,6 +11,7 @@ namespace BTCPayServer.Security
             {
                 options.AddPolicy(p);
             }
+            options.AddPolicy(Policies.CanModifyStoreSettingsUnscoped);
             options.AddPolicy(CanGetRates.Key);
             return options;
         }