Skip to content

Commit f60468c

Browse files
committed
fix: harden automation token deletion, cleanup and privileged checks
- Check privileged roles in memory or in SQL; Npgsql can't translate predicates over the roles enum array, which made these paths throw - Token delete refuses via a restricting FK instead of cascading, so a privileged account is never deleted with its token - Cleanup job logs only the accounts it actually deleted - Link automated accounts to their token on insert - Refuse the token for privileged API-token callers too - Drop the token and user caches; changes apply immediately - Integration tests for the token flows and the cleanup job
1 parent 88a8ca6 commit f60468c

21 files changed

Lines changed: 673 additions & 160 deletions

‎API.IntegrationTests/Tests/AutomationTokenTests.cs‎

Lines changed: 407 additions & 0 deletions
Large diffs are not rendered by default.

‎API/Controller/Account/LoginV2.cs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,12 @@
66
using Microsoft.AspNetCore.RateLimiting;
77
using OpenShock.API.Services.Account;
88
using OpenShock.Common.Errors;
9-
using OpenShock.Common.Extensions;
9+
using OpenShock.Common.OpenShockDb;
1010
using OpenShock.Common.Problems;
1111
using OpenShock.Common.Services.AutomationTokens;
1212
using OpenShock.API.Errors;
1313
using OpenShock.API.Models.Response;
1414
using OpenShock.API.Services.Turnstile;
15-
using OpenShock.Common.OpenShockDb;
1615
using Results = OpenShock.Common.Results;
1716

1817
using OpenShock.Internal.Common.Problems;

‎API/Controller/Account/SignupV2.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,9 @@ public async Task<IActionResult> SignUpV2(
4949
var turnstileError = await VerifyTurnstileAsync(turnstileService, body.TurnstileResponse, cancellationToken);
5050
if (turnstileError is not null) return turnstileError;
5151

52-
var creationAction = await _accountService.CreateAccountWithActivationFlowAsync(body.Email, body.Username, body.Password);
52+
// Created with an automation token, the account is linked to it on insert, which makes it an automated account.
53+
var creationAction = await _accountService.CreateAccountWithActivationFlowAsync(
54+
body.Email, body.Username, body.Password, automationTokens.Current?.Id);
5355
if (creationAction is not User created)
5456
{
5557
return creationAction switch
@@ -59,8 +61,6 @@ public async Task<IActionResult> SignUpV2(
5961
};
6062
}
6163

62-
// Links the new account to the automation token that created it, making it an automated account.
63-
// No-op when no automation token resolved.
6464
await automationTokens.RecordSignupAsync(created.Id, cancellationToken);
6565

6666
return Ok();

‎API/Controller/Admin/AutomationTokenCreate.cs‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ public sealed partial class AdminController
1919
[ProducesResponseType<CreatedAutomationTokenDto>(StatusCodes.Status200OK)]
2020
public async Task<CreatedAutomationTokenDto> CreateAutomationToken(
2121
[FromBody] CreateAutomationTokenDto body,
22-
[FromServices] IAutomationTokenService automationTokens,
2322
[FromServices] IAuditService auditService,
2423
CancellationToken ct)
2524
{
@@ -50,8 +49,6 @@ await auditService.LogAsync(
5049

5150
await transaction.CommitAsync(ct);
5251

53-
automationTokens.InvalidateCache();
54-
5552
return new CreatedAutomationTokenDto
5653
{
5754
Id = token.Id,
Lines changed: 28 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,71 +1,79 @@
11
using Microsoft.AspNetCore.Mvc;
22
using Microsoft.EntityFrameworkCore;
3+
using Npgsql;
34
using OpenShock.Common.Models;
45
using OpenShock.Common.OpenShockDb;
56
using OpenShock.Common.Services.Audit;
6-
using OpenShock.Common.Services.AutomationTokens;
77

88
namespace OpenShock.API.Controller.Admin;
99

1010
public sealed partial class AdminController
1111
{
1212
/// <summary>
1313
/// Deletes an automation token together with every account it created. Refused while any of those
14-
/// accounts holds a privileged role. Other API instances may keep honoring the token for up to 30 seconds
14+
/// accounts holds a privileged role
1515
/// </summary>
1616
[HttpDelete("automationTokens/{id}")]
1717
[ProducesResponseType(StatusCodes.Status200OK)]
1818
[ProducesResponseType(StatusCodes.Status404NotFound)]
1919
[ProducesResponseType(StatusCodes.Status409Conflict)]
2020
public async Task<IActionResult> DeleteAutomationToken(
2121
[FromRoute] Guid id,
22-
[FromServices] IAutomationTokenService automationTokens,
2322
[FromServices] IAuditService auditService,
2423
CancellationToken ct)
2524
{
2625
PedanticallyEnsureAdmin();
2726

28-
var token = await _db.AutomationTokens.FirstOrDefaultAsync(t => t.Id == id, ct);
27+
var token = await _db.AutomationTokens.AsNoTracking().FirstOrDefaultAsync(t => t.Id == id, ct);
2928
if (token is null) return NotFound();
3029

3130
await using var transaction = await _db.Database.BeginTransactionAsync(ct);
3231

3332
var createdUsers = await _db.Users
3433
.Where(u => u.CreatedByAutomationTokenId == id)
35-
.Select(u => new
36-
{
37-
u.Id,
38-
IsPrivileged = u.Roles.Any(r => r == RoleType.Staff || r == RoleType.Admin || r == RoleType.System)
39-
})
34+
.Select(u => new { u.Id, u.Roles })
4035
.ToListAsync(ct);
4136

42-
// The accounts go through the database's cascade, which can't skip privileged ones, so refuse instead.
43-
// The same roles AccountService.DeleteAccountAsync treats as privileged.
44-
if (createdUsers.Any(u => u.IsPrivileged))
45-
return Problem("This automation token created a privileged account; remove its privileged roles before deleting the token.", statusCode: StatusCodes.Status409Conflict);
37+
if (createdUsers.Any(u => PrivilegedRoles.Any(u.Roles))) return PrivilegedAccountConflict();
4638

47-
_db.AutomationTokens.Remove(token);
48-
await _db.SaveChangesAsync(ct);
39+
var userIds = createdUsers.Select(u => u.Id).ToArray();
40+
41+
// The roles are checked again as the rows are deleted, so an account promoted since it was read is kept.
42+
await _db.Database.ExecuteSqlAsync(
43+
$"DELETE FROM users WHERE id = ANY({userIds}) AND NOT (roles && {PrivilegedRoles.All})", ct);
44+
45+
// The foreign key restricts, so this fails while any account still references the token: a privileged
46+
// one, or one created or promoted meanwhile. On success, exactly userIds went with it.
47+
try
48+
{
49+
await _db.AutomationTokens.Where(t => t.Id == id).ExecuteDeleteAsync(ct);
50+
}
51+
catch (PostgresException e) when (e.SqlState == PostgresErrorCodes.ForeignKeyViolation)
52+
{
53+
return PrivilegedAccountConflict();
54+
}
4955

5056
await auditService.LogAsync(
5157
CurrentUser.Id,
5258
action: AuditAction.AutomationTokenDeleted,
5359
actorId: CurrentUser.Id,
54-
metadata: new AutomationTokenDeletedMetadata(token.Id, token.Name, createdUsers.Count),
60+
metadata: new AutomationTokenDeletedMetadata(token.Id, token.Name, userIds.Length),
5561
cancellationToken: ct);
5662

5763
await transaction.CommitAsync(ct);
5864

59-
automationTokens.InvalidateCache();
60-
6165
// Each deleted account's own audit log goes with it, so the log line is the lasting record of what went.
62-
foreach (var user in createdUsers)
66+
foreach (var userId in userIds)
6367
{
6468
_logger.LogInformation(
6569
"Deleted account {UserId} together with automation token {AutomationTokenId}",
66-
user.Id, token.Id);
70+
userId, token.Id);
6771
}
6872

6973
return Ok();
74+
75+
IActionResult PrivilegedAccountConflict() => Problem(
76+
"This automation token created a privileged account; remove its privileged roles before deleting the token.",
77+
statusCode: StatusCodes.Status409Conflict);
7078
}
7179
}

‎API/Controller/Admin/AutomationTokenPatch.cs‎

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,13 @@
55
using OpenShock.Common.Models;
66
using OpenShock.Common.OpenShockDb;
77
using OpenShock.Common.Services.Audit;
8-
using OpenShock.Common.Services.AutomationTokens;
98

109
namespace OpenShock.API.Controller.Admin;
1110

1211
public sealed partial class AdminController
1312
{
1413
/// <summary>
15-
/// Updates an automation token. Other API instances may keep the old settings for up to 30 seconds
14+
/// Updates an automation token
1615
/// </summary>
1716
[HttpPatch("automationTokens/{id}")]
1817
[Consumes(MediaTypeNames.Application.Json)]
@@ -21,7 +20,6 @@ public sealed partial class AdminController
2120
public async Task<IActionResult> PatchAutomationToken(
2221
[FromRoute] Guid id,
2322
[FromBody] PatchAutomationTokenDto body,
24-
[FromServices] IAutomationTokenService automationTokens,
2523
[FromServices] IAuditService auditService,
2624
CancellationToken ct)
2725
{
@@ -30,20 +28,20 @@ public async Task<IActionResult> PatchAutomationToken(
3028
var token = await _db.AutomationTokens.FirstOrDefaultAsync(t => t.Id == id, ct);
3129
if (token is null) return NotFound();
3230

33-
if (body.Name is not null) token.Name = body.Name.Trim();
34-
if (body.Types is not null) token.Types = [.. body.Types.Distinct()];
35-
if (body.AutoCleanupAfter is not null) token.AutoCleanupAfter = body.AutoCleanupAfter;
36-
if (body.AutoCleanupUsers is not null)
37-
{
38-
token.AutoCleanupUsers = body.AutoCleanupUsers.Value;
31+
var autoCleanupUsers = body.AutoCleanupUsers ?? token.AutoCleanupUsers;
3932

40-
// Switching cleanup off without naming a delay clears the delay, so it can be unset at all.
41-
if (!token.AutoCleanupUsers && body.AutoCleanupAfter is null) token.AutoCleanupAfter = null;
42-
}
33+
// Switching cleanup off without naming a delay clears the delay, so it can be unset at all.
34+
var autoCleanupAfter = body.AutoCleanupAfter ??
35+
(body.AutoCleanupUsers == false ? null : token.AutoCleanupAfter);
4336

44-
if (token.AutoCleanupUsers && token.AutoCleanupAfter is null)
37+
if (autoCleanupUsers && autoCleanupAfter is null)
4538
return Problem("AutoCleanupAfter is required when AutoCleanupUsers is true.", statusCode: StatusCodes.Status400BadRequest);
4639

40+
if (body.Name is not null) token.Name = body.Name.Trim();
41+
if (body.Types is not null) token.Types = [.. body.Types.Distinct()];
42+
token.AutoCleanupUsers = autoCleanupUsers;
43+
token.AutoCleanupAfter = autoCleanupAfter;
44+
4745
await using var transaction = await _db.Database.BeginTransactionAsync(ct);
4846

4947
await _db.SaveChangesAsync(ct);
@@ -57,8 +55,6 @@ await auditService.LogAsync(
5755

5856
await transaction.CommitAsync(ct);
5957

60-
automationTokens.InvalidateCache();
61-
6258
return Ok(AutomationTokenDto.FromEntity(token));
6359
}
6460
}

‎API/Controller/Admin/AutomationTokenRotate.cs‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,13 @@ namespace OpenShock.API.Controller.Admin;
1212
public sealed partial class AdminController
1313
{
1414
/// <summary>
15-
/// Rotates the secret of an automation token. The new secret is only returned once, in this response.
16-
/// Other API instances may keep accepting the old secret for up to 30 seconds
15+
/// Rotates the secret of an automation token. The new secret is only returned once, in this response
1716
/// </summary>
1817
[HttpPost("automationTokens/{id}/rotate")]
1918
[ProducesResponseType<CreatedAutomationTokenDto>(StatusCodes.Status200OK)]
2019
[ProducesResponseType(StatusCodes.Status404NotFound)]
2120
public async Task<IActionResult> RotateAutomationToken(
2221
[FromRoute] Guid id,
23-
[FromServices] IAutomationTokenService automationTokens,
2422
[FromServices] IAuditService auditService,
2523
CancellationToken ct)
2624
{
@@ -48,8 +46,6 @@ await auditService.LogAsync(
4846

4947
await transaction.CommitAsync(ct);
5048

51-
automationTokens.InvalidateCache();
52-
5349
// Accounts the token created stay linked to it (and to its cleanup schedule) across rotations,
5450
// since the token id doesn't change.
5551

‎API/Controller/Tokens/ReportTokens.cs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
using Microsoft.AspNetCore.RateLimiting;
1010
using OpenShock.API.Errors;
1111
using OpenShock.API.Services.Turnstile;
12-
using OpenShock.Common.Extensions;
1312
using OpenShock.Common.OpenShockDb;
1413
using OpenShock.Common.Results;
1514
using OpenShock.Common.Services.AutomationTokens;
@@ -54,7 +53,7 @@ public async Task<IActionResult> ReportTokens(
5453
return Problem(new OpenShockProblem("InternalServerError", "Internal Server Error", HttpStatusCode.InternalServerError));
5554
}
5655

57-
// The middleware only screens cookie sessions for privileged accounts; this also covers API-token callers.
56+
// The middleware already refused the token for a privileged session; this audits the use.
5857
if (!await automationTokens.TryRecordUseAsync(CurrentUser.Id, AutomationTokenFlow.ReportTokens, cancellationToken))
5958
return Problem(AutomationTokenError.NotAllowedForAccount);
6059

‎API/Services/Account/AccountService.cs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ private async Task<bool> IsEmailProviderBlacklisted(string email)
9191
return await _db.EmailProviderBlacklists.AnyAsync(e => e.Domain == domain);
9292
}
9393

94-
private async Task<AccountCreationResult> CreateAccount(string email, string username, string password, bool verifyOnCreation)
94+
private async Task<AccountCreationResult> CreateAccount(string email, string username, string password, bool verifyOnCreation, Guid? createdByAutomationTokenId)
9595
{
9696
email = email.ToLowerInvariant();
9797

@@ -106,7 +106,8 @@ private async Task<AccountCreationResult> CreateAccount(string email, string use
106106
Id = Guid.CreateVersion7(),
107107
Name = username,
108108
Email = email,
109-
PasswordHash = HashingUtils.HashPassword(password)
109+
PasswordHash = HashingUtils.HashPassword(password),
110+
CreatedByAutomationTokenId = createdByAutomationTokenId
110111
};
111112
_db.Users.Add(user);
112113

@@ -124,13 +125,13 @@ await _db.Users
124125
}
125126

126127
/// <inheritdoc />
127-
public async Task<AccountCreationResult> CreateAccountWithActivationFlowAsync(string email, string username, string password)
128+
public async Task<AccountCreationResult> CreateAccountWithActivationFlowAsync(string email, string username, string password, Guid? createdByAutomationTokenId = null)
128129
{
129130
// With mail disabled the activation email is never delivered, so an activation flow would leave
130131
// the account permanently unusable. Activate on creation instead.
131132
var mailEnabled = _mailOptions.IsEnabled;
132133

133-
var accountCreate = await CreateAccount(email, username, password, !mailEnabled);
134+
var accountCreate = await CreateAccount(email, username, password, !mailEnabled, createdByAutomationTokenId);
134135
if (accountCreate.Value is not User user || !mailEnabled) return accountCreate;
135136

136137
// The real activation token is minted by the outbox delivery job at send time; here we record the

‎API/Services/Account/IAccountService.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,9 @@ public interface IAccountService
1515
/// <param name="email"></param>
1616
/// <param name="username"></param>
1717
/// <param name="password"></param>
18+
/// <param name="createdByAutomationTokenId">The automation token creating the account, which makes it an automated account</param>
1819
/// <returns></returns>
19-
public Task<AccountCreationResult> CreateAccountWithActivationFlowAsync(string email, string username, string password);
20+
public Task<AccountCreationResult> CreateAccountWithActivationFlowAsync(string email, string username, string password, Guid? createdByAutomationTokenId = null);
2021

2122
/// <summary>
2223
/// Creates an OAuth-only (passwordless) account and links the external identity in a single transaction.

0 commit comments

Comments
 (0)