Public Access
Let a team be joined only by somebody who is already here
An invitation decided access from an assertion about an address. Everything else
in this model decides it from something a person did — an admin naming an
account, a key holder wrapping a vault key to a key they verified — and this was
the one place a token's email claim was the thing that let somebody in.
It was guarded as tightly as that can be guarded: the claim was refused outright
on an unverified or absent `email_verified`, with no setting to relax it. But the
guard and the risk were the same shape. The whole defence was one boolean sent by
a system the deployment does not control.
So `POST /teams/{id}/members` is the only way in, and an address with no account
is refused with `no-such-account` — which is now the end of the road rather than
the signal to invite. Both clients say the remedy: that person signs in here
once, which is what creates the account, and then they can be added. The desktop
leaves the address in the box, because a message telling you to come back later
is one you act on later.
Gone with it: the `team_invitation` table, the claim hook in the sign-in path,
and `Oidc:EmailVerifiedClaim`, which that hook was the only reader of. Nothing in
the server now reads the email claim to decide anything.
Pending invitations are dropped rather than converted. Converting one would mean
creating a membership because an address matched, which is the property being
removed — and an invitation to an address that did have an account here had
already been claimed by the hourly sweep, so what is left is offers to people who
never arrived.
Two tests carry the property rather than the feature: the endpoint inventory
asserts the three routes are absent, and the API suite adds an address that has
no account, watches the refusal, then signs that address in and checks it joined
nothing. Without the second half, a server that merely renamed the deferred path
would pass.
This commit is contained in:
@@ -17,51 +17,27 @@ public interface ICurrentUserContext
|
||||
Task<UserAccount> GetOrProvisionAsync(CancellationToken cancellationToken);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Turns pending team invitations addressed to a verified email into memberships.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// Declared here, beside its only caller, and implemented in <c>Features/Teams</c>. The direction is
|
||||
/// deliberate: sign-in is what an invitation waits for, so the sign-in path names the shape it needs
|
||||
/// and the teams feature supplies it — rather than <see cref="ICurrentUserContext"/>, which every
|
||||
/// endpoint in the server depends on, growing a reference into one feature's folder.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public interface ITeamInvitationClaim
|
||||
{
|
||||
/// <summary>
|
||||
/// Claims every live invitation addressed to <paramref name="email"/> for this account.
|
||||
/// </summary>
|
||||
/// <param name="user">The account signing in.</param>
|
||||
/// <param name="email">The address the token asserted, or null if it asserted none.</param>
|
||||
/// <param name="emailVerified">
|
||||
/// Whether the provider marked that address verified. False refuses the claim outright and is the
|
||||
/// whole of what stops an invitation being taken by anybody able to assert somebody else's
|
||||
/// address.
|
||||
/// </param>
|
||||
/// <param name="cancellationToken">Cancellation.</param>
|
||||
/// <returns>How many invitations became memberships.</returns>
|
||||
Task<int> ClaimAsync(
|
||||
UserAccount user,
|
||||
string? email,
|
||||
bool emailVerified,
|
||||
CancellationToken cancellationToken);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Request-scoped caller identity with just-in-time provisioning.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// Identity is keyed on <c>(issuer, subject)</c>, never on email. Matching an existing account by
|
||||
/// email means anyone who can obtain a token bearing a victim's email address — from any configured
|
||||
/// provider — inherits that victim's vaults, so it is opt-in configuration and off by default.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Nothing here reads the email claim for authorization, and there is deliberately no hook left for
|
||||
/// anything that would.</b> This class used to claim pending team invitations on the way past, which
|
||||
/// made a provider's assertion about an address into a decision about who joins a team; invitations are
|
||||
/// gone and membership is granted only to an account somebody named — see
|
||||
/// <c>docs/adr/0009-team-access-model.md</c>. The address is still recorded, for display.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
internal sealed class CurrentUserContext(
|
||||
IHttpContextAccessor accessor,
|
||||
DodoDbContext database,
|
||||
IOptions<Setup.OidcOptions> oidcOptions,
|
||||
ITeamInvitationClaim invitations,
|
||||
TimeProvider clock)
|
||||
: ICurrentUserContext
|
||||
{
|
||||
@@ -73,8 +49,7 @@ internal sealed class CurrentUserContext(
|
||||
/// UPDATE on the hot path of every authenticated call and — because <c>user_account</c> carries
|
||||
/// the xmin concurrency token — would start losing races between a user's own overlapping
|
||||
/// requests. Writing it never is what made the old "last active" column impossible to offer
|
||||
/// honestly. An hour answers the question a colleague actually asks, which is "this week or not",
|
||||
/// and it is also the window on which a pending invitation is swept for.
|
||||
/// honestly. An hour answers the question a colleague actually asks, which is "this week or not".
|
||||
/// </remarks>
|
||||
private static readonly TimeSpan LastSeenWindow = TimeSpan.FromHours(1);
|
||||
|
||||
@@ -101,7 +76,6 @@ internal sealed class CurrentUserContext(
|
||||
var options = oidcOptions.Value;
|
||||
var email = principal.FindFirstValue(options.EmailClaim);
|
||||
var displayName = principal.FindFirstValue(options.NameClaim);
|
||||
var emailVerified = IsVerified(principal, options.EmailVerifiedClaim);
|
||||
|
||||
var existing = await FindAsync(issuer, subject, cancellationToken).ConfigureAwait(false);
|
||||
|
||||
@@ -110,52 +84,28 @@ internal sealed class CurrentUserContext(
|
||||
cached = await ProvisionAsync(issuer, subject, email, displayName, cancellationToken)
|
||||
.ConfigureAwait(false);
|
||||
|
||||
// A first sign-in is exactly what an invitation is waiting for, so it is claimed at once
|
||||
// rather than on the next hourly sweep — which would leave somebody staring at a team
|
||||
// list that does not yet contain the team they were told they had been added to.
|
||||
await invitations
|
||||
.ClaimAsync(cached, email, emailVerified, cancellationToken)
|
||||
.ConfigureAwait(false);
|
||||
|
||||
return cached;
|
||||
}
|
||||
|
||||
cached = existing;
|
||||
|
||||
await RefreshLastSeenAsync(existing, email, emailVerified, cancellationToken)
|
||||
.ConfigureAwait(false);
|
||||
await RefreshLastSeenAsync(existing, cancellationToken).ConfigureAwait(false);
|
||||
|
||||
return cached;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Records that this account is active, and sweeps for invitations it can now claim.
|
||||
/// Records that this account is active.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// The two are one operation because they want the same rate. Both are housekeeping nobody is
|
||||
/// waiting on, and doing them together costs one extra round trip per account per hour rather
|
||||
/// than two.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The sweep is what makes claiming recoverable rather than one-shot. A claim that failed at
|
||||
/// provisioning — or an invitation issued in the window between an account being created and this
|
||||
/// person next signing in — is picked up here instead of being stranded for ever.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <c>ExecuteUpdateAsync</c> rather than the change tracker, and the predicate rather than a
|
||||
/// 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>
|
||||
/// </remarks>
|
||||
private async Task RefreshLastSeenAsync(
|
||||
UserAccount user,
|
||||
string? email,
|
||||
bool emailVerified,
|
||||
CancellationToken cancellationToken)
|
||||
private async Task RefreshLastSeenAsync(UserAccount user, CancellationToken cancellationToken)
|
||||
{
|
||||
var now = clock.GetUtcNow();
|
||||
|
||||
@@ -171,20 +121,8 @@ internal sealed class CurrentUserContext(
|
||||
setters => setters.SetProperty(u => u.LastSeenAtUtc, now),
|
||||
cancellationToken)
|
||||
.ConfigureAwait(false);
|
||||
|
||||
await invitations
|
||||
.ClaimAsync(user, email, emailVerified, cancellationToken)
|
||||
.ConfigureAwait(false);
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// A JWT boolean arrives as the string "true", so this parses rather than compares against a
|
||||
/// constant. Anything else — absent, "false", or a value this does not understand — is false,
|
||||
/// because the failure that matters is treating an unverified address as verified.
|
||||
/// </remarks>
|
||||
private static bool IsVerified(ClaimsPrincipal principal, string claimType) =>
|
||||
bool.TryParse(principal.FindFirstValue(claimType), out var verified) && verified;
|
||||
|
||||
private Task<UserAccount?> FindAsync(string issuer, string subject, CancellationToken cancellationToken) =>
|
||||
database.Users.SingleOrDefaultAsync(
|
||||
u => u.Issuer == issuer && u.Subject == subject && u.DeletedAtUtc == null,
|
||||
|
||||
Reference in New Issue
Block a user