Public Access
Add the member the directory cannot see, rather than inviting them
ADD MEMBER quietly issued an invitation instead of adding anybody, for everyone who had signed in here and not yet enrolled. The screen told them that address had no account, the members list did not change, and the person only actually joined on the next hourly sweep. The client decided whether an address had an account by asking the public-key directory, and the directory answers a narrower question than that. It drops every account with no current key — deliberately, because an entry exists to be wrapped to and one carrying no key is a check a caller forgets exactly once. An account exists from its owner's first authenticated request and publishes nothing until they choose a passphrase on their own machine, so every account is missing from the directory for that whole window and some indefinitely. A miss there is not an absent account, and reading it as one was the bug. The server would have taken the add. TeamService.AddMemberAsync only requires the account row, and TeamMemberSummary.IsEnrolled exists precisely so a member with no key can be listed — added on Monday, enrolled on Tuesday. The client never asked. So the directory is still asked first and the miss is retried as an add by address, and only a server saying there is no such account reaches the invitation. AddTeamMemberRequest gained an Email used when UserId is empty. The lookup-first ordering is kept because it is load-bearing for sharing and not for this: the key verified before a vault key is wrapped is the one the lookup returned, and nothing is wrapped by adding somebody. That is why resolving the address server-side is safe here and would not be there. NoSuchAccount is its own code rather than folded into InvalidTeam, because it is the one add failure the caller can act on unprompted — there is nobody to add, so invite them — and a code shared with a rejected role would leave them guessing which had happened. It does answer whether an address has an account here, which CreateTeamInvitationRequest deliberately does not. That is the property traded for the fix; the exposure is bounded by the admin check the add already needed, and it is the same fact the member list shows a moment later. Adding by a user id that does not exist now answers 404 no-such-account rather than 400 invalid-team, and nothing depended on the old pairing. The two silent returns are gone. Offline and no-team-selected set nothing and returned, so those failures were visible only as a flicker of the busy flag — which reads as a button that does nothing at all. The success line reads the enrollment flag too, because pointing an unenrolled member at SHARE KEY is pointing at a button that will refuse; their row already says it holds no key. Why nothing caught it. FakeVaultServer had one list, so it could not tell an account that does not exist from one that exists and has not enrolled — the distinction this whole path turns on — and every account it knew was enrolled by construction. It grows an accounts list beside the directory and reports IsEnrolled from whether the directory has them, rather than hardcoding true. The regression test asserts Invitations is empty, which is what fails against the old behaviour. Four tests: that pair in the shell suite, and in the API suite an unenrolled account added by address after its own directory lookup comes back empty, and an unknown address refused under the new code. 1495 tests pass.
This commit is contained in:
@@ -356,6 +356,13 @@ internal sealed class AddTeamMemberEndpoint(ICurrentUserContext currentUser, Tea
|
|||||||
return Problems.Coded(
|
return Problems.Coded(
|
||||||
StatusCodes.Status400BadRequest, ProblemCodes.InvalidTeam, exception.Message);
|
StatusCodes.Status400BadRequest, ProblemCodes.InvalidTeam, exception.Message);
|
||||||
}
|
}
|
||||||
|
catch (NoSuchAccountException exception)
|
||||||
|
{
|
||||||
|
// Its own code so the caller can offer an invitation rather than report a failure. 404
|
||||||
|
// rather than 400: the request was well formed and named something that is not here.
|
||||||
|
return Problems.Coded(
|
||||||
|
StatusCodes.Status404NotFound, ProblemCodes.NoSuchAccount, exception.Message);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -34,6 +34,14 @@ internal sealed class LastTeamOwnerException(string message) : Exception(message
|
|||||||
/// </remarks>
|
/// </remarks>
|
||||||
internal sealed class TeamNotEmptyException(string message) : Exception(message);
|
internal sealed class TeamNotEmptyException(string message) : Exception(message);
|
||||||
|
|
||||||
|
/// <summary>The address given to an add has no account on this server.</summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// Separate from <see cref="TeamInvalidException"/> because the caller can act on it without being
|
||||||
|
/// told to: there is nobody to add, so the address is invited instead. Folded into the general code it
|
||||||
|
/// would be indistinguishable from a rejected role, and a client would have to guess which it was.
|
||||||
|
/// </remarks>
|
||||||
|
internal sealed class NoSuchAccountException(string message) : Exception(message);
|
||||||
|
|
||||||
/// <summary>An invitation was rejected.</summary>
|
/// <summary>An invitation was rejected.</summary>
|
||||||
/// <remarks>
|
/// <remarks>
|
||||||
/// Separate from <see cref="TeamInvalidException"/> because its commonest cause has a different
|
/// Separate from <see cref="TeamInvalidException"/> because its commonest cause has a different
|
||||||
|
|||||||
@@ -484,6 +484,12 @@ internal sealed class TeamService(
|
|||||||
/// restore their revoked key grants: those were wrapped to a generation the vault has since been
|
/// restore their revoked key grants: those were wrapped to a generation the vault has since been
|
||||||
/// flagged to leave behind, and a member holding Share has to wrap the key afresh.
|
/// flagged to leave behind, and a member holding Share has to wrap the key afresh.
|
||||||
/// </para>
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// Enrollment is not required of the account being added, and asking for it would be asking the
|
||||||
|
/// wrong question. A membership is authorization and grants nothing readable — that is the whole
|
||||||
|
/// of ADR 0009 — so somebody can be added on Monday and publish a key on Tuesday, which is what
|
||||||
|
/// <c>TeamMemberSummary.IsEnrolled</c> is for.
|
||||||
|
/// </para>
|
||||||
/// </remarks>
|
/// </remarks>
|
||||||
internal async Task<TeamMemberSummary> AddMemberAsync(
|
internal async Task<TeamMemberSummary> AddMemberAsync(
|
||||||
UserAccount actor,
|
UserAccount actor,
|
||||||
@@ -500,18 +506,7 @@ internal sealed class TeamService(
|
|||||||
+ "by adding somebody.");
|
+ "by adding somebody.");
|
||||||
}
|
}
|
||||||
|
|
||||||
var target = await database.Users
|
var target = await ResolveTargetAsync(request, cancellationToken).ConfigureAwait(false);
|
||||||
.SingleOrDefaultAsync(
|
|
||||||
u => u.Id == request.UserId && u.DeletedAtUtc == null,
|
|
||||||
cancellationToken)
|
|
||||||
.ConfigureAwait(false)
|
|
||||||
|
|
||||||
// Safe to be specific: the caller supplied this id from a directory lookup they just
|
|
||||||
// made, so it confirms nothing they did not already know.
|
|
||||||
?? throw new TeamInvalidException(
|
|
||||||
"No such account on this server. A member has to sign in here once before they can "
|
|
||||||
+ "be added — that is what creates the account and publishes the key a vault would "
|
|
||||||
+ "be shared with.");
|
|
||||||
|
|
||||||
var now = clock.GetUtcNow();
|
var now = clock.GetUtcNow();
|
||||||
|
|
||||||
@@ -551,6 +546,66 @@ internal sealed class TeamService(
|
|||||||
return await DescribeAsync(target, membership, cancellationToken).ConfigureAwait(false);
|
return await DescribeAsync(target, membership, cancellationToken).ConfigureAwait(false);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Finds the account an add is aimed at, by id when the caller has one and by address otherwise.
|
||||||
|
/// </summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// <para>
|
||||||
|
/// The two are not interchangeable and the order matters. An id came from a directory lookup the
|
||||||
|
/// caller has already made, so it names an account whose key they have seen; an address is what is
|
||||||
|
/// left when the directory could not answer, which it cannot for anybody who has signed in but not
|
||||||
|
/// yet published a key.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// Only the active, undeleted account matters here, and the address is matched the way the
|
||||||
|
/// directory matches it — the email column is citext, so the comparison is case-insensitive in the
|
||||||
|
/// database and the partial unique index on it means at most one row can answer.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// Both misses are specific, and neither is a new oracle. An id confirms nothing the caller did not
|
||||||
|
/// already know from the lookup that produced it. An address is answered only for an admin or owner
|
||||||
|
/// of the team the add names — checked by the endpoint before this runs — and is the same fact the
|
||||||
|
/// member list would show them a moment later. It carries its own code so the caller can invite the
|
||||||
|
/// address instead of reporting a failure at somebody who simply is not here yet.
|
||||||
|
/// </para>
|
||||||
|
/// </remarks>
|
||||||
|
private async Task<UserAccount> ResolveTargetAsync(
|
||||||
|
AddTeamMemberRequest request,
|
||||||
|
CancellationToken cancellationToken)
|
||||||
|
{
|
||||||
|
if (request.UserId != Guid.Empty)
|
||||||
|
{
|
||||||
|
return await database.Users
|
||||||
|
.SingleOrDefaultAsync(
|
||||||
|
u => u.Id == request.UserId && u.DeletedAtUtc == null,
|
||||||
|
cancellationToken)
|
||||||
|
.ConfigureAwait(false)
|
||||||
|
|
||||||
|
?? throw new NoSuchAccountException(
|
||||||
|
"No such account on this server. A member has to sign in here once before they "
|
||||||
|
+ "can be added — that is what creates the account.");
|
||||||
|
}
|
||||||
|
|
||||||
|
var email = (request.Email ?? string.Empty).Trim();
|
||||||
|
|
||||||
|
if (email.Length == 0)
|
||||||
|
{
|
||||||
|
throw new TeamInvalidException(
|
||||||
|
"Say who to add: either a user id from the directory, or the email address they sign "
|
||||||
|
+ "in with.");
|
||||||
|
}
|
||||||
|
|
||||||
|
return await database.Users
|
||||||
|
.SingleOrDefaultAsync(
|
||||||
|
u => u.Email == email && u.DeletedAtUtc == null && u.Status == UserStatus.Active,
|
||||||
|
cancellationToken)
|
||||||
|
.ConfigureAwait(false)
|
||||||
|
|
||||||
|
?? throw new NoSuchAccountException(
|
||||||
|
"No account here uses that address yet. Invite it instead — they join when they "
|
||||||
|
+ "first sign in.");
|
||||||
|
}
|
||||||
|
|
||||||
/// <remarks>
|
/// <remarks>
|
||||||
/// Enrollment is looked up rather than inferred, because it is the one field on a member row that
|
/// Enrollment is looked up rather than inferred, because it is the one field on a member row that
|
||||||
/// is about them and not about the membership: somebody can be added on Monday and set their
|
/// is about them and not about the membership: somebody can be added on Monday and set their
|
||||||
|
|||||||
@@ -470,16 +470,31 @@ internal sealed partial class TeamsViewModel(
|
|||||||
/// Adds a member, by looking their address up in the directory first.
|
/// Adds a member, by looking their address up in the directory first.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
/// <remarks>
|
/// <remarks>
|
||||||
/// Two calls rather than one, and the order is the point: the directory is what turns an address into
|
/// <para>
|
||||||
/// an account and a public key, and the key that gets verified before any sharing is the one that
|
/// The directory is asked first, and the order is the point: it is what turns an address into an
|
||||||
/// lookup returned. Letting the server resolve an address to an account inside the add would put an
|
/// account <em>and a public key</em>, and the key that gets verified before any sharing is the one
|
||||||
/// unwitnessed step between the two.
|
/// that lookup returned. Resolving the address server-side when the directory could answer would
|
||||||
|
/// put an unwitnessed step between the two.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// <b>A directory miss is not an absent account, and treating it as one was a bug worth naming.</b>
|
||||||
|
/// The directory returns only accounts that have published a key, so everybody between their first
|
||||||
|
/// sign-in and their enrollment is missing from it. Falling straight through to an invitation told
|
||||||
|
/// somebody who was standing right there that they had no account here, left the members list
|
||||||
|
/// unchanged, and made them wait for a sweep that runs at most hourly. So the miss is retried as an
|
||||||
|
/// add by address, and only a server that says there is no such account reaches the invitation.
|
||||||
|
/// </para>
|
||||||
/// </remarks>
|
/// </remarks>
|
||||||
[RelayCommand]
|
[RelayCommand]
|
||||||
private async Task AddMemberAsync(CancellationToken cancellationToken)
|
private async Task AddMemberAsync(CancellationToken cancellationToken)
|
||||||
{
|
{
|
||||||
if (connection() is not { } server || SelectedTeam is not { } team)
|
if (connection() is not { } server || SelectedTeam is not { } team)
|
||||||
{
|
{
|
||||||
|
// Never silent. This command's failures used to be visible only as a flicker of the busy
|
||||||
|
// flag, which reads as a button that does nothing at all.
|
||||||
|
Status = connection() is null
|
||||||
|
? "Offline. Adding a member changes who the server will serve, so it needs a connection."
|
||||||
|
: "Select a team on the left first — a member is added to one team, not to all of them.";
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -496,30 +511,59 @@ internal sealed partial class TeamsViewModel(
|
|||||||
var found = await server.Directory.LookupByEmailAsync(email, cancellationToken)
|
var found = await server.Directory.LookupByEmailAsync(email, cancellationToken)
|
||||||
.ConfigureAwait(true);
|
.ConfigureAwait(true);
|
||||||
|
|
||||||
if (found.Count == 0)
|
var request = found.Count > 0
|
||||||
|
? new AddTeamMemberRequest(found[0].UserId, NewMemberRole)
|
||||||
|
: new AddTeamMemberRequest(Guid.Empty, NewMemberRole, email);
|
||||||
|
|
||||||
|
TeamMemberSummary member;
|
||||||
|
|
||||||
|
try
|
||||||
{
|
{
|
||||||
|
member = await server.Teams
|
||||||
|
.AddTeamMemberAsync(team.TeamId, request, cancellationToken)
|
||||||
|
.ConfigureAwait(true);
|
||||||
|
}
|
||||||
|
catch (DodoSshApiException exception)
|
||||||
|
when (string.Equals(
|
||||||
|
exception.Code, ProblemCodes.NoSuchAccount, StringComparison.Ordinal))
|
||||||
|
{
|
||||||
|
// The address really is unknown here, which only the server can say. This is the one
|
||||||
|
// route to an invitation, and it is now a fact rather than an inference from silence.
|
||||||
await InviteAsync(server, team, email, cancellationToken).ConfigureAwait(true);
|
await InviteAsync(server, team, email, cancellationToken).ConfigureAwait(true);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
var member = await server.Teams
|
|
||||||
.AddTeamMemberAsync(
|
|
||||||
team.TeamId,
|
|
||||||
new AddTeamMemberRequest(found[0].UserId, NewMemberRole),
|
|
||||||
cancellationToken)
|
|
||||||
.ConfigureAwait(true);
|
|
||||||
|
|
||||||
InviteEmail = string.Empty;
|
InviteEmail = string.Empty;
|
||||||
|
|
||||||
await ReloadAsync(cancellationToken).ConfigureAwait(true);
|
await ReloadAsync(cancellationToken).ConfigureAwait(true);
|
||||||
|
|
||||||
// Said out loud, every time. The single most common misunderstanding this design invites is
|
Status = Describe(member);
|
||||||
// that adding somebody gave them the vault.
|
|
||||||
Status = $"Added {member.Email ?? member.DisplayName ?? "the account"} as a member. They "
|
|
||||||
+ "cannot read anything yet — select a vault below and share its key.";
|
|
||||||
}).ConfigureAwait(true);
|
}).ConfigureAwait(true);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// What just happened to the account that was added, and what is still owed them.
|
||||||
|
/// </summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// Both branches say out loud that nothing readable was granted, because the single most common
|
||||||
|
/// misunderstanding this design invites is that adding somebody gave them the vault. The unenrolled
|
||||||
|
/// branch says more, and has to: their row will sit in the list saying it holds no key, and without
|
||||||
|
/// this somebody would read that as the addition having half-failed rather than as a colleague who
|
||||||
|
/// has not finished setting their machine up. It is also the one case where SHARE KEY cannot be the
|
||||||
|
/// next step, so pointing at it would be pointing at a button that will refuse.
|
||||||
|
/// </remarks>
|
||||||
|
private static string Describe(TeamMemberSummary member)
|
||||||
|
{
|
||||||
|
var who = member.Email ?? member.DisplayName ?? "the account";
|
||||||
|
|
||||||
|
return member.IsEnrolled
|
||||||
|
? $"Added {who} as a member. They cannot read anything yet — select a vault below and "
|
||||||
|
+ "share its key."
|
||||||
|
: $"Added {who} as a member. They have no key yet, so their row says so and no vault can "
|
||||||
|
+ "be shared with them until they finish signing in on their own machine. The "
|
||||||
|
+ "membership is real in the meantime.";
|
||||||
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Invites an address the directory does not know.
|
/// Invites an address the directory does not know.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
@@ -531,6 +575,11 @@ internal sealed partial class TeamsViewModel(
|
|||||||
/// is reported afterwards, because the difference decides what they have to do next.
|
/// is reported afterwards, because the difference decides what they have to do next.
|
||||||
/// </para>
|
/// </para>
|
||||||
/// <para>
|
/// <para>
|
||||||
|
/// It is reached only after the server has said there is no such account. The directory's silence
|
||||||
|
/// is not enough and never was: it omits everybody who has not published a key, so inviting on the
|
||||||
|
/// strength of it told people with accounts that they had none.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
/// The message has to carry the whole mechanism. Nothing is sent — this server has no outbound
|
/// The message has to carry the whole mechanism. Nothing is sent — this server has no outbound
|
||||||
/// mail — so somebody who reads "invited" and waits has been misled by an interface that knew
|
/// mail — so somebody who reads "invited" and waits has been misled by an interface that knew
|
||||||
/// better.
|
/// better.
|
||||||
|
|||||||
@@ -119,6 +119,25 @@ public static class ProblemCodes
|
|||||||
/// </remarks>
|
/// </remarks>
|
||||||
public const string TeamNotEmpty = "team-not-empty";
|
public const string TeamNotEmpty = "team-not-empty";
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// An address given to <c>POST /api/v1/teams/{teamId}/members</c> has no account on this server.
|
||||||
|
/// </summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// <para>
|
||||||
|
/// Its own code rather than folded into <see cref="InvalidTeam"/> because it is the one add failure
|
||||||
|
/// with a remedy the client can take unprompted: there is nobody to add, so invite the address
|
||||||
|
/// instead. A client that could not tell this apart from a rejected role would have to either
|
||||||
|
/// invite on every failure or never.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// It answers whether an address has an account here, which <c>CreateTeamInvitationRequest</c>
|
||||||
|
/// deliberately does not. The exposure is bounded by the same authorization the add already needs —
|
||||||
|
/// only an admin or owner of the team reaches it — and it is what the caller learns anyway the
|
||||||
|
/// moment the account appears in the member list.
|
||||||
|
/// </para>
|
||||||
|
/// </remarks>
|
||||||
|
public const string NoSuchAccount = "no-such-account";
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// An invitation was rejected: a malformed address, an unknown or ownership role, an expiry the
|
/// An invitation was rejected: a malformed address, an unknown or ownership role, an expiry the
|
||||||
/// server will not issue, or an address that already has an account here.
|
/// server will not issue, or an address that already has an account here.
|
||||||
|
|||||||
@@ -13,6 +13,7 @@ const DodoSSH.Contracts.ProblemCodes.InvalidTeamInvitation = "invalid-team-invit
|
|||||||
const DodoSSH.Contracts.ProblemCodes.InvalidVaultGrant = "invalid-vault-grant" -> string!
|
const DodoSSH.Contracts.ProblemCodes.InvalidVaultGrant = "invalid-vault-grant" -> string!
|
||||||
const DodoSSH.Contracts.ProblemCodes.LastTeamOwner = "last-team-owner" -> string!
|
const DodoSSH.Contracts.ProblemCodes.LastTeamOwner = "last-team-owner" -> string!
|
||||||
const DodoSSH.Contracts.ProblemCodes.MalformedRequest = "malformed-request" -> string!
|
const DodoSSH.Contracts.ProblemCodes.MalformedRequest = "malformed-request" -> string!
|
||||||
|
const DodoSSH.Contracts.ProblemCodes.NoSuchAccount = "no-such-account" -> string!
|
||||||
const DodoSSH.Contracts.ProblemCodes.PushBatchTooLarge = "push-batch-too-large" -> string!
|
const DodoSSH.Contracts.ProblemCodes.PushBatchTooLarge = "push-batch-too-large" -> string!
|
||||||
const DodoSSH.Contracts.ProblemCodes.RelayLimitReached = "relay-limit-reached" -> string!
|
const DodoSSH.Contracts.ProblemCodes.RelayLimitReached = "relay-limit-reached" -> string!
|
||||||
const DodoSSH.Contracts.ProblemCodes.RelayTargetRejected = "relay-target-rejected" -> string!
|
const DodoSSH.Contracts.ProblemCodes.RelayTargetRejected = "relay-target-rejected" -> string!
|
||||||
@@ -23,8 +24,10 @@ const DodoSSH.Contracts.ProblemCodes.TypeBaseUri = "https://dodossh.dev/problems
|
|||||||
const DodoSSH.Contracts.ProblemCodes.VaultConflict = "vault-conflict" -> string!
|
const DodoSSH.Contracts.ProblemCodes.VaultConflict = "vault-conflict" -> string!
|
||||||
DodoSSH.Contracts.AddTeamMemberRequest
|
DodoSSH.Contracts.AddTeamMemberRequest
|
||||||
DodoSSH.Contracts.AddTeamMemberRequest.<Clone>$() -> DodoSSH.Contracts.AddTeamMemberRequest!
|
DodoSSH.Contracts.AddTeamMemberRequest.<Clone>$() -> DodoSSH.Contracts.AddTeamMemberRequest!
|
||||||
DodoSSH.Contracts.AddTeamMemberRequest.AddTeamMemberRequest(System.Guid UserId, DodoSSH.Contracts.TeamMemberRole Role) -> void
|
DodoSSH.Contracts.AddTeamMemberRequest.AddTeamMemberRequest(System.Guid UserId, DodoSSH.Contracts.TeamMemberRole Role, string? Email = null) -> void
|
||||||
DodoSSH.Contracts.AddTeamMemberRequest.Deconstruct(out System.Guid UserId, out DodoSSH.Contracts.TeamMemberRole Role) -> void
|
DodoSSH.Contracts.AddTeamMemberRequest.Deconstruct(out System.Guid UserId, out DodoSSH.Contracts.TeamMemberRole Role, out string? Email) -> void
|
||||||
|
DodoSSH.Contracts.AddTeamMemberRequest.Email.get -> string?
|
||||||
|
DodoSSH.Contracts.AddTeamMemberRequest.Email.init -> void
|
||||||
DodoSSH.Contracts.AddTeamMemberRequest.Equals(DodoSSH.Contracts.AddTeamMemberRequest? other) -> bool
|
DodoSSH.Contracts.AddTeamMemberRequest.Equals(DodoSSH.Contracts.AddTeamMemberRequest? other) -> bool
|
||||||
DodoSSH.Contracts.AddTeamMemberRequest.Role.get -> DodoSSH.Contracts.TeamMemberRole
|
DodoSSH.Contracts.AddTeamMemberRequest.Role.get -> DodoSSH.Contracts.TeamMemberRole
|
||||||
DodoSSH.Contracts.AddTeamMemberRequest.Role.init -> void
|
DodoSSH.Contracts.AddTeamMemberRequest.Role.init -> void
|
||||||
|
|||||||
@@ -210,14 +210,43 @@ public sealed record TeamMemberSummary(
|
|||||||
|
|
||||||
/// <summary>Adds a member to a team.</summary>
|
/// <summary>Adds a member to a team.</summary>
|
||||||
/// <remarks>
|
/// <remarks>
|
||||||
/// By user id rather than by email, and the id comes from a directory lookup the caller has already
|
/// <para>
|
||||||
/// made. That ordering is not incidental: whoever adds a member is usually about to wrap a vault key
|
/// <b>By user id when the caller has one, and the id comes from a directory lookup they have already
|
||||||
/// to their public key, and the key they must verify is the one the directory returned. Adding by
|
/// made.</b> That ordering is not incidental: whoever adds a member is usually about to wrap a vault
|
||||||
/// email here would put an account resolution the client never saw between those two steps.
|
/// key to their public key, and the key they must verify is the one the directory returned. Resolving
|
||||||
|
/// an address server-side when an id was available would put an account resolution the client never
|
||||||
|
/// saw between those two steps.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// <b><see cref="Email"/> exists because the directory cannot answer for everybody.</b> It returns
|
||||||
|
/// only accounts that have published a key — an entry exists to be wrapped to, and one carrying no key
|
||||||
|
/// is a check callers forget exactly once — so an account between its first sign-in and its enrollment
|
||||||
|
/// is invisible there. It is still an account, and it can still be a member: membership is server-side
|
||||||
|
/// authorization and grants nothing readable, which is why <see cref="TeamMemberSummary.IsEnrolled"/>
|
||||||
|
/// exists to say that a member has no key yet. Without this field such a person could not be added at
|
||||||
|
/// all, and a caller reading the directory's silence as "no account here" would invite an address that
|
||||||
|
/// already has one.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// No key is verified on this path, and none needs to be: nothing is wrapped by adding somebody. The
|
||||||
|
/// key that matters is fetched and checked at share time, from the directory, by the machine holding
|
||||||
|
/// the vault key.
|
||||||
|
/// </para>
|
||||||
/// </remarks>
|
/// </remarks>
|
||||||
/// <param name="UserId">The account to add, as returned by the directory.</param>
|
/// <param name="UserId">
|
||||||
|
/// The account to add, as returned by the directory. <see cref="Guid.Empty"/> defers to
|
||||||
|
/// <see cref="Email"/>.
|
||||||
|
/// </param>
|
||||||
/// <param name="Role">Role to grant.</param>
|
/// <param name="Role">Role to grant.</param>
|
||||||
public sealed record AddTeamMemberRequest(Guid UserId, TeamMemberRole Role);
|
/// <param name="Email">
|
||||||
|
/// The address to resolve, used only when <see cref="UserId"/> is <see cref="Guid.Empty"/>. Matched
|
||||||
|
/// case-insensitively, exactly as the directory matches. An address with no account here is refused
|
||||||
|
/// with <see cref="ProblemCodes.NoSuchAccount"/> so the caller can offer an invitation instead.
|
||||||
|
/// </param>
|
||||||
|
public sealed record AddTeamMemberRequest(
|
||||||
|
Guid UserId,
|
||||||
|
TeamMemberRole Role,
|
||||||
|
string? Email = null);
|
||||||
|
|
||||||
/// <summary>Changes a member's role.</summary>
|
/// <summary>Changes a member's role.</summary>
|
||||||
/// <param name="Role">The new role.</param>
|
/// <param name="Role">The new role.</param>
|
||||||
|
|||||||
@@ -450,6 +450,69 @@ public sealed class TeamEndpointTests(ApiFixture fixture)
|
|||||||
vault.TeamId.ShouldBe(team.TeamId);
|
vault.TeamId.ShouldBe(team.TeamId);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <remarks>
|
||||||
|
/// <para>
|
||||||
|
/// An account exists from its owner's first authenticated request and publishes no key until they
|
||||||
|
/// enroll, and the directory omits it for that whole window — deliberately, because an entry exists
|
||||||
|
/// to be wrapped to. So the lookup is asserted empty first: that is not a missing account, and a
|
||||||
|
/// caller that read it as one would invite an address that already has one.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// Adding by address is what covers the gap, and the member it produces says <c>IsEnrolled</c>
|
||||||
|
/// false. Membership is authorization and grants nothing readable, so there is nothing inconsistent
|
||||||
|
/// about a member with no key — it is the state everybody passes through.
|
||||||
|
/// </para>
|
||||||
|
/// </remarks>
|
||||||
|
[Fact]
|
||||||
|
public async Task AnAccountThatHasNotEnrolled_CanBeAddedByAddressThoughTheDirectoryOmitsIt()
|
||||||
|
{
|
||||||
|
var owner = await EnrolledClientAsync("unenrolled-owner", "uowner@example.com");
|
||||||
|
|
||||||
|
var address = NewAddress();
|
||||||
|
var (_, userId) = await SignInAsync(address);
|
||||||
|
|
||||||
|
var team = await CreateTeamAsync(owner, "Newcomers");
|
||||||
|
|
||||||
|
var found = await ReadAsync<IReadOnlyList<DirectoryEntry>>(
|
||||||
|
owner, $"/api/v1/directory?email={Uri.EscapeDataString(address)}");
|
||||||
|
|
||||||
|
found.ShouldBeEmpty("they have published no key, so there is nothing to wrap to");
|
||||||
|
|
||||||
|
var member = await PostAsync<AddTeamMemberRequest, TeamMemberSummary>(
|
||||||
|
owner,
|
||||||
|
MembersUrl(team.TeamId),
|
||||||
|
new AddTeamMemberRequest(Guid.Empty, TeamMemberRole.Member, address));
|
||||||
|
|
||||||
|
member.UserId.ShouldBe(userId);
|
||||||
|
member.IsEnrolled.ShouldBeFalse();
|
||||||
|
member.Role.ShouldBe(TeamMemberRole.Member);
|
||||||
|
|
||||||
|
var listed = await ReadAsync<IReadOnlyList<TeamMemberSummary>>(
|
||||||
|
owner, MembersUrl(team.TeamId));
|
||||||
|
|
||||||
|
listed.ShouldContain(row => row.UserId == userId && !row.IsEnrolled);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <remarks>
|
||||||
|
/// The other side of it. An address with no account is refused under its own code rather than the
|
||||||
|
/// general one, because the caller can act on it unprompted — there is nobody to add, so invite
|
||||||
|
/// them — and a code shared with a rejected role would leave them guessing which had happened.
|
||||||
|
/// </remarks>
|
||||||
|
[Fact]
|
||||||
|
public async Task AddingAnAddressWithNoAccount_IsRefusedWithItsOwnCode()
|
||||||
|
{
|
||||||
|
var owner = await EnrolledClientAsync("no-account-owner", "naowner@example.com");
|
||||||
|
|
||||||
|
var team = await CreateTeamAsync(owner, "Nobody");
|
||||||
|
|
||||||
|
var response = await owner.PostContractAsync(
|
||||||
|
MembersUrl(team.TeamId),
|
||||||
|
new AddTeamMemberRequest(Guid.Empty, TeamMemberRole.Member, NewAddress()));
|
||||||
|
|
||||||
|
await ShouldBeProblemAsync(
|
||||||
|
response, HttpStatusCode.NotFound, ProblemCodes.NoSuchAccount);
|
||||||
|
}
|
||||||
|
|
||||||
/// <remarks>
|
/// <remarks>
|
||||||
/// A viewer may read the vault and may not write to it. The failure this guards is the quiet one: a
|
/// A viewer may read the vault and may not write to it. The failure this guards is the quiet one: a
|
||||||
/// role that resolved to the wrong flags would let somebody who was added to look at a vault change
|
/// role that resolved to the wrong flags would let somebody who was added to look at a vault change
|
||||||
|
|||||||
@@ -31,6 +31,17 @@ internal sealed partial class FakeVaultServer : ITeamApi, IDirectoryApi, IVaultG
|
|||||||
private readonly List<DirectoryEntry> directory = [];
|
private readonly List<DirectoryEntry> directory = [];
|
||||||
private readonly Dictionary<Guid, List<TeamInvitationSummary>> invitations = [];
|
private readonly Dictionary<Guid, List<TeamInvitationSummary>> invitations = [];
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Every account on this fake server, enrolled or not.
|
||||||
|
/// </summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// Kept apart from <see cref="directory"/> because the real server keeps them apart, and the gap
|
||||||
|
/// between the two is where a real bug lived: the directory omits anybody who has not published a
|
||||||
|
/// key, so a fake that had only one list could not tell an account that does not exist from one
|
||||||
|
/// that exists and has not enrolled — which is exactly the distinction the add path turns on.
|
||||||
|
/// </remarks>
|
||||||
|
private readonly List<(Guid UserId, string Email, string DisplayName)> accounts = [];
|
||||||
|
|
||||||
/// <inheritdoc />
|
/// <inheritdoc />
|
||||||
public ITeamApi Teams => this;
|
public ITeamApi Teams => this;
|
||||||
|
|
||||||
@@ -75,6 +86,27 @@ internal sealed partial class FakeVaultServer : ITeamApi, IDirectoryApi, IVaultG
|
|||||||
KeyGeneration: 1,
|
KeyGeneration: 1,
|
||||||
KeyLogSequence: sequence));
|
KeyLogSequence: sequence));
|
||||||
|
|
||||||
|
accounts.Add((userId, email, displayName));
|
||||||
|
|
||||||
|
return userId;
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Registers an account that has signed in here but has not enrolled a key.
|
||||||
|
/// </summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// Normal rather than exotic: an account exists from its owner's first authenticated request and
|
||||||
|
/// stays keyless until they choose a passphrase on their own machine. It is absent from the
|
||||||
|
/// directory throughout, because a directory entry exists to be wrapped to and this one has nothing
|
||||||
|
/// to wrap. It can still be made a member — membership grants nothing readable.
|
||||||
|
/// </remarks>
|
||||||
|
/// <returns>Their user id.</returns>
|
||||||
|
internal Guid AddUnenrolledAccount(string email, string displayName)
|
||||||
|
{
|
||||||
|
var userId = Guid.CreateVersion7();
|
||||||
|
|
||||||
|
accounts.Add((userId, email, displayName));
|
||||||
|
|
||||||
return userId;
|
return userId;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -241,27 +273,40 @@ internal sealed partial class FakeVaultServer : ITeamApi, IDirectoryApi, IVaultG
|
|||||||
return members.TryGetValue(teamId, out var list) ? [.. list] : [];
|
return members.TryGetValue(teamId, out var list) ? [.. list] : [];
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <inheritdoc />
|
/// <summary>Adds a member, resolved by id when the caller has one and by address otherwise.</summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// Resolved against <see cref="accounts"/> rather than <see cref="directory"/>, which is the whole
|
||||||
|
/// point of the two being separate here: an account with no published key is missing from the
|
||||||
|
/// directory and is still perfectly addable. <c>IsEnrolled</c> is reported from whether the
|
||||||
|
/// directory has them rather than hardcoded, so a member row can say it holds no key.
|
||||||
|
/// </remarks>
|
||||||
public Task<TeamMemberSummary> AddTeamMemberAsync(
|
public Task<TeamMemberSummary> AddTeamMemberAsync(
|
||||||
Guid teamId,
|
Guid teamId,
|
||||||
AddTeamMemberRequest request,
|
AddTeamMemberRequest request,
|
||||||
CancellationToken cancellationToken)
|
CancellationToken cancellationToken)
|
||||||
{
|
{
|
||||||
var entry = directory.Find(candidate => candidate.UserId == request.UserId)
|
var account = request.UserId != Guid.Empty
|
||||||
?? throw new DodoSshApiException(
|
? accounts.Find(candidate => candidate.UserId == request.UserId)
|
||||||
System.Net.HttpStatusCode.BadRequest,
|
: accounts.Find(candidate => string.Equals(
|
||||||
ProblemCodes.InvalidTeam,
|
candidate.Email, request.Email, StringComparison.OrdinalIgnoreCase));
|
||||||
|
|
||||||
|
if (account.UserId == Guid.Empty)
|
||||||
|
{
|
||||||
|
throw new DodoSshApiException(
|
||||||
|
System.Net.HttpStatusCode.NotFound,
|
||||||
|
ProblemCodes.NoSuchAccount,
|
||||||
"No such account on this server.");
|
"No such account on this server.");
|
||||||
|
}
|
||||||
|
|
||||||
// LastActiveAt is left null: this account has been added, not seen. The owner's row carries a
|
// LastActiveAt is left null: this account has been added, not seen. The owner's row carries a
|
||||||
// real one, so both branches of the interface's "last active / never" split are exercised.
|
// real one, so both branches of the interface's "last active / never" split are exercised.
|
||||||
var member = new TeamMemberSummary(
|
var member = new TeamMemberSummary(
|
||||||
entry.UserId,
|
account.UserId,
|
||||||
entry.Email,
|
account.Email,
|
||||||
entry.DisplayName,
|
account.DisplayName,
|
||||||
request.Role,
|
request.Role,
|
||||||
TeamMemberStatus.Active,
|
TeamMemberStatus.Active,
|
||||||
IsEnrolled: true,
|
IsEnrolled: directory.Exists(entry => entry.UserId == account.UserId),
|
||||||
DateTimeOffset.UnixEpoch,
|
DateTimeOffset.UnixEpoch,
|
||||||
LastActiveAt: null);
|
LastActiveAt: null);
|
||||||
|
|
||||||
|
|||||||
@@ -502,6 +502,69 @@ public sealed class TeamSharingTests : IAsyncLifetime
|
|||||||
teams.Status.ShouldContain("cannot send mail");
|
teams.Status.ShouldContain("cannot send mail");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <remarks>
|
||||||
|
/// <para>
|
||||||
|
/// The regression this whole path was rewritten for. An account exists from its owner's first
|
||||||
|
/// authenticated request and publishes no key until they choose a passphrase on their own machine,
|
||||||
|
/// and the directory omits it for that entire window — an entry exists to be wrapped to, and this
|
||||||
|
/// one has nothing to wrap. Reading that silence as "there is no such account" meant ADD MEMBER
|
||||||
|
/// quietly issued an invitation instead: the members list did not change, the screen said they had
|
||||||
|
/// no account here, and they only actually joined on the next hourly sweep.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// So the assertion is that they are a <em>member</em>, not an invitation, and that the row says
|
||||||
|
/// what is true of them — no key, so nothing can be shared with them yet.
|
||||||
|
/// </para>
|
||||||
|
/// </remarks>
|
||||||
|
[Fact]
|
||||||
|
public async Task AddingAnAccountThatHasNotEnrolled_MakesThemAMemberWithNoKey()
|
||||||
|
{
|
||||||
|
await UnlockedAsync();
|
||||||
|
|
||||||
|
var teams = shell.Teams;
|
||||||
|
var colleague = server.AddUnenrolledAccount("carol@example.com", "Carol Example");
|
||||||
|
|
||||||
|
await CreateTeamAsync(teams, "Platform", "platform");
|
||||||
|
|
||||||
|
teams.InviteEmail = "carol@example.com";
|
||||||
|
await teams.AddMemberCommand.ExecuteAsync(null);
|
||||||
|
|
||||||
|
teams.Invitations.ShouldBeEmpty("they have an account here, so there is nothing to invite");
|
||||||
|
|
||||||
|
teams.Members.Count.ShouldBe(2, teams.Status);
|
||||||
|
|
||||||
|
var member = teams.Members.Single(row => row.UserId == colleague);
|
||||||
|
|
||||||
|
member.Email.ShouldBe("carol@example.com");
|
||||||
|
|
||||||
|
// The label the user asked to see, and the reason SHARE KEY is not the next step.
|
||||||
|
member.KeyState.ShouldContain("no key yet");
|
||||||
|
|
||||||
|
teams.Status.ShouldContain("Added");
|
||||||
|
teams.Status.ShouldContain("no key yet");
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <remarks>
|
||||||
|
/// The other half of the pair above: an address with no account at all still falls through to an
|
||||||
|
/// invitation. It is the server that decides which, so this proves the fall-through survived being
|
||||||
|
/// moved behind it rather than being replaced by an error.
|
||||||
|
/// </remarks>
|
||||||
|
[Fact]
|
||||||
|
public async Task AddingAnAddressWithNoAccount_StillInvitesRatherThanFailing()
|
||||||
|
{
|
||||||
|
await UnlockedAsync();
|
||||||
|
|
||||||
|
var teams = shell.Teams;
|
||||||
|
|
||||||
|
await CreateTeamAsync(teams, "Platform", "platform");
|
||||||
|
|
||||||
|
teams.InviteEmail = "stranger@example.com";
|
||||||
|
await teams.AddMemberCommand.ExecuteAsync(null);
|
||||||
|
|
||||||
|
teams.Members.ShouldHaveSingleItem("nobody has joined — they have only been invited");
|
||||||
|
teams.Invitations.ShouldHaveSingleItem().Email.ShouldBe("stranger@example.com");
|
||||||
|
}
|
||||||
|
|
||||||
/// <remarks>
|
/// <remarks>
|
||||||
/// A withdrawn invitation stays on the list saying it was withdrawn, rather than vanishing. One that
|
/// A withdrawn invitation stays on the list saying it was withdrawn, rather than vanishing. One that
|
||||||
/// disappeared would read as never having been sent, which is the same thing the screen looks like
|
/// disappeared would read as never having been sent, which is the same thing the screen looks like
|
||||||
|
|||||||
Reference in New Issue
Block a user