Public Access
Server: send each item at the revision its ciphertext is sealed at, keep team archiving with the owner, refuse suspended accounts, and stop enrolment failing after an hour
- Pull hydrated every historical change entry with the item's current ciphertext under that entry's old revision. The AAD binds the revision, so a client opened it at the wrong version, failed, and parked its own pending edit as undecryptable. Entries an item has since moved past are left out; the later entry carries the same payload under the right number, and a fresh device no longer downloads an item once per edit it ever had. - Deleting a team's last vault archived the team whoever deleted it, so an admin could archive it out from under its owner. Only the owner's delete archives it now, and TeamService.ArchiveAsync checks. - UserStatus was checked nowhere on the request path. ActiveAccountRequirement on both policies answers 403 account-not-active for a suspended or deprovisioned account. - The hourly last-seen update moves xmin under the tracked user, and enrolment then saved that user against the stale token: a 500 for anybody enrolling more than an hour after first signing in. The entity is reloaded after the update.
This commit is contained in:
1 parent
f332d9a56f
commit
6846f67c20
14 files changed
+277
-12
No files matched your search
@@ -0,0 +1,49 @@
|
||||
using DodoSSH.Domain;
|
||||
using Microsoft.AspNetCore.Authorization;
|
||||
|
||||
namespace DodoSSH.Api.Authorization;
|
||||
|
||||
/// <summary>Requires the caller's account to be <see cref="UserStatus.Active"/>.</summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// <see cref="UserStatus.Suspended"/> is documented as "sign-in blocked", and nothing blocked it: the identity
|
||||
/// provider still issues tokens for the person, every endpoint resolved them through the current-user context,
|
||||
/// and that context looked an account up by issuer, subject and deletion alone. A suspended account could pull
|
||||
/// and push vaults, take relay tickets and hold the events socket open. This is the check, on both policies, so
|
||||
/// that no endpoint can be reached without it.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// A connection already open when the account is suspended — a relay tunnel, the events socket — is not
|
||||
/// reached by this; it ends on its own idle and lifetime limits.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
internal sealed class ActiveAccountRequirement : IAuthorizationRequirement;
|
||||
|
||||
/// <summary>Satisfies <see cref="ActiveAccountRequirement"/> from the account row.</summary>
|
||||
internal sealed class ActiveAccountHandler(ICurrentUserContext currentUser, IHttpContextAccessor accessor)
|
||||
: AuthorizationHandler<ActiveAccountRequirement>
|
||||
{
|
||||
/// <inheritdoc />
|
||||
protected override async Task HandleRequirementAsync(
|
||||
AuthorizationHandlerContext context,
|
||||
ActiveAccountRequirement requirement)
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(context);
|
||||
|
||||
// The policy's authentication requirement already refuses an anonymous caller with a 401; failing this
|
||||
// as well would turn that into a 403.
|
||||
if (context.User.Identity?.IsAuthenticated != true)
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
var cancellationToken = accessor.HttpContext?.RequestAborted ?? CancellationToken.None;
|
||||
|
||||
var user = await currentUser.GetOrProvisionAsync(cancellationToken).ConfigureAwait(false);
|
||||
|
||||
if (user.Status == UserStatus.Active)
|
||||
{
|
||||
context.Succeed(requirement);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -102,8 +102,14 @@ internal sealed class CurrentUserContext(
|
||||
/// read-then-write: <c>user_account</c> carries the xmin concurrency token, so two overlapping
|
||||
/// requests from one user would each read the row, each set the timestamp, and the second would
|
||||
/// fail on a version that had moved under it. This writes at most one row and cannot conflict.
|
||||
/// The tracked entity is deliberately left alone — a value up to an hour stale in memory changes
|
||||
/// nothing, and marking it modified would enlist the user row in whatever the request saves next.
|
||||
/// <para>
|
||||
/// ◆ <b>But the write moves xmin under the tracked entity, so the entity is re-read after it.</b> It is
|
||||
/// not marked modified — that would enlist the user row in whatever the request saves next — and a
|
||||
/// stale timestamp in memory would change nothing on its own. A stale concurrency token does: the next
|
||||
/// save in the same request that touches this user (enrolment stamps <c>EnrolledAtUtc</c>) compared
|
||||
/// against the version from before the update and failed, so enrolling more than an hour after first
|
||||
/// signing in answered 500. Only when a row was actually written, which is once an hour at most.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
private async Task RefreshLastSeenAsync(UserAccount user, CancellationToken cancellationToken)
|
||||
{
|
||||
@@ -114,13 +120,18 @@ internal sealed class CurrentUserContext(
|
||||
return;
|
||||
}
|
||||
|
||||
await database.Users
|
||||
var written = await database.Users
|
||||
.Where(u => u.Id == user.Id
|
||||
&& (u.LastSeenAtUtc == null || u.LastSeenAtUtc < now - LastSeenWindow))
|
||||
.ExecuteUpdateAsync(
|
||||
setters => setters.SetProperty(u => u.LastSeenAtUtc, now),
|
||||
cancellationToken)
|
||||
.ConfigureAwait(false);
|
||||
|
||||
if (written > 0)
|
||||
{
|
||||
await database.Entry(user).ReloadAsync(cancellationToken).ConfigureAwait(false);
|
||||
}
|
||||
}
|
||||
|
||||
private Task<UserAccount?> FindAsync(string issuer, string subject, CancellationToken cancellationToken) =>
|
||||
|
||||
@@ -80,6 +80,26 @@ internal sealed class DodoAuthorizationResultHandler : IAuthorizationMiddlewareR
|
||||
ArgumentNullException.ThrowIfNull(context);
|
||||
ArgumentNullException.ThrowIfNull(authorizeResult);
|
||||
|
||||
var accountNotActive = authorizeResult.Forbidden
|
||||
&& authorizeResult.AuthorizationFailure is { } refusal
|
||||
&& refusal.FailedRequirements.OfType<ActiveAccountRequirement>().Any();
|
||||
|
||||
if (accountNotActive)
|
||||
{
|
||||
await TypedResults.Problem(
|
||||
detail: "This account has been suspended. Ask whoever runs this DodoSSH server.",
|
||||
statusCode: StatusCodes.Status403Forbidden,
|
||||
type: ProblemCodes.TypeBaseUri + ProblemCodes.AccountNotActive,
|
||||
extensions: new Dictionary<string, object?>(StringComparer.Ordinal)
|
||||
{
|
||||
["code"] = ProblemCodes.AccountNotActive,
|
||||
})
|
||||
.ExecuteAsync(context)
|
||||
.ConfigureAwait(false);
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
var enrollmentMissing = authorizeResult.Forbidden
|
||||
&& authorizeResult.AuthorizationFailure is { } failure
|
||||
&& failure.FailedRequirements.OfType<EnrolledRequirement>().Any();
|
||||
|
||||
@@ -536,9 +536,21 @@ internal sealed class SyncService(
|
||||
|
||||
/// <summary>Attaches current row state to change-log entries.</summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// A page of changes can mix item types, so rows are loaded per type — one query each, not one per
|
||||
/// change — and then matched back in the log's order. Ordering is the log's, never the load's: a pull
|
||||
/// that returned changes grouped by type would hand the client a sequence its cursor cannot resume from.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// ◆ <b>An entry an item has since moved past is left out.</b> The log keeps every revision, but the
|
||||
/// table keeps only the latest ciphertext — so hydrating an old entry paired that entry's revision number
|
||||
/// with the newest payload, and the payload's AAD binds the revision it was sealed at. A client opened it
|
||||
/// at the old number, failed, and parked its own unsent edit to that item as undecryptable. The later
|
||||
/// entry is in this page or a later one, since the item's <c>ChangeSequence</c> is that entry's sequence;
|
||||
/// it carries the same payload under the right number, so nothing is lost by sending it once. Leaving
|
||||
/// the old entries out also stops a fresh device downloading an item once per edit it ever had. The
|
||||
/// cursor is computed from the unfiltered page by the caller, so it still moves past them.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
private async Task<List<Contracts.SyncChange>> HydrateAsync(
|
||||
Vault vault,
|
||||
@@ -571,10 +583,13 @@ internal sealed class SyncService(
|
||||
}
|
||||
}
|
||||
|
||||
return [.. changes.Select(change => Describe(
|
||||
change,
|
||||
kinds.GetValueOrDefault(change.EntityType),
|
||||
rows.GetValueOrDefault((change.EntityType, change.EntityId))))];
|
||||
return [.. changes
|
||||
.Where(change => rows.GetValueOrDefault((change.EntityType, change.EntityId)) is not { } row
|
||||
|| row.ChangeSequence <= change.Sequence)
|
||||
.Select(change => Describe(
|
||||
change,
|
||||
kinds.GetValueOrDefault(change.EntityType),
|
||||
rows.GetValueOrDefault((change.EntityType, change.EntityId))))];
|
||||
}
|
||||
|
||||
/// <summary>Turns one change-log entry and its row into what a client receives.</summary>
|
||||
|
||||
@@ -179,8 +179,8 @@ internal sealed class ArchiveTeamEndpoint(ICurrentUserContext currentUser, TeamS
|
||||
}
|
||||
|
||||
// Owner, not admin. An admin the owner promoted must not be able to archive the team out
|
||||
// from under them — that is the boundary IsOwner exists to draw. Nothing behind this
|
||||
// re-checks it, unlike the transfer below, so this line is the whole of the guard.
|
||||
// from under them — that is the boundary IsOwner exists to draw. TeamService.ArchiveAsync
|
||||
// checks it again, because deleting a team's last vault archives it too.
|
||||
if (!access.IsOwner)
|
||||
{
|
||||
return Problems.Coded(
|
||||
|
||||
@@ -244,8 +244,16 @@ internal sealed class TeamService(
|
||||
Team team,
|
||||
CancellationToken cancellationToken)
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(actor);
|
||||
ArgumentNullException.ThrowIfNull(team);
|
||||
|
||||
// Both callers check this first and answer the caller properly; this is what stops a third one that
|
||||
// forgets from letting an admin archive the team out from under its owner.
|
||||
if (!(await ResolveAsync(actor.Id, team.Id, cancellationToken).ConfigureAwait(false)).IsOwner)
|
||||
{
|
||||
throw new InvalidOperationException("Only the owner of a team can archive it.");
|
||||
}
|
||||
|
||||
var vaultCount = await CountVaultsAsync(team.Id, cancellationToken).ConfigureAwait(false);
|
||||
|
||||
if (vaultCount > 0)
|
||||
|
||||
@@ -191,7 +191,12 @@ internal sealed class DeleteVaultEndpoint(
|
||||
{
|
||||
var orphaned = await grants.DeleteVaultAsync(user, access.Vault!, ct).ConfigureAwait(false);
|
||||
|
||||
if (orphaned is not null)
|
||||
// Archived only by its owner. Archiving revokes every membership, the owner's included, and an
|
||||
// admin the owner promoted must not be able to do that by deleting vaults one at a time when the
|
||||
// archive endpoint refuses them outright. An admin's delete stands; the empty team is left for
|
||||
// the owner, who can archive it or make a vault in it.
|
||||
if (orphaned is not null
|
||||
&& (await teams.ResolveAsync(user.Id, orphaned.Id, ct).ConfigureAwait(false)).IsOwner)
|
||||
{
|
||||
await teams.ArchiveAsync(user, orphaned, ct).ConfigureAwait(false);
|
||||
}
|
||||
|
||||
@@ -55,6 +55,7 @@ builder.Services.AddDodoRelay();
|
||||
// Scoped rather than the AddAuthorization default of singleton: the handler reads the request's
|
||||
// DbContext, and a singleton would capture one for the lifetime of the process.
|
||||
builder.Services.AddScoped<IAuthorizationHandler, EnrolledHandler>();
|
||||
builder.Services.AddScoped<IAuthorizationHandler, ActiveAccountHandler>();
|
||||
|
||||
// Replaces the framework default, which is also registered as a singleton. Last registration wins.
|
||||
builder.Services.AddSingleton<IAuthorizationMiddlewareResultHandler, DodoAuthorizationResultHandler>();
|
||||
|
||||
@@ -62,7 +62,14 @@ internal static class Auth
|
||||
|
||||
private static void AddDodoPolicies(AuthorizationOptions options)
|
||||
{
|
||||
options.AddPolicy(AuthenticatedPolicy, policy => policy.RequireAuthenticatedUser());
|
||||
// Both policies also require the account to be active. A suspended or deprovisioned account still
|
||||
// holds a token the identity provider will vouch for until it expires — the status is this server's
|
||||
// own decision, so it is this server that has to refuse. See ActiveAccountHandler.
|
||||
options.AddPolicy(
|
||||
AuthenticatedPolicy,
|
||||
policy => policy
|
||||
.RequireAuthenticatedUser()
|
||||
.AddRequirements(new Authorization.ActiveAccountRequirement()));
|
||||
|
||||
// Enrollment state lives in the database, so this is satisfied by EnrolledHandler.
|
||||
// An unmet EnrolledRequirement is rewritten into a ProblemDetails carrying
|
||||
@@ -72,7 +79,7 @@ internal static class Auth
|
||||
EnrolledPolicy,
|
||||
policy => policy
|
||||
.RequireAuthenticatedUser()
|
||||
.AddRequirements(new Authorization.EnrolledRequirement()));
|
||||
.AddRequirements(new Authorization.ActiveAccountRequirement(), new Authorization.EnrolledRequirement()));
|
||||
|
||||
// Deny by default: an endpoint without an explicit policy still requires a caller.
|
||||
options.FallbackPolicy = options.GetPolicy(AuthenticatedPolicy);
|
||||
|
||||
@@ -28,6 +28,9 @@ public static class ProblemCodes
|
||||
/// <summary>The caller has not yet enrolled a public key, so no vault is reachable.</summary>
|
||||
public const string EnrollmentRequired = "enrollment-required";
|
||||
|
||||
/// <summary>The caller's account has been suspended or deprovisioned by an operator.</summary>
|
||||
public const string AccountNotActive = "account-not-active";
|
||||
|
||||
/// <summary>Enrollment was attempted for a user who already holds a different current key.</summary>
|
||||
public const string AlreadyEnrolled = "already-enrolled";
|
||||
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
#nullable enable
|
||||
const DodoSSH.Contracts.ProblemCodes.AlreadyEnrolled = "already-enrolled" -> string!
|
||||
const DodoSSH.Contracts.ProblemCodes.ClientTooOld = "client-too-old" -> string!
|
||||
const DodoSSH.Contracts.ProblemCodes.AccountNotActive = "account-not-active" -> string!
|
||||
const DodoSSH.Contracts.ProblemCodes.EnrollmentRequired = "enrollment-required" -> string!
|
||||
const DodoSSH.Contracts.ProblemCodes.EventsUnavailable = "events-unavailable" -> string!
|
||||
const DodoSSH.Contracts.ProblemCodes.Forbidden = "forbidden" -> string!
|
||||
|
||||
Reference in new issue
Block a user