Comments subject authorization, tenancy, and privacy

The hard authorization problem is not whether a caller has comments.comment.create. It is whether that caller may view and comment on the specific Document/doc-42 or Ticket/ticket-9. Current Comments implements only the first decision.

1. Two authorization decisions

The generic permission decides who may use the comment capability. The subject policy decides whether the resource exists, is visible, and is in a state that accepts comments. Both decisions are required.

Every current ResourceDescriptor is fixed to (comments, comment). A principal with Comments View can request the thread for an arbitrary resource key; one with Create can write to arbitrary strings even when no owner resource exists.

2. Owner rules are not connected

For example, Document stores AllowComments, but Comments neither depends on Documents nor reads that property. Ticket visibility, lifecycle state, and participant rules are likewise absent from the HTTP handlers.

Consequently, these statements are false for the current source:

• an owner module authorizes the resource before comment creation;
• setting AllowComments=false prevents comments;
• deleting an owner resource automatically hides or removes comments;
• comment notifications are published after persistence.

There are no such calls, events, or subscribers in the path.

3. Register a narrow subject policy

The generic module should not reference every owner’s Infrastructure. Let each owner register a narrow policy instead:

public interface ICommentSubjectPolicy
{
    // Each owner registers one canonical resource type, such as Document or Ticket.
    string EntityType { get; }

    // Return the owner's authoritative decision without exposing its persistence model.
    Task<Result<CommentSubjectDecision>> AuthorizeAsync(
        string entityId,
        CommentOperation operation,
        CurrentUser caller,
        CancellationToken cancellationToken);
}

public sealed record CommentSubjectDecision(
    string CanonicalEntityType,
    string CanonicalEntityId,
    bool Exists,
    bool AllowComments,
    string? VisibilityVersion);

public async Task<Result<CommentSubjectDecision>> AuthorizeAsync(
    string documentId,
    CommentOperation operation,
    CurrentUser caller,
    CancellationToken cancellationToken)
{
    // Read the authoritative document in the effective tenant; never trust a
    // TenantId supplied alongside an untrusted comment request.
    var document = await documents.FindAsync(
        caller.EffectiveTenantId, documentId, cancellationToken);
    if (document is null)
        return Result.Failure<CommentSubjectDecision>(DocsErrors.Document.NotFound);

    // Reuse Documents visibility semantics instead of checking only Comments.
    var access = await documentAccess.AuthorizeAsync(
        document, Map(operation), caller, cancellationToken);
    if (access.IsFailure)
        return Result.Failure<CommentSubjectDecision>(access.Error);

    // Creation can enforce AllowComments; read/moderation exceptions must be explicit.
    if (operation == CommentOperation.Create && !document.AllowComments)
        return Result.Failure<CommentSubjectDecision>(CommentErrors.Disabled);

    return new CommentSubjectDecision(
        "Document", document.Id, true, document.AllowComments, null);
}

Unknown EntityType values must fail closed. The registry should canonicalize casing and aliases so document, Document, and docs.document do not form unrelated threads.

4. Current tenant-source split

Comments uses two tenant sources:

The defect stays hidden when both values match. Under impersonation, platform operations, or background work, divergence can cause Create to miss an existing key, miss its parent, then write into the effective tenant; Get can return an empty thread while Update/Delete still find the effective-tenant row.

Every path should capture one immutable EffectiveTenant snapshot and use it for the owner policy and comment store alike.

5. AuthorId trust boundary

Create does not call RequireUserId:

var user = currentUser.User;

// A human user stores a stable UserId. Client/no-user calls collapse to "0".
var authorId = user.UserId?.ToString() ?? "0";

// Edit/delete later compare against UserId again, so ordinary users cannot
// claim rows whose AuthorId was written as "0".
var isAuthor = record.AuthorId == user.UserId?.ToString();

If Comments supports human authors only, require a UserId at creation. If service clients are allowed, persist AuthorType and AuthorId together instead of giving every non-user caller the same "0" identity.

6. Hard-coded admin bypass

Update/Delete evaluates Roles.Contains("admin", OrdinalIgnoreCase). This is neither a catalog permission nor a module/tenant/resource-scoped decision. It prevents a distinct moderator grant, couples behavior to a role name, and produces no dedicated record of an administrative action.

Replace it with comments.comment.moderate or an ICommentModerationPolicy, then audit the operator, reason, target subject, original author, and outcome.

7. Read authorization must happen before query

GetComments currently queries a free-form subject immediately after generic View permission. The owner policy must first establish visibility. For activity streams or multi-subject reads, do not load everything and filter in memory; push an owner-approved subject set or data scope into the query.

8. Privacy and retention

Comment bodies can contain personal data, secrets, and attachment links. Current Comments has no sensitive-data classification, subject export/erasure contributor, legal hold, owner-deletion retention policy, or cleanup protocol for notifications, search, and audit copies.

Clearing Body in the primary row is useful data minimization, but not complete erasure. Production design should register Comments with GDPR SAR/erasure orchestration and let each owner select cascade, retained placeholder, or legal hold when a resource is deleted.

9. Content safety

The module stores an unparsed string. A 10,000-character limit does not prevent XSS, malicious links, spam, or Unicode spoofing. A production integration should:

• store source plain text/Markdown separately from rendered output;
• use a safe Markdown renderer and URL-scheme allowlist;
• run typed moderation/spam decisions before persistence;
• log only CommentId and a subject hash, never Body;
• bound and encode notification excerpts;
• allow attachments only through a Finalized FileId, not arbitrary HTML URLs.

10. Security test matrix

Back to Comments