From e3342c4b887cb0ac08efb4655e4e35d2ceffb470 Mon Sep 17 00:00:00 2001 From: "Mars.P" Date: Tue, 13 Oct 2026 11:34:19 +0800 Subject: [PATCH] Shred retired secrets and gate spending a member's token Deleting a git provider connection marked its credentials Revoked but kept every token and the connection's OAuth client secret decryptable in the database, and never told the provider, so an OAuth refresh token stayed valid at GitHub/GitLab while the UI said "Revoked". The delete now retires each credential through the same path a departure or an unlink uses (token blanked, Revoked, soft-deleted, act-as identities gone), nulls the client secret, and revokes each OAuth credential's tokens at the provider after the transaction commits, best effort and bounded, so a rolled-back delete never ends a token. The refresh-then-access revoke a Disconnect already did moves into IOAuthGrantRevoker, shared by both paths. The delete revokes those tokens only, never the person's whole grant. Two teams may connect through the same OAuth App, and GitHub keeps one grant per user and app: DELETE /applications/{client_id}/grant would end the person's token in every other team too. OAuthRevokeInput gains WholeGrant; a Disconnect keeps the grant delete it always sent, while the connection delete asks GitHub's DELETE .../token for the access token and sends no refresh token, which GitHub cannot end alone. GitLab revokes per token (RFC 7009) either way. A refresh in flight when a retirement commits finds its row blanked and revokes the pair the provider just issued, which nothing stores. It sent that pair as a grant delete, so a departure or a relink racing a GitHub refresh, and now a connection delete or the sweep below, ended the person's tokens in every other team. The refresher now ends the pair the way the write that blanked the row ends tokens: a Disconnect leaves the row listed and still ends the grant; a retirement soft-deletes it, and only the pair is ended. A connection-wide connection's group hooks outlived the delete too: still registered at GitLab/GitHub, still delivering, their HMAC secrets decryptable, and their registering token gone, so nothing could ever remove them. The delete now retires them first, while that token still works, the way a switch back to per-repository scope does, and blanks the secret of every hook row the connection keeps. Deleted variables, revoked storage credentials and rotated or cleared deployment storage defaults kept their ciphertext forever, because 0012's check demanded a ciphertext on every Secret row and the 0126 and 0173 guards refused every UPDATE. Migration 0249 lets a deleted Secret hold none, lets a revision be blanked exactly once and only when its credential is Revoked, lets a default's envelope be blanked exactly once and only when no template names it, and blanks what retirement already left behind. A superseded revision of an Active credential is kept: storage profiles pin an exact revision and the resolver still decrypts it for bytes stored through it. The materializer decrypts only the envelope its template names, so the one an edit replaces is blanked in the same transaction. The old checks reject shredded rows, so 0249's rollback note re-adds them NOT VALID; a test runs that note on a shredded database, and the note now says how to roll forward again afterwards. 0249 runs once, at the first new pod's start, but the previous build keeps retiring secrets without blanking them wherever it still runs: beside the new one during a rolling deploy, or alone after a rollback. An hourly recurring sweep (job, command, handler, service) blanks exactly the rows 0249's backfill blanks, so those are shredded on the next tick; a test holds the sweep to the migration's own assertions. Listing a credential's accessible repositories presents the token to the provider, yet needed only team membership, so a Viewer could spend the Owner's personal token to list private repositories outside the team. The query now takes IRequireCredentialSpend: the owner of a Personal credential, or a repos.manage holder; the rule is shared with IRequireCredentialManagement. --- ...edentialManagementAuthorizationBehavior.cs | 40 +- .../CredentialOwnerOrPermission.cs | 57 +++ .../CredentialSpendAuthorizationBehavior.cs | 25 ++ .../ShredRetiredSecretsCommandHandler.cs | 21 + .../RetiredSecretShredRecurringJob.cs | 17 + .../Mediation/MediatorModule.cs | 3 + .../DbUpFiles/0249_shred_retired_secrets.sql | 223 ++++++++++ .../Entities/StorageCredentialRevision.cs | 3 +- .../Entities/StorageDefaultCredential.cs | 12 +- .../Persistence/Entities/Variable.cs | 8 +- .../StorageCredentialConfiguration.cs | 2 +- .../StorageDefaultConfiguration.cs | 2 +- .../Services/Credentials/CredentialService.cs | 53 +-- .../Credentials/IRetiredSecretShredService.cs | 16 + .../Credentials/RetiredSecretShredService.cs | 85 ++++ .../Services/OAuth/CredentialPayloadReader.cs | 14 +- .../Services/OAuth/GitHubOAuthClient.cs | 16 +- .../OAuth/ICredentialPayloadReader.cs | 7 + .../Services/OAuth/IOAuthClient.cs | 8 + .../Services/OAuth/IOAuthGrantRevoker.cs | 25 ++ .../Services/OAuth/OAuthGrantRevoker.cs | 61 +++ .../Services/OAuth/OAuthTokenRefresher.cs | 17 +- .../Services/Projects/ProjectService.cs | 11 +- .../ProviderInstanceService.Retirement.cs | 169 ++++++++ .../ProviderInstanceService.cs | 34 +- .../Services/Variables/VariableRetirement.cs | 21 + .../Services/Variables/VariableService.cs | 5 +- .../Credentials/StorageCredentialService.cs | 17 + .../Defaults/StorageDefaultService.cs | 24 +- .../Authorization/IRequireCredentialSpend.cs | 15 + .../Credentials/ShredRetiredSecretsCommand.cs | 19 + .../DeleteProviderInstanceCommand.cs | 8 +- .../Storage/StorageDefaultCommands.cs | 3 +- .../ListAccessibleRepositoriesQuery.cs | 7 +- .../AccessibleRepositoriesGateE2ETests.cs | 95 ++++ .../DeletedVariableShredE2ETests.cs | 51 +++ .../Infrastructure/RecordingProviderStub.cs | 144 ++++++ .../ProviderInstanceDeleteRevokeE2ETests.cs | 274 ++++++++++++ .../StorageCredentialsEndpointE2ETests.cs | 24 +- .../StorageDefaultSecretShredE2ETests.cs | 134 ++++++ .../TeamPermissionEnforcementTests.cs | 36 ++ .../RetiredSecretShredSweepFlowTests.cs | 237 ++++++++++ .../OAuth/CredentialPayloadReaderFlowTests.cs | 65 +++ .../RetiredSecretShredMigrationTests.cs | 410 ++++++++++++++++++ .../StorageDefaultPersistenceTests.cs | 61 +++ .../SharedOAuthAppRevocationFlowTests.cs | 316 ++++++++++++++ ...pdateAndDeleteProviderInstanceFlowTests.cs | 257 ++++++++++- .../Variables/VariableShredFlowTests.cs | 95 ++++ .../StorageCredentialShredTests.cs | 153 +++++++ .../StorageDefaultMaterializerFlowTests.cs | 58 +++ .../SweepCommandTransactionFlowTests.cs | 1 + .../Architecture/CredentialSpendGateTests.cs | 51 +++ .../RequestAuthorizationInventoryTests.cs | 1 + .../Jobs/RetiredSecretShredDispatchTests.cs | 93 ++++ .../OAuth/OAuthClientRevokeTests.cs | 139 ++++++ .../OAuth/OAuthGrantRevokerTests.cs | 119 +++++ .../OAuth/OAuthTokenRefresherTests.cs | 34 +- .../Variables/VariableRetirementTests.cs | 40 ++ 58 files changed, 3754 insertions(+), 182 deletions(-) create mode 100644 backend/src/CodeSpace.Core/Authorization/CredentialOwnerOrPermission.cs create mode 100644 backend/src/CodeSpace.Core/Authorization/CredentialSpendAuthorizationBehavior.cs create mode 100644 backend/src/CodeSpace.Core/Handlers/CommandHandlers/Credentials/ShredRetiredSecretsCommandHandler.cs create mode 100644 backend/src/CodeSpace.Core/Jobs/RecurringJobs/RetiredSecretShredRecurringJob.cs create mode 100644 backend/src/CodeSpace.Core/Persistence/DbUpFiles/0249_shred_retired_secrets.sql create mode 100644 backend/src/CodeSpace.Core/Services/Credentials/IRetiredSecretShredService.cs create mode 100644 backend/src/CodeSpace.Core/Services/Credentials/RetiredSecretShredService.cs create mode 100644 backend/src/CodeSpace.Core/Services/OAuth/IOAuthGrantRevoker.cs create mode 100644 backend/src/CodeSpace.Core/Services/OAuth/OAuthGrantRevoker.cs create mode 100644 backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderInstanceService.Retirement.cs create mode 100644 backend/src/CodeSpace.Core/Services/Variables/VariableRetirement.cs create mode 100644 backend/src/CodeSpace.Messages/Authorization/IRequireCredentialSpend.cs create mode 100644 backend/src/CodeSpace.Messages/Commands/Credentials/ShredRetiredSecretsCommand.cs create mode 100644 backend/tests/CodeSpace.E2ETests/Credentials/AccessibleRepositoriesGateE2ETests.cs create mode 100644 backend/tests/CodeSpace.E2ETests/Credentials/DeletedVariableShredE2ETests.cs create mode 100644 backend/tests/CodeSpace.E2ETests/Infrastructure/RecordingProviderStub.cs create mode 100644 backend/tests/CodeSpace.E2ETests/Repositories/ProviderInstanceDeleteRevokeE2ETests.cs create mode 100644 backend/tests/CodeSpace.E2ETests/Storage/StorageDefaultSecretShredE2ETests.cs create mode 100644 backend/tests/CodeSpace.IntegrationTests/Credentials/RetiredSecretShredSweepFlowTests.cs create mode 100644 backend/tests/CodeSpace.IntegrationTests/OAuth/CredentialPayloadReaderFlowTests.cs create mode 100644 backend/tests/CodeSpace.IntegrationTests/Persistence/RetiredSecretShredMigrationTests.cs create mode 100644 backend/tests/CodeSpace.IntegrationTests/ProviderInstances/SharedOAuthAppRevocationFlowTests.cs create mode 100644 backend/tests/CodeSpace.IntegrationTests/Variables/VariableShredFlowTests.cs create mode 100644 backend/tests/CodeSpace.IntegrationTests/Workflows/Artifacts/Credentials/StorageCredentialShredTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Architecture/CredentialSpendGateTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Jobs/RetiredSecretShredDispatchTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/OAuth/OAuthClientRevokeTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/OAuth/OAuthGrantRevokerTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Variables/VariableRetirementTests.cs diff --git a/backend/src/CodeSpace.Core/Authorization/CredentialManagementAuthorizationBehavior.cs b/backend/src/CodeSpace.Core/Authorization/CredentialManagementAuthorizationBehavior.cs index 244f6ca25..efd7d346e 100644 --- a/backend/src/CodeSpace.Core/Authorization/CredentialManagementAuthorizationBehavior.cs +++ b/backend/src/CodeSpace.Core/Authorization/CredentialManagementAuthorizationBehavior.cs @@ -1,10 +1,6 @@ -using CodeSpace.Core.Persistence.Db; -using CodeSpace.Core.Services.Identity; using CodeSpace.Messages.Authorization; using CodeSpace.Messages.Constants; -using CodeSpace.Messages.Enums; using MediatR; -using Microsoft.EntityFrameworkCore; namespace CodeSpace.Core.Authorization; @@ -15,44 +11,14 @@ namespace CodeSpace.Core.Authorization; /// public sealed class CredentialManagementAuthorizationBehavior : IPipelineBehavior where TRequest : IRequireCredentialManagement { - private readonly CodeSpaceDbContext _db; - private readonly ICurrentTeam _currentTeam; - private readonly ICurrentUser _currentUser; - private readonly TeamMembershipResolver _resolver; + private readonly CredentialOwnerOrPermission _rule; - public CredentialManagementAuthorizationBehavior(CodeSpaceDbContext db, ICurrentTeam currentTeam, ICurrentUser currentUser, TeamMembershipResolver resolver) - { - _db = db; - _currentTeam = currentTeam; - _currentUser = currentUser; - _resolver = resolver; - } + public CredentialManagementAuthorizationBehavior(CredentialOwnerOrPermission rule) { _rule = rule; } public async Task Handle(TRequest request, RequestHandlerDelegate next, CancellationToken cancellationToken) { - if (await IsCallersOwnPersonalCredentialAsync(request.CredentialId, cancellationToken).ConfigureAwait(false)) return await next().ConfigureAwait(false); - - await EnsureCredentialsManageAsync(cancellationToken).ConfigureAwait(false); + await _rule.EnsureAsync(request.CredentialId, TeamPermissions.CredentialsManage, cancellationToken).ConfigureAwait(false); return await next().ConfigureAwait(false); } - - private async Task IsCallersOwnPersonalCredentialAsync(Guid credentialId, CancellationToken cancellationToken) - { - if (_currentUser.Id is not { } callerId) return false; - - return await _db.Credential.AsNoTracking() - .AnyAsync(c => c.Id == credentialId && c.DeletedDate == null && c.Ownership == CredentialOwnership.Personal && c.OwnerUserId == callerId, cancellationToken) - .ConfigureAwait(false); - } - - private async Task EnsureCredentialsManageAsync(CancellationToken cancellationToken) - { - var teamId = _currentTeam.Id ?? throw TenantAccessDeniedException.NoTeamSelected(_currentUser.Id, HeaderCurrentTeam.HeaderName); - - var role = await _resolver.ResolveRoleAsync(teamId, cancellationToken).ConfigureAwait(false); - - if (!TeamPermissionMatrix.IsGranted(role, TeamPermissions.CredentialsManage)) - throw TenantAccessDeniedException.MissingTeamPermission(_currentUser.Id, teamId, role, TeamPermissions.CredentialsManage); - } } diff --git a/backend/src/CodeSpace.Core/Authorization/CredentialOwnerOrPermission.cs b/backend/src/CodeSpace.Core/Authorization/CredentialOwnerOrPermission.cs new file mode 100644 index 000000000..d3ff3a80c --- /dev/null +++ b/backend/src/CodeSpace.Core/Authorization/CredentialOwnerOrPermission.cs @@ -0,0 +1,57 @@ +using CodeSpace.Core.DependencyInjection; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Services.Identity; +using CodeSpace.Messages.Authorization; +using CodeSpace.Messages.Enums; +using Microsoft.EntityFrameworkCore; + +namespace CodeSpace.Core.Authorization; + +/// +/// The rule every credential tier inside applies, +/// with its own permission: the owner of a Personal credential passes on the membership that behavior already proved; +/// anyone else — on a teammate's credential, or on the team's own, which has no owner — needs the named team permission. +/// An instance Admin resolves as an Owner (), the standing +/// gives them in every team. +/// +public sealed class CredentialOwnerOrPermission : IScopedDependency +{ + private readonly CodeSpaceDbContext _db; + private readonly ICurrentTeam _currentTeam; + private readonly ICurrentUser _currentUser; + private readonly TeamMembershipResolver _resolver; + + public CredentialOwnerOrPermission(CodeSpaceDbContext db, ICurrentTeam currentTeam, ICurrentUser currentUser, TeamMembershipResolver resolver) + { + _db = db; + _currentTeam = currentTeam; + _currentUser = currentUser; + _resolver = resolver; + } + + public async Task EnsureAsync(Guid credentialId, string permission, CancellationToken cancellationToken) + { + if (await IsCallersOwnPersonalCredentialAsync(credentialId, cancellationToken).ConfigureAwait(false)) return; + + await EnsurePermissionAsync(permission, cancellationToken).ConfigureAwait(false); + } + + private async Task IsCallersOwnPersonalCredentialAsync(Guid credentialId, CancellationToken cancellationToken) + { + if (_currentUser.Id is not { } callerId) return false; + + return await _db.Credential.AsNoTracking() + .AnyAsync(c => c.Id == credentialId && c.DeletedDate == null && c.Ownership == CredentialOwnership.Personal && c.OwnerUserId == callerId, cancellationToken) + .ConfigureAwait(false); + } + + private async Task EnsurePermissionAsync(string permission, CancellationToken cancellationToken) + { + var teamId = _currentTeam.Id ?? throw TenantAccessDeniedException.NoTeamSelected(_currentUser.Id, HeaderCurrentTeam.HeaderName); + + var role = await _resolver.ResolveRoleAsync(teamId, cancellationToken).ConfigureAwait(false); + + if (!TeamPermissionMatrix.IsGranted(role, permission)) + throw TenantAccessDeniedException.MissingTeamPermission(_currentUser.Id, teamId, role, permission); + } +} diff --git a/backend/src/CodeSpace.Core/Authorization/CredentialSpendAuthorizationBehavior.cs b/backend/src/CodeSpace.Core/Authorization/CredentialSpendAuthorizationBehavior.cs new file mode 100644 index 000000000..a593a0e29 --- /dev/null +++ b/backend/src/CodeSpace.Core/Authorization/CredentialSpendAuthorizationBehavior.cs @@ -0,0 +1,25 @@ +using CodeSpace.Messages.Authorization; +using CodeSpace.Messages.Constants; +using MediatR; + +namespace CodeSpace.Core.Authorization; + +/// +/// The owner of a Personal credential may present it to the provider on membership alone; every other caller needs +/// , the bar of the bind such a listing feeds. Runs inside +/// , which has already proven the credential is the +/// header team's and the caller a member of it. +/// +public sealed class CredentialSpendAuthorizationBehavior : IPipelineBehavior where TRequest : IRequireCredentialSpend +{ + private readonly CredentialOwnerOrPermission _rule; + + public CredentialSpendAuthorizationBehavior(CredentialOwnerOrPermission rule) { _rule = rule; } + + public async Task Handle(TRequest request, RequestHandlerDelegate next, CancellationToken cancellationToken) + { + await _rule.EnsureAsync(request.CredentialId, TeamPermissions.ReposManage, cancellationToken).ConfigureAwait(false); + + return await next().ConfigureAwait(false); + } +} diff --git a/backend/src/CodeSpace.Core/Handlers/CommandHandlers/Credentials/ShredRetiredSecretsCommandHandler.cs b/backend/src/CodeSpace.Core/Handlers/CommandHandlers/Credentials/ShredRetiredSecretsCommandHandler.cs new file mode 100644 index 000000000..7afe0556b --- /dev/null +++ b/backend/src/CodeSpace.Core/Handlers/CommandHandlers/Credentials/ShredRetiredSecretsCommandHandler.cs @@ -0,0 +1,21 @@ +using CodeSpace.Core.Services.Credentials; +using CodeSpace.Messages.Commands.Credentials; +using MediatR; + +namespace CodeSpace.Core.Handlers.CommandHandlers.Credentials; + +/// Thin dispatcher (Rule 16) — the production caller of . +public sealed class ShredRetiredSecretsCommandHandler : IRequestHandler +{ + private readonly IRetiredSecretShredService _shred; + + public ShredRetiredSecretsCommandHandler(IRetiredSecretShredService shred) + { + _shred = shred; + } + + public async Task Handle(ShredRetiredSecretsCommand request, CancellationToken cancellationToken) + { + return await _shred.ShredAsync(cancellationToken).ConfigureAwait(false); + } +} diff --git a/backend/src/CodeSpace.Core/Jobs/RecurringJobs/RetiredSecretShredRecurringJob.cs b/backend/src/CodeSpace.Core/Jobs/RecurringJobs/RetiredSecretShredRecurringJob.cs new file mode 100644 index 000000000..9ecf52261 --- /dev/null +++ b/backend/src/CodeSpace.Core/Jobs/RecurringJobs/RetiredSecretShredRecurringJob.cs @@ -0,0 +1,17 @@ +using CodeSpace.Messages.Commands.Credentials; +using MediatR; + +namespace CodeSpace.Core.Jobs.RecurringJobs; + +/// Every hour: blank the secrets a build older than migration 0249 retired without blanking — any an older pod retires during a rolling deploy, and every one retired after a rollback — since 0249's own backfill ran once (thin Rule-14 dispatcher). A blanked row is no longer a candidate, so an idle tick changes nothing. +public sealed class RetiredSecretShredRecurringJob : IRecurringJob +{ + private readonly IMediator _mediator; + + public RetiredSecretShredRecurringJob(IMediator mediator) { _mediator = mediator; } + + public string JobId => nameof(RetiredSecretShredRecurringJob); + public string CronExpression => "0 * * * *"; + + public async Task Execute() => await _mediator.Send(new ShredRetiredSecretsCommand()).ConfigureAwait(false); +} diff --git a/backend/src/CodeSpace.Core/Mediation/MediatorModule.cs b/backend/src/CodeSpace.Core/Mediation/MediatorModule.cs index aa750d826..dae9a9b92 100644 --- a/backend/src/CodeSpace.Core/Mediation/MediatorModule.cs +++ b/backend/src/CodeSpace.Core/Mediation/MediatorModule.cs @@ -40,6 +40,8 @@ protected override void Load(ContainerBuilder builder) // already vetted, this adds the role check for write/action requests // CredentialManagement — inside Credential (whose marker it extends): the owner of a Personal // credential passes on membership, anyone else needs credentials.manage + // CredentialSpend — the same shape for presenting a credential to its provider: the owner of a + // Personal credential passes on membership, anyone else needs repos.manage // BotVisibility — opts the request into seeing bot users (IBotInclusive); default excludes // Transactional — innermost of these, wraps DB writes only after auth has passed // @@ -56,6 +58,7 @@ protected override void Load(ContainerBuilder builder) builder.RegisterGeneric(typeof(RepositoryAccessAuthorizationBehavior<,>)).As(typeof(IPipelineBehavior<,>)).InstancePerLifetimeScope(); builder.RegisterGeneric(typeof(CredentialAccessAuthorizationBehavior<,>)).As(typeof(IPipelineBehavior<,>)).InstancePerLifetimeScope(); builder.RegisterGeneric(typeof(CredentialManagementAuthorizationBehavior<,>)).As(typeof(IPipelineBehavior<,>)).InstancePerLifetimeScope(); + builder.RegisterGeneric(typeof(CredentialSpendAuthorizationBehavior<,>)).As(typeof(IPipelineBehavior<,>)).InstancePerLifetimeScope(); builder.RegisterGeneric(typeof(BotVisibilityBehavior<,>)).As(typeof(IPipelineBehavior<,>)).InstancePerLifetimeScope(); builder.RegisterGeneric(typeof(TransactionalBehavior<,>)).As(typeof(IPipelineBehavior<,>)).InstancePerLifetimeScope(); diff --git a/backend/src/CodeSpace.Core/Persistence/DbUpFiles/0249_shred_retired_secrets.sql b/backend/src/CodeSpace.Core/Persistence/DbUpFiles/0249_shred_retired_secrets.sql new file mode 100644 index 000000000..58a50384a --- /dev/null +++ b/backend/src/CodeSpace.Core/Persistence/DbUpFiles/0249_shred_retired_secrets.sql @@ -0,0 +1,223 @@ +-- 0249_shred_retired_secrets.sql +-- +-- A retired secret keeps no ciphertext. Until now four kinds of retirement left theirs decryptable forever, readable by +-- anyone holding a database copy and the key: +-- +-- * deleting a git provider connection marked its credentials Revoked and soft-deleted, but kept each token in +-- credential.encrypted_payload, the connection's OAuth client secret in provider_instance.oauth_client_secret_enc, +-- and each group hook's HMAC secret in connection_webhook.secret_enc; +-- * deleting a variable kept a Secret's AES-GCM envelope in variable.value_encrypted, because +-- chk_variable_value_exclusive (0012) required one on every Secret row, deleted or not; +-- * revoking a storage credential kept every revision's envelope in storage_credential_revision.encrypted_payload, +-- because storage_credential_revision_guard (0126) refused every UPDATE; +-- * rotating or clearing a deployment storage default's secret repointed the template and kept the envelope it +-- replaced in storage_default_credential.encrypted_payload, because storage_default_credential_guard (0173) refused +-- every UPDATE. +-- +-- The application now blanks each of these when it retires one. This script relaxes the three guards just enough to +-- let it, and blanks what retirement already left behind. It changes no live secret: +-- +-- * credential: blanked only when Revoked or soft-deleted. Every other retirement path (Disconnect, unlinking an +-- identity, a departure) already blanked the payload; the rows left holding one are a deleted connection's. Nothing +-- reads a Revoked or soft-deleted credential's token: credential succession reads only ids and dates. +-- * provider_instance: the client secret is blanked only on a soft-deleted connection. Nothing restores one. +-- * connection_webhook: a soft-deleted connection's hooks are taken out of service (Cancelled) and their secrets +-- blanked. Ingestion refuses a Cancelled hook before it reads the secret, the registrar only ever claims an +-- Enqueued row, and a deleted connection's hooks could never be retired by the application: their registering +-- token is already blanked, so a hook still registered at the provider has to be removed there by hand. Its +-- external_id and owner_path stay, as the record of which hook that is. +-- * variable: a Secret may hold no ciphertext once deleted_date is set — and only then; a live Secret must still +-- carry one. Every read filters on deleted_date IS NULL and nothing restores a deleted variable. +-- * storage_credential_revision: exactly one UPDATE is admitted per revision — the one that blanks its payload and +-- changes nothing else — and only once its credential is Revoked. A Revoked credential resolves for nobody (the +-- secret resolver answers NotActive before it reads an envelope), revocation is terminal, and it is refused while a +-- stored location or a live profile still resolves through the credential. A superseded revision of an ACTIVE +-- credential is not blanked and may not be: a storage profile pins an exact revision (db:{id}:{revision}), so it +-- still decrypts the bytes stored through it, and routed bytes are permanent. A revision is never inserted blank. +-- * storage_default_credential: exactly one UPDATE is admitted per envelope — the one that blanks its payload and +-- changes nothing else — and only while no storage_default names it. The materializer is the only reader, and it +-- decrypts only the envelope its template names, read under a share lock on that template; an envelope is only +-- ever named by the template that appended it. Fingerprint and hint stay as history. An envelope is never inserted +-- blank. +-- +-- Safe on a populated database: the new checks are weaker than the old ones, so every existing row passes them; the +-- backfills touch only retired rows. Never refuses. +-- +-- Rollback. The blanked ciphertexts do not come back — that is the point — and so 0012's, 0126's and 0173's checks +-- cannot be put back as they were: once anything is shredded, by this script or by the application, a deleted Secret +-- variable holds NULL and a shredded revision or envelope holds '', exactly the rows those checks reject, so re-adding +-- them as written fails with 23514. Re-add them NOT VALID (enforced on every row written from then on, not on the rows +-- already shredded), and put 0173's trigger back on UPDATE and DELETE only: +-- +-- ALTER TABLE variable DROP CONSTRAINT chk_variable_value_exclusive; +-- ALTER TABLE variable ADD CONSTRAINT chk_variable_value_exclusive CHECK ((value_type = 'Secret' AND value_encrypted IS NOT NULL AND value_plain IS NULL) OR (value_type <> 'Secret' AND value_plain IS NOT NULL AND value_encrypted IS NULL)) NOT VALID; +-- ALTER TABLE storage_credential_revision DROP CONSTRAINT ck_storage_credential_revision_encrypted_payload; +-- ALTER TABLE storage_credential_revision ADD CONSTRAINT ck_storage_credential_revision_encrypted_payload CHECK (btrim(encrypted_payload) <> '') NOT VALID; +-- ALTER TABLE storage_default_credential DROP CONSTRAINT ck_storage_default_credential_encrypted_payload; +-- ALTER TABLE storage_default_credential ADD CONSTRAINT ck_storage_default_credential_encrypted_payload CHECK (btrim(encrypted_payload) <> '') NOT VALID; +-- DROP TRIGGER storage_default_credential_enforce_append_only ON storage_default_credential; +-- CREATE TRIGGER storage_default_credential_enforce_append_only BEFORE UPDATE OR DELETE ON storage_default_credential FOR EACH ROW EXECUTE FUNCTION storage_default_credential_guard(); +-- +-- then re-run, verbatim, 0126's CREATE OR REPLACE FUNCTION storage_credential_revision_guard() block and 0173's +-- CREATE OR REPLACE FUNCTION storage_default_credential_guard() block. Restoring those functions is safe on any +-- database: a trigger judges only the rows written after it. The hooks this script cancelled stay cancelled. +-- +-- Rolling the application back needs none of this: the previous build runs on this schema as it stands. Once the old +-- checks are back, though, this build cannot retire a secret — deleting a Secret variable fails with 23514 and revoking +-- a storage credential or editing a default's secret is refused — so before rolling forward again, delete this script's +-- journal row and let the next start apply it once more; every statement in it is safe to run twice: +-- +-- DELETE FROM schemaversions WHERE scriptname = '0249_shred_retired_secrets.sql'; + +-- ── Variables ──────────────────────────────────────────────────────────────── + +ALTER TABLE variable DROP CONSTRAINT IF EXISTS chk_variable_value_exclusive; + +ALTER TABLE variable ADD CONSTRAINT chk_variable_value_exclusive CHECK ( + (value_type = 'Secret' AND value_plain IS NULL AND (value_encrypted IS NOT NULL OR deleted_date IS NOT NULL)) + OR + (value_type <> 'Secret' AND value_plain IS NOT NULL AND value_encrypted IS NULL) +); + +UPDATE variable SET value_encrypted = NULL WHERE deleted_date IS NOT NULL AND value_encrypted IS NOT NULL; + +-- ── Storage credential revisions ───────────────────────────────────────────── + +ALTER TABLE storage_credential_revision DROP CONSTRAINT IF EXISTS ck_storage_credential_revision_encrypted_payload; + +ALTER TABLE storage_credential_revision ADD CONSTRAINT ck_storage_credential_revision_encrypted_payload CHECK (encrypted_payload = '' OR btrim(encrypted_payload) <> ''); + +COMMENT ON COLUMN storage_credential_revision.encrypted_payload IS + 'Complete IPayloadEncryptor/Data Protection ciphertext envelope for one provider SecretSchema payload; never plaintext. Empty once the credential is Revoked (0249).'; + +CREATE OR REPLACE FUNCTION storage_credential_revision_guard() RETURNS trigger AS $$ +DECLARE + credential_state VARCHAR(16); + latest_revision INTEGER; +BEGIN + -- The one UPDATE a revision ever admits: its payload blanked, every other column as it was, its credential Revoked. + -- Compared as whole rows minus the payload, so a column added later is covered too. + IF TG_OP = 'UPDATE' THEN + IF OLD.encrypted_payload <> '' AND NEW.encrypted_payload = '' + AND to_jsonb(NEW) - 'encrypted_payload' = to_jsonb(OLD) - 'encrypted_payload' THEN + SELECT state INTO credential_state + FROM storage_credential + WHERE team_id = OLD.team_id AND id = OLD.storage_credential_id; + + IF credential_state = 'Revoked' THEN + RETURN NEW; + END IF; + + RAISE EXCEPTION + 'storage_credential_revision payload may be blanked only once its credential is Revoked — a profile pins an exact revision, and an Active credential still decrypts it for the bytes stored through it (credential_id=%, revision=%).', + OLD.storage_credential_id, OLD.revision; + END IF; + END IF; + + IF TG_OP <> 'INSERT' THEN + RAISE EXCEPTION + 'storage_credential_revision is immutable — % rejected (credential_id=%, revision=%). Append a new revision instead.', + TG_OP, OLD.storage_credential_id, OLD.revision; + END IF; + + IF NEW.encrypted_payload = '' THEN + RAISE EXCEPTION 'storage_credential_revision payload is required — only a revoked credential''s revision is ever blank (credential_id=%).', NEW.storage_credential_id; + END IF; + + -- Serialize append with revocation. If revocation commits first, this append fails; if append commits first, the + -- revocation follows it and becomes the terminal boundary. The tenant-bound FK remains the final missing-parent guard. + SELECT state INTO credential_state + FROM storage_credential + WHERE team_id = NEW.team_id AND id = NEW.storage_credential_id + FOR UPDATE; + + IF FOUND AND credential_state = 'Revoked' THEN + RAISE EXCEPTION 'storage_credential Revoked state is terminal — revision append rejected (id=%).', NEW.storage_credential_id; + END IF; + + IF FOUND THEN + SELECT COALESCE(MAX(revision), 0) INTO latest_revision + FROM storage_credential_revision + WHERE team_id = NEW.team_id AND storage_credential_id = NEW.storage_credential_id; + + IF NEW.revision <> latest_revision + 1 THEN + RAISE EXCEPTION + 'storage_credential_revision is a contiguous append-only sequence (credential_id=%, latest=%, attempted=%).', + NEW.storage_credential_id, latest_revision, NEW.revision; + END IF; + END IF; + + RETURN NEW; +END; +$$ LANGUAGE plpgsql; + +UPDATE storage_credential_revision AS revision + SET encrypted_payload = '' + FROM storage_credential AS credential + WHERE credential.team_id = revision.team_id + AND credential.id = revision.storage_credential_id + AND credential.state = 'Revoked' + AND revision.encrypted_payload <> ''; + +-- ── Git provider credentials and connections ───────────────────────────────── + +UPDATE credential SET encrypted_payload = '' WHERE encrypted_payload <> '' AND (deleted_date IS NOT NULL OR status = 'Revoked'); + +UPDATE provider_instance SET oauth_client_secret_enc = NULL WHERE deleted_date IS NOT NULL AND oauth_client_secret_enc IS NOT NULL; + +UPDATE connection_webhook AS hook + SET secret_enc = '', registration_status = 'Cancelled' + FROM provider_instance AS instance + WHERE instance.id = hook.provider_instance_id + AND instance.deleted_date IS NOT NULL + AND (hook.secret_enc <> '' OR hook.registration_status <> 'Cancelled'); + +-- ── Deployment storage default envelopes ───────────────────────────────────── + +ALTER TABLE storage_default_credential DROP CONSTRAINT IF EXISTS ck_storage_default_credential_encrypted_payload; + +ALTER TABLE storage_default_credential ADD CONSTRAINT ck_storage_default_credential_encrypted_payload CHECK (encrypted_payload = '' OR btrim(encrypted_payload) <> ''); + +COMMENT ON COLUMN storage_default_credential.encrypted_payload IS + 'Complete IPayloadEncryptor/Data Protection ciphertext envelope for one provider SecretSchema payload; never plaintext. Empty once no template names the envelope (0249).'; + +CREATE OR REPLACE FUNCTION storage_default_credential_guard() RETURNS trigger AS $$ +BEGIN + IF TG_OP = 'INSERT' THEN + IF NEW.encrypted_payload = '' THEN + RAISE EXCEPTION 'storage_default_credential payload is required -- only an envelope no template names is ever blank (id=%).', NEW.id; + END IF; + + RETURN NEW; + END IF; + + -- The one UPDATE an envelope ever admits: its payload blanked, every other column as it was, no template naming it. + -- Compared as whole rows minus the payload, so a column added later is covered too. + IF TG_OP = 'UPDATE' + AND OLD.encrypted_payload <> '' AND NEW.encrypted_payload = '' + AND to_jsonb(NEW) - 'encrypted_payload' = to_jsonb(OLD) - 'encrypted_payload' THEN + IF NOT EXISTS (SELECT 1 FROM storage_default WHERE credential_id = OLD.id) THEN + RETURN NEW; + END IF; + + RAISE EXCEPTION + 'storage_default_credential payload may be blanked only once no template names it -- the materializer decrypts the envelope its template points at (id=%).', + OLD.id; + END IF; + + RAISE EXCEPTION + 'storage_default_credential is append-only -- % rejected (id=%). Insert a new envelope and repoint storage_default.credential_id.', + TG_OP, OLD.id; +END; +$$ LANGUAGE plpgsql; + +DROP TRIGGER IF EXISTS storage_default_credential_enforce_append_only ON storage_default_credential; + +CREATE TRIGGER storage_default_credential_enforce_append_only + BEFORE INSERT OR UPDATE OR DELETE ON storage_default_credential + FOR EACH ROW EXECUTE FUNCTION storage_default_credential_guard(); + +UPDATE storage_default_credential AS envelope + SET encrypted_payload = '' + WHERE envelope.encrypted_payload <> '' + AND NOT EXISTS (SELECT 1 FROM storage_default AS template WHERE template.credential_id = envelope.id); diff --git a/backend/src/CodeSpace.Core/Persistence/Entities/StorageCredentialRevision.cs b/backend/src/CodeSpace.Core/Persistence/Entities/StorageCredentialRevision.cs index d4378450c..71e36d13f 100644 --- a/backend/src/CodeSpace.Core/Persistence/Entities/StorageCredentialRevision.cs +++ b/backend/src/CodeSpace.Core/Persistence/Entities/StorageCredentialRevision.cs @@ -18,7 +18,8 @@ public class StorageCredentialRevision : IEntity /// /// Self-contained ciphertext produced by the established payload-encryption primitive. It is never plaintext JSON, - /// an individual key/token, or a model-credential reference. + /// an individual key/token, or a model-credential reference. Empty once the credential is Revoked — the one change a + /// revision ever admits (0249). /// public string EncryptedPayload { get; set; } = string.Empty; diff --git a/backend/src/CodeSpace.Core/Persistence/Entities/StorageDefaultCredential.cs b/backend/src/CodeSpace.Core/Persistence/Entities/StorageDefaultCredential.cs index b83a6912c..3db934cca 100644 --- a/backend/src/CodeSpace.Core/Persistence/Entities/StorageDefaultCredential.cs +++ b/backend/src/CodeSpace.Core/Persistence/Entities/StorageDefaultCredential.cs @@ -8,8 +8,9 @@ namespace CodeSpace.Core.Persistence.Entities; /// primitive already in the build. /// /// Rotation appends a new row and repoints ; the superseded envelope -/// stays as history. No runtime path decrypts this yet — the materializer lane is the intended reader, and it will -/// re-encrypt the secret into the team's own before anything resolves through it. +/// stays as history, its payload blanked once no template names it (0249). The materializer is the only reader: it +/// decrypts the envelope its template names and re-encrypts the secret into the team's own +/// before anything resolves through it. /// public class StorageDefaultCredential : IEntity { @@ -18,7 +19,10 @@ public class StorageDefaultCredential : IEntity /// Canonical major-versioned storage provider key, for example aliyun-oss/v1. public string ProviderTypeKey { get; set; } = string.Empty; - /// Self-contained ciphertext produced by the established payload-encryption primitive. Never plaintext JSON or an individual key. + /// + /// Self-contained ciphertext produced by the established payload-encryption primitive. Never plaintext JSON or an + /// individual key. Empty once no template names the envelope — the one change it ever admits (0249). + /// public string EncryptedPayload { get; set; } = string.Empty; /// Optional pre-sanitized operator hint (for example a short masked tail); never consumed at runtime. @@ -27,7 +31,7 @@ public class StorageDefaultCredential : IEntity /// SHA-256 of the encrypted envelope, allowing safe diagnostics without logging or indexing the envelope. public string EnvelopeFingerprint { get; set; } = string.Empty; - /// Immutable append timestamp and actor. There are no last-modified fields because mutation is illegal. + /// Immutable append timestamp and actor. There are no last-modified fields: the only change admitted is blanking the payload. public DateTimeOffset CreatedDate { get; set; } public Guid CreatedBy { get; set; } } diff --git a/backend/src/CodeSpace.Core/Persistence/Entities/Variable.cs b/backend/src/CodeSpace.Core/Persistence/Entities/Variable.cs index c0bcd21e5..b52ef400c 100644 --- a/backend/src/CodeSpace.Core/Persistence/Entities/Variable.cs +++ b/backend/src/CodeSpace.Core/Persistence/Entities/Variable.cs @@ -15,8 +15,9 @@ namespace CodeSpace.Core.Persistence.Entities; /// else routes through . /// /// The DB CHECK constraint chk_variable_value_exclusive guarantees exactly one -/// value column is populated, matching the ValueType. Application code does NOT need to -/// defensively null-check both columns — the constraint makes inconsistent rows impossible. +/// value column is populated, matching the ValueType — except on a deleted Secret, whose +/// ciphertext is dropped (0249) and which no read ever reaches. Application code does NOT +/// need to defensively null-check both columns of a live row. /// /// is denormalised even for Workflow scope so tenant-filter /// sweeps (the X-Team-Id middleware checks) work without joining workflow. @@ -54,7 +55,8 @@ public class Variable : IEntity, IAuditable /// /// AES-256-GCM envelope [nonce(12) || ciphertext || tag(16)] when - /// is . Null otherwise. + /// is . Null otherwise, and on a + /// deleted Secret (see VariableRetirement). /// public byte[]? ValueEncrypted { get; set; } diff --git a/backend/src/CodeSpace.Core/Persistence/EntityConfigurations/StorageCredentialConfiguration.cs b/backend/src/CodeSpace.Core/Persistence/EntityConfigurations/StorageCredentialConfiguration.cs index b1c9a9547..36667845a 100644 --- a/backend/src/CodeSpace.Core/Persistence/EntityConfigurations/StorageCredentialConfiguration.cs +++ b/backend/src/CodeSpace.Core/Persistence/EntityConfigurations/StorageCredentialConfiguration.cs @@ -40,7 +40,7 @@ public void Configure(EntityTypeBuilder builder) { builder.ToTable("storage_credential_revision", table => { - table.HasCheckConstraint("ck_storage_credential_revision_encrypted_payload", "btrim(encrypted_payload) <> ''"); + table.HasCheckConstraint("ck_storage_credential_revision_encrypted_payload", "encrypted_payload = '' OR btrim(encrypted_payload) <> ''"); table.HasCheckConstraint("ck_storage_credential_revision_envelope_fingerprint", "envelope_fingerprint ~ '^sha256:[0-9a-f]{64}$'"); table.HasCheckConstraint("ck_storage_credential_revision_number", "revision > 0"); table.HasCheckConstraint("ck_storage_credential_revision_provider_type_key", "provider_type_key ~ '^[a-z0-9][a-z0-9.-]*/v[1-9][0-9]*$'"); diff --git a/backend/src/CodeSpace.Core/Persistence/EntityConfigurations/StorageDefaultConfiguration.cs b/backend/src/CodeSpace.Core/Persistence/EntityConfigurations/StorageDefaultConfiguration.cs index a018206eb..8758f2dbe 100644 --- a/backend/src/CodeSpace.Core/Persistence/EntityConfigurations/StorageDefaultConfiguration.cs +++ b/backend/src/CodeSpace.Core/Persistence/EntityConfigurations/StorageDefaultConfiguration.cs @@ -43,7 +43,7 @@ public void Configure(EntityTypeBuilder builder) { builder.ToTable("storage_default_credential", table => { - table.HasCheckConstraint("ck_storage_default_credential_encrypted_payload", "btrim(encrypted_payload) <> ''"); + table.HasCheckConstraint("ck_storage_default_credential_encrypted_payload", "encrypted_payload = '' OR btrim(encrypted_payload) <> ''"); table.HasCheckConstraint("ck_storage_default_credential_envelope_fingerprint", "envelope_fingerprint ~ '^sha256:[0-9a-f]{64}$'"); table.HasCheckConstraint("ck_storage_default_credential_provider_type_key", "provider_type_key ~ '^[a-z0-9][a-z0-9.-]*/v[1-9][0-9]*$'"); table.HasCheckConstraint("ck_storage_default_credential_safe_hint", "safe_hint IS NULL OR (char_length(safe_hint) BETWEEN 1 AND 32 AND btrim(safe_hint) <> '' AND safe_hint !~ '[[:cntrl:]]')"); diff --git a/backend/src/CodeSpace.Core/Services/Credentials/CredentialService.cs b/backend/src/CodeSpace.Core/Services/Credentials/CredentialService.cs index e50fb9346..4c6c0b3fe 100644 --- a/backend/src/CodeSpace.Core/Services/Credentials/CredentialService.cs +++ b/backend/src/CodeSpace.Core/Services/Credentials/CredentialService.cs @@ -26,7 +26,7 @@ public sealed class CredentialService : ICredentialService, IScopedDependency private readonly ICurrentUser _currentUser; private readonly IPayloadEncryptor _encryptor; private readonly ICredentialPayloadSerializer _serializer; - private readonly IOAuthClientRegistry _oauthClients; + private readonly IOAuthGrantRevoker _grants; private readonly IProviderRegistry _registry; private readonly IProviderModuleCatalog _modules; private readonly IScopeChecker _scopeChecker; @@ -35,14 +35,14 @@ public sealed class CredentialService : ICredentialService, IScopedDependency private readonly ITeamMemberService _members; private readonly ILogger _logger; - public CredentialService(CodeSpaceDbContext db, ICurrentTeam currentTeam, ICurrentUser currentUser, IPayloadEncryptor encryptor, ICredentialPayloadSerializer serializer, IOAuthClientRegistry oauthClients, IProviderRegistry registry, IProviderModuleCatalog modules, IScopeChecker scopeChecker, ICredentialSuccessionService succession, ICredentialRevocationService revocation, ITeamMemberService members, ILogger logger) + public CredentialService(CodeSpaceDbContext db, ICurrentTeam currentTeam, ICurrentUser currentUser, IPayloadEncryptor encryptor, ICredentialPayloadSerializer serializer, IOAuthGrantRevoker grants, IProviderRegistry registry, IProviderModuleCatalog modules, IScopeChecker scopeChecker, ICredentialSuccessionService succession, ICredentialRevocationService revocation, ITeamMemberService members, ILogger logger) { _db = db; _currentTeam = currentTeam; _currentUser = currentUser; _encryptor = encryptor; _serializer = serializer; - _oauthClients = oauthClients; + _grants = grants; _registry = registry; _modules = modules; _scopeChecker = scopeChecker; @@ -272,13 +272,13 @@ await _db.Credential.Include(c => c.ProviderInstance).FirstOrDefaultAsync(c => c if (string.IsNullOrWhiteSpace(credential.ProviderInstance.OauthClientId) || string.IsNullOrWhiteSpace(credential.ProviderInstance.OauthClientSecretEnc)) return (Ok: false, Error: "provider instance missing OAuth client credentials"); - var payload = TryReadOAuthPayload(credential); + var payload = _grants.ReadPayload(credential); if (payload == null) return (Ok: false, Error: "credential payload is not a valid OAuth payload"); try { - await RevokeOAuthTokensAsync(credential.ProviderInstance, payload, cancellationToken).ConfigureAwait(false); + await _grants.RevokeAsync(credential.ProviderInstance, _encryptor.Decrypt(credential.ProviderInstance.OauthClientSecretEnc!), payload, wholeGrant: true, cancellationToken).ConfigureAwait(false); return (Ok: true, Error: null); } catch (Exception ex) @@ -288,49 +288,6 @@ await _db.Credential.Include(c => c.ProviderInstance).FirstOrDefaultAsync(c => c } } - private OAuthPayload? TryReadOAuthPayload(Credential credential) - { - try - { - var json = _encryptor.Decrypt(credential.EncryptedPayload); - return _serializer.Deserialize(AuthType.OAuth, json) as OAuthPayload; - } - catch - { - return null; - } - } - - private async Task RevokeOAuthTokensAsync(ProviderInstance instance, OAuthPayload payload, CancellationToken cancellationToken) - { - var client = _oauthClients.Get(instance.Provider); - var clientSecret = _encryptor.Decrypt(instance.OauthClientSecretEnc!); - - // Revoke refresh_token first (more durable — kills the grant on most providers), then - // access_token. Both are idempotent on the provider side; the caller's catch clears - // local payload regardless. - if (!string.IsNullOrEmpty(payload.RefreshToken)) - { - await client.RevokeAsync(new OAuthRevokeInput - { - Instance = instance, - ClientId = instance.OauthClientId!, - ClientSecret = clientSecret, - Token = payload.RefreshToken, - TokenTypeHint = "refresh_token" - }, cancellationToken).ConfigureAwait(false); - } - - await client.RevokeAsync(new OAuthRevokeInput - { - Instance = instance, - ClientId = instance.OauthClientId!, - ClientSecret = clientSecret, - Token = payload.AccessToken, - TokenTypeHint = "access_token" - }, cancellationToken).ConfigureAwait(false); - } - private static void MarkRevokedLocally(Credential credential, string? providerError) { // Clear encrypted payload — even if the provider call failed, removing token material diff --git a/backend/src/CodeSpace.Core/Services/Credentials/IRetiredSecretShredService.cs b/backend/src/CodeSpace.Core/Services/Credentials/IRetiredSecretShredService.cs new file mode 100644 index 000000000..660bab1d7 --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/Credentials/IRetiredSecretShredService.cs @@ -0,0 +1,16 @@ +namespace CodeSpace.Core.Services.Credentials; + +/// +/// Blanks the ciphertext of secrets that are retired but still decryptable. Every retirement in this build blanks its own +/// secret; this catches what a build older than migration 0249 retires without blanking — during a rolling deploy, or +/// after a rollback — since 0249's own backfill ran once, before those rows existed. +/// +public interface IRetiredSecretShredService +{ + /// + /// Blank every retired secret still holding ciphertext: exactly the rows 0249's backfill blanks. Idempotent and safe on + /// several workers at once — a blanked row is no longer a candidate, and each statement re-checks its row as it + /// writes. Returns how many rows this pass blanked. + /// + Task ShredAsync(CancellationToken cancellationToken); +} diff --git a/backend/src/CodeSpace.Core/Services/Credentials/RetiredSecretShredService.cs b/backend/src/CodeSpace.Core/Services/Credentials/RetiredSecretShredService.cs new file mode 100644 index 000000000..63174a2f7 --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/Credentials/RetiredSecretShredService.cs @@ -0,0 +1,85 @@ +using CodeSpace.Core.DependencyInjection; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Messages.Enums; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging; + +namespace CodeSpace.Core.Services.Credentials; + +/// +/// The pass behind : 0249's backfill (0249_shred_retired_secrets.sql), kind +/// for kind and predicate for predicate, run as one conditional UPDATE per kind. Each predicate is the row's retirement — +/// deleted, Revoked, or no longer named — so a live secret is never a candidate, and each matches the relaxed guard that +/// admits the blanking: a revision only once its credential is Revoked, an envelope only while no template names it, a +/// Secret variable's NULL only once it is deleted. Each retirement is terminal — nothing restores a deleted row, revives a +/// Revoked one, or names an envelope again — so a row once a candidate stays one; Postgres re-checks a row's own predicate +/// when a concurrent writer changed it first, and the guards re-check theirs as each row is written. +/// +/// What it does not do is what 0249 does not do: a connection an older build deleted keeps its group hooks registered +/// at the provider — they are Cancelled here with their secrets blanked, refused at ingestion, and removed at the provider +/// by hand — and its OAuth grants are not revoked at the provider. Only this build's delete asks the provider, while the +/// tokens it needs still open. +/// +public sealed class RetiredSecretShredService : IRetiredSecretShredService, IScopedDependency +{ + private readonly CodeSpaceDbContext _db; + private readonly ILogger _logger; + + public RetiredSecretShredService(CodeSpaceDbContext db, ILogger logger) + { + _db = db; + _logger = logger; + } + + public async Task ShredAsync(CancellationToken cancellationToken) + { + var shredded = await ShredDeletedSecretVariablesAsync(cancellationToken).ConfigureAwait(false); + shredded += await ShredRevokedStorageCredentialRevisionsAsync(cancellationToken).ConfigureAwait(false); + shredded += await ShredRetiredGitCredentialsAsync(cancellationToken).ConfigureAwait(false); + shredded += await ShredDeletedConnectionClientSecretsAsync(cancellationToken).ConfigureAwait(false); + shredded += await RetireDeletedConnectionWebhooksAsync(cancellationToken).ConfigureAwait(false); + shredded += await ShredUnnamedStorageDefaultEnvelopesAsync(cancellationToken).ConfigureAwait(false); + + if (shredded > 0) _logger.LogInformation("Shredded {SecretCount} retired secrets that were still decryptable, retired by a build older than migration 0249", shredded); + + return shredded; + } + + private async Task ShredDeletedSecretVariablesAsync(CancellationToken cancellationToken) => + await _db.Variable + .Where(v => v.DeletedDate != null && v.ValueEncrypted != null) + .ExecuteUpdateAsync(s => s.SetProperty(v => v.ValueEncrypted, (byte[]?)null), cancellationToken) + .ConfigureAwait(false); + + private async Task ShredRevokedStorageCredentialRevisionsAsync(CancellationToken cancellationToken) => + await _db.StorageCredentialRevision + .Where(r => r.EncryptedPayload != string.Empty && _db.StorageCredential.Any(c => c.TeamId == r.TeamId && c.Id == r.StorageCredentialId && c.State == StorageCredentialState.Revoked)) + .ExecuteUpdateAsync(s => s.SetProperty(r => r.EncryptedPayload, string.Empty), cancellationToken) + .ConfigureAwait(false); + + private async Task ShredRetiredGitCredentialsAsync(CancellationToken cancellationToken) => + await _db.Credential + .Where(c => c.EncryptedPayload != string.Empty && (c.DeletedDate != null || c.Status == CredentialStatus.Revoked)) + .ExecuteUpdateAsync(s => s.SetProperty(c => c.EncryptedPayload, string.Empty), cancellationToken) + .ConfigureAwait(false); + + private async Task ShredDeletedConnectionClientSecretsAsync(CancellationToken cancellationToken) => + await _db.ProviderInstance + .Where(p => p.DeletedDate != null && p.OauthClientSecretEnc != null) + .ExecuteUpdateAsync(s => s.SetProperty(p => p.OauthClientSecretEnc, (string?)null), cancellationToken) + .ConfigureAwait(false); + + /// A deleted connection's group hooks: out of service and holding no secret, the provider's hook id kept as the record of what to remove there by hand. + private async Task RetireDeletedConnectionWebhooksAsync(CancellationToken cancellationToken) => + await _db.ConnectionWebhook + .Where(w => (w.SecretEnc != string.Empty || w.RegistrationStatus != RepositoryWebhookRegistrationStatus.Cancelled) && _db.ProviderInstance.Any(p => p.Id == w.ProviderInstanceId && p.DeletedDate != null)) + .ExecuteUpdateAsync(s => s.SetProperty(w => w.SecretEnc, string.Empty).SetProperty(w => w.RegistrationStatus, RepositoryWebhookRegistrationStatus.Cancelled), cancellationToken) + .ConfigureAwait(false); + + private async Task ShredUnnamedStorageDefaultEnvelopesAsync(CancellationToken cancellationToken) => + await _db.StorageDefaultCredential + .Where(e => e.EncryptedPayload != string.Empty && !_db.StorageDefault.Any(t => t.CredentialId == e.Id)) + .ExecuteUpdateAsync(s => s.SetProperty(e => e.EncryptedPayload, string.Empty), cancellationToken) + .ConfigureAwait(false); +} diff --git a/backend/src/CodeSpace.Core/Services/OAuth/CredentialPayloadReader.cs b/backend/src/CodeSpace.Core/Services/OAuth/CredentialPayloadReader.cs index 0b23d5748..917ec1abc 100644 --- a/backend/src/CodeSpace.Core/Services/OAuth/CredentialPayloadReader.cs +++ b/backend/src/CodeSpace.Core/Services/OAuth/CredentialPayloadReader.cs @@ -10,16 +10,22 @@ public sealed class CredentialPayloadReader : ICredentialPayloadReader, IScopedD public CredentialPayloadReader(CodeSpaceConnectionString connectionString) { _connectionString = connectionString; } - public async Task ReadEncryptedPayloadAsync(Guid credentialId, CancellationToken cancellationToken) + public async Task ReadEncryptedPayloadAsync(Guid credentialId, CancellationToken cancellationToken) => + await ReadScalarAsync("SELECT encrypted_payload FROM credential WHERE id = @id", credentialId, cancellationToken).ConfigureAwait(false) as string + ?? throw new InvalidOperationException($"Credential {credentialId} no longer exists"); + + public async Task IsRetiredAsync(Guid credentialId, CancellationToken cancellationToken) => + (bool)(await ReadScalarAsync("SELECT NOT EXISTS (SELECT 1 FROM credential WHERE id = @id AND deleted_date IS NULL)", credentialId, cancellationToken).ConfigureAwait(false))!; + + private async Task ReadScalarAsync(string sql, Guid credentialId, CancellationToken cancellationToken) { await using var conn = new NpgsqlConnection(_connectionString.Value); await conn.OpenAsync(cancellationToken).ConfigureAwait(false); await using var cmd = conn.CreateCommand(); - cmd.CommandText = "SELECT encrypted_payload FROM credential WHERE id = @id"; + cmd.CommandText = sql; cmd.Parameters.AddWithValue("@id", credentialId); - return await cmd.ExecuteScalarAsync(cancellationToken).ConfigureAwait(false) as string - ?? throw new InvalidOperationException($"Credential {credentialId} no longer exists"); + return await cmd.ExecuteScalarAsync(cancellationToken).ConfigureAwait(false); } } diff --git a/backend/src/CodeSpace.Core/Services/OAuth/GitHubOAuthClient.cs b/backend/src/CodeSpace.Core/Services/OAuth/GitHubOAuthClient.cs index 29c2e099b..ae848d236 100644 --- a/backend/src/CodeSpace.Core/Services/OAuth/GitHubOAuthClient.cs +++ b/backend/src/CodeSpace.Core/Services/OAuth/GitHubOAuthClient.cs @@ -88,14 +88,20 @@ public async Task RefreshAsync(OAuthRefreshInput input, Canc return await PostTokenAsync(input.Instance, form, cancellationToken).ConfigureAwait(false); } + /// + /// DELETE /applications/{client_id}/grant for a revoke: it ends the + /// user's grant — every access and refresh token GitHub ever issued that user under the client id, including one + /// another team connected through the same OAuth App holds. Otherwise DELETE /applications/{client_id}/token, + /// which ends the one access token presented. GitHub has no endpoint that ends a refresh token on its own, so outside + /// a whole-grant revoke a refresh token is not sent at all. Both endpoints authenticate as the app and take the token + /// as access_token; both are idempotent — a token already gone answers 404 or 422. + /// public async Task RevokeAsync(OAuthRevokeInput input, CancellationToken cancellationToken) { - // DELETE /applications/{client_id}/grant revokes the user's grant entirely — kills - // every access + refresh token GitHub ever issued under our client_id for that user. - // Surgical alternative is /token (revoke single token) but for a CodeSpace credential - // we always want the whole grant gone. Idempotent: subsequent calls return 404. + if (!input.WholeGrant && input.TokenTypeHint == "refresh_token") return; + var apiBase = ResolveApiBase(input.Instance); - var url = new Uri($"{apiBase}/applications/{Uri.EscapeDataString(input.ClientId)}/grant"); + var url = new Uri($"{apiBase}/applications/{Uri.EscapeDataString(input.ClientId)}/{(input.WholeGrant ? "grant" : "token")}"); using var http = _httpClientFactory.CreateClient(); using var request = new HttpRequestMessage(HttpMethod.Delete, url) diff --git a/backend/src/CodeSpace.Core/Services/OAuth/ICredentialPayloadReader.cs b/backend/src/CodeSpace.Core/Services/OAuth/ICredentialPayloadReader.cs index eadec5a92..c4522665a 100644 --- a/backend/src/CodeSpace.Core/Services/OAuth/ICredentialPayloadReader.cs +++ b/backend/src/CodeSpace.Core/Services/OAuth/ICredentialPayloadReader.cs @@ -10,4 +10,11 @@ namespace CodeSpace.Core.Services.OAuth; public interface ICredentialPayloadReader { Task ReadEncryptedPayloadAsync(Guid credentialId, CancellationToken cancellationToken); + + /// + /// True once the credential is soft-deleted — a retirement: a connection delete, a member's departure or a relink, + /// each of which blanks the token and takes the row out of the team's list — or gone. False while the row is listed, + /// a Disconnected one included: a Disconnect blanks the token and leaves the row Revoked in the list. + /// + Task IsRetiredAsync(Guid credentialId, CancellationToken cancellationToken); } diff --git a/backend/src/CodeSpace.Core/Services/OAuth/IOAuthClient.cs b/backend/src/CodeSpace.Core/Services/OAuth/IOAuthClient.cs index 95360e949..4f0ae92c3 100644 --- a/backend/src/CodeSpace.Core/Services/OAuth/IOAuthClient.cs +++ b/backend/src/CodeSpace.Core/Services/OAuth/IOAuthClient.cs @@ -65,6 +65,14 @@ public sealed record OAuthRevokeInput /// "access_token" or "refresh_token" — providers use this only as a hint. public string? TokenTypeHint { get; init; } + + /// + /// True ends the user's whole grant to the OAuth app where a provider revokes per grant: on GitHub, every token it ever + /// issued that user under , wherever it is held — including a token another team connected + /// through the same app. False ends only . A provider that revokes per token (GitLab, RFC 7009) + /// sends the same request either way. Default true: a Disconnect ends the grant, as it always has. + /// + public bool WholeGrant { get; init; } = true; } public sealed record OAuthTokenResponse diff --git a/backend/src/CodeSpace.Core/Services/OAuth/IOAuthGrantRevoker.cs b/backend/src/CodeSpace.Core/Services/OAuth/IOAuthGrantRevoker.cs new file mode 100644 index 000000000..4ca178dfd --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/OAuth/IOAuthGrantRevoker.cs @@ -0,0 +1,25 @@ +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Messages.Credentials; + +namespace CodeSpace.Core.Services.OAuth; + +/// +/// The provider side of ending an OAuth credential: a Disconnect asks it for one credential, deleting a connection for +/// every credential connected through it. +/// +public interface IOAuthGrantRevoker +{ + /// The OAuth tokens a credential holds, or null when its stored payload does not open or does not read as OAuth tokens. + OAuthPayload? ReadPayload(Credential credential); + + /// + /// Ask the provider names to revoke the refresh token, when there is one, then the access + /// token, presenting the connection's OAuth app as OauthClientId and . The secret + /// is handed in, not read off the connection, so a caller that has already blanked it can still revoke. + /// is on every request: a Disconnect ends the + /// user's grant to the app; a connection delete ends only these tokens, because the grant also carries the user's + /// tokens in every other team connected through the same app. Throws what the provider answers; the caller decides + /// that the revoke is best effort. + /// + Task RevokeAsync(ProviderInstance instance, string clientSecret, OAuthPayload payload, bool wholeGrant, CancellationToken cancellationToken); +} diff --git a/backend/src/CodeSpace.Core/Services/OAuth/OAuthGrantRevoker.cs b/backend/src/CodeSpace.Core/Services/OAuth/OAuthGrantRevoker.cs new file mode 100644 index 000000000..58ba0a543 --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/OAuth/OAuthGrantRevoker.cs @@ -0,0 +1,61 @@ +using CodeSpace.Core.DependencyInjection; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Credentials; +using CodeSpace.Messages.Credentials; +using CodeSpace.Messages.Enums; + +namespace CodeSpace.Core.Services.OAuth; + +/// +/// Scoped, not singleton, so a scope that swaps the OAuth client registry (a test standing in for the provider) is the +/// registry this revoker asks. +/// +public sealed class OAuthGrantRevoker : IOAuthGrantRevoker, IScopedDependency +{ + private readonly IOAuthClientRegistry _oauthClients; + private readonly IPayloadEncryptor _encryptor; + private readonly ICredentialPayloadSerializer _serializer; + + public OAuthGrantRevoker(IOAuthClientRegistry oauthClients, IPayloadEncryptor encryptor, ICredentialPayloadSerializer serializer) + { + _oauthClients = oauthClients; + _encryptor = encryptor; + _serializer = serializer; + } + + public OAuthPayload? ReadPayload(Credential credential) + { + try + { + var json = _encryptor.Decrypt(credential.EncryptedPayload); + return _serializer.Deserialize(AuthType.OAuth, json) as OAuthPayload; + } + catch + { + return null; + } + } + + public async Task RevokeAsync(ProviderInstance instance, string clientSecret, OAuthPayload payload, bool wholeGrant, CancellationToken cancellationToken) + { + var client = _oauthClients.Get(instance.Provider); + + // Revoke refresh_token first (more durable — kills the grant on most providers), then + // access_token. Both are idempotent on the provider side; the caller's catch clears + // local payload regardless. + if (!string.IsNullOrEmpty(payload.RefreshToken)) + await client.RevokeAsync(Input(instance, clientSecret, payload.RefreshToken, "refresh_token", wholeGrant), cancellationToken).ConfigureAwait(false); + + await client.RevokeAsync(Input(instance, clientSecret, payload.AccessToken, "access_token", wholeGrant), cancellationToken).ConfigureAwait(false); + } + + private static OAuthRevokeInput Input(ProviderInstance instance, string clientSecret, string token, string tokenTypeHint, bool wholeGrant) => new() + { + Instance = instance, + ClientId = instance.OauthClientId!, + ClientSecret = clientSecret, + Token = token, + TokenTypeHint = tokenTypeHint, + WholeGrant = wholeGrant + }; +} diff --git a/backend/src/CodeSpace.Core/Services/OAuth/OAuthTokenRefresher.cs b/backend/src/CodeSpace.Core/Services/OAuth/OAuthTokenRefresher.cs index 1ae5e3b43..4bba84745 100644 --- a/backend/src/CodeSpace.Core/Services/OAuth/OAuthTokenRefresher.cs +++ b/backend/src/CodeSpace.Core/Services/OAuth/OAuthTokenRefresher.cs @@ -137,15 +137,20 @@ private OAuthPayload Adopt(Credential credential, string storedPayload) } /// - /// The pair the provider issued for a credential revoked meanwhile is stored nowhere; revoke it there too, so the grant - /// the user disconnected does not stay live. Best effort, like the revocation itself. + /// The pair the provider issued for a credential revoked meanwhile is stored nowhere; revoke it there too, the way the + /// write that emptied the row ends tokens. A Disconnect ends the user's grant, so the pair goes with it and the grant + /// the user disconnected does not stay live. A retirement — a connection delete, a departure, a relink — soft-deletes + /// the row and ends tokens, never the grant: on GitHub the grant also carries the user's tokens in every other team + /// connected through the same OAuth App. Best effort, like the revocation itself. /// private async Task RevokeIssuedTokensAsync(ProviderContext context, OAuthPayload issued) { try { - await RevokeAtProviderAsync(context.Instance, issued.RefreshToken!, "refresh_token").ConfigureAwait(false); - await RevokeAtProviderAsync(context.Instance, issued.AccessToken, "access_token").ConfigureAwait(false); + var wholeGrant = !await _payloadReader.IsRetiredAsync(context.Credential.Id, CancellationToken.None).ConfigureAwait(false); + + await RevokeAtProviderAsync(context.Instance, issued.RefreshToken!, "refresh_token", wholeGrant).ConfigureAwait(false); + await RevokeAtProviderAsync(context.Instance, issued.AccessToken, "access_token", wholeGrant).ConfigureAwait(false); } catch (Exception ex) { @@ -153,9 +158,9 @@ private async Task RevokeIssuedTokensAsync(ProviderContext context, OAuthPayload } } - private async Task RevokeAtProviderAsync(ProviderInstance instance, string token, string tokenTypeHint) + private async Task RevokeAtProviderAsync(ProviderInstance instance, string token, string tokenTypeHint, bool wholeGrant) { - var input = new OAuthRevokeInput { Instance = instance, ClientId = instance.OauthClientId!, ClientSecret = _encryptor.Decrypt(instance.OauthClientSecretEnc!), Token = token, TokenTypeHint = tokenTypeHint }; + var input = new OAuthRevokeInput { Instance = instance, ClientId = instance.OauthClientId!, ClientSecret = _encryptor.Decrypt(instance.OauthClientSecretEnc!), Token = token, TokenTypeHint = tokenTypeHint, WholeGrant = wholeGrant }; await _oauthClients.Get(instance.Provider).RevokeAsync(input, CancellationToken.None).ConfigureAwait(false); } diff --git a/backend/src/CodeSpace.Core/Services/Projects/ProjectService.cs b/backend/src/CodeSpace.Core/Services/Projects/ProjectService.cs index 0a9cd1a07..d23c101dc 100644 --- a/backend/src/CodeSpace.Core/Services/Projects/ProjectService.cs +++ b/backend/src/CodeSpace.Core/Services/Projects/ProjectService.cs @@ -2,6 +2,7 @@ using System.Text.RegularExpressions; using CodeSpace.Core.DependencyInjection; using CodeSpace.Core.Services.Slugs; +using CodeSpace.Core.Services.Variables; using CodeSpace.Core.Persistence.Db; using CodeSpace.Core.Persistence.Entities; using CodeSpace.Messages.Constants; @@ -324,17 +325,13 @@ public async Task DeleteAsync(Guid teamId, Guid projectId, Guid actorUserId, Can project.LastModifiedBy = actorUserId; // Cascade: soft-delete every project-scoped variable so {{project.{Slug}.X}} references - // resolve to null going forward. We don't hard-delete — operator audit needs the trail. + // resolve to null going forward. We don't hard-delete — operator audit needs the trail — + // but a Secret's ciphertext goes, as on every variable delete. var variableRows = await _db.Variable .Where(v => v.Scope == VariableScope.Project && v.ScopeId == projectId && v.DeletedDate == null) .ToListAsync(cancellationToken).ConfigureAwait(false); - foreach (var v in variableRows) - { - v.DeletedDate = now; - v.LastModifiedDate = now; - v.LastModifiedBy = actorUserId; - } + foreach (var v in variableRows) VariableRetirement.Retire(v, now, actorUserId); await _db.SaveChangesAsync(cancellationToken).ConfigureAwait(false); _logger.LogInformation( diff --git a/backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderInstanceService.Retirement.cs b/backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderInstanceService.Retirement.cs new file mode 100644 index 000000000..62b889336 --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderInstanceService.Retirement.cs @@ -0,0 +1,169 @@ +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Messages.Credentials; +using CodeSpace.Messages.Enums; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging; + +namespace CodeSpace.Core.Services.ProviderInstances; + +/// +/// What deleting a connection does to the hooks and tokens connected through it. Its group hooks come down first, while +/// the tokens that registered them still open (). Then each token is retired +/// the way every other retirement retires one (): token +/// blanked, Revoked, soft-deleted, repositories bound through it told they need a new credential, act-as identities +/// linked to it gone. The connection's own OAuth client secret is blanked by the delete. Nothing it held stays +/// decryptable in the database. +/// +/// Each OAuth credential's tokens are then revoked at the provider, best effort, and only once the delete has +/// committed: a provider asked from inside the transaction would have ended them even when the delete rolled back, leaving +/// Active credentials on a token the provider no longer honours. The tokens and the client secret that revoke needs are +/// read before they are blanked and held in memory until then. Only those tokens, never the user's whole grant: the grant +/// is the user's, per OAuth app, and another team connected through the same app holds tokens under it that this delete +/// must leave working — on GitHub, the grant delete would end them all. +/// +public sealed partial class ProviderInstanceService +{ + /// + /// The longest a delete waits on the provider for all of its revokes together. The delete has committed by then and + /// the caller is still waiting for its answer; a provider that never answers — often the very reason a connection is + /// being removed — must not hold that answer for a socket timeout per token. + /// + internal static readonly TimeSpan ProviderRevocationBudget = TimeSpan.FromSeconds(30); + + private const string RetiredByDelete = "Provider instance deleted"; + + /// + /// Takes the connection's group hooks out of service the way a switch back to per-repository scope does: each is + /// deleted at the provider best effort, the Registered rows go, the rest are cancelled. It has to happen here, before + /// : a hook is deleted with the token that registered it, and once that token is + /// blanked nothing could ever remove a hook still delivering the group's events. Like a forced unbind's hook teardown, + /// the provider is asked from inside the transaction; a delete that then rolled back would leave a hook row naming a + /// hook that is gone, never a live hook with no way to remove it. + /// + /// Then every hook row the connection keeps loses its secret. None is in service, so no delivery is verified + /// against it again — ingestion refuses a retired hook before it reads the secret. + /// + private async Task RetireConnectionWebhooksAsync(ProviderInstance instance, CancellationToken cancellationToken) + { + var retired = await _connectionHooks.RetireAllAsync(instance, cancellationToken).ConfigureAwait(false); + + await ShredConnectionWebhookSecretsAsync(instance.Id, cancellationToken).ConfigureAwait(false); + + if (retired > 0) _logger.LogInformation("Retired {ConnectionWebhookCount} group hooks of provider instance {ProviderInstanceId} before deleting it", retired, instance.Id); + } + + private async Task ShredConnectionWebhookSecretsAsync(Guid providerInstanceId, CancellationToken cancellationToken) => + await _db.ConnectionWebhook + .Where(w => w.ProviderInstanceId == providerInstanceId && w.SecretEnc != string.Empty) + .ExecuteUpdateAsync(s => s.SetProperty(w => w.SecretEnc, string.Empty), cancellationToken) + .ConfigureAwait(false); + + private async Task RetireCredentialsAsync(ProviderInstance instance, CancellationToken cancellationToken) + { + var credentials = await LoadUnretiredCredentialsAsync(instance.Id, cancellationToken).ConfigureAwait(false); + var grants = ReadGrants(instance, credentials); + + foreach (var credential in credentials) await RetireAsync(credential, cancellationToken).ConfigureAwait(false); + + await RevokeGrantsAfterCommitAsync(instance, grants, cancellationToken).ConfigureAwait(false); + + return credentials.Count; + } + + /// Every credential of the connection that still holds a token: not soft-deleted, and not already Revoked — a Disconnect blanked those. + private async Task> LoadUnretiredCredentialsAsync(Guid providerInstanceId, CancellationToken cancellationToken) => + await _db.Credential + .Where(c => c.ProviderInstanceId == providerInstanceId && c.DeletedDate == null && c.Status != CredentialStatus.Revoked) + .ToListAsync(cancellationToken).ConfigureAwait(false); + + private async Task RetireAsync(Credential credential, CancellationToken cancellationToken) + { + await _revocation.RetireAsync(credential, cancellationToken).ConfigureAwait(false); + + credential.LastError = RetiredByDelete; + } + + /// + /// What revoking each OAuth credential's tokens at the provider takes, read while the credentials and the connection still hold it. + /// Nothing when the connection has no OAuth app (only personal and group tokens, which no provider endpoint revokes), + /// or when its client secret no longer opens. + /// + private OAuthGrants ReadGrants(ProviderInstance instance, IReadOnlyList credentials) + { + var clientSecret = ReadClientSecret(instance); + + if (clientSecret == null) return OAuthGrants.None; + + var tokens = credentials + .Where(c => c.AuthType == AuthType.OAuth) + .Select(c => (c.Id, Payload: _grants.ReadPayload(c))) + .Where(grant => grant.Payload != null) + .Select(grant => (grant.Id, grant.Payload!)) + .ToList(); + + return new OAuthGrants(clientSecret, tokens); + } + + private string? ReadClientSecret(ProviderInstance instance) + { + if (string.IsNullOrWhiteSpace(instance.OauthClientId) || string.IsNullOrWhiteSpace(instance.OauthClientSecretEnc)) return null; + + try + { + return _encryptor.Decrypt(instance.OauthClientSecretEnc); + } + catch (Exception ex) + { + _logger.LogWarning(ex, "The OAuth client secret of provider instance {ProviderInstanceId} does not open; its credentials are retired here but cannot be revoked at the provider", instance.Id); + return null; + } + } + + private async Task RevokeGrantsAfterCommitAsync(ProviderInstance instance, OAuthGrants grants, CancellationToken cancellationToken) + { + if (grants.Tokens.Count == 0) return; + + await _postCommit.RunAfterCommitAsync(ct => RevokeGrantsAsync(instance, grants, ct), cancellationToken).ConfigureAwait(false); + } + + private async Task RevokeGrantsAsync(ProviderInstance instance, OAuthGrants grants, CancellationToken cancellationToken) + { + using var budget = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + budget.CancelAfter(ProviderRevocationBudget); + + foreach (var (credentialId, payload) in grants.Tokens) await RevokeGrantAsync(instance, grants.ClientSecret, credentialId, payload, budget.Token).ConfigureAwait(false); + } + + private async Task RevokeGrantAsync(ProviderInstance instance, string clientSecret, Guid credentialId, OAuthPayload payload, CancellationToken cancellationToken) + { + try + { + await _grants.RevokeAsync(instance, clientSecret, payload, wholeGrant: false, cancellationToken).ConfigureAwait(false); + + _logger.LogInformation("Revoked the OAuth tokens of credential {CredentialId} at the provider after its provider instance {ProviderInstanceId} was deleted", credentialId, instance.Id); + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Revoking the OAuth tokens of credential {CredentialId} at the provider failed after its provider instance {ProviderInstanceId} was deleted; the token is already gone from CodeSpace and stays valid at the provider until it expires or is revoked there", credentialId, instance.Id); + } + } + + /// + /// The connection's client secret and each OAuth credential's tokens, in memory only, between the delete's read of + /// them and the provider's revoke. A class, not a record: a record's generated ToString would print the secret. + /// + private sealed class OAuthGrants + { + public static readonly OAuthGrants None = new(string.Empty, Array.Empty<(Guid, OAuthPayload)>()); + + public OAuthGrants(string clientSecret, IReadOnlyList<(Guid CredentialId, OAuthPayload Payload)> tokens) + { + ClientSecret = clientSecret; + Tokens = tokens; + } + + public string ClientSecret { get; } + + public IReadOnlyList<(Guid CredentialId, OAuthPayload Payload)> Tokens { get; } + } +} diff --git a/backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderInstanceService.cs b/backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderInstanceService.cs index c0eabf03b..0cad3147e 100644 --- a/backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderInstanceService.cs +++ b/backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderInstanceService.cs @@ -3,9 +3,11 @@ using CodeSpace.Core.Persistence.Entities; using CodeSpace.Core.Services.Credentials; using CodeSpace.Core.Services.Identity; +using CodeSpace.Core.Services.OAuth; using CodeSpace.Core.Middlewares.Transactional; using CodeSpace.Core.Services.Redaction; using CodeSpace.Core.Services.RepositoryBinding; +using CodeSpace.Core.Services.Webhooks.Registration; using CodeSpace.Core.Services.Webhooks.Scope; using CodeSpace.Messages.Commands.ProviderInstances; using CodeSpace.Messages.Dtos.ProviderInstances; @@ -17,7 +19,7 @@ namespace CodeSpace.Core.Services.ProviderInstances; -public sealed class ProviderInstanceService : IProviderInstanceService, IScopedDependency +public sealed partial class ProviderInstanceService : IProviderInstanceService, IScopedDependency { private readonly CodeSpaceDbContext _db; private readonly ICurrentTeam _currentTeam; @@ -25,11 +27,14 @@ public sealed class ProviderInstanceService : IProviderInstanceService, IScopedD private readonly IPayloadEncryptor _encryptor; private readonly IRepositoryBindingService _binding; private readonly IWebhookScopeTransitionService _webhookScope; + private readonly IConnectionWebhookProvisioner _connectionHooks; + private readonly ICredentialRevocationService _revocation; + private readonly IOAuthGrantRevoker _grants; private readonly IPostCommitActions _postCommit; private readonly TimeProvider _clock; private readonly ILogger _logger; - public ProviderInstanceService(CodeSpaceDbContext db, ICurrentTeam currentTeam, ICurrentUser currentUser, IPayloadEncryptor encryptor, IRepositoryBindingService binding, IWebhookScopeTransitionService webhookScope, IPostCommitActions postCommit, TimeProvider clock, ILogger logger) + public ProviderInstanceService(CodeSpaceDbContext db, ICurrentTeam currentTeam, ICurrentUser currentUser, IPayloadEncryptor encryptor, IRepositoryBindingService binding, IWebhookScopeTransitionService webhookScope, IConnectionWebhookProvisioner connectionHooks, ICredentialRevocationService revocation, IOAuthGrantRevoker grants, IPostCommitActions postCommit, TimeProvider clock, ILogger logger) { _db = db; _currentTeam = currentTeam; @@ -37,6 +42,9 @@ public ProviderInstanceService(CodeSpaceDbContext db, ICurrentTeam currentTeam, _encryptor = encryptor; _binding = binding; _webhookScope = webhookScope; + _connectionHooks = connectionHooks; + _revocation = revocation; + _grants = grants; _postCommit = postCommit; _clock = clock; _logger = logger; @@ -230,8 +238,10 @@ public async Task DeleteAsync(Guid providerInstanc var instance = await LoadOwnedInstanceAsync(providerInstanceId, teamId, cancellationToken).ConfigureAwait(false); var unboundCount = await ResolveBoundRepositoriesAsync(instance, force, cancellationToken).ConfigureAwait(false); - var revokedCount = await CascadeRevokeCredentialsAsync(instance.Id, cancellationToken).ConfigureAwait(false); + await RetireConnectionWebhooksAsync(instance, cancellationToken).ConfigureAwait(false); + var revokedCount = await RetireCredentialsAsync(instance, cancellationToken).ConfigureAwait(false); + instance.OauthClientSecretEnc = null; instance.DeletedDate = _clock.GetUtcNow(); return new DeleteProviderInstanceResult @@ -355,24 +365,6 @@ private async Task ResolveBoundRepositoriesAsync(ProviderInstance instance, return activeRepoIds.Count; } - private async Task CascadeRevokeCredentialsAsync(Guid providerInstanceId, CancellationToken cancellationToken) - { - var now = _clock.GetUtcNow(); - - var activeCredentials = await _db.Credential - .Where(c => c.ProviderInstanceId == providerInstanceId && c.DeletedDate == null && c.Status == CredentialStatus.Active) - .ToListAsync(cancellationToken).ConfigureAwait(false); - - foreach (var credential in activeCredentials) - { - credential.Status = CredentialStatus.Revoked; - credential.DeletedDate = now; - credential.LastError = "Provider instance deleted"; - } - - return activeCredentials.Count; - } - /// Empty / whitespace-only client IDs collapse to null so the form's "not yet configured" state and a literal empty string aren't treated as distinct identities by the unique index. private static string? NormaliseClientId(string? raw) => string.IsNullOrWhiteSpace(raw) ? null : raw.Trim(); diff --git a/backend/src/CodeSpace.Core/Services/Variables/VariableRetirement.cs b/backend/src/CodeSpace.Core/Services/Variables/VariableRetirement.cs new file mode 100644 index 000000000..a8a9823bc --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/Variables/VariableRetirement.cs @@ -0,0 +1,21 @@ +using CodeSpace.Core.Persistence.Entities; + +namespace CodeSpace.Core.Services.Variables; + +/// +/// What deleting a variable leaves: the row, for the audit trail, and nothing a secret could be recovered from. Every +/// read of a variable filters on deleted_date IS NULL and nothing restores a deleted one, so a deleted Secret's +/// ciphertext could only ever be opened by someone holding a copy of the database and the master key. A plain value is +/// no secret and stays as the record of what the variable held. One rule for every way a variable is deleted — on its +/// own, or with the project it belongs to. +/// +public static class VariableRetirement +{ + public static void Retire(Variable row, DateTimeOffset now, Guid actorUserId) + { + row.DeletedDate = now; + row.LastModifiedDate = now; + row.LastModifiedBy = actorUserId; + row.ValueEncrypted = null; + } +} diff --git a/backend/src/CodeSpace.Core/Services/Variables/VariableService.cs b/backend/src/CodeSpace.Core/Services/Variables/VariableService.cs index d79531e1b..a2a3b284f 100644 --- a/backend/src/CodeSpace.Core/Services/Variables/VariableService.cs +++ b/backend/src/CodeSpace.Core/Services/Variables/VariableService.cs @@ -84,10 +84,7 @@ public async Task DeleteAsync(VariableScope scope, Guid scopeId, Guid expectedTe var row = await LoadActiveRowAsync(scope, scopeId, name, cancellationToken).ConfigureAwait(false); if (row == null) return; // idempotent — no-op on missing-name - var now = DateTimeOffset.UtcNow; - row.DeletedDate = now; - row.LastModifiedDate = now; - row.LastModifiedBy = actorUserId; + VariableRetirement.Retire(row, DateTimeOffset.UtcNow, actorUserId); await _db.SaveChangesAsync(cancellationToken).ConfigureAwait(false); diff --git a/backend/src/CodeSpace.Core/Services/Workflows/Artifacts/Credentials/StorageCredentialService.cs b/backend/src/CodeSpace.Core/Services/Workflows/Artifacts/Credentials/StorageCredentialService.cs index 2619d4d9a..83b0bb6f3 100644 --- a/backend/src/CodeSpace.Core/Services/Workflows/Artifacts/Credentials/StorageCredentialService.cs +++ b/backend/src/CodeSpace.Core/Services/Workflows/Artifacts/Credentials/StorageCredentialService.cs @@ -130,9 +130,26 @@ public async Task CreateAsync(Guid teamId, Guid actor credential.RevokedBy = actorId; await SaveConcurrentAsync("The storage credential changed before it could be revoked.", cancellationToken).ConfigureAwait(false); + await ShredRevisionsAsync(teamId, credential.Id, cancellationToken).ConfigureAwait(false); return Metadata(Projection(credential, current)); } + /// + /// Blank the encrypted payload of every revision of a credential that has just been revoked; revision, provider, + /// hint, envelope fingerprint and authorship stay for the audit trail. Nothing reads them again — the resolver admits + /// only an Active credential, revocation is terminal, and it was refused while a stored location or a live profile + /// still resolved through the credential — so the ciphertext could only ever be opened from a database copy. + /// + /// After the revocation is saved, and as its own statement, because the revision guard admits the blanking only + /// once it sees the credential Revoked. Superseded revisions of an Active credential are never blanked: a profile + /// pins an exact revision, so each of them still decrypts the bytes stored through it. + /// + private async Task ShredRevisionsAsync(Guid teamId, Guid credentialId, CancellationToken cancellationToken) => + await _db.StorageCredentialRevision + .Where(value => value.TeamId == teamId && value.StorageCredentialId == credentialId && value.EncryptedPayload != string.Empty) + .ExecuteUpdateAsync(setters => setters.SetProperty(value => value.EncryptedPayload, string.Empty), cancellationToken) + .ConfigureAwait(false); + /// /// Revocation is terminal and the secret resolver admits none but an Active credential, so revoking one a stored /// object still depends on makes those bytes permanently unreadable — the same stranding the profile lifecycle diff --git a/backend/src/CodeSpace.Core/Services/Workflows/Artifacts/Defaults/StorageDefaultService.cs b/backend/src/CodeSpace.Core/Services/Workflows/Artifacts/Defaults/StorageDefaultService.cs index c8daaf98b..d5436e0ff 100644 --- a/backend/src/CodeSpace.Core/Services/Workflows/Artifacts/Defaults/StorageDefaultService.cs +++ b/backend/src/CodeSpace.Core/Services/Workflows/Artifacts/Defaults/StorageDefaultService.cs @@ -73,12 +73,15 @@ public async Task CreateAsync(Guid actorId, CreateStorageD var prepared = PrepareTemplate(Input(command, template.DataClassTypeKey)); var now = _clock.GetUtcNow(); + var previousCredentialId = template.CredentialId; var credentialId = await ResolveCredentialAsync(template, command, actorId, now).ConfigureAwait(false); Apply(template, prepared, actorId, now); template.Revision = checked(template.Revision + 1); template.CredentialId = credentialId; await SaveAsync("The deployment storage default changed before this edit could be applied.", cancellationToken).ConfigureAwait(false); + await ShredReplacedEnvelopeAsync(previousCredentialId, credentialId, cancellationToken).ConfigureAwait(false); + return await RequireAsync(template.Id, cancellationToken).ConfigureAwait(false); } @@ -140,7 +143,8 @@ private PreparedTemplate PrepareTemplate(TemplateInput input) /// /// Which envelope the edited template points at: a newly appended one when a secret is supplied, none when the - /// operator clears it, otherwise the one it already had. Superseded envelopes are never overwritten in place. + /// operator clears it, otherwise the one it already had. A superseded envelope is never rewritten, only blanked once + /// the edit has saved (). /// /// A RETAINED envelope must still belong to the template's provider. Repointing a template at a different /// provider while silently keeping the old provider's secret would leave a template that reads as complete and @@ -158,6 +162,24 @@ private PreparedTemplate PrepareTemplate(TemplateInput input) return retained; } + /// + /// Blanks the envelope an edit stopped naming: the one a new secret replaced, or the one a clear detached. Only the + /// materializer decrypts an envelope, and only the one its template names, under a share lock on that template, so + /// this one has no reader left; kept, it would leave a deployment-wide cloud key decryptable from every copy of the + /// database for good. Its fingerprint and hint stay as history. Runs after the repoint has saved, because 0249's guard + /// admits the blanking only once no template names the envelope, and asks the same here so an envelope some other + /// template still names is left alone rather than failing the edit. + /// + private async Task ShredReplacedEnvelopeAsync(Guid? previousCredentialId, Guid? credentialId, CancellationToken cancellationToken) + { + if (previousCredentialId is not { } replaced || replaced == credentialId) return; + + await _db.StorageDefaultCredential + .Where(envelope => envelope.Id == replaced && envelope.EncryptedPayload != string.Empty && !_db.StorageDefault.Any(template => template.CredentialId == envelope.Id)) + .ExecuteUpdateAsync(setters => setters.SetProperty(envelope => envelope.EncryptedPayload, string.Empty), cancellationToken) + .ConfigureAwait(false); + } + private async Task EnsureRetainedCredentialMatchesAsync(Guid credentialId, string providerTypeKey) { var module = RequireModule(providerTypeKey); diff --git a/backend/src/CodeSpace.Messages/Authorization/IRequireCredentialSpend.cs b/backend/src/CodeSpace.Messages/Authorization/IRequireCredentialSpend.cs new file mode 100644 index 000000000..28e8cad4a --- /dev/null +++ b/backend/src/CodeSpace.Messages/Authorization/IRequireCredentialSpend.cs @@ -0,0 +1,15 @@ +namespace CodeSpace.Messages.Authorization; + +/// +/// Marker — a request that presents one credential, named by its id, to its provider on the caller's behalf. Its owner +/// may spend their own Personal credential; anyone else needs TeamPermissions.ReposManage. +/// +/// Extends , so the credential is first proven to be the X-Team-Id +/// team's and the caller a member of it; this tier only decides whether membership is enough. It is not: a provider +/// answers with everything the token's account can see, so on membership alone any Viewer could spend the Owner's +/// personal token and read back private repositories outside the team. The listings such a request serves feed a bind, +/// and a bind takes repos.manage — the same bar here, for a teammate's credential and for the team's own. +/// +public interface IRequireCredentialSpend : IRequireCredentialAccess +{ +} diff --git a/backend/src/CodeSpace.Messages/Commands/Credentials/ShredRetiredSecretsCommand.cs b/backend/src/CodeSpace.Messages/Commands/Credentials/ShredRetiredSecretsCommand.cs new file mode 100644 index 000000000..42c1aa068 --- /dev/null +++ b/backend/src/CodeSpace.Messages/Commands/Credentials/ShredRetiredSecretsCommand.cs @@ -0,0 +1,19 @@ +using CodeSpace.Messages.Mediation; + +namespace CodeSpace.Messages.Commands.Credentials; + +/// +/// Blank the ciphertext of every secret that is retired but still decryptable — the rows migration 0249 blanks on upgrade: +/// deleted Secret variables, revoked or soft-deleted git tokens, a deleted connection's OAuth client secret and group +/// hooks, every revision of a revoked storage credential, and deployment-default envelopes no template names. Retiring +/// one blanks it in this build; 0249 blanks what earlier builds left, but runs once, so anything a build older than 0249 +/// retires afterwards — an older pod still serving during a rolling deploy, or the whole fleet after a rollback — keeps +/// its ciphertext until this runs. Idempotent: a blanked row is no longer a candidate, so an idle pass changes nothing. +/// NOT tenant-scoped: a system-wide repair that runs without an actor context (mirrors +/// RetireDepartedMembersCredentialsCommand). +/// +/// NOT transactional (): each kind of secret is blanked by one statement that +/// commits on its own, so a statement that fails leaves the others' work in place and its own rows candidates for the +/// next pass. One transaction around the pass would hold every candidate row's lock until the last statement. +/// +public sealed record ShredRetiredSecretsCommand : ICommand, INonTransactionalCommand; diff --git a/backend/src/CodeSpace.Messages/Commands/ProviderInstances/DeleteProviderInstanceCommand.cs b/backend/src/CodeSpace.Messages/Commands/ProviderInstances/DeleteProviderInstanceCommand.cs index 70f68e8a1..41d377af6 100644 --- a/backend/src/CodeSpace.Messages/Commands/ProviderInstances/DeleteProviderInstanceCommand.cs +++ b/backend/src/CodeSpace.Messages/Commands/ProviderInstances/DeleteProviderInstanceCommand.cs @@ -16,8 +16,12 @@ namespace CodeSpace.Messages.Commands.ProviderInstances; /// "Unbind all and remove" one-click path the UI offers when the operator explicitly /// confirms the impact. /// -/// Active credentials are always cascade-revoked in the same transaction — they're -/// useless once the provider is gone, and leaving them as Active would lie to the UI. +/// The connection's group hooks are deleted at the provider first, best effort, while the +/// tokens that registered them still work; the hook rows it keeps are out of service and lose +/// their secrets. Active credentials are then retired in the same transaction — token blanked, +/// Revoked, soft-deleted — and the instance's OAuth client secret is blanked with it, so nothing +/// the connection held stays decryptable. Each OAuth grant is then revoked at the provider, best +/// effort, after the transaction commits. /// public sealed record DeleteProviderInstanceCommand : ICommand, IRequireTeamPermission { diff --git a/backend/src/CodeSpace.Messages/Commands/Storage/StorageDefaultCommands.cs b/backend/src/CodeSpace.Messages/Commands/Storage/StorageDefaultCommands.cs index 369f64dae..d4eb7786f 100644 --- a/backend/src/CodeSpace.Messages/Commands/Storage/StorageDefaultCommands.cs +++ b/backend/src/CodeSpace.Messages/Commands/Storage/StorageDefaultCommands.cs @@ -37,7 +37,8 @@ public sealed record CreateStorageDefaultCommand : ICommand /// Replaces the template's configuration and advances its revision. Omitting Secret keeps the attached /// envelope; supplying one appends a new envelope and repoints the template at it; ClearCredential detaches -/// the envelope for a provider that needs none. Superseded envelopes are never overwritten in place. +/// the envelope for a provider that needs none. A replaced or detached envelope is never rewritten: it keeps its +/// fingerprint and hint as history, and its payload is blanked, since nothing reads it any more. /// public sealed record UpdateStorageDefaultCommand : ICommand, IRequireGlobalPermission { diff --git a/backend/src/CodeSpace.Messages/Queries/Credentials/ListAccessibleRepositoriesQuery.cs b/backend/src/CodeSpace.Messages/Queries/Credentials/ListAccessibleRepositoriesQuery.cs index 477af217c..f5e540338 100644 --- a/backend/src/CodeSpace.Messages/Queries/Credentials/ListAccessibleRepositoriesQuery.cs +++ b/backend/src/CodeSpace.Messages/Queries/Credentials/ListAccessibleRepositoriesQuery.cs @@ -4,7 +4,12 @@ namespace CodeSpace.Messages.Queries.Credentials; -public sealed record ListAccessibleRepositoriesQuery : IQuery, IRequireCredentialAccess +/// +/// Lists the repositories a credential's account can see at its provider — the picker a bind feeds from. It presents the +/// credential to the provider, so it is : the owner of a Personal credential, or a +/// repos.manage holder; never a member spending a teammate's or the team's token on membership alone. +/// +public sealed record ListAccessibleRepositoriesQuery : IQuery, IRequireCredentialSpend { /// Default page size — matches the PR-list default and keeps the picker snappy even on accounts with thousands of repos. public const int DefaultPerPage = 30; diff --git a/backend/tests/CodeSpace.E2ETests/Credentials/AccessibleRepositoriesGateE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Credentials/AccessibleRepositoriesGateE2ETests.cs new file mode 100644 index 000000000..dc6b1962e --- /dev/null +++ b/backend/tests/CodeSpace.E2ETests/Credentials/AccessibleRepositoriesGateE2ETests.cs @@ -0,0 +1,95 @@ +using System.Net; +using System.Net.Http.Json; +using System.Text.Json; +using CodeSpace.E2ETests.Infrastructure; +using CodeSpace.Messages.Enums; +using Shouldly; + +namespace CodeSpace.E2ETests.Credentials; + +/// +/// Listing what a credential reaches (GET /api/credentials/{id}/accessible-repositories) presents that credential to the +/// provider. On membership alone a Viewer could make the platform call GitLab with the Owner's personal token and read +/// back every project the Owner's account can see — private ones outside the team included. Now a Personal credential +/// lists for its owner or a repos.manage holder, and a team-service credential for a repos.manage holder; everyone else +/// is refused before the token leaves the platform. +/// +/// Driven through the real HTTP surface (POST /api/provider-instances, POST /api/credentials/pat and +/// /group-access-token, GET accessible-repositories), the real mediator pipeline and the real GitLab provider over +/// NGitLab, against a GitLab on a loopback port that records which token each request carried. +/// +/// Fidelity: HIGH. Only GitLab is a stub, and it is a real socket the production client speaks HTTP to. +/// +[Trait("Category", "E2E")] +[Trait("Surface", "Http")] +public sealed class AccessibleRepositoriesGateE2ETests : IClassFixture +{ + private readonly TenantHarness _tenants; + + public AccessibleRepositoriesGateE2ETests(TaskLaunchApiFactory factory) { _tenants = new TenantHarness(factory); } + + public enum Holder { OwnersPersonal, MembersPersonal, Team } + + [Theory] + [InlineData(TeamRole.Viewer, Holder.OwnersPersonal, HttpStatusCode.Forbidden)] + [InlineData(TeamRole.Member, Holder.OwnersPersonal, HttpStatusCode.Forbidden)] + [InlineData(TeamRole.Member, Holder.Team, HttpStatusCode.Forbidden)] + [InlineData(TeamRole.Viewer, Holder.Team, HttpStatusCode.Forbidden)] + [InlineData(TeamRole.Member, Holder.MembersPersonal, HttpStatusCode.OK)] + [InlineData(TeamRole.Owner, Holder.OwnersPersonal, HttpStatusCode.OK)] + [InlineData(TeamRole.Admin, Holder.OwnersPersonal, HttpStatusCode.OK)] + [InlineData(TeamRole.Admin, Holder.Team, HttpStatusCode.OK)] + public async Task Only_the_owner_of_a_personal_credential_or_a_repos_manage_holder_lists_what_it_reaches(TeamRole caller, Holder holder, HttpStatusCode expected) + { + await using var gitLab = RecordingProviderStub.Start(); + var world = await SeedAsync(gitLab.BaseUrl); + var (credentialId, token) = world.Credential(holder); + + var listing = await _tenants.SendAsync(HttpMethod.Get, $"/api/credentials/{credentialId}/accessible-repositories", world.User(caller), world.Team); + + listing.StatusCode.ShouldBe(expected, $"{caller} listing through {holder} — {await TenantHarness.DescribeAsync(listing)}"); + + if (expected == HttpStatusCode.OK) + { + gitLab.Carrying(token).ShouldNotBeEmpty("fixture check: an allowed listing presents the token to GitLab"); + return; + } + + gitLab.Carrying(token).ShouldBeEmpty($"{caller} was refused before the {holder} token was ever presented to the provider"); + } + + private sealed record World(Guid Team, Guid Owner, Guid Admin, Guid Member, Guid Viewer, (Guid, string) OwnersPat, (Guid, string) MembersPat, (Guid, string) TeamToken) + { + public Guid User(TeamRole role) => role switch { TeamRole.Owner => Owner, TeamRole.Admin => Admin, TeamRole.Member => Member, _ => Viewer }; + + public (Guid Id, string Token) Credential(Holder holder) => holder switch { Holder.OwnersPersonal => OwnersPat, Holder.MembersPersonal => MembersPat, _ => TeamToken }; + } + + /// A team of four roles on one GitLab connection: the Owner's personal token, the Member's (added for them by the Owner), and a team-service group token. + private async Task SeedAsync(string gitLabUrl) + { + var owner = await _tenants.AccountAsync(); + var admin = await _tenants.AccountAsync(); + var member = await _tenants.AccountAsync(); + var viewer = await _tenants.AccountAsync(); + var team = await _tenants.TeamAsync((owner, TeamRole.Owner), (admin, TeamRole.Admin), (member, TeamRole.Member), (viewer, TeamRole.Viewer)); + var canary = Guid.NewGuid().ToString("N"); + var instanceId = await CreatedIdAsync(await _tenants.SendAsync(HttpMethod.Post, "/api/provider-instances", owner, team, new { provider = "GitLab", displayName = "gitlab", baseUrl = gitLabUrl })); + + var ownersPat = $"glpat-owner-{canary}"; + var membersPat = $"glpat-member-{canary}"; + var teamToken = $"glgat-team-{canary}"; + var ownersId = await CreatedIdAsync(await _tenants.SendAsync(HttpMethod.Post, "/api/credentials/pat", owner, team, new { providerInstanceId = instanceId, displayName = "owner pat", token = ownersPat })); + var membersId = await CreatedIdAsync(await _tenants.SendAsync(HttpMethod.Post, "/api/credentials/pat", owner, team, new { providerInstanceId = instanceId, ownerUserId = member, displayName = "member pat", token = membersPat })); + var teamId = await CreatedIdAsync(await _tenants.SendAsync(HttpMethod.Post, "/api/credentials/group-access-token", owner, team, new { providerInstanceId = instanceId, displayName = "team token", token = teamToken })); + + return new World(team, owner, admin, member, viewer, (ownersId, ownersPat), (membersId, membersPat), (teamId, teamToken)); + } + + private static async Task CreatedIdAsync(HttpResponseMessage response) + { + response.StatusCode.ShouldBe(HttpStatusCode.OK, await TenantHarness.DescribeAsync(response)); + + return (await response.Content.ReadFromJsonAsync()).GetProperty("id").GetGuid(); + } +} diff --git a/backend/tests/CodeSpace.E2ETests/Credentials/DeletedVariableShredE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Credentials/DeletedVariableShredE2ETests.cs new file mode 100644 index 000000000..2172a7ea3 --- /dev/null +++ b/backend/tests/CodeSpace.E2ETests/Credentials/DeletedVariableShredE2ETests.cs @@ -0,0 +1,51 @@ +using System.Net; +using CodeSpace.E2ETests.Infrastructure; +using CodeSpace.Messages.Enums; +using Microsoft.EntityFrameworkCore; +using Shouldly; + +namespace CodeSpace.E2ETests.Credentials; + +/// +/// DELETE /api/team-variables/{name} on a Secret leaves its row for the audit trail and no ciphertext in it. Before, the +/// row kept its AES-GCM envelope forever, so a deleted secret stayed recoverable from any database copy by whoever held +/// the variable master key. +/// +/// Driven through the real HTTP surface (PUT, DELETE and GET /api/team-variables), the real mediator pipeline and +/// its transaction, real encryption and Postgres. Fidelity: HIGH — nothing is stubbed. +/// +[Trait("Category", "E2E")] +[Trait("Surface", "Http")] +public sealed class DeletedVariableShredE2ETests : IClassFixture +{ + private readonly TenantHarness _tenants; + + public DeletedVariableShredE2ETests(TaskLaunchApiFactory factory) { _tenants = new TenantHarness(factory); } + + [Fact] + public async Task Deleting_a_secret_team_variable_leaves_its_row_without_ciphertext_and_frees_the_name() + { + var owner = await _tenants.AccountAsync(); + var team = await _tenants.TeamAsync((owner, TeamRole.Owner)); + var body = new { name = "DEPLOY_TOKEN", valueType = "Secret", value = $"CANARY-deleted-variable-{Guid.NewGuid():N}" }; + + await ShouldSucceedAsync(await _tenants.SendAsync(HttpMethod.Put, "/api/team-variables/DEPLOY_TOKEN", owner, team, body), "set"); + await ShouldSucceedAsync(await _tenants.SendAsync(HttpMethod.Delete, "/api/team-variables/DEPLOY_TOKEN", owner, team), "delete"); + + var deleted = await _tenants.DbAsync(db => db.Variable.AsNoTracking().SingleAsync(v => v.TeamId == team && v.Name == "DEPLOY_TOKEN" && v.DeletedDate != null)); + deleted.ValueEncrypted.ShouldBeNull("the deleted secret's ciphertext must not outlive the delete — check VariableService.DeleteAsync if this fails"); + deleted.ValueType.ShouldBe(VariableValueType.Secret, "the row itself stays as the audit trail"); + + await ShouldSucceedAsync(await _tenants.SendAsync(HttpMethod.Put, "/api/team-variables/DEPLOY_TOKEN", owner, team, body), "set again under the freed name"); + (await ShouldSucceedAsync(await _tenants.SendAsync(HttpMethod.Get, "/api/team-variables", owner, team), "list")).ShouldContain("DEPLOY_TOKEN"); + } + + private static async Task ShouldSucceedAsync(HttpResponseMessage response, string step) + { + var text = await response.Content.ReadAsStringAsync(); + + response.IsSuccessStatusCode.ShouldBeTrue($"{step}: got {(int)response.StatusCode}: {text}"); + + return text; + } +} diff --git a/backend/tests/CodeSpace.E2ETests/Infrastructure/RecordingProviderStub.cs b/backend/tests/CodeSpace.E2ETests/Infrastructure/RecordingProviderStub.cs new file mode 100644 index 000000000..b480a3a43 --- /dev/null +++ b/backend/tests/CodeSpace.E2ETests/Infrastructure/RecordingProviderStub.cs @@ -0,0 +1,144 @@ +using System.Collections.Concurrent; +using System.Net; +using System.Net.Sockets; +using System.Text; +using System.Text.Json; + +namespace CodeSpace.E2ETests.Infrastructure; + +/// +/// A git provider on a loopback port, for tests that need to see which token reached the provider and through which call. +/// It answers GitLab's token-revocation endpoint (POST /oauth/revoke) with 200, GitHub's OAuth App revocation +/// endpoints (DELETE /api/v3/applications/{client_id}/grant and /token) with 204 as GitHub does, GitHub's +/// token endpoint as sets it, and every other request with an empty JSON list — enough for the +/// real GitLab provider to list repositories — and records each request whole: method, path and query, headers and body. +/// The production clients speak real HTTP to it. Requests are served concurrently. +/// +internal sealed class RecordingProviderStub : IAsyncDisposable +{ + private readonly HttpListener _listener = new(); + private readonly CancellationTokenSource _stopping = new(); + private readonly ConcurrentQueue _requests = new(); + private readonly Task _accept; + private RefreshAnswer? _refresh; + + private RecordingProviderStub() + { + // Reserve an ephemeral loopback port; retry binding only if another process won the close/bind race. + for (var attempt = 0; ; attempt++) + { + var port = FreeLoopbackPort(); + BaseUrl = $"http://127.0.0.1:{port}"; + _listener.Prefixes.Clear(); + _listener.Prefixes.Add($"{BaseUrl}/"); + try { _listener.Start(); break; } + catch (HttpListenerException) when (attempt < 4) { } + } + + _accept = Task.Run(AcceptAsync); + } + + public string BaseUrl { get; } + + public IReadOnlyCollection Requests => _requests; + + /// Every request that carried anywhere — its query, a header, or its body. + public IReadOnlyList Carrying(string secret) => _requests.Where(r => r.Whole.Contains(secret, StringComparison.Ordinal)).ToList(); + + public static RecordingProviderStub Start() => new(); + + /// + /// Answer GitHub's token endpoint (POST /login/oauth/access_token) with the pair , + /// once has run — a write that lands while GitHub + /// holds a refresh. Requests are served concurrently, so that write may reach this stub itself. + /// + public void AnswerRefresh(string accessToken, string refreshToken, Func whileRefreshing) => _refresh = new RefreshAnswer(accessToken, refreshToken, whileRefreshing); + + /// A loopback port with nothing listening on it — a provider that is down. Allocated by the OS, then released. + public static int FreeLoopbackPort() + { + using var probe = new TcpListener(IPAddress.Loopback, 0); + probe.Start(); + var port = ((IPEndPoint)probe.LocalEndpoint).Port; + probe.Stop(); + return port; + } + + private async Task AcceptAsync() + { + try + { + while (!_stopping.IsCancellationRequested) + { + var context = await _listener.GetContextAsync().WaitAsync(_stopping.Token); + _ = ServeAsync(context); + } + } + catch (Exception) when (_stopping.IsCancellationRequested) { } + } + + private async Task ServeAsync(HttpListenerContext context) + { + try + { + using var reader = new StreamReader(context.Request.InputStream, Encoding.UTF8); + var body = await reader.ReadToEndAsync(_stopping.Token); + var headers = string.Join("\n", context.Request.Headers.AllKeys.Select(key => $"{key}: {context.Request.Headers[key]}")); + var pathAndQuery = context.Request.Url!.PathAndQuery; + + _requests.Enqueue(new RecordedRequest(context.Request.HttpMethod, pathAndQuery, headers, body)); + + var (status, answer) = await AnswerAsync(pathAndQuery); + var reply = Encoding.UTF8.GetBytes(answer); + context.Response.StatusCode = status; + context.Response.ContentType = "application/json"; + context.Response.ContentLength64 = reply.Length; + await context.Response.OutputStream.WriteAsync(reply, _stopping.Token); + } + catch (Exception) when (_stopping.IsCancellationRequested) { } + finally + { + context.Response.Close(); + } + } + + private async Task<(int Status, string Body)> AnswerAsync(string pathAndQuery) + { + if (pathAndQuery.StartsWith("/api/v3/applications/", StringComparison.Ordinal)) return (204, string.Empty); + if (pathAndQuery.StartsWith("/oauth/revoke", StringComparison.Ordinal)) return (200, "{}"); + if (_refresh != null && pathAndQuery.StartsWith("/login/oauth/access_token", StringComparison.Ordinal)) return (200, await RefreshAsync(_refresh)); + + return (200, "[]"); + } + + private static async Task RefreshAsync(RefreshAnswer refresh) + { + await refresh.WhileRefreshing(); + + return JsonSerializer.Serialize(new { access_token = refresh.AccessToken, refresh_token = refresh.RefreshToken, expires_in = 28800, token_type = "bearer", scope = "" }); + } + + public async ValueTask DisposeAsync() + { + _stopping.Cancel(); + _listener.Stop(); + _listener.Close(); + + try { await _accept; } + catch (Exception) { /* the accept loop ends with the listener */ } + + _stopping.Dispose(); + } + + private sealed record RefreshAnswer(string AccessToken, string RefreshToken, Func WhileRefreshing); + + internal sealed record RecordedRequest(string Method, string PathAndQuery, string Headers, string Body) + { + public string Whole => $"{Method} {PathAndQuery}\n{Headers}\n\n{Body}"; + + /// The form fields of a url-encoded body, decoded. + public IReadOnlyDictionary Form => Body.Split('&', StringSplitOptions.RemoveEmptyEntries) + .Select(pair => pair.Split('=', 2)) + .ToDictionary(pair => WebUtility.UrlDecode(pair[0]), pair => pair.Length > 1 ? WebUtility.UrlDecode(pair[1]) : string.Empty, StringComparer.Ordinal); + } +} diff --git a/backend/tests/CodeSpace.E2ETests/Repositories/ProviderInstanceDeleteRevokeE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Repositories/ProviderInstanceDeleteRevokeE2ETests.cs new file mode 100644 index 000000000..1869a0481 --- /dev/null +++ b/backend/tests/CodeSpace.E2ETests/Repositories/ProviderInstanceDeleteRevokeE2ETests.cs @@ -0,0 +1,274 @@ +using System.Net; +using System.Net.Http.Json; +using System.Text.Json; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Credentials; +using CodeSpace.E2ETests.Infrastructure; +using CodeSpace.Messages.Credentials; +using CodeSpace.Messages.Enums; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; +using Shouldly; + +namespace CodeSpace.E2ETests.Repositories; + +/// +/// Deleting a git provider connection ends every token connected through it, here and at the provider. Before, the +/// delete marked them Revoked and left the tokens and the connection's OAuth client secret decryptable in the database, +/// and never told the provider, so an OAuth refresh token stayed good at GitLab while the UI said "Revoked". +/// +/// Driven through the real HTTP surface (POST and DELETE /api/provider-instances, POST /api/credentials/pat, +/// GET /api/credentials), the real mediator pipeline and its transaction, the real GitLabOAuthClient speaking +/// RFC 7009 over HTTP, and Postgres, against a GitLab on a loopback port that records every request. One seam: the OAuth +/// credential is written straight to the database, sealed by the production encryptor, as an OAuth connect would leave it +/// — the connect flow is not what is under test. +/// +/// Fidelity: HIGH. Only GitLab (and GitHub, for the shared-OAuth-App case) is a stub, and it is a real socket the +/// production client speaks HTTP to. +/// +[Trait("Category", "E2E")] +[Trait("Surface", "Http")] +public sealed class ProviderInstanceDeleteRevokeE2ETests : IClassFixture +{ + private readonly TaskLaunchApiFactory _factory; + private readonly TenantHarness _tenants; + + public ProviderInstanceDeleteRevokeE2ETests(TaskLaunchApiFactory factory) + { + _factory = factory; + _tenants = new TenantHarness(factory); + } + + [Fact] + public async Task Deleting_a_connection_revokes_its_oauth_grants_at_the_provider_and_leaves_no_secret_decryptable() + { + await using var gitLab = RecordingProviderStub.Start(); + var canary = Guid.NewGuid().ToString("N"); + var world = await SeedConnectionAsync(gitLab.BaseUrl, canary); + + var delete = await _tenants.SendAsync(HttpMethod.Delete, $"/api/provider-instances/{world.InstanceId}", world.Admin, world.Team); + + delete.StatusCode.ShouldBe(HttpStatusCode.OK, await TenantHarness.DescribeAsync(delete)); + (await delete.Content.ReadFromJsonAsync()).GetProperty("revokedCredentialCount").GetInt32().ShouldBe(2); + + var revokes = gitLab.Requests.Where(r => r.Method == "POST" && r.PathAndQuery == "/oauth/revoke").ToList(); + revokes.Select(r => (r.Form["token"], r.Form["token_type_hint"])).ShouldBe(new[] { ($"rt-{canary}", "refresh_token"), ($"at-{canary}", "access_token") }, "GitLab must be asked to revoke the refresh token, then the access token — check the post-commit revoke in ProviderInstanceService and the stub's request log if this fails"); + revokes.ShouldAllBe(r => r.Form["client_id"] == world.ClientId && r.Form["client_secret"] == $"client-secret-{canary}"); + gitLab.Carrying($"glpat-{canary}").ShouldBeEmpty("a personal access token has no revocation endpoint and is never sent anywhere by a delete"); + + var stored = await StoredAsync(world); + stored.Payloads.ShouldAllBe(p => p == string.Empty, "a deleted connection's tokens must not stay decryptable in the database"); + stored.ClientSecret.ShouldBeNull("nor its OAuth client secret"); + + var listed = await _tenants.SendAsync(HttpMethod.Get, "/api/credentials", world.Owner, world.Team); + (await listed.Content.ReadAsStringAsync()).ShouldNotContain(world.OAuthCredentialId.ToString(), Case.Insensitive, "a retired credential is out of the team's list"); + } + + [Fact] + public async Task Deleting_a_connection_whose_provider_is_down_still_retires_everything_here() + { + var down = $"http://127.0.0.1:{RecordingProviderStub.FreeLoopbackPort()}"; + var canary = Guid.NewGuid().ToString("N"); + var world = await SeedConnectionAsync(down, canary); + + var delete = await _tenants.SendAsync(HttpMethod.Delete, $"/api/provider-instances/{world.InstanceId}", world.Admin, world.Team); + + delete.StatusCode.ShouldBe(HttpStatusCode.OK, $"the provider-side revoke is best effort; a provider that does not answer must not fail the delete — {await TenantHarness.DescribeAsync(delete)}"); + + var stored = await StoredAsync(world); + stored.Payloads.ShouldAllBe(p => p == string.Empty); + stored.ClientSecret.ShouldBeNull(); + } + + /// + /// Two teams connect GitHub through the same OAuth App and the same person authorizes it in both. GitHub's grant delete + /// ends every token the person holds under the app, the other team's included, so deleting one team's connection asks + /// GitHub to end only the access token that team held — and sends no refresh token, which GitHub cannot end alone. A + /// Disconnect still ends the whole grant, as it always has. + /// + [Fact] + public async Task Deleting_a_GitHub_connection_ends_only_its_own_token_while_a_disconnect_still_ends_the_grant() + { + await using var gitHub = RecordingProviderStub.Start(); + var canary = Guid.NewGuid().ToString("N"); + var clientId = $"shared-app-{canary}"; + var deleted = await SeedGitHubConnectionAsync(gitHub.BaseUrl, clientId, $"A-{canary}"); + var kept = await SeedGitHubConnectionAsync(gitHub.BaseUrl, clientId, $"B-{canary}"); + + var delete = await _tenants.SendAsync(HttpMethod.Delete, $"/api/provider-instances/{deleted.InstanceId}", deleted.Admin, deleted.Team); + + delete.StatusCode.ShouldBe(HttpStatusCode.OK, await TenantHarness.DescribeAsync(delete)); + var revokes = gitHub.Requests.Where(r => r.Method == "DELETE" && r.PathAndQuery.StartsWith("/api/v3/applications/", StringComparison.Ordinal)).ToList(); + revokes.Select(r => (r.PathAndQuery, r.Body)).ShouldBe(new[] { ($"/api/v3/applications/{clientId}/token", $"{{\"access_token\":\"at-A-{canary}\"}}") }, $"a delete ends the one access token its team held, never the person's grant, which team B's token lives under too — check the post-commit revoke in ProviderInstanceService; requests seen: {string.Join(", ", gitHub.Requests.Select(r => $"{r.Method} {r.PathAndQuery}"))}"); + gitHub.Carrying($"rt-A-{canary}").ShouldBeEmpty("GitHub has no endpoint that ends a refresh token alone, so none is sent"); + gitHub.Carrying($"at-B-{canary}").ShouldBeEmpty("team B's token is never presented by team A's delete"); + + var disconnect = await _tenants.SendAsync(HttpMethod.Post, $"/api/credentials/{kept.OAuthCredentialId}/revoke", kept.Owner, kept.Team); + + disconnect.StatusCode.ShouldBe(HttpStatusCode.OK, await TenantHarness.DescribeAsync(disconnect)); + gitHub.Carrying($"at-B-{canary}").ShouldHaveSingleItem().PathAndQuery.ShouldBe($"/api/v3/applications/{clientId}/grant", "a Disconnect ends the person's whole grant to the app, unchanged"); + } + + /// + /// Team A's token is inside the refresh window and a listing refreshes it; team A's connection is deleted while GitHub + /// holds that refresh. The pair GitHub then issues is stored nowhere, so the refresher ends it at GitHub — by token, as + /// the delete itself ends team A's token: a grant delete would end the person's token in team B too. + /// + [Fact] + public async Task A_GitHub_connection_deleted_while_its_token_refreshes_ends_only_the_pair_GitHub_issued() + { + await using var gitHub = RecordingProviderStub.Start(); + var canary = Guid.NewGuid().ToString("N"); + var clientId = $"shared-app-{canary}"; + var deleted = await SeedGitHubConnectionAsync(gitHub.BaseUrl, clientId, $"A-{canary}", DateTimeOffset.UtcNow.AddSeconds(30)); + await SeedGitHubConnectionAsync(gitHub.BaseUrl, clientId, $"B-{canary}"); + HttpStatusCode? deleteStatus = null; + gitHub.AnswerRefresh($"at-A2-{canary}", $"rt-A2-{canary}", whileRefreshing: async () => deleteStatus = (await _tenants.SendAsync(HttpMethod.Delete, $"/api/provider-instances/{deleted.InstanceId}", deleted.Admin, deleted.Team)).StatusCode); + + var listing = await _tenants.SendAsync(HttpMethod.Get, $"/api/credentials/{deleted.OAuthCredentialId}/accessible-repositories", deleted.Owner, deleted.Team); + + deleteStatus.ShouldBe(HttpStatusCode.OK, $"fixture check: the delete landed while GitHub held the refresh; requests seen: {string.Join(", ", gitHub.Requests.Select(r => $"{r.Method} {r.PathAndQuery}"))}"); + listing.IsSuccessStatusCode.ShouldBeFalse("a listing whose connection was deleted under it gets no token"); + var revokes = gitHub.Requests.Where(r => r.Method == "DELETE" && r.PathAndQuery.StartsWith("/api/v3/applications/", StringComparison.Ordinal)).ToList(); + revokes.Select(r => (r.PathAndQuery, r.Body)).ShouldBe(new[] { ($"/api/v3/applications/{clientId}/token", $"{{\"access_token\":\"at-A-{canary}\"}}"), ($"/api/v3/applications/{clientId}/token", $"{{\"access_token\":\"at-A2-{canary}\"}}") }, $"the delete ends the token team A held, then the refresher ends the pair GitHub issued meanwhile — each by token, never the person's grant, which team B's token lives under too; check OAuthTokenRefresher.RevokeIssuedTokensAsync. Requests seen: {string.Join(", ", gitHub.Requests.Select(r => $"{r.Method} {r.PathAndQuery}"))}"); + gitHub.Carrying($"at-B-{canary}").ShouldBeEmpty("team B's token is never presented by team A's delete or its refresh"); + } + + /// + /// A connection-wide connection whose group hook was registered at GitLab with the owner's personal access token. The + /// delete blanks that token, so the hook must come down first, while the token still opens: afterwards nothing could + /// remove it, and it would go on delivering the group's events with its HMAC secret still decryptable here. A hook + /// row retired earlier stays as the record it is, holding no secret. + /// + [Fact] + public async Task Deleting_a_connection_wide_connection_takes_its_group_hook_down_at_the_provider_before_its_token_goes() + { + await using var gitLab = RecordingProviderStub.Start(); + var canary = Guid.NewGuid().ToString("N"); + var world = await SeedConnectionAsync(gitLab.BaseUrl, canary); + var retiredEarlier = await SeedGroupHooksAsync(world, $"acme-{canary}", $"hook-{canary}", canary); + + var delete = await _tenants.SendAsync(HttpMethod.Delete, $"/api/provider-instances/{world.InstanceId}", world.Admin, world.Team); + + delete.StatusCode.ShouldBe(HttpStatusCode.OK, await TenantHarness.DescribeAsync(delete)); + + var hookDeletes = gitLab.Requests.Where(r => r.Method == "DELETE" && r.PathAndQuery == $"/api/v4/groups/acme-{canary}/hooks/hook-{canary}").ToList(); + hookDeletes.Count.ShouldBe(1, $"GitLab must be asked to delete the group hook — check the hook retirement in ProviderInstanceService.DeleteAsync; requests seen: {string.Join(", ", gitLab.Requests.Select(r => $"{r.Method} {r.PathAndQuery}"))}"); + hookDeletes[0].Headers.ShouldContain($"glpat-{canary}", Case.Sensitive, "the hook is deleted with the token that registered it, before the delete blanks that token"); + + var hooks = await _tenants.DbAsync(db => db.ConnectionWebhook.AsNoTracking().Where(w => w.ProviderInstanceId == world.InstanceId).ToListAsync()); + hooks.Select(h => h.Id).ShouldBe(new[] { retiredEarlier }, "the Registered row described the hook that is now gone"); + hooks.ShouldAllBe(h => h.RegistrationStatus == RepositoryWebhookRegistrationStatus.Cancelled && h.SecretEnc == string.Empty, "a deleted connection keeps no hook secret decryptable"); + } + + private sealed record Connection(Guid Owner, Guid Admin, Guid Team, Guid InstanceId, string ClientId, Guid OAuthCredentialId, Guid PatCredentialId); + + /// + /// Puts the connection in connection-wide scope with a Registered group hook on , registered + /// by the owner's personal access token, beside a Cancelled one an earlier scope switch left. Written directly, as the + /// registrar leaves them — the registration is not what is under test. Returns the Cancelled row's id. + /// + private async Task SeedGroupHooksAsync(Connection world, string ownerPath, string externalId, string canary) + { + using var scope = _factory.Services.CreateScope(); + var encryptor = scope.ServiceProvider.GetRequiredService(); + var registered = Guid.NewGuid(); + var cancelled = Guid.NewGuid(); + + await _tenants.DbAsync(async db => + { + var instance = await db.ProviderInstance.SingleAsync(p => p.Id == world.InstanceId); + instance.WebhookScope = ProviderWebhookScope.Connection; + + db.ConnectionWebhook.Add(GroupHook(registered, world, ownerPath, externalId, RepositoryWebhookRegistrationStatus.Registered, encryptor.Encrypt($"hook-secret-{canary}"))); + db.ConnectionWebhook.Add(GroupHook(cancelled, world, $"{ownerPath}-old", externalId: null, RepositoryWebhookRegistrationStatus.Cancelled, encryptor.Encrypt($"old-hook-secret-{canary}"))); + + return await db.SaveChangesAsync(); + }); + + return cancelled; + } + + private static ConnectionWebhook GroupHook(Guid id, Connection world, string ownerPath, string? externalId, RepositoryWebhookRegistrationStatus status, string secretEnc) => new() + { + Id = id, ProviderInstanceId = world.InstanceId, CredentialId = world.PatCredentialId, OwnerPath = ownerPath, ExternalId = externalId, + CallbackUrl = $"https://codespace.test/api/webhooks/connection/{id}", SecretEnc = secretEnc, SubscribedEvents = new List { "push" }, RegistrationStatus = status, + }; + + /// + /// The Owner adds a GitLab connection with an OAuth app and a personal access token through the API; the Owner's OAuth + /// credential is sealed by the production encryptor and written directly, as a completed connect leaves it. + /// + private async Task SeedConnectionAsync(string gitLabUrl, string canary) + { + var owner = await _tenants.AccountAsync(); + var admin = await _tenants.AccountAsync(); + var team = await _tenants.TeamAsync((owner, TeamRole.Owner), (admin, TeamRole.Admin)); + var clientId = $"client-{canary}"; + var instanceId = await CreatedIdAsync(await _tenants.SendAsync(HttpMethod.Post, "/api/provider-instances", owner, team, new { provider = "GitLab", displayName = "gitlab", baseUrl = gitLabUrl, oauthClientId = clientId, oauthClientSecret = $"client-secret-{canary}" })); + var patId = await CreatedIdAsync(await _tenants.SendAsync(HttpMethod.Post, "/api/credentials/pat", owner, team, new { providerInstanceId = instanceId, displayName = "owner pat", token = $"glpat-{canary}" })); + var oauthId = Guid.NewGuid(); + + using (var scope = _factory.Services.CreateScope()) + { + var payload = scope.ServiceProvider.GetRequiredService().Serialize(new OAuthPayload { AccessToken = $"at-{canary}", RefreshToken = $"rt-{canary}" }); + var sealedPayload = scope.ServiceProvider.GetRequiredService().Encrypt(payload); + + await _tenants.DbAsync(async db => + { + db.Credential.Add(new Credential { Id = oauthId, TeamId = team, ProviderInstanceId = instanceId, OwnerUserId = owner, Ownership = CredentialOwnership.Personal, AuthType = AuthType.OAuth, DisplayName = "owner oauth", EncryptedPayload = sealedPayload, Status = CredentialStatus.Active }); + return await db.SaveChangesAsync(); + }); + } + + return new Connection(owner, admin, team, instanceId, clientId, oauthId, patId); + } + + private sealed record GitHubConnection(Guid Owner, Guid Admin, Guid Team, Guid InstanceId, Guid OAuthCredentialId); + + /// + /// A team whose Owner adds a GitHub connection on through the API with the OAuth App + /// , and the Owner's OAuth credential (at-{tag}, rt-{tag}, expiring at + /// when given) sealed by the production encryptor and written directly, as a completed + /// connect leaves it. + /// + private async Task SeedGitHubConnectionAsync(string gitHubUrl, string clientId, string tag, DateTimeOffset? expiresAt = null) + { + var owner = await _tenants.AccountAsync(); + var admin = await _tenants.AccountAsync(); + var team = await _tenants.TeamAsync((owner, TeamRole.Owner), (admin, TeamRole.Admin)); + var instanceId = await CreatedIdAsync(await _tenants.SendAsync(HttpMethod.Post, "/api/provider-instances", owner, team, new { provider = "GitHub", displayName = "github", baseUrl = gitHubUrl, oauthClientId = clientId, oauthClientSecret = "shared-app-secret" })); + var oauthId = Guid.NewGuid(); + + using (var scope = _factory.Services.CreateScope()) + { + var payload = scope.ServiceProvider.GetRequiredService().Serialize(new OAuthPayload { AccessToken = $"at-{tag}", RefreshToken = $"rt-{tag}", ExpiresAt = expiresAt }); + var sealedPayload = scope.ServiceProvider.GetRequiredService().Encrypt(payload); + + await _tenants.DbAsync(async db => + { + db.Credential.Add(new Credential { Id = oauthId, TeamId = team, ProviderInstanceId = instanceId, OwnerUserId = owner, Ownership = CredentialOwnership.Personal, AuthType = AuthType.OAuth, DisplayName = "owner oauth", EncryptedPayload = sealedPayload, ExpiresDate = expiresAt, Status = CredentialStatus.Active }); + return await db.SaveChangesAsync(); + }); + } + + return new GitHubConnection(owner, admin, team, instanceId, oauthId); + } + + private async Task<(IReadOnlyList Payloads, string? ClientSecret)> StoredAsync(Connection world) + { + var payloads = await _tenants.DbAsync(db => db.Credential.AsNoTracking().Where(c => c.Id == world.OAuthCredentialId || c.Id == world.PatCredentialId).Select(c => c.EncryptedPayload).ToListAsync()); + var clientSecret = await _tenants.DbAsync(db => db.ProviderInstance.AsNoTracking().Where(p => p.Id == world.InstanceId).Select(p => p.OauthClientSecretEnc).SingleAsync()); + + payloads.Count.ShouldBe(2, "fixture check: both credentials were seeded"); + + return (payloads, clientSecret); + } + + private static async Task CreatedIdAsync(HttpResponseMessage response) + { + response.StatusCode.ShouldBe(HttpStatusCode.OK, await TenantHarness.DescribeAsync(response)); + + return (await response.Content.ReadFromJsonAsync()).GetProperty("id").GetGuid(); + } +} diff --git a/backend/tests/CodeSpace.E2ETests/Storage/StorageCredentialsEndpointE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Storage/StorageCredentialsEndpointE2ETests.cs index 53da4acea..5f421dca6 100644 --- a/backend/tests/CodeSpace.E2ETests/Storage/StorageCredentialsEndpointE2ETests.cs +++ b/backend/tests/CodeSpace.E2ETests/Storage/StorageCredentialsEndpointE2ETests.cs @@ -85,21 +85,12 @@ public async Task Admin_creates_lists_gets_rotates_and_revokes_without_exposing_ rotated.GetProperty("xmin").GetUInt32().ShouldNotBe(created.GetProperty("xmin").GetUInt32()); AssertSafeWire(rotatedWire, rotatedSecret); - var revoke = await SendAsync(world.UserId, world.TeamId, HttpMethod.Post, $"/api/storage/credentials/{credentialId}/revoke", new - { - expectedXmin = rotated.GetProperty("xmin").GetUInt32(), expectedCurrentRevision = 2, - }); - revoke.StatusCode.ShouldBe(HttpStatusCode.OK, await DescribeAsync(revoke)); - var revoked = Json(await revoke.Content.ReadAsStringAsync()); - revoked.GetProperty("state").GetString().ShouldBe("Revoked"); - revoked.GetProperty("revokedDate").ValueKind.ShouldBe(JsonValueKind.String); - using var scope = _application.Services.CreateScope(); var db = scope.ServiceProvider.GetRequiredService(); var revisions = await db.StorageCredentialRevision.AsNoTracking().Where(value => value.StorageCredentialId == credentialId).OrderBy(value => value.Revision).ToListAsync(); revisions.Count.ShouldBe(2); var encryptor = scope.ServiceProvider.GetRequiredService(); - encryptor.Decrypt(revisions[0].EncryptedPayload).ShouldBe("""{"accessKey":"AK-ABCD","mode":"write"}"""); + encryptor.Decrypt(revisions[0].EncryptedPayload).ShouldBe("""{"accessKey":"AK-ABCD","mode":"write"}""", "while the credential is Active the superseded revision stays readable: a profile may pin it"); encryptor.Decrypt(revisions[1].EncryptedPayload).ShouldBe("""{"accessKey":"AK-WXYZ","mode":"read"}"""); revisions[0].EncryptedPayload.ShouldNotContain(firstSecret); revisions[1].EncryptedPayload.ShouldNotContain(rotatedSecret); @@ -107,6 +98,19 @@ public async Task Admin_creates_lists_gets_rotates_and_revokes_without_exposing_ revisions[1].EnvelopeFingerprint.ShouldBe(Fingerprint(revisions[1].EncryptedPayload)); createdWire.ShouldNotContain(revisions[0].EncryptedPayload); rotatedWire.ShouldNotContain(revisions[1].EncryptedPayload); + + var revoke = await SendAsync(world.UserId, world.TeamId, HttpMethod.Post, $"/api/storage/credentials/{credentialId}/revoke", new + { + expectedXmin = rotated.GetProperty("xmin").GetUInt32(), expectedCurrentRevision = 2, + }); + revoke.StatusCode.ShouldBe(HttpStatusCode.OK, await DescribeAsync(revoke)); + var revoked = Json(await revoke.Content.ReadAsStringAsync()); + revoked.GetProperty("state").GetString().ShouldBe("Revoked"); + revoked.GetProperty("revokedDate").ValueKind.ShouldBe(JsonValueKind.String); + + var retired = await db.StorageCredentialRevision.AsNoTracking().Where(value => value.StorageCredentialId == credentialId).OrderBy(value => value.Revision).ToListAsync(); + retired.ShouldAllBe(value => value.EncryptedPayload == string.Empty, "a revoked credential resolves for nobody, so none of its revisions may stay decryptable"); + retired.Select(value => value.EnvelopeFingerprint).ShouldBe(revisions.Select(value => value.EnvelopeFingerprint), "the fingerprints stay for the audit trail"); } [Fact] diff --git a/backend/tests/CodeSpace.E2ETests/Storage/StorageDefaultSecretShredE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Storage/StorageDefaultSecretShredE2ETests.cs new file mode 100644 index 000000000..0b05939ed --- /dev/null +++ b/backend/tests/CodeSpace.E2ETests/Storage/StorageDefaultSecretShredE2ETests.cs @@ -0,0 +1,134 @@ +using System.Net; +using System.Net.Http.Headers; +using System.Net.Http.Json; +using System.Text.Json; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Credentials; +using CodeSpace.E2ETests.Infrastructure; +using CodeSpace.Messages.Constants; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; +using Shouldly; + +namespace CodeSpace.E2ETests.Storage; + +/// +/// Rotating a deployment storage default's secret, and clearing it, through the real admin HTTP surface. Each rotation +/// appends a new envelope; the one it replaced used to stay decryptable forever, so a cloud key an operator rotated out +/// because it leaked stayed recoverable from every database copy. Now the envelope a template stops naming is blanked +/// in the same transaction, and only the one it names still opens. +/// +/// Fidelity: HIGH. The real ApiUser capability resolution, MediatR pipeline and its transaction, the +/// production encryptor, and Postgres with 0249's guard. Its own database (one fixture per class), because a template is +/// one row per data class for the whole deployment and can never be deleted. +/// +[Trait("Category", "E2E")] +[Trait("Surface", "Http")] +public sealed class StorageDefaultSecretShredE2ETests : IClassFixture +{ + private readonly TaskLaunchApiFactory _factory; + + public StorageDefaultSecretShredE2ETests(TaskLaunchApiFactory factory) { _factory = factory; } + + [Fact] + public async Task Rotating_then_clearing_a_templates_secret_leaves_only_the_envelope_it_names_decryptable() + { + var operatorId = await SeedCapabilityHolderAsync(); + var canary = Guid.NewGuid().ToString("N"); + + var created = await SendAsync(operatorId, HttpMethod.Post, "/api/admin/storage-defaults", Template(new { accessKeyId = "AK", accessKeySecret = $"first-{canary}" })); + var defaultId = created.GetProperty("id").GetGuid(); + var first = await NamedEnvelopeAsync(defaultId); + + var rotated = await SendAsync(operatorId, HttpMethod.Put, $"/api/admin/storage-defaults/{defaultId}", Edit(created, secret: new { accessKeyId = "AK", accessKeySecret = $"second-{canary}" })); + var second = await NamedEnvelopeAsync(defaultId); + + second.ShouldNotBe(first, "fixture check: a supplied secret appends a new envelope"); + (await EnvelopeAsync(first!.Value)).ShouldBe(string.Empty, "the rotated-out key has no reader left and must not stay decryptable — check StorageDefaultService.UpdateAsync and 0249's storage_default_credential_guard"); + (await DecryptAsync(second!.Value)).ShouldContain($"second-{canary}", Case.Sensitive, "the envelope the template names is the one the materializer decrypts"); + + var cleared = await SendAsync(operatorId, HttpMethod.Put, $"/api/admin/storage-defaults/{defaultId}", Edit(rotated, clearCredential: true)); + + cleared.GetProperty("hasCredential").GetBoolean().ShouldBeFalse(); + (await NamedEnvelopeAsync(defaultId)).ShouldBeNull(); + (await EnvelopeAsync(second.Value)).ShouldBe(string.Empty, "a cleared key has no reader left either"); + } + + private static object Template(object secret) => new + { + dataClassTypeKey = "agent-run-log/v1", + providerTypeKey = "aliyun-oss/v1", + nonSecretConfig = new { endpoint = "oss-cn-hangzhou.aliyuncs.com", bucket = "codespace-artifacts" }, + namespaceRoot = "codespace-defaults/", + adoptionPolicy = "Automatic", + isEnabled = true, + secret, + }; + + private static object Edit(JsonElement current, object? secret = null, bool clearCredential = false) => new + { + expectedXmin = current.GetProperty("xmin").GetUInt32(), + expectedRevision = current.GetProperty("revision").GetInt32(), + providerTypeKey = "aliyun-oss/v1", + nonSecretConfig = new { endpoint = "oss-cn-hangzhou.aliyuncs.com", bucket = "codespace-artifacts" }, + namespaceRoot = "codespace-defaults/", + adoptionPolicy = "Automatic", + secret, + clearCredential, + }; + + private async Task SendAsync(Guid userId, HttpMethod method, string path, object body) + { + var request = new HttpRequestMessage(method, path) { Content = JsonContent.Create(body) }; + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", TestToken.Mint(userId, TestToken.SeedStamp)); + + var response = await _factory.CreateClient().SendAsync(request); + var text = await response.Content.ReadAsStringAsync(); + + response.StatusCode.ShouldBe(HttpStatusCode.OK, $"{method} {path} got {(int)response.StatusCode}: {text}"); + + return JsonDocument.Parse(text).RootElement.Clone(); + } + + private async Task NamedEnvelopeAsync(Guid defaultId) => + await DbAsync(db => db.StorageDefault.AsNoTracking().Where(row => row.Id == defaultId).Select(row => row.CredentialId).SingleAsync()); + + private async Task EnvelopeAsync(Guid envelopeId) => + await DbAsync(db => db.StorageDefaultCredential.AsNoTracking().Where(row => row.Id == envelopeId).Select(row => row.EncryptedPayload).SingleAsync()); + + private async Task DecryptAsync(Guid envelopeId) + { + var envelope = await EnvelopeAsync(envelopeId); + + using var scope = _factory.Services.CreateScope(); + + return scope.ServiceProvider.GetRequiredService().Decrypt(envelope); + } + + private async Task DbAsync(Func> read) + { + using var scope = _factory.Services.CreateScope(); + + return await read(scope.ServiceProvider.GetRequiredService()); + } + + /// A deployment admin: an account holding storage.defaults.manage through a real role row, as ApiUser resolves it. + private async Task SeedCapabilityHolderAsync() + { + using var scope = _factory.Services.CreateScope(); + var db = scope.ServiceProvider.GetRequiredService(); + var suffix = Guid.NewGuid().ToString("N")[..8]; + var permissionId = await db.Permission.AsNoTracking().Where(value => value.Name == Permissions.StorageDefaultsManage).Select(value => value.Id).SingleAsync(); + var user = new User { Id = Guid.NewGuid(), SecurityStamp = TestToken.SeedStamp, Email = $"shred-operator-{suffix}@test.local", Name = "Deployment Admin", CreatedBy = SystemUsers.SeederId, LastModifiedBy = SystemUsers.SeederId }; + var role = new Role { Id = Guid.NewGuid(), Name = $"storage-defaults-shred-{suffix}", IsSystem = false, Status = true }; + + db.User.Add(user); + db.Role.Add(role); + db.RolePermission.Add(new RolePermission { Id = Guid.NewGuid(), RoleId = role.Id, PermissionId = permissionId }); + db.RoleUser.Add(new RoleUser { Id = Guid.NewGuid(), RoleId = role.Id, UserId = user.Id }); + await db.SaveChangesAsync(); + + return user.Id; + } +} diff --git a/backend/tests/CodeSpace.IntegrationTests/Authorization/TeamPermissionEnforcementTests.cs b/backend/tests/CodeSpace.IntegrationTests/Authorization/TeamPermissionEnforcementTests.cs index 2722425cf..7d7acfa47 100644 --- a/backend/tests/CodeSpace.IntegrationTests/Authorization/TeamPermissionEnforcementTests.cs +++ b/backend/tests/CodeSpace.IntegrationTests/Authorization/TeamPermissionEnforcementTests.cs @@ -14,6 +14,7 @@ using CodeSpace.Messages.Credentials; using CodeSpace.Messages.Dtos.Workflows; using CodeSpace.Messages.Enums; +using CodeSpace.Messages.Queries.Credentials; using CodeSpace.Messages.Queries.Workflows; using MediatR; using Microsoft.EntityFrameworkCore; @@ -213,6 +214,41 @@ public async Task Below_Admin_only_the_callers_own_personal_credential_can_be_re (await LoadCredentialStatusAsync(credentialId).ConfigureAwait(false)).ShouldBe(CredentialStatus.Revoked); } + /// + /// Listing what a credential reaches presents it to the provider, so on membership alone a Viewer could spend the + /// Owner's personal token to enumerate everything the Owner's account can see there — private repositories outside + /// the team included. Below Admin only the caller's own Personal credential lists; a teammate's and the team's own + /// take repos.manage, the bar of the bind the listing feeds. The allow path reaches the provider lookup, which the Git + /// kind cannot answer — anything but a denial is past the gate. + /// + [Theory] + [MemberData(nameof(RevokesByRoleAndHolder))] + public async Task Below_Admin_only_the_callers_own_personal_credential_lists_what_it_reaches(TeamRole role, CredentialHolder holder, bool denied) + { + var team = await SeedTeamAsync().ConfigureAwait(false); + var caller = team.UserFor(role); + var teammate = role == TeamRole.Owner ? team.Admin : team.Owner; + var credentialId = await SeedCredentialAsync(team.TeamId, holder switch { CredentialHolder.Caller => caller, CredentialHolder.Teammate => teammate, _ => null }).ConfigureAwait(false); + + using var scope = _fixture.BeginScopeAs(caller, team.TeamId); + var thrown = await Record.ExceptionAsync(() => scope.Resolve().Send(new ListAccessibleRepositoriesQuery { CredentialId = credentialId })).ConfigureAwait(false); + + AssertDenial(thrown, denied, role, TeamPermissions.ReposManage); + } + + [Fact] + public async Task An_instance_Admin_outside_the_team_lists_through_a_members_personal_credential() + { + // The instance Admin role has an Owner's standing in every team (TeamStanding), membership row or not. + var team = await SeedTeamAsync().ConfigureAwait(false); + var credentialId = await SeedCredentialAsync(team.TeamId, team.Member).ConfigureAwait(false); + + using var scope = _fixture.BeginScopeAs(Guid.NewGuid(), team.TeamId, Roles.Admin); + var thrown = await Record.ExceptionAsync(() => scope.Resolve().Send(new ListAccessibleRepositoriesQuery { CredentialId = credentialId })).ConfigureAwait(false); + + thrown.ShouldNotBeOfType(); + } + [Fact] public async Task The_global_Admin_role_still_bypasses() { diff --git a/backend/tests/CodeSpace.IntegrationTests/Credentials/RetiredSecretShredSweepFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/Credentials/RetiredSecretShredSweepFlowTests.cs new file mode 100644 index 000000000..2090df06b --- /dev/null +++ b/backend/tests/CodeSpace.IntegrationTests/Credentials/RetiredSecretShredSweepFlowTests.cs @@ -0,0 +1,237 @@ +using Autofac; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Workflows.Artifacts.Providers.Local; +using CodeSpace.IntegrationTests.Infrastructure; +using CodeSpace.Messages.Commands.Credentials; +using CodeSpace.Messages.Enums; +using MediatR; +using Microsoft.EntityFrameworkCore; +using Npgsql; +using Shouldly; + +namespace CodeSpace.IntegrationTests.Credentials; + +/// +/// A rolling deploy runs the previous build beside this one after 0249 has run, and a rollback runs it alone. That build +/// retires secrets without blanking them: its provider-instance delete marks tokens Revoked and keeps them, its variable +/// delete keeps the ciphertext, its storage revoke keeps every revision, its storage-default rotation keeps the envelope it +/// replaced. 0249 ran once, before those rows existed. The recurring sweep must blank them on its next tick — and touch +/// nothing live. +/// +/// Mixed-version, simulated: the previous build's retirements are its own statements, replayed as raw SQL against +/// this build's schema (0249's relaxed guards included), on rows seeded through the real model. The sweep is sent through +/// the real mediator — the pipeline its recurring job uses — and every assertion reads rows this test owns in a fresh +/// scope, never the sweep's global count: this suite shares one database. +/// +[Collection(PostgresCollection.Name)] +[Trait("Category", "Integration")] +public class RetiredSecretShredSweepFlowTests +{ + private readonly PostgresFixture _fixture; + + public RetiredSecretShredSweepFlowTests(PostgresFixture fixture) { _fixture = fixture; } + + [Fact] + public async Task What_the_previous_build_retires_after_0249_is_shredded_by_the_next_sweep_and_nothing_live_is() + { + var world = await SeedAsync(); + + await RetireAsThePreviousBuildDoesAsync(world); + (await PayloadAsync(world.RetiredCredentialId)).ShouldNotBeEmpty("fixture check: the previous build's delete keeps the token"); + + await SweepAsync(); + + await ShouldHoldNoRetiredSecretAndEveryLiveOneAsync(world); + } + + [Fact] + public async Task A_second_sweep_finds_nothing_left_to_change() + { + var world = await SeedAsync(); + await RetireAsThePreviousBuildDoesAsync(world); + await SweepAsync(); + + await Should.NotThrowAsync(SweepAsync, "a blanked revision or envelope is no longer a candidate — the guards refuse a second blanking, so a sweep that re-selected one would fail every tick"); + + await ShouldHoldNoRetiredSecretAndEveryLiveOneAsync(world); + } + + private async Task SweepAsync() + { + using var scope = _fixture.BeginScope(); + + await scope.Resolve().Send(new ShredRetiredSecretsCommand()); + } + + private async Task ShouldHoldNoRetiredSecretAndEveryLiveOneAsync(World world) + { + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + + (await PayloadAsync(world.RetiredCredentialId)).ShouldBe(string.Empty, "the previous build's provider delete left the token Revoked and decryptable"); + (await PayloadAsync(world.LiveCredentialId)).ShouldBe("CfDJ8-CANARY-live-token", "a live connection's token is untouched"); + (await db.ProviderInstance.AsNoTracking().Where(p => p.Id == world.DeletedInstanceId).Select(p => p.OauthClientSecretEnc).SingleAsync()).ShouldBeNull("a deleted connection's client secret"); + (await db.ProviderInstance.AsNoTracking().Where(p => p.Id == world.LiveInstanceId).Select(p => p.OauthClientSecretEnc).SingleAsync()).ShouldBe("CfDJ8-CANARY-live-client-secret"); + + var deletedHook = await db.ConnectionWebhook.AsNoTracking().SingleAsync(w => w.ProviderInstanceId == world.DeletedInstanceId); + deletedHook.ShouldSatisfyAllConditions( + h => h.RegistrationStatus.ShouldBe(RepositoryWebhookRegistrationStatus.Cancelled, "a deleted connection's hook is out of service — ingestion refuses it before it reads a secret"), + h => h.SecretEnc.ShouldBe(string.Empty), + h => h.ExternalId.ShouldBe("hook-777", "the provider's hook id stays, as the record of what to remove at the provider by hand")); + (await db.ConnectionWebhook.AsNoTracking().SingleAsync(w => w.ProviderInstanceId == world.LiveInstanceId)).ShouldSatisfyAllConditions( + h => h.RegistrationStatus.ShouldBe(RepositoryWebhookRegistrationStatus.Registered), + h => h.SecretEnc.ShouldBe("CfDJ8-CANARY-live-hook-secret", "a live hook keeps the secret its deliveries are verified against")); + + (await db.Variable.AsNoTracking().SingleAsync(v => v.Id == world.DeletedVariableId)).ValueEncrypted.ShouldBeNull("the previous build's variable delete kept the ciphertext"); + (await db.Variable.AsNoTracking().SingleAsync(v => v.Id == world.LiveVariableId)).ValueEncrypted.ShouldBe(new byte[] { 7, 7, 7, 7 }); + + (await RevisionsAsync(db, world.RevokedStorageCredentialId)).ShouldBe(new[] { string.Empty, string.Empty }, ignoreOrder: false, customMessage: "the previous build's storage revoke kept every revision"); + (await RevisionsAsync(db, world.ActiveStorageCredentialId)).ShouldBe(new[] { "CfDJ8-CANARY-active-rev-1", "CfDJ8-CANARY-active-rev-2" }, ignoreOrder: false, customMessage: "an Active credential's superseded revision may be pinned by a profile and is kept"); + + (await EnvelopeAsync(db, world.ReplacedEnvelopeId)).ShouldBe(string.Empty, "the previous build's rotation kept the envelope it replaced"); + (await EnvelopeAsync(db, world.RotatedEnvelopeId)).ShouldBe("CfDJ8-CANARY-rotated-envelope", "the envelope the template now names is the one the materializer decrypts"); + } + + /// + /// The previous build's retirements, statement for statement: ProviderInstanceService.DeleteAsync's + /// cascade-revoke then soft delete, VariableService.DeleteAsync, StorageCredentialService.RevokeAsync, + /// and StorageDefaultService.UpdateAsync appending a new envelope and repointing the template. None of them + /// blanks anything. Each must still be accepted by this build's schema. + /// + private async Task RetireAsThePreviousBuildDoesAsync(World world) + { + await using var connection = new NpgsqlConnection(_fixture.ConnectionString); + await connection.OpenAsync(); + await using var command = new NpgsqlCommand( + """ + UPDATE credential SET status = 'Revoked', deleted_date = now(), last_error = 'Provider instance deleted' WHERE provider_instance_id = @deleted AND deleted_date IS NULL AND status = 'Active'; + UPDATE provider_instance SET deleted_date = now() WHERE id = @deleted; + UPDATE variable SET deleted_date = now(), last_modified_date = now(), last_modified_by = @actor WHERE id = @variable; + UPDATE storage_credential SET state = 'Revoked', revoked_date = now(), revoked_by = @actor WHERE id = @storage; + INSERT INTO storage_default_credential (id, provider_type_key, encrypted_payload, envelope_fingerprint, created_date, created_by) VALUES (@rotated, @provider, 'CfDJ8-CANARY-rotated-envelope', @fingerprint, now(), @actor); + UPDATE storage_default SET credential_id = @rotated, revision = revision + 1, last_modified_date = now(), last_modified_by = @actor WHERE id = @template; + """, connection); + + command.Parameters.AddWithValue("deleted", world.DeletedInstanceId); + command.Parameters.AddWithValue("actor", world.ActorId); + command.Parameters.AddWithValue("variable", world.DeletedVariableId); + command.Parameters.AddWithValue("storage", world.RevokedStorageCredentialId); + command.Parameters.AddWithValue("rotated", world.RotatedEnvelopeId); + command.Parameters.AddWithValue("provider", LocalRwxArtifactStorageDriverFactory.TypeKey); + command.Parameters.AddWithValue("fingerprint", $"sha256:{new string('d', 64)}"); + command.Parameters.AddWithValue("template", world.TemplateId); + + await command.ExecuteNonQueryAsync(); + } + + private sealed record World(Guid ActorId, Guid DeletedInstanceId, Guid LiveInstanceId, Guid RetiredCredentialId, Guid LiveCredentialId, Guid DeletedVariableId, Guid LiveVariableId, Guid RevokedStorageCredentialId, Guid ActiveStorageCredentialId, Guid TemplateId, Guid ReplacedEnvelopeId, Guid RotatedEnvelopeId); + + /// Everything live, as this build or the previous one creates it: two connections with a token and a group hook each, two Secret variables, two storage credentials rotated once, and a deployment default naming its envelope. + private async Task SeedAsync() + { + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + var suffix = Guid.NewGuid().ToString("N")[..10]; + var actor = new User { Id = Guid.NewGuid(), Email = $"sweep-{suffix}@test.local", Name = $"sweep-{suffix}" }; + var team = new Team { Id = Guid.NewGuid(), Slug = $"sweep-{suffix}", Name = "Sweep Team" }; + + db.User.Add(actor); + db.Team.Add(team); + db.TeamMembership.Add(new TeamMembership { Id = Guid.NewGuid(), TeamId = team.Id, UserId = actor.Id, Role = TeamRole.Owner }); + + var deletedInstance = Instance(team.Id, $"https://gone-{suffix}.example", "CfDJ8-CANARY-deleted-client-secret"); + var liveInstance = Instance(team.Id, $"https://live-{suffix}.example", "CfDJ8-CANARY-live-client-secret"); + var retired = Token(team.Id, deletedInstance.Id, "CfDJ8-CANARY-retired-token"); + var live = Token(team.Id, liveInstance.Id, "CfDJ8-CANARY-live-token"); + + db.ProviderInstance.AddRange(deletedInstance, liveInstance); + db.Credential.AddRange(retired, live); + + var deletedVariable = SecretVariable(team.Id, "DELETED_SECRET", new byte[] { 1, 2, 3, 4 }); + var liveVariable = SecretVariable(team.Id, "LIVE_SECRET", new byte[] { 7, 7, 7, 7 }); + + db.Variable.AddRange(deletedVariable, liveVariable); + await db.SaveChangesAsync(); + + db.ConnectionWebhook.AddRange(Hook(deletedInstance.Id, retired.Id, "hook-777", "CfDJ8-CANARY-deleted-hook-secret"), Hook(liveInstance.Id, live.Id, "hook-888", "CfDJ8-CANARY-live-hook-secret")); + + var revoked = await StorageCredentialAsync(db, team.Id, actor.Id, "revoked"); + var active = await StorageCredentialAsync(db, team.Id, actor.Id, "active"); + var replaced = new StorageDefaultCredential { Id = Guid.NewGuid(), ProviderTypeKey = LocalRwxArtifactStorageDriverFactory.TypeKey, EncryptedPayload = "CfDJ8-CANARY-replaced-envelope", EnvelopeFingerprint = $"sha256:{new string('c', 64)}", CreatedDate = DateTimeOffset.UtcNow, CreatedBy = actor.Id }; + var template = Template(actor.Id, replaced.Id); + + db.StorageDefaultCredential.Add(replaced); + db.StorageDefault.Add(template); + await db.SaveChangesAsync(); + + return new World(actor.Id, deletedInstance.Id, liveInstance.Id, retired.Id, live.Id, deletedVariable.Id, liveVariable.Id, revoked.Id, active.Id, template.Id, replaced.Id, Guid.NewGuid()); + } + + private static ProviderInstance Instance(Guid teamId, string baseUrl, string clientSecretEnc) => new() + { + Id = Guid.NewGuid(), TeamId = teamId, Provider = ProviderKind.GitLab, DisplayName = "gitlab", BaseUrl = baseUrl, OauthClientId = "client", OauthClientSecretEnc = clientSecretEnc, WebhookScope = ProviderWebhookScope.Connection + }; + + private static Credential Token(Guid teamId, Guid instanceId, string payload) => new() + { + Id = Guid.NewGuid(), TeamId = teamId, ProviderInstanceId = instanceId, Ownership = CredentialOwnership.TeamService, AuthType = AuthType.OAuth, DisplayName = "token", EncryptedPayload = payload, Status = CredentialStatus.Active + }; + + private static Variable SecretVariable(Guid teamId, string name, byte[] ciphertext) => new() + { + Id = Guid.NewGuid(), Scope = VariableScope.Team, ScopeId = teamId, TeamId = teamId, Name = name, ValueType = VariableValueType.Secret, ValueEncrypted = ciphertext + }; + + private static ConnectionWebhook Hook(Guid instanceId, Guid credentialId, string externalId, string secretEnc) + { + var id = Guid.NewGuid(); + + return new ConnectionWebhook + { + Id = id, ProviderInstanceId = instanceId, CredentialId = credentialId, OwnerPath = "acme", ExternalId = externalId, CallbackUrl = $"https://codespace.test/api/webhooks/connection/{id}", + SecretEnc = secretEnc, SubscribedEvents = new List { "push" }, RegistrationStatus = RepositoryWebhookRegistrationStatus.Registered + }; + } + + /// A template on a data class of its own — a data class is instance-wide, so this test claims none another test uses. + private static StorageDefault Template(Guid actorId, Guid credentialId) => new() + { + Id = Guid.NewGuid(), DataClassTypeKey = $"t{Guid.NewGuid():N}/v1", Revision = 1, ProviderTypeKey = LocalRwxArtifactStorageDriverFactory.TypeKey, NonSecretConfigJson = "{}", NamespaceRoot = "codespace-defaults/", + CredentialId = credentialId, AdoptionPolicy = StorageDefaultAdoptionPolicy.Explicit, IsEnabled = true, CreatedDate = DateTimeOffset.UtcNow, CreatedBy = actorId, LastModifiedDate = DateTimeOffset.UtcNow, LastModifiedBy = actorId + }; + + /// A storage credential rotated once, through the append-only path its guard admits. + private static async Task StorageCredentialAsync(CodeSpaceDbContext db, Guid teamId, Guid actorId, string name) + { + var credential = new StorageCredential { Id = Guid.NewGuid(), TeamId = teamId, StableName = $"{name}-{Guid.NewGuid():N}", CurrentRevision = 1, State = StorageCredentialState.Active, CreatedDate = DateTimeOffset.UtcNow, CreatedBy = actorId }; + credential.Revisions.Add(StorageRevision(credential, actorId, 1, $"CfDJ8-CANARY-{name}-rev-1")); + db.StorageCredential.Add(credential); + await db.SaveChangesAsync(); + + db.StorageCredentialRevision.Add(StorageRevision(credential, actorId, 2, $"CfDJ8-CANARY-{name}-rev-2")); + credential.CurrentRevision = 2; + await db.SaveChangesAsync(); + + return credential; + } + + private static StorageCredentialRevision StorageRevision(StorageCredential credential, Guid actorId, int revision, string payload) => new() + { + Id = Guid.NewGuid(), TeamId = credential.TeamId, StorageCredentialId = credential.Id, Revision = revision, ProviderTypeKey = LocalRwxArtifactStorageDriverFactory.TypeKey, + EncryptedPayload = payload, EnvelopeFingerprint = $"sha256:{new string((char)('a' + revision), 64)}", CreatedDate = DateTimeOffset.UtcNow, CreatedBy = actorId + }; + + private async Task PayloadAsync(Guid credentialId) + { + using var scope = _fixture.BeginScope(); + + return await scope.Resolve().Credential.AsNoTracking().Where(c => c.Id == credentialId).Select(c => c.EncryptedPayload).SingleAsync(); + } + + private static async Task> RevisionsAsync(CodeSpaceDbContext db, Guid credentialId) => + await db.StorageCredentialRevision.AsNoTracking().Where(r => r.StorageCredentialId == credentialId).OrderBy(r => r.Revision).Select(r => r.EncryptedPayload).ToListAsync(); + + private static async Task EnvelopeAsync(CodeSpaceDbContext db, Guid envelopeId) => + await db.StorageDefaultCredential.AsNoTracking().Where(e => e.Id == envelopeId).Select(e => e.EncryptedPayload).SingleAsync(); +} diff --git a/backend/tests/CodeSpace.IntegrationTests/OAuth/CredentialPayloadReaderFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/OAuth/CredentialPayloadReaderFlowTests.cs new file mode 100644 index 000000000..97bdce049 --- /dev/null +++ b/backend/tests/CodeSpace.IntegrationTests/OAuth/CredentialPayloadReaderFlowTests.cs @@ -0,0 +1,65 @@ +using Autofac; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.OAuth; +using CodeSpace.IntegrationTests.Infrastructure; +using CodeSpace.Messages.Enums; +using Shouldly; + +namespace CodeSpace.IntegrationTests.OAuth; + +/// +/// A refresh overtaken by a write that blanked its row ends the pair it was issued the way that write ends tokens, and +/// tells the two writes apart by : a retirement (connection delete, +/// departure, relink) soft-deletes the row; a Disconnect leaves it listed, Revoked. A row that is gone counts as retired, +/// so the refresher never ends a grant on a guess. Read through the production reader's detached connection against +/// Postgres, on rows written as each path leaves them. +/// +[Collection(PostgresCollection.Name)] +[Trait("Category", "Integration")] +public class CredentialPayloadReaderFlowTests +{ + private readonly PostgresFixture _fixture; + + public CredentialPayloadReaderFlowTests(PostgresFixture fixture) { _fixture = fixture; } + + public enum StoredState { Active, Disconnected, Retired, Gone } + + [Theory] + [InlineData(StoredState.Active, false)] + [InlineData(StoredState.Disconnected, false)] + [InlineData(StoredState.Retired, true)] + [InlineData(StoredState.Gone, true)] + public async Task A_credential_is_retired_once_soft_deleted_or_gone_and_not_while_listed(StoredState state, bool expectedRetired) + { + var credentialId = state == StoredState.Gone ? Guid.NewGuid() : await SeedCredentialAsync(state); + + using var scope = _fixture.BeginScope(); + var retired = await scope.Resolve().IsRetiredAsync(credentialId, CancellationToken.None); + + retired.ShouldBe(expectedRetired, $"a {state} credential decides whether a refresh it overtook ends the user's whole grant — check the query in CredentialPayloadReader.IsRetiredAsync"); + } + + /// A credential as each path leaves it: a Disconnect blanks and revokes it in place; a retirement also soft-deletes it. + private async Task SeedCredentialAsync(StoredState state) + { + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + var team = new Team { Id = Guid.NewGuid(), Slug = $"reader-{Guid.NewGuid().ToString("N")[..8]}", Name = "Reader" }; + var instance = new ProviderInstance { Id = Guid.NewGuid(), TeamId = team.Id, Provider = ProviderKind.GitHub, DisplayName = "GitHub", BaseUrl = "https://github.example" }; + var credential = new Credential + { + Id = Guid.NewGuid(), TeamId = team.Id, ProviderInstanceId = instance.Id, Ownership = CredentialOwnership.TeamService, AuthType = AuthType.OAuth, DisplayName = "octocat", + EncryptedPayload = state == StoredState.Active ? "CfDJ8-CANARY-live-token" : string.Empty, + Status = state == StoredState.Active ? CredentialStatus.Active : CredentialStatus.Revoked, + DeletedDate = state == StoredState.Retired ? DateTimeOffset.UtcNow : null + }; + + db.Team.Add(team); + db.ProviderInstance.Add(instance); + db.Credential.Add(credential); + await db.SaveChangesAsync(); + + return credential.Id; + } +} diff --git a/backend/tests/CodeSpace.IntegrationTests/Persistence/RetiredSecretShredMigrationTests.cs b/backend/tests/CodeSpace.IntegrationTests/Persistence/RetiredSecretShredMigrationTests.cs new file mode 100644 index 000000000..da52b6cb3 --- /dev/null +++ b/backend/tests/CodeSpace.IntegrationTests/Persistence/RetiredSecretShredMigrationTests.cs @@ -0,0 +1,410 @@ +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Credentials; +using CodeSpace.Core.Services.Workflows.Artifacts.Providers.Local; +using CodeSpace.IntegrationTests.Infrastructure; +using CodeSpace.Messages.Enums; +using DbUp; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging.Abstractions; +using Npgsql; +using Shouldly; + +namespace CodeSpace.IntegrationTests.Persistence; + +/// +/// 0249 over a database that already holds what retirement used to leave behind: a deleted connection's Revoked tokens, +/// OAuth client secret and group-hook secrets, a deleted Secret variable's ciphertext, every revision of a revoked storage +/// credential, and the deployment-default envelopes a rotation or a clear left unnamed — all still decryptable. The +/// upgrade blanks exactly those and nothing live, relaxes the three schema guards that forbade it, and must not refuse: +/// a migration that throws is a deployment that does not start. +/// +/// Driven through the production over its own database, migrated up to (not including) +/// 0249 and seeded through the real model — the schema a deployment upgrading across 0249 meets. Dropped on dispose; the +/// shared test database is never touched. +/// +/// 0249 runs once. The same rows written after it — by a build older than 0249 still serving, or after a rollback — +/// are the recurring sweep's (), and it is held to the same assertions. +/// +[Collection(PostgresCollection.Name)] +[Trait("Category", "Integration")] +public sealed class RetiredSecretShredMigrationTests +{ + private const string Shredding = "0249"; + + private readonly PostgresFixture _fixture; + + public RetiredSecretShredMigrationTests(PostgresFixture fixture) { _fixture = fixture; } + + [Fact] + public async Task Upgrading_shreds_what_retirement_left_behind_and_leaves_every_live_secret_alone() + { + await using var database = await PreUpgradeDatabase.CreateAsync(_fixture.ConnectionString, Shredding); + var world = await SeedAsync(database.ConnectionString); + + new DbUpRunner(database.ConnectionString).Run(); + + await using var db = Context(database.ConnectionString); + + await ShouldHoldNoRetiredSecretAndEveryLiveOneAsync(db, world); + } + + /// + /// The same rows written AFTER the upgrade — what a build older than 0249 leaves when it retires something while it + /// still serves during a rolling deploy, or after a rollback. 0249 has already run and does not run again; the + /// recurring sweep must shred exactly what it would have. Same seed, same assertions as the upgrade above, so the two + /// cannot drift apart on this corpus: a kind or a predicate the sweep misses, or one it over-reaches, fails here. + /// + [Fact] + public async Task The_sweep_shreds_what_an_older_build_retires_after_the_upgrade_exactly_as_the_upgrade_does() + { + await using var database = await PreUpgradeDatabase.CreateAsync(_fixture.ConnectionString, Shredding); + + new DbUpRunner(database.ConnectionString).Run(); + + var world = await SeedAsync(database.ConnectionString); + + await using (var sweep = Context(database.ConnectionString)) + { + (await new RetiredSecretShredService(sweep, NullLogger.Instance).ShredAsync(CancellationToken.None)).ShouldBeGreaterThan(0, "fixture check: the older build's retirements were written after 0249 and left their ciphertext"); + } + + await using var db = Context(database.ConnectionString); + + await ShouldHoldNoRetiredSecretAndEveryLiveOneAsync(db, world); + } + + private static async Task ShouldHoldNoRetiredSecretAndEveryLiveOneAsync(CodeSpaceDbContext db, World world) + { + (await PayloadAsync(db, world.RetiredCredentialId)).ShouldBe(string.Empty, "a deleted connection's Revoked token must not stay decryptable"); + (await PayloadAsync(db, world.SoftDeletedCredentialId)).ShouldBe(string.Empty, "a soft-deleted token is out of every list and every read; nothing presents it again"); + (await PayloadAsync(db, world.DisconnectedCredentialId)).ShouldBe(string.Empty, "a Revoked token on a live connection is no more usable than one on a deleted connection"); + (await PayloadAsync(db, world.LiveCredentialId)).ShouldBe("CfDJ8-CANARY-live-token", "a live token is untouched"); + (await ClientSecretAsync(db, world.DeletedInstanceId)).ShouldBeNull("a deleted connection's OAuth client secret must not stay decryptable"); + (await ClientSecretAsync(db, world.LiveInstanceId)).ShouldBe("CfDJ8-CANARY-live-client-secret", "a live connection keeps its client secret"); + + (await VariableAsync(db, world.DeletedSecretVariableId)).ValueEncrypted.ShouldBeNull("a deleted Secret variable's ciphertext must not survive the upgrade"); + (await VariableAsync(db, world.LiveSecretVariableId)).ValueEncrypted.ShouldBe(world.LiveCiphertext, "a live Secret keeps its ciphertext byte for byte"); + (await VariableAsync(db, world.DeletedPlainVariableId)).ValuePlain.ShouldBe("\"plain-kept\"", "a plain value is no secret and stays"); + + (await RevisionPayloadsAsync(db, world.RevokedStorageCredentialId)).ShouldBe(new[] { string.Empty, string.Empty }, ignoreOrder: false, customMessage: "every revision of a revoked storage credential is blanked"); + (await RevisionPayloadsAsync(db, world.ActiveStorageCredentialId)).ShouldBe(new[] { "CfDJ8-CANARY-active-rev-1", "CfDJ8-CANARY-active-rev-2" }, ignoreOrder: false, customMessage: "an Active credential's revisions — the superseded one too, which a profile may pin — are untouched"); + + var deletedHooks = await HooksAsync(db, world.DeletedInstanceId); + deletedHooks.Count.ShouldBe(3, "fixture check: a Registered, a Pending and a Cancelled hook were seeded on the deleted connection"); + deletedHooks.ShouldAllBe(h => h.RegistrationStatus == RepositoryWebhookRegistrationStatus.Cancelled && h.SecretEnc == string.Empty, "a deleted connection's group hooks keep no secret, and none stays in service with a secret it no longer holds"); + deletedHooks.Single(h => h.OwnerPath == "acme").ExternalId.ShouldBe("hook-777", "the provider's hook id stays: it is what an operator removes by hand, since no token is left to remove it"); + (await HooksAsync(db, world.LiveInstanceId)).ShouldHaveSingleItem().ShouldSatisfyAllConditions( + h => h.RegistrationStatus.ShouldBe(RepositoryWebhookRegistrationStatus.Registered, "a live connection's hook stays in service"), + h => h.SecretEnc.ShouldBe("CfDJ8-CANARY-live-hook-secret", "and keeps the secret its deliveries are verified against")); + + (await EnvelopeAsync(db, world.SupersededEnvelopeId)).ShouldBe(string.Empty, "a deployment-default envelope no template names has no reader left"); + (await EnvelopeAsync(db, world.NamedEnvelopeId)).ShouldBe("CfDJ8-CANARY-named-envelope", "the envelope a template names is the one the materializer decrypts"); + } + + [Fact] + public async Task After_the_upgrade_the_guards_still_refuse_everything_but_a_retirement() + { + await using var database = await PreUpgradeDatabase.CreateAsync(_fixture.ConnectionString, Shredding); + var world = await SeedAsync(database.ConnectionString); + + new DbUpRunner(database.ConnectionString).Run(); + + (await RefusalAsync(database.ConnectionString, $"UPDATE storage_credential_revision SET encrypted_payload = '' WHERE storage_credential_id = '{world.ActiveStorageCredentialId}' AND revision = 1")).ShouldContain("Revoked"); + (await RefusalAsync(database.ConnectionString, $"UPDATE storage_credential_revision SET envelope_fingerprint = 'sha256:{new string('f', 64)}' WHERE storage_credential_id = '{world.RevokedStorageCredentialId}'")).ShouldContain("immutable"); + (await RefusalAsync(database.ConnectionString, $"UPDATE variable SET value_encrypted = NULL WHERE id = '{world.LiveSecretVariableId}'")).ShouldContain("chk_variable_value_exclusive"); + (await RefusalAsync(database.ConnectionString, $"UPDATE storage_default_credential SET encrypted_payload = '' WHERE id = '{world.NamedEnvelopeId}'")).ShouldContain("no template names it"); + (await RefusalAsync(database.ConnectionString, $"UPDATE storage_default_credential SET safe_hint = 'rewritten' WHERE id = '{world.SupersededEnvelopeId}'")).ShouldContain("append-only"); + (await RefusalAsync(database.ConnectionString, $"DELETE FROM storage_default_credential WHERE id = '{world.SupersededEnvelopeId}'")).ShouldContain("append-only"); + (await RefusalAsync(database.ConnectionString, $"INSERT INTO storage_default_credential (id, provider_type_key, encrypted_payload, envelope_fingerprint, created_date, created_by) SELECT gen_random_uuid(), provider_type_key, '', envelope_fingerprint, now(), created_by FROM storage_default_credential WHERE id = '{world.NamedEnvelopeId}'")).ShouldContain("payload is required"); + } + + /// + /// The rollback the script's header documents, run as written on a database the script has shredded. The old checks + /// reject exactly the rows a shred leaves — a deleted Secret with no ciphertext, a blank revision or envelope — so a + /// note that restored them as they were would fail with 23514 during the very incident it is followed in. The + /// statements are read out of the header itself, so the note cannot drift from what this proves. + /// + [Fact] + public async Task The_documented_rollback_runs_on_a_shredded_database_and_restores_the_old_rules_for_new_rows() + { + await using var database = await PreUpgradeDatabase.CreateAsync(_fixture.ConnectionString, Shredding); + var world = await SeedAsync(database.ConnectionString); + + new DbUpRunner(database.ConnectionString).Run(); + + var rollback = RollbackScript(); + + rollback.ShouldContain("NOT VALID", customMessage: "fixture check: the statements were read out of 0249's header"); + + await ExecuteAsync(database.ConnectionString, rollback); + + (await RefusalAsync(database.ConnectionString, $"UPDATE storage_credential_revision SET encrypted_payload = '' WHERE storage_credential_id = '{world.ActiveStorageCredentialId}' AND revision = 1")).ShouldContain("immutable"); + (await RefusalAsync(database.ConnectionString, $"UPDATE storage_default_credential SET encrypted_payload = 'x' WHERE id = '{world.NamedEnvelopeId}'")).ShouldContain("append-only"); + (await RefusalAsync(database.ConnectionString, $"UPDATE variable SET value_encrypted = NULL, deleted_date = now() WHERE id = '{world.LiveSecretVariableId}'")).ShouldContain("chk_variable_value_exclusive"); + } + + /// + /// Rolling forward after that rollback, the way the header says: drop the script's journal row and start again. With + /// the old checks back, this build could not retire a secret — a Secret variable's delete would fail with 23514 and + /// the recurring sweep would fail every tick — so the script has to apply a second time over its own shredded rows, + /// and admit the application's blanking again. + /// + [Fact] + public async Task Rolling_forward_after_the_documented_rollback_applies_the_script_again_and_retirement_blanks_again() + { + await using var database = await PreUpgradeDatabase.CreateAsync(_fixture.ConnectionString, Shredding); + var world = await SeedAsync(database.ConnectionString); + + new DbUpRunner(database.ConnectionString).Run(); + await ExecuteAsync(database.ConnectionString, RollbackScript()); + + var rollForward = HeaderStatements("DELETE FROM schemaversions ").ShouldHaveSingleItem("fixture check: the roll-forward statement was read out of 0249's header"); + + await ExecuteAsync(database.ConnectionString, rollForward); + new DbUpRunner(database.ConnectionString).Run(); + + await ExecuteAsync(database.ConnectionString, $"UPDATE variable SET value_encrypted = NULL, deleted_date = now() WHERE id = '{world.LiveSecretVariableId}'"); + + await using var db = Context(database.ConnectionString); + + (await VariableAsync(db, world.LiveSecretVariableId)).ValueEncrypted.ShouldBeNull("a Secret variable's delete blanks it again, as VariableRetirement does"); + (await new RetiredSecretShredService(db, NullLogger.Instance).ShredAsync(CancellationToken.None)).ShouldBe(0, "the second run's backfill found the rows the first left blank, and the sweep has nothing left to do"); + } + + /// + /// 0249's rollback: the statements its header lists, then 0126's and 0173's guard functions verbatim, as the header + /// says to. + /// + private static string RollbackScript() + { + var statements = HeaderStatements("ALTER TABLE ", "DROP TRIGGER ", "CREATE TRIGGER "); + + return string.Join("\n", statements.Append(FunctionBlock("0126_", "storage_credential_revision_guard")).Append(FunctionBlock("0173_", "storage_default_credential_guard"))); + } + + /// The statements 0249's header comment lists that start with one of . + private static IReadOnlyList HeaderStatements(params string[] starts) => + File.ReadLines(Script("0249_")) + .TakeWhile(line => line.StartsWith("--", StringComparison.Ordinal)) + .Select(line => line.Length > 2 ? line[2..].Trim() : string.Empty) + .Where(line => starts.Any(start => line.StartsWith(start, StringComparison.Ordinal))) + .ToList(); + + private static string FunctionBlock(string scriptPrefix, string function) + { + var script = File.ReadAllText(Script(scriptPrefix)); + var start = script.IndexOf($"CREATE OR REPLACE FUNCTION {function}()", StringComparison.Ordinal); + const string end = "$$ LANGUAGE plpgsql;"; + + start.ShouldBeGreaterThanOrEqualTo(0, $"fixture check: {scriptPrefix} defines {function}"); + + return script[start..(script.IndexOf(end, start, StringComparison.Ordinal) + end.Length)]; + } + + private static string Script(string prefix) => + Directory.GetFiles(Path.Combine(Path.GetDirectoryName(typeof(DbUpRunner).Assembly.Location)!, DbUpRunner.ScriptFolder), $"{prefix}*.sql").ShouldHaveSingleItem(); + + private static async Task ExecuteAsync(string connectionString, string sql) + { + await using var connection = new NpgsqlConnection(connectionString); + await connection.OpenAsync(); + await using var command = new NpgsqlCommand(sql, connection); + + await command.ExecuteNonQueryAsync(); + } + + private static async Task PayloadAsync(CodeSpaceDbContext db, Guid credentialId) => + await db.Credential.AsNoTracking().Where(c => c.Id == credentialId).Select(c => c.EncryptedPayload).SingleAsync(); + + private static async Task ClientSecretAsync(CodeSpaceDbContext db, Guid instanceId) => + await db.ProviderInstance.AsNoTracking().Where(p => p.Id == instanceId).Select(p => p.OauthClientSecretEnc).SingleAsync(); + + private static async Task VariableAsync(CodeSpaceDbContext db, Guid variableId) => + await db.Variable.AsNoTracking().SingleAsync(v => v.Id == variableId); + + private static async Task> RevisionPayloadsAsync(CodeSpaceDbContext db, Guid credentialId) => + await db.StorageCredentialRevision.AsNoTracking().Where(r => r.StorageCredentialId == credentialId).OrderBy(r => r.Revision).Select(r => r.EncryptedPayload).ToListAsync(); + + private static async Task> HooksAsync(CodeSpaceDbContext db, Guid instanceId) => + await db.ConnectionWebhook.AsNoTracking().Where(w => w.ProviderInstanceId == instanceId).ToListAsync(); + + private static async Task EnvelopeAsync(CodeSpaceDbContext db, Guid envelopeId) => + await db.StorageDefaultCredential.AsNoTracking().Where(e => e.Id == envelopeId).Select(e => e.EncryptedPayload).SingleAsync(); + + private static async Task RefusalAsync(string connectionString, string sql) + { + await using var connection = new NpgsqlConnection(connectionString); + await connection.OpenAsync(); + await using var command = new NpgsqlCommand(sql, connection); + + var refused = await Should.ThrowAsync(() => command.ExecuteNonQueryAsync()); + + return $"{refused.ConstraintName} {refused.MessageText}"; + } + + private sealed record World(Guid DeletedInstanceId, Guid LiveInstanceId, Guid RetiredCredentialId, Guid SoftDeletedCredentialId, Guid DisconnectedCredentialId, Guid LiveCredentialId, Guid DeletedSecretVariableId, Guid LiveSecretVariableId, Guid DeletedPlainVariableId, byte[] LiveCiphertext, Guid RevokedStorageCredentialId, Guid ActiveStorageCredentialId, Guid SupersededEnvelopeId, Guid NamedEnvelopeId); + + /// + /// The rows the pre-0249 code leaves: what its provider-instance delete, variable delete, storage revoke and + /// deployment-default rotation write, beside live ones of each kind. + /// + private static async Task SeedAsync(string connectionString) + { + await using var db = Context(connectionString); + var now = DateTimeOffset.UtcNow; + var suffix = Guid.NewGuid().ToString("N")[..10]; + var actor = new User { Id = Guid.NewGuid(), Email = $"shred-{suffix}@test.local", Name = $"shred-{suffix}" }; + var team = new Team { Id = Guid.NewGuid(), Slug = $"shred-{suffix}", Name = "Shred Team", Kind = TeamKind.Workspace }; + + db.User.Add(actor); + db.Team.Add(team); + db.TeamMembership.Add(new TeamMembership { Id = Guid.NewGuid(), TeamId = team.Id, UserId = actor.Id, Role = TeamRole.Owner }); + + var deletedInstance = new ProviderInstance { Id = Guid.NewGuid(), TeamId = team.Id, Provider = ProviderKind.GitLab, DisplayName = "gone", BaseUrl = $"https://gone-{suffix}.example", OauthClientId = "client", OauthClientSecretEnc = "CfDJ8-CANARY-deleted-client-secret", DeletedDate = now }; + var liveInstance = new ProviderInstance { Id = Guid.NewGuid(), TeamId = team.Id, Provider = ProviderKind.GitLab, DisplayName = "live", BaseUrl = $"https://live-{suffix}.example", OauthClientId = "client", OauthClientSecretEnc = "CfDJ8-CANARY-live-client-secret" }; + var retired = Token(team.Id, deletedInstance.Id, "CfDJ8-CANARY-retired-token", CredentialStatus.Revoked, now); + var softDeleted = Token(team.Id, liveInstance.Id, "CfDJ8-CANARY-soft-deleted-token", CredentialStatus.Active, now); + var disconnected = Token(team.Id, liveInstance.Id, "CfDJ8-CANARY-disconnected-token", CredentialStatus.Revoked, deletedDate: null); + var live = Token(team.Id, liveInstance.Id, "CfDJ8-CANARY-live-token", CredentialStatus.Active, deletedDate: null); + + db.ProviderInstance.AddRange(deletedInstance, liveInstance); + db.Credential.AddRange(retired, softDeleted, disconnected, live); + + var liveCiphertext = new byte[] { 7, 7, 7, 7 }; + var deletedSecret = Variable(team.Id, "DELETED_SECRET", VariableValueType.Secret, plain: null, encrypted: new byte[] { 1, 2, 3, 4 }, now); + var liveSecret = Variable(team.Id, "LIVE_SECRET", VariableValueType.Secret, plain: null, encrypted: liveCiphertext, deletedDate: null); + var deletedPlain = Variable(team.Id, "DELETED_PLAIN", VariableValueType.String, plain: "\"plain-kept\"", encrypted: null, now); + + db.Variable.AddRange(deletedSecret, liveSecret, deletedPlain); + await db.SaveChangesAsync(); + + var revoked = await StorageCredentialAsync(db, team.Id, actor.Id, "revoked"); + var active = await StorageCredentialAsync(db, team.Id, actor.Id, "active"); + + revoked.State = StorageCredentialState.Revoked; + revoked.RevokedDate = now; + revoked.RevokedBy = actor.Id; + + db.ConnectionWebhook.AddRange( + Hook(deletedInstance.Id, retired.Id, "acme", RepositoryWebhookRegistrationStatus.Registered, "hook-777", "CfDJ8-CANARY-deleted-hook-secret"), + Hook(deletedInstance.Id, retired.Id, "acme/web", RepositoryWebhookRegistrationStatus.Pending, externalId: null, "CfDJ8-CANARY-pending-hook-secret"), + Hook(deletedInstance.Id, retired.Id, "acme/old", RepositoryWebhookRegistrationStatus.Cancelled, externalId: null, "CfDJ8-CANARY-cancelled-hook-secret"), + Hook(liveInstance.Id, live.Id, "acme", RepositoryWebhookRegistrationStatus.Registered, "hook-888", "CfDJ8-CANARY-live-hook-secret")); + + var superseded = Envelope(actor.Id, "CfDJ8-CANARY-superseded-envelope"); + var named = Envelope(actor.Id, "CfDJ8-CANARY-named-envelope"); + + db.StorageDefaultCredential.AddRange(superseded, named); + db.StorageDefault.Add(Template(actor.Id, named.Id, now)); + await db.SaveChangesAsync(); + + return new World(deletedInstance.Id, liveInstance.Id, retired.Id, softDeleted.Id, disconnected.Id, live.Id, deletedSecret.Id, liveSecret.Id, deletedPlain.Id, liveCiphertext, revoked.Id, active.Id, superseded.Id, named.Id); + } + + private static ConnectionWebhook Hook(Guid instanceId, Guid credentialId, string ownerPath, RepositoryWebhookRegistrationStatus status, string? externalId, string secretEnc) + { + var id = Guid.NewGuid(); + + return new ConnectionWebhook + { + Id = id, ProviderInstanceId = instanceId, CredentialId = credentialId, OwnerPath = ownerPath, ExternalId = externalId, CallbackUrl = $"https://codespace.test/api/webhooks/connection/{id}", + SecretEnc = secretEnc, SubscribedEvents = new List { "push" }, RegistrationStatus = status, + }; + } + + private static StorageDefaultCredential Envelope(Guid actorId, string payload) => new() + { + Id = Guid.NewGuid(), ProviderTypeKey = LocalRwxArtifactStorageDriverFactory.TypeKey, EncryptedPayload = payload, EnvelopeFingerprint = $"sha256:{new string('c', 64)}", CreatedDate = DateTimeOffset.UtcNow, CreatedBy = actorId, + }; + + /// A template on a key of its own — a data class is instance-wide, so the shred test claims none another test uses. + private static StorageDefault Template(Guid actorId, Guid credentialId, DateTimeOffset now) => new() + { + Id = Guid.NewGuid(), DataClassTypeKey = $"t{Guid.NewGuid():N}/v1", Revision = 2, ProviderTypeKey = LocalRwxArtifactStorageDriverFactory.TypeKey, NonSecretConfigJson = "{}", NamespaceRoot = "codespace-defaults/", + CredentialId = credentialId, AdoptionPolicy = StorageDefaultAdoptionPolicy.Explicit, IsEnabled = true, CreatedDate = now, CreatedBy = actorId, LastModifiedDate = now, LastModifiedBy = actorId, + }; + + private static Credential Token(Guid teamId, Guid instanceId, string payload, CredentialStatus status, DateTimeOffset? deletedDate) => new() + { + Id = Guid.NewGuid(), TeamId = teamId, ProviderInstanceId = instanceId, Ownership = CredentialOwnership.TeamService, AuthType = AuthType.OAuth, + DisplayName = "token", EncryptedPayload = payload, Status = status, DeletedDate = deletedDate, + }; + + private static Variable Variable(Guid teamId, string name, VariableValueType type, string? plain, byte[]? encrypted, DateTimeOffset? deletedDate) => new() + { + Id = Guid.NewGuid(), Scope = VariableScope.Team, ScopeId = teamId, TeamId = teamId, Name = name, ValueType = type, ValuePlain = plain, ValueEncrypted = encrypted, DeletedDate = deletedDate, + }; + + /// A storage credential rotated once, through the append-only path its guard admits. + private static async Task StorageCredentialAsync(CodeSpaceDbContext db, Guid teamId, Guid actorId, string name) + { + var credential = new StorageCredential { Id = Guid.NewGuid(), TeamId = teamId, StableName = $"{name}-{Guid.NewGuid():N}", CurrentRevision = 1, State = StorageCredentialState.Active, CreatedDate = DateTimeOffset.UtcNow, CreatedBy = actorId }; + credential.Revisions.Add(StorageRevision(credential, actorId, 1, $"CfDJ8-CANARY-{name}-rev-1")); + db.StorageCredential.Add(credential); + await db.SaveChangesAsync(); + + db.StorageCredentialRevision.Add(StorageRevision(credential, actorId, 2, $"CfDJ8-CANARY-{name}-rev-2")); + credential.CurrentRevision = 2; + await db.SaveChangesAsync(); + + return credential; + } + + private static StorageCredentialRevision StorageRevision(StorageCredential credential, Guid actorId, int revision, string payload) => new() + { + Id = Guid.NewGuid(), TeamId = credential.TeamId, StorageCredentialId = credential.Id, Revision = revision, ProviderTypeKey = LocalRwxArtifactStorageDriverFactory.TypeKey, + EncryptedPayload = payload, EnvelopeFingerprint = $"sha256:{new string((char)('a' + revision), 64)}", CreatedDate = DateTimeOffset.UtcNow, CreatedBy = actorId, + }; + + private static CodeSpaceDbContext Context(string connectionString) => + new(new DbContextOptionsBuilder().UseNpgsql(connectionString).UseSnakeCaseNamingConvention().Options); + + /// + /// Its own database, migrated only up to (not including) firstPendingScript, so the runner meets exactly the + /// schema a deployment upgrading across that script meets. The name carries a GUID; dropped on dispose. + /// + private sealed class PreUpgradeDatabase : IAsyncDisposable + { + private readonly string _name; + + private PreUpgradeDatabase(string connectionString, string name) { ConnectionString = connectionString; _name = name; } + + public string ConnectionString { get; } + + public static async Task CreateAsync(string sharedConnectionString, string firstPendingScript) + { + var name = $"codespace_shred_{Guid.NewGuid():N}"; + var database = new PreUpgradeDatabase(new NpgsqlConnectionStringBuilder(sharedConnectionString) { Database = name }.ConnectionString, name); + + await database.AdminAsync($"CREATE DATABASE \"{name}\""); + + try + { + var scripts = Path.Combine(Path.GetDirectoryName(typeof(DbUpRunner).Assembly.Location)!, DbUpRunner.ScriptFolder); + var upgrade = DeployChanges.To.PostgresqlDatabase(database.ConnectionString) + .WithScriptsFromFileSystem(scripts, path => string.CompareOrdinal(Path.GetFileName(path)[..4], firstPendingScript) < 0) + .WithVariablesDisabled().WithTransaction().LogToNowhere().Build().PerformUpgrade(); + + if (!upgrade.Successful) throw upgrade.Error; + + return database; + } + catch + { + await database.DisposeAsync(); + throw; + } + } + + public async ValueTask DisposeAsync() => await AdminAsync($"DROP DATABASE IF EXISTS \"{_name}\" WITH (FORCE)"); + + private async Task AdminAsync(string sql) + { + await using var admin = new NpgsqlConnection(new NpgsqlConnectionStringBuilder(ConnectionString) { Database = "postgres" }.ConnectionString); + await admin.OpenAsync(); + await using var command = new NpgsqlCommand(sql, admin); + await command.ExecuteNonQueryAsync(); + } + } +} diff --git a/backend/tests/CodeSpace.IntegrationTests/Persistence/StorageDefaultPersistenceTests.cs b/backend/tests/CodeSpace.IntegrationTests/Persistence/StorageDefaultPersistenceTests.cs index 0ced825d6..0b37f96fd 100644 --- a/backend/tests/CodeSpace.IntegrationTests/Persistence/StorageDefaultPersistenceTests.cs +++ b/backend/tests/CodeSpace.IntegrationTests/Persistence/StorageDefaultPersistenceTests.cs @@ -160,6 +160,48 @@ public async Task Credential_envelope_is_append_only() } } + /// + /// The one change an envelope ever admits (0249): its payload blanked, every other column as it was, once no template + /// names it — the materializer decrypts only the envelope its template points at, so a rotated-out or cleared one has + /// no reader left, and keeping it would leave a deployment-wide cloud key readable from every backup forever. The + /// envelope a template still names is refused, a second blanking is refused, and an envelope is never inserted blank. + /// + [Fact] + public async Task Credential_envelope_admits_one_blanking_and_only_once_no_template_names_it() + { + var (_, actorId) = await SeedTeamAsync(); + var named = Credential(actorId, "CfDJ8-CANARY-named-envelope"); + var superseded = Credential(actorId, "CfDJ8-CANARY-superseded-envelope"); + + using (var scope = _fixture.BeginScope()) + { + var db = scope.Resolve(); + db.StorageDefaultCredential.AddRange(named, superseded); + db.StorageDefault.Add(Template(UniqueDataClass(), actorId, named.Id)); + await db.SaveChangesAsync(); + } + + (await SetEnvelopeAsync(superseded.Id)).ShouldBe(1, "an envelope no template names has no reader left and may be blanked"); + (await EnvelopeAsync(superseded.Id)).ShouldBe(string.Empty); + + foreach (var payload in new[] { string.Empty, "CfDJ8-CANARY-refilled" }) + { + var again = await Should.ThrowAsync(() => SetEnvelopeAsync(superseded.Id, payload)); + again.MessageText.ShouldContain("append-only", customMessage: "a blanked envelope is final — it is blanked once and nothing writes into it again"); + } + + var stillNamed = await Should.ThrowAsync(() => SetEnvelopeAsync(named.Id)); + stillNamed.MessageText.ShouldContain("no template names it", customMessage: "the materializer still decrypts the envelope its template points at"); + (await EnvelopeAsync(named.Id)).ShouldBe("CfDJ8-CANARY-named-envelope"); + + using (var scope = _fixture.BeginScope()) + { + var db = scope.Resolve(); + db.StorageDefaultCredential.Add(Credential(actorId, string.Empty)); + (await db.SaveChangesAsync().ShouldThrowAsync()).InnerException?.Message.ShouldContain("payload is required"); + } + } + /// /// One adoption per (team, data class), and the profile it names must belong to that same team — a materialization /// that recorded another team's profile would be provenance pointing at storage the team does not own. @@ -255,6 +297,25 @@ private async Task SeedProfileAsync(Guid teamId, Guid actorId) return profile.Id; } + /// Sets an envelope's payload the way a raw UPDATE would — by default to empty, the one change 0249 admits. + private async Task SetEnvelopeAsync(Guid credentialId, string payload = "") + { + await using var connection = new NpgsqlConnection(_fixture.ConnectionString); + await connection.OpenAsync(); + await using var command = new NpgsqlCommand("UPDATE storage_default_credential SET encrypted_payload = @payload WHERE id = @id", connection); + command.Parameters.AddWithValue("payload", payload); + command.Parameters.AddWithValue("id", credentialId); + + return await command.ExecuteNonQueryAsync(); + } + + private async Task EnvelopeAsync(Guid credentialId) + { + using var scope = _fixture.BeginScope(); + + return await scope.Resolve().StorageDefaultCredential.AsNoTracking().Where(value => value.Id == credentialId).Select(value => value.EncryptedPayload).SingleAsync(); + } + private async Task<(Guid TeamId, Guid ActorId)> SeedTeamAsync() { using var scope = _fixture.BeginScope(); diff --git a/backend/tests/CodeSpace.IntegrationTests/ProviderInstances/SharedOAuthAppRevocationFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/ProviderInstances/SharedOAuthAppRevocationFlowTests.cs new file mode 100644 index 000000000..8c21fde87 --- /dev/null +++ b/backend/tests/CodeSpace.IntegrationTests/ProviderInstances/SharedOAuthAppRevocationFlowTests.cs @@ -0,0 +1,316 @@ +using System.Collections.Concurrent; +using System.Text; +using System.Text.Json; +using Autofac; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Credentials; +using CodeSpace.Core.Services.Providers; +using CodeSpace.Core.Services.Providers.Auth; +using CodeSpace.IntegrationTests.Infrastructure; +using CodeSpace.IntegrationTests.Webhooks; +using CodeSpace.Messages.Commands.Credentials; +using CodeSpace.Messages.Commands.ProviderInstances; +using CodeSpace.Messages.Constants; +using CodeSpace.Messages.Credentials; +using CodeSpace.Messages.Enums; +using MediatR; +using Microsoft.EntityFrameworkCore; +using Npgsql; +using Shouldly; + +namespace CodeSpace.IntegrationTests.ProviderInstances; + +/// +/// Two teams connect GitHub through the same OAuth App — the unique index is per team, so nothing stops them sharing a +/// client id — and the same person authorizes it in both. GitHub keeps one grant per user and app: its grant delete ends +/// every token issued under it, the other team's included. Deleting one team's connection must end only the tokens that +/// team held; a Disconnect keeps ending the whole grant, as it always has. The same holds for the pair GitHub issues a +/// refresh that a retirement or a Disconnect overtakes. +/// +/// Drives the real chain: the mediator pipeline and its transaction, ProviderInstanceService and +/// CredentialService, the post-commit revoke, the real GitHubOAuthClient speaking HTTP, and Postgres — +/// against a GitHub on a loopback port that keeps grants the way GitHub does, so what each call ends is read off the +/// provider's own state rather than inferred from which URL was hit. +/// +[Collection(PostgresCollection.Name)] +[Trait("Category", "Integration")] +public class SharedOAuthAppRevocationFlowTests +{ + private const string ClientSecret = "shared-app-secret"; + + private readonly PostgresFixture _fixture; + + public SharedOAuthAppRevocationFlowTests(PostgresFixture fixture) { _fixture = fixture; } + + /// What lands while team A's refresh is at GitHub and blanks the token it read. + public enum RetiringWrite + { + /// This build's connection delete: it retires the credential (blanked, Revoked, soft-deleted) and ends team A's token by token. + ConnectionDelete, + + /// The previous build's connection delete — Revoked and soft-deleted, token kept, nothing asked of GitHub — then this build's sweep blanking it: a rolling deploy or a rollback. + PreviousBuildDeleteThenSweep + } + + [Fact] + public async Task Deleting_one_teams_connection_ends_only_that_teams_token_and_not_the_grant_another_team_shares() + { + using var gitHub = new StubProviderHost(); + var world = await SeedSharedAppAsync(gitHub, gitHub); + + await DeleteConnectionAsync(world.TeamA, world.InstanceA); + + world.Grants.IsLive(world.AccessTokenA).ShouldBeFalse($"team A's token must end at GitHub — check the post-commit revoke in ProviderInstanceService; GitHub saw: {Seen(gitHub)}"); + world.Grants.IsLive(world.AccessTokenB).ShouldBeTrue($"team B connected through the same OAuth App and must keep working; a grant delete would have ended its token too. GitHub saw: {Seen(gitHub)}"); + gitHub.Requests.ShouldNotContain(r => r.PathAndQuery.Contains("/grant"), "a connection delete never asks GitHub for the user's whole grant"); + gitHub.Requests.ShouldNotContain(r => r.Body.Contains(world.RefreshTokenA), "GitHub has no endpoint that ends a refresh token alone, so none is sent"); + (await StoredAccessTokenAsync(world.CredentialB)).ShouldBe(world.AccessTokenB, "the other team's credential is untouched here too"); + } + + [Fact] + public async Task A_disconnect_still_ends_the_whole_grant() + { + using var gitHub = new StubProviderHost(); + var world = await SeedSharedAppAsync(gitHub, gitHub); + + var result = await DisconnectAsync(world.TeamB, world.CredentialB); + + result.ProviderAcknowledged.ShouldBeTrue($"GitHub answered the grant delete; saw: {Seen(gitHub)}"); + gitHub.Requests.ShouldContain(r => r.Method == "DELETE" && r.PathAndQuery.EndsWith($"/applications/{world.ClientId}/grant", StringComparison.Ordinal), "a Disconnect ends the user's grant to the app, unchanged"); + world.Grants.IsLive(world.AccessTokenB).ShouldBeFalse(); + world.Grants.IsLive(world.AccessTokenA).ShouldBeFalse("the grant is the user's, not the team's: ending it ends the token the other team holds too — which is why a connection delete must not ask for it"); + } + + /// + /// Team A's token is inside the refresh window, and the connection is retired while GitHub holds the refresh: the + /// rotated pair GitHub then issues is stored nowhere, so the refresher ends it at GitHub itself. A retirement ends + /// tokens, never the person's grant — the same rule the delete's own revoke follows — so team B, connected through the + /// same OAuth App by the same person, keeps working. The refresher tells a retirement from a Disconnect by the + /// credential's soft delete, which only a retirement writes. + /// + [Theory] + [InlineData(RetiringWrite.ConnectionDelete)] + [InlineData(RetiringWrite.PreviousBuildDeleteThenSweep)] + public async Task A_connection_retired_while_its_refresh_is_at_GitHub_ends_only_the_pair_GitHub_issued(RetiringWrite retirement) + { + using var gitHub = new StubProviderHost(); + using var signIn = new StubProviderHost(); + var world = await SeedSharedAppAsync(gitHub, signIn); + world.Grants.ServeRefresh(signIn, "octocat", world.RotatedAccessTokenA, world.RotatedRefreshTokenA, whileRefreshing: () => RetireAsync(world, retirement).GetAwaiter().GetResult()); + + var failure = await Should.ThrowAsync(() => ResolveTokenAsync(world.CredentialA)); + + failure.Message.ShouldContain("was revoked", customMessage: "fixture check: the retirement blanked the row while GitHub answered, so the refresh found it empty"); + world.Grants.IsLive(world.RotatedAccessTokenA).ShouldBeFalse($"the pair issued while the connection was retired is held by nothing and must end at GitHub; GitHub saw: {Seen(gitHub)}"); + gitHub.Requests.ShouldNotContain(r => r.PathAndQuery.Contains("/grant"), $"a retirement racing a refresh never asks GitHub for the user's whole grant — check OAuthTokenRefresher.RevokeIssuedTokensAsync; GitHub saw: {Seen(gitHub)}"); + world.Grants.IsLive(world.AccessTokenB).ShouldBeTrue("team B connected through the same OAuth App and must keep working"); + gitHub.Requests.ShouldNotContain(r => r.Body.Contains(world.RotatedRefreshTokenA), "GitHub has no endpoint that ends a refresh token alone, so none is sent"); + } + + /// The sibling of the retirement race: a Disconnect landing while the refresh is at GitHub still ends the grant, the pair just issued included. + [Fact] + public async Task A_disconnect_while_a_refresh_is_at_GitHub_still_ends_the_whole_grant() + { + using var gitHub = new StubProviderHost(); + using var signIn = new StubProviderHost(); + var world = await SeedSharedAppAsync(gitHub, signIn); + world.Grants.ServeRefresh(signIn, "octocat", world.RotatedAccessTokenA, world.RotatedRefreshTokenA, whileRefreshing: () => DisconnectAsync(world.TeamA, world.CredentialA).GetAwaiter().GetResult()); + + var failure = await Should.ThrowAsync(() => ResolveTokenAsync(world.CredentialA)); + + failure.Message.ShouldContain("was revoked", customMessage: "fixture check: the Disconnect blanked the row while GitHub answered, so the refresh found it empty"); + gitHub.Requests.ShouldContain(r => r.Method == "DELETE" && r.PathAndQuery.EndsWith($"/applications/{world.ClientId}/grant", StringComparison.Ordinal) && r.Body.Contains(world.RotatedAccessTokenA), $"the pair issued during a Disconnect ends the way the Disconnect does — the whole grant; GitHub saw: {Seen(gitHub)}"); + world.Grants.IsLive(world.RotatedAccessTokenA).ShouldBeFalse(); + } + + private sealed record SharedApp(GitHubGrants Grants, string ClientId, Guid TeamA, Guid InstanceA, Guid CredentialA, string AccessTokenA, string RefreshTokenA, string RotatedAccessTokenA, string RotatedRefreshTokenA, Guid TeamB, Guid InstanceB, Guid CredentialB, string AccessTokenB); + + /// + /// The same person connected in two teams, each team with its own GitHub connection on the same host and OAuth App — + /// two credentials, two access tokens, one grant at GitHub. is GitHub's API, where tokens are + /// revoked; its sign-in host, where they are refreshed — two listeners, so a write landing + /// while a refresh is answered can reach the API. Team A's token is inside the refresh window; team B's is not. + /// + private async Task SeedSharedAppAsync(StubProviderHost gitHub, StubProviderHost signIn) + { + var canary = Guid.NewGuid().ToString("N")[..10]; + var clientId = $"shared-app-{canary}"; + var grants = GitHubGrants.Serve(gitHub, clientId, ClientSecret); + var (teamA, instanceA) = await SeedConnectionAsync(signIn.BaseUrl, gitHub.BaseUrl, clientId).ConfigureAwait(false); + var (teamB, instanceB) = await SeedConnectionAsync(signIn.BaseUrl, gitHub.BaseUrl, clientId).ConfigureAwait(false); + var credentialA = await SeedOAuthCredentialAsync(teamA, instanceA, $"at-A-{canary}", $"rt-A-{canary}", DateTimeOffset.UtcNow.AddSeconds(30)).ConfigureAwait(false); + var credentialB = await SeedOAuthCredentialAsync(teamB, instanceB, $"at-B-{canary}", $"rt-B-{canary}", DateTimeOffset.UtcNow.AddHours(8)).ConfigureAwait(false); + + grants.Issue("octocat", $"at-A-{canary}", $"at-B-{canary}"); + + return new SharedApp(grants, clientId, teamA, instanceA, credentialA, $"at-A-{canary}", $"rt-A-{canary}", $"at-A2-{canary}", $"rt-A2-{canary}", teamB, instanceB, credentialB, $"at-B-{canary}"); + } + + private async Task<(Guid TeamId, Guid InstanceId)> SeedConnectionAsync(string signInUrl, string apiUrl, string clientId) + { + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + var suffix = Guid.NewGuid().ToString("N")[..8]; + var team = new Team { Id = Guid.NewGuid(), Slug = $"shared-app-{suffix}", Name = "Shared app" }; + var instance = new ProviderInstance + { + Id = Guid.NewGuid(), TeamId = team.Id, Provider = ProviderKind.GitHub, DisplayName = "GitHub", BaseUrl = signInUrl, ApiUrl = apiUrl, + OauthClientId = clientId, OauthClientSecretEnc = scope.Resolve().Encrypt(ClientSecret) + }; + + db.Team.Add(team); + db.ProviderInstance.Add(instance); + await db.SaveChangesAsync().ConfigureAwait(false); + + return (team.Id, instance.Id); + } + + private async Task SeedOAuthCredentialAsync(Guid teamId, Guid instanceId, string accessToken, string refreshToken, DateTimeOffset expiresAt) + { + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + var payload = scope.Resolve().Serialize(new OAuthPayload { AccessToken = accessToken, RefreshToken = refreshToken, ExpiresAt = expiresAt }); + var credential = new Credential + { + Id = Guid.NewGuid(), TeamId = teamId, ProviderInstanceId = instanceId, Ownership = CredentialOwnership.TeamService, AuthType = AuthType.OAuth, + DisplayName = "octocat", EncryptedPayload = scope.Resolve().Encrypt(payload), ExpiresDate = expiresAt, Status = CredentialStatus.Active + }; + + db.Credential.Add(credential); + await db.SaveChangesAsync().ConfigureAwait(false); + + return credential.Id; + } + + private async Task DeleteConnectionAsync(Guid teamId, Guid instanceId) + { + using var scope = _fixture.BeginScopeAs(Guid.NewGuid(), teamId, Roles.Admin); + + await scope.Resolve().Send(new DeleteProviderInstanceCommand { ProviderInstanceId = instanceId }).ConfigureAwait(false); + } + + private async Task DisconnectAsync(Guid teamId, Guid credentialId) + { + using var scope = _fixture.BeginScopeAs(Guid.NewGuid(), teamId, Roles.Admin); + + return await scope.Resolve().Send(new RevokeCredentialCommand { CredentialId = credentialId }).ConfigureAwait(false); + } + + private Task RetireAsync(SharedApp world, RetiringWrite retirement) => retirement switch + { + RetiringWrite.ConnectionDelete => DeleteConnectionAsync(world.TeamA, world.InstanceA), + RetiringWrite.PreviousBuildDeleteThenSweep => DeleteAsThePreviousBuildThenSweepAsync(world.InstanceA), + _ => throw new ArgumentOutOfRangeException(nameof(retirement), retirement, null) + }; + + /// + /// The previous build's ProviderInstanceService.DeleteAsync, statement for statement — cascade-revoke then soft + /// delete, every token kept, nothing asked of GitHub — replayed as raw SQL against this build's schema, then this + /// build's recurring sweep through the real mediator, which blanks the token that delete kept. + /// + private async Task DeleteAsThePreviousBuildThenSweepAsync(Guid instanceId) + { + await using (var connection = new NpgsqlConnection(_fixture.ConnectionString)) + { + await connection.OpenAsync().ConfigureAwait(false); + await using var command = new NpgsqlCommand( + """ + UPDATE credential SET status = 'Revoked', deleted_date = now(), last_error = 'Provider instance deleted' WHERE provider_instance_id = @deleted AND deleted_date IS NULL AND status = 'Active'; + UPDATE provider_instance SET deleted_date = now() WHERE id = @deleted; + """, connection); + command.Parameters.AddWithValue("deleted", instanceId); + await command.ExecuteNonQueryAsync().ConfigureAwait(false); + } + + using var scope = _fixture.BeginScope(); + await scope.Resolve().Send(new ShredRetiredSecretsCommand()).ConfigureAwait(false); + } + + /// A run resolving the credential's token: the real auth resolver → GitHub OAuth strategy → refresher chain, on the row as a run loads it. + private async Task ResolveTokenAsync(Guid credentialId) + { + using var scope = _fixture.BeginScope(); + var credential = await scope.Resolve().Credential.AsNoTracking().Include(c => c.ProviderInstance).SingleAsync(c => c.Id == credentialId).ConfigureAwait(false); + var auth = await scope.Resolve().ResolveAsync(new ProviderContext(credential.ProviderInstance, credential), CancellationToken.None).ConfigureAwait(false); + + return auth.Token; + } + + private async Task StoredAccessTokenAsync(Guid credentialId) + { + using var scope = _fixture.BeginScope(); + var stored = await scope.Resolve().Credential.AsNoTracking().Where(c => c.Id == credentialId).Select(c => c.EncryptedPayload).SingleAsync().ConfigureAwait(false); + var payload = scope.Resolve().Deserialize(AuthType.OAuth, scope.Resolve().Decrypt(stored)); + + return payload.ShouldBeOfType().AccessToken; + } + + private static string Seen(StubProviderHost gitHub) => string.Join(", ", gitHub.Requests.Select(r => $"{r.Method} {r.PathAndQuery}")); + + /// + /// A GitHub OAuth App's tokens as GitHub keeps them: each access token issued to one user, all of a user's tokens one + /// grant. DELETE /applications/{client_id}/grant ends every token of the user the presented token belongs to; + /// DELETE /applications/{client_id}/token ends the presented token alone. Both answer 204, 404 for a token + /// GitHub does not know, and 401 unless the request authenticates as the app. + /// + private sealed class GitHubGrants + { + private readonly ConcurrentDictionary _userByLiveToken = new(StringComparer.Ordinal); + private readonly string _basicAuth; + + private GitHubGrants(string clientId, string clientSecret) { _basicAuth = "Basic " + Convert.ToBase64String(Encoding.ASCII.GetBytes($"{clientId}:{clientSecret}")); } + + public static GitHubGrants Serve(StubProviderHost host, string clientId, string clientSecret) + { + var grants = new GitHubGrants(clientId, clientSecret); + + host.Answer("DELETE", $"/applications/{clientId}/grant", grants.DeleteGrant); + host.Answer("DELETE", $"/applications/{clientId}/token", grants.DeleteToken); + + return grants; + } + + public void Issue(string user, params string[] accessTokens) + { + foreach (var token in accessTokens) _userByLiveToken[token] = user; + } + + public bool IsLive(string accessToken) => _userByLiveToken.ContainsKey(accessToken); + + /// + /// GitHub's token endpoint answering a refresh: runs while GitHub holds the + /// request — a write that lands before its answer — then the pair GitHub issues is live and + /// returned. The sign-in host is not the API host, so that write can reach the API meanwhile. + /// + public void ServeRefresh(StubProviderHost signIn, string user, string accessToken, string refreshToken, Action whileRefreshing) + { + signIn.Answer("POST", "/login/oauth/access_token", _ => + { + whileRefreshing(); + Issue(user, accessToken); + + return new StubProviderHost.StubReply(200, JsonSerializer.Serialize(new { access_token = accessToken, refresh_token = refreshToken, expires_in = 28800, refresh_token_expires_in = 15897600, token_type = "bearer", scope = "" })); + }); + } + + private StubProviderHost.StubReply DeleteGrant(StubProviderHost.RecordedRequest request) + { + if (request.AuthorizationHeader != _basicAuth) return new StubProviderHost.StubReply(401, """{"message":"Requires authentication"}"""); + if (!_userByLiveToken.TryGetValue(AccessToken(request), out var user)) return new StubProviderHost.StubReply(404, """{"message":"Not Found"}"""); + + foreach (var token in _userByLiveToken.Where(entry => entry.Value == user).Select(entry => entry.Key).ToList()) _userByLiveToken.TryRemove(token, out _); + + return new StubProviderHost.StubReply(204, string.Empty); + } + + private StubProviderHost.StubReply DeleteToken(StubProviderHost.RecordedRequest request) + { + if (request.AuthorizationHeader != _basicAuth) return new StubProviderHost.StubReply(401, """{"message":"Requires authentication"}"""); + + return _userByLiveToken.TryRemove(AccessToken(request), out _) ? new StubProviderHost.StubReply(204, string.Empty) : new StubProviderHost.StubReply(404, """{"message":"Not Found"}"""); + } + + private static string AccessToken(StubProviderHost.RecordedRequest request) => JsonDocument.Parse(request.Body).RootElement.GetProperty("access_token").GetString()!; + } +} diff --git a/backend/tests/CodeSpace.IntegrationTests/ProviderInstances/UpdateAndDeleteProviderInstanceFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/ProviderInstances/UpdateAndDeleteProviderInstanceFlowTests.cs index 1836c2170..5aeb21ee8 100644 --- a/backend/tests/CodeSpace.IntegrationTests/ProviderInstances/UpdateAndDeleteProviderInstanceFlowTests.cs +++ b/backend/tests/CodeSpace.IntegrationTests/ProviderInstances/UpdateAndDeleteProviderInstanceFlowTests.cs @@ -2,10 +2,14 @@ using CodeSpace.Core.Persistence.Db; using CodeSpace.Core.Persistence.Entities; using CodeSpace.Core.Services.Credentials; +using CodeSpace.Core.Services.OAuth; +using CodeSpace.Core.Services.Providers; +using CodeSpace.Core.Services.Providers.Capabilities; using CodeSpace.IntegrationTests.Infrastructure; using CodeSpace.Messages.Commands.ProviderInstances; using CodeSpace.Messages.Constants; using CodeSpace.Messages.Credentials; +using CodeSpace.Messages.Dtos.Providers; using CodeSpace.Messages.Enums; using MediatR; using Microsoft.EntityFrameworkCore; @@ -186,25 +190,100 @@ public async Task Delete_soft_deletes_provider_when_no_dependencies() saved.DeletedDate.ShouldNotBeNull(); } + /// + /// Deleting a connection ends every token connected through it the way any other retirement does — blanked, Revoked, + /// soft-deleted, the act-as identities linked to it gone — and drops the connection's OAuth client secret, so none of + /// it stays decryptable in the database. Each OAuth grant is then revoked at the provider, but only once the delete + /// has committed: a provider asked from inside the transaction would have lost the grant even if the delete rolled + /// back. The recorder reads the credential through a connection of its own at the moment the provider is asked, which + /// is what anyone outside the delete's transaction would see. + /// [Fact] - public async Task Delete_cascade_revokes_active_credentials() + public async Task Delete_retires_its_credentials_and_client_secret_then_revokes_each_grant_at_the_provider_after_commit() { - // The cleanup motivation: a deleted provider's tokens are useless. If we left them - // Active in the DB, ListCredentials would still show them and the UI would lie. var (teamId, instanceId) = await SeedProviderAsync(withOAuth: true).ConfigureAwait(false); var credentialId = await SeedCredentialAsync(teamId, instanceId).ConfigureAwait(false); + var identityId = await SeedIdentityAsync(teamId, instanceId, credentialId).ConfigureAwait(false); + var provider = new RevocationRecorder(() => ReadCommittedPayloadAsync(credentialId)); - using var scope = _fixture.BeginScopeAs(Guid.NewGuid(), teamId, Roles.Admin); - var mediator = scope.Resolve(); + var result = await DeleteAsync(teamId, instanceId, provider).ConfigureAwait(false); - await mediator.Send(new DeleteProviderInstanceCommand { ProviderInstanceId = instanceId }).ConfigureAwait(false); + result.RevokedCredentialCount.ShouldBe(1); using var verify = _fixture.BeginScope(); var db = verify.Resolve(); var cred = await db.Credential.AsNoTracking().IgnoreQueryFilters().SingleAsync(c => c.Id == credentialId).ConfigureAwait(false); + var instance = await db.ProviderInstance.AsNoTracking().SingleAsync(p => p.Id == instanceId).ConfigureAwait(false); + var identity = await db.UserProviderIdentity.AsNoTracking().SingleAsync(i => i.Id == identityId).ConfigureAwait(false); cred.Status.ShouldBe(CredentialStatus.Revoked); cred.DeletedDate.ShouldNotBeNull(); + cred.EncryptedPayload.ShouldBe(string.Empty, "a deleted connection's tokens must not stay decryptable in the database"); + cred.LastError.ShouldBe("Provider instance deleted"); + instance.OauthClientSecretEnc.ShouldBeNull("the deleted connection's OAuth client secret must not stay decryptable either"); + instance.OauthClientId.ShouldBe("client-id", "the client id is no secret and stays for the audit trail"); + identity.DeletedDate.ShouldNotBeNull("an act-as identity is unusable without its credential and goes with it, as on every other retirement"); + + provider.Revokes.Select(r => (r.Input.Token, r.Input.TokenTypeHint)).ShouldBe(new[] { ("rt-CANARY", "refresh_token"), ("at-CANARY", "access_token") }); + provider.Revokes.ShouldAllBe(r => r.Input.ClientId == "client-id" && r.Input.ClientSecret == "original-secret"); + provider.Revokes.ShouldAllBe(r => !r.Input.WholeGrant, "a delete ends the tokens this connection held, never the user's whole grant: another team connected through the same OAuth App holds tokens under it too"); + provider.Revokes.ShouldAllBe(r => r.CommittedPayload == string.Empty, "the provider was asked while the delete was uncommitted — a rolled-back delete would leave Active credentials on a grant the provider already ended"); + } + + [Fact] + public async Task Delete_retires_its_credentials_even_when_the_provider_refuses_the_revoke() + { + var (teamId, instanceId) = await SeedProviderAsync(withOAuth: true).ConfigureAwait(false); + var credentialId = await SeedCredentialAsync(teamId, instanceId).ConfigureAwait(false); + var provider = new RevocationRecorder(() => ReadCommittedPayloadAsync(credentialId)) { Refuse = true }; + + var result = await DeleteAsync(teamId, instanceId, provider).ConfigureAwait(false); + + result.RevokedCredentialCount.ShouldBe(1, "the provider's answer is best effort; the delete stands without it"); + provider.Revokes.ShouldNotBeEmpty("fixture check: the provider was asked"); + (await ReadCommittedPayloadAsync(credentialId).ConfigureAwait(false)).ShouldBe(string.Empty); + } + + [Fact] + public async Task Delete_blanks_a_personal_access_token_without_asking_the_provider() + { + // A PAT has no revocation endpoint — only its owner deleting it on the provider's side ends it there. + var (teamId, instanceId) = await SeedProviderAsync(withOAuth: true).ConfigureAwait(false); + var credentialId = await SeedCredentialAsync(teamId, instanceId, new PatPayload { Token = "glpat-CANARY" }).ConfigureAwait(false); + var provider = new RevocationRecorder(() => ReadCommittedPayloadAsync(credentialId)); + + await DeleteAsync(teamId, instanceId, provider).ConfigureAwait(false); + + provider.Revokes.ShouldBeEmpty(); + (await ReadCommittedPayloadAsync(credentialId).ConfigureAwait(false)).ShouldBe(string.Empty); + } + + /// + /// A connection-wide connection registered its group hooks at the provider with one of its own credentials, and the + /// delete blanks every one of those. So the hooks come down first, while that token still opens: afterwards nothing + /// could remove them, and each would go on delivering the group's events with its HMAC secret still decryptable here. + /// A provider that refuses the delete changes nothing here — the row goes either way, as on a scope switch — and the + /// hook rows the connection keeps are out of service and hold no secret. + /// + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task Delete_takes_its_group_hooks_down_with_a_live_token_and_keeps_no_hook_secret(bool providerRefuses) + { + var (teamId, instanceId) = await SeedProviderAsync(withOAuth: true, webhookScope: ProviderWebhookScope.Connection).ConfigureAwait(false); + var credentialId = await SeedCredentialAsync(teamId, instanceId, new PatPayload { Token = "ghp-HOOK-CANARY" }).ConfigureAwait(false); + await SeedConnectionHookAsync(instanceId, credentialId, "acme", RepositoryWebhookRegistrationStatus.Registered, "hook-777").ConfigureAwait(false); + var retiredEarlier = await SeedConnectionHookAsync(instanceId, credentialId, "acme/old", RepositoryWebhookRegistrationStatus.Cancelled, externalId: null).ConfigureAwait(false); + var groupHooks = new GroupHookRecorder { Refuse = providerRefuses }; + + await DeleteAsync(teamId, instanceId, new RevocationRecorder(() => ReadCommittedPayloadAsync(credentialId)), groupHooks: groupHooks).ConfigureAwait(false); + + groupHooks.Deletes.Select(d => (d.OwnerPath, d.ExternalId, ReadPatToken(d.Payload))).ShouldBe(new[] { ("acme", "hook-777", "ghp-HOOK-CANARY") }, ignoreOrder: false, customMessage: "the group hook must be deleted at the provider with the token that registered it, before the delete blanks that token — check the hook retirement in ProviderInstanceService.DeleteAsync"); + + var hooks = await LoadConnectionHooksAsync(instanceId).ConfigureAwait(false); + + hooks.Select(h => h.Id).ShouldBe(new[] { retiredEarlier }, "the Registered row described a hook that is gone (or that the operator now removes by hand); keeping it would claim coverage for a deleted connection"); + hooks.ShouldAllBe(h => h.RegistrationStatus == RepositoryWebhookRegistrationStatus.Cancelled && h.SecretEnc == string.Empty, "a deleted connection keeps no hook in service and no hook secret decryptable"); } [Fact] @@ -235,13 +314,7 @@ public async Task Delete_with_force_cascades_unbind_then_removes_provider() var repoIdA = await SeedRepositoryAsync(teamId, instanceId, credentialId, name: "api").ConfigureAwait(false); var repoIdB = await SeedRepositoryAsync(teamId, instanceId, credentialId, name: "web").ConfigureAwait(false); - DeleteProviderInstanceResult result; - - using (var scope = _fixture.BeginScopeAs(Guid.NewGuid(), teamId, Roles.Admin)) - { - var mediator = scope.Resolve(); - result = await mediator.Send(new DeleteProviderInstanceCommand { ProviderInstanceId = instanceId, Force = true }).ConfigureAwait(false); - } + var result = await DeleteAsync(teamId, instanceId, new RevocationRecorder(() => ReadCommittedPayloadAsync(credentialId)), force: true).ConfigureAwait(false); result.UnboundRepositoryCount.ShouldBe(2); result.RevokedCredentialCount.ShouldBe(1); @@ -258,10 +331,11 @@ public async Task Delete_with_force_cascades_unbind_then_removes_provider() repoRows.Count.ShouldBe(2); repoRows.ShouldAllBe(r => r.DeletedDate != null); - // Credential: revoked. + // Credential: revoked, and its token gone. var credRow = await db.Credential.AsNoTracking().IgnoreQueryFilters().SingleAsync(c => c.Id == credentialId).ConfigureAwait(false); credRow.Status.ShouldBe(CredentialStatus.Revoked); credRow.DeletedDate.ShouldNotBeNull(); + credRow.EncryptedPayload.ShouldBe(string.Empty); } [Fact] @@ -301,7 +375,7 @@ private async Task SeedTeamAsync() return team.Id; } - private async Task<(Guid TeamId, Guid InstanceId)> SeedProviderAsync(bool withOAuth, string? baseUrl = null, Guid? existingTeamId = null) + private async Task<(Guid TeamId, Guid InstanceId)> SeedProviderAsync(bool withOAuth, string? baseUrl = null, Guid? existingTeamId = null, ProviderWebhookScope webhookScope = ProviderWebhookScope.Repository) { var teamId = existingTeamId ?? await SeedTeamAsync().ConfigureAwait(false); @@ -317,7 +391,8 @@ private async Task SeedTeamAsync() DisplayName = "GitHub", BaseUrl = baseUrl ?? "https://github.com", OauthClientId = withOAuth ? "client-id" : null, - OauthClientSecretEnc = withOAuth ? encryptor.Encrypt("original-secret") : null + OauthClientSecretEnc = withOAuth ? encryptor.Encrypt("original-secret") : null, + WebhookScope = webhookScope }; db.ProviderInstance.Add(instance); @@ -326,21 +401,21 @@ private async Task SeedTeamAsync() return (teamId, instance.Id); } - private async Task SeedCredentialAsync(Guid teamId, Guid instanceId) + private async Task SeedCredentialAsync(Guid teamId, Guid instanceId, CredentialPayload? token = null) { using var scope = _fixture.BeginScope(); var db = scope.Resolve(); var encryptor = scope.Resolve(); var serializer = scope.Resolve(); - var payload = new OAuthPayload { AccessToken = "at", RefreshToken = "rt" }; + var payload = token ?? new OAuthPayload { AccessToken = "at-CANARY", RefreshToken = "rt-CANARY" }; var credential = new Credential { Id = Guid.NewGuid(), TeamId = teamId, ProviderInstanceId = instanceId, - AuthType = AuthType.OAuth, - DisplayName = "OAuth", + AuthType = payload.Type, + DisplayName = payload.Type.ToString(), EncryptedPayload = encryptor.Encrypt(serializer.Serialize(payload)), Status = CredentialStatus.Active }; @@ -351,6 +426,148 @@ private async Task SeedCredentialAsync(Guid teamId, Guid instanceId) return credential.Id; } + /// The team owner's act-as link to the credential, as connecting through OAuth leaves one. + private async Task SeedIdentityAsync(Guid teamId, Guid instanceId, Guid credentialId) + { + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + var ownerId = await db.TeamMembership.AsNoTracking().Where(m => m.TeamId == teamId).Select(m => m.UserId).SingleAsync().ConfigureAwait(false); + var identity = new UserProviderIdentity { Id = Guid.NewGuid(), UserId = ownerId, ProviderInstanceId = instanceId, CredentialId = credentialId, ProviderUserId = "42", ProviderUsername = "owner" }; + + db.UserProviderIdentity.Add(identity); + await db.SaveChangesAsync().ConfigureAwait(false); + + return identity.Id; + } + + /// + /// The delete as an Admin sends it, with the provider's revocation endpoint replaced by and, + /// when given, its group-hook endpoint by . + /// + private async Task DeleteAsync(Guid teamId, Guid instanceId, RevocationRecorder provider, bool force = false, GroupHookRecorder? groupHooks = null) + { + using var scope = _fixture.BeginScopeAs(Guid.NewGuid(), teamId, Roles.Admin); + using var delete = scope.BeginLifetimeScope(b => + { + b.RegisterInstance(provider).SingleInstance(); + + if (groupHooks != null) b.RegisterInstance(new ProviderRegistry(new IProviderCapability[] { groupHooks })).SingleInstance(); + }); + + return await delete.Resolve().Send(new DeleteProviderInstanceCommand { ProviderInstanceId = instanceId, Force = force }).ConfigureAwait(false); + } + + /// The credential's payload as a connection outside any open transaction reads it — the committed state. + private async Task ReadCommittedPayloadAsync(Guid credentialId) + { + using var scope = _fixture.BeginScope(); + + return await scope.Resolve().Credential.AsNoTracking().Where(c => c.Id == credentialId).Select(c => c.EncryptedPayload).SingleAsync().ConfigureAwait(false); + } + + /// + /// Stands in for the provider's revocation endpoint: records each revoke with the credential's committed payload at + /// that moment, and refuses every one when told to, as a provider that is down or rejects the client would. + /// + private sealed class RevocationRecorder : IOAuthClient, IOAuthClientRegistry + { + private readonly Func> _readCommittedPayload; + + public RevocationRecorder(Func> readCommittedPayload) { _readCommittedPayload = readCommittedPayload; } + + public List<(OAuthRevokeInput Input, string CommittedPayload)> Revokes { get; } = new(); + + public bool Refuse { get; init; } + + public ProviderKind Kind => ProviderKind.GitHub; + + public IOAuthClient Get(ProviderKind kind) => this; + + public Uri BuildAuthorizeUrl(OAuthAuthorizeInput input) => throw new NotSupportedException(); + + public Task ExchangeCodeAsync(OAuthCodeExchangeInput input, CancellationToken cancellationToken) => throw new NotSupportedException(); + + public Task RefreshAsync(OAuthRefreshInput input, CancellationToken cancellationToken) => throw new NotSupportedException(); + + public async Task RevokeAsync(OAuthRevokeInput input, CancellationToken cancellationToken) + { + Revokes.Add((input, await _readCommittedPayload().ConfigureAwait(false))); + + if (Refuse) throw new HttpRequestException("stub provider: revocation refused"); + } + } + + /// A group hook of the connection, registered by , in . + private async Task SeedConnectionHookAsync(Guid instanceId, Guid credentialId, string ownerPath, RepositoryWebhookRegistrationStatus status, string? externalId) + { + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + var id = Guid.NewGuid(); + + db.ConnectionWebhook.Add(new ConnectionWebhook + { + Id = id, + ProviderInstanceId = instanceId, + CredentialId = credentialId, + OwnerPath = ownerPath, + ExternalId = externalId, + CallbackUrl = $"https://codespace.test/api/webhooks/connection/{id}", + SecretEnc = scope.Resolve().Encrypt($"hook-secret-CANARY-{id:N}"), + SubscribedEvents = new List { "push" }, + RegistrationStatus = status + }); + + await db.SaveChangesAsync().ConfigureAwait(false); + + return id; + } + + private async Task> LoadConnectionHooksAsync(Guid instanceId) + { + using var scope = _fixture.BeginScope(); + + return await scope.Resolve().ConnectionWebhook.AsNoTracking().Where(w => w.ProviderInstanceId == instanceId).ToListAsync().ConfigureAwait(false); + } + + /// The personal access token a sealed payload holds, or empty when the payload has been blanked. + private string ReadPatToken(string encryptedPayload) + { + if (string.IsNullOrEmpty(encryptedPayload)) return string.Empty; + + using var scope = _fixture.BeginScope(); + var json = scope.Resolve().Decrypt(encryptedPayload); + + return scope.Resolve().Deserialize(AuthType.Pat, json).Token; + } + + /// + /// Stands in for the provider's group-hook endpoint: records each delete with the sealed payload of the credential + /// the call presents at that moment, and refuses every one when told to, as a provider that is down would. + /// + /// Parameterless and inert on purpose: the fixture's capability scan activates every capability class in this + /// assembly into the container's registry, where the production GitHub provider, registered first, answers before + /// it. Only the delete test's own registry reaches it. + /// + private sealed class GroupHookRecorder : IConnectionWebhookRegistrationCapability + { + public List<(string OwnerPath, string ExternalId, string Payload)> Deletes { get; } = new(); + + public bool Refuse { get; init; } + + public ProviderKind Kind => ProviderKind.GitHub; + + public Task FindConnectionWebhookByCallbackUrlAsync(ProviderContext context, string ownerPath, string callbackUrl, CancellationToken cancellationToken) => throw new NotSupportedException(); + + public Task RegisterConnectionWebhookAsync(ProviderContext context, string ownerPath, WebhookRegistration request, CancellationToken cancellationToken) => throw new NotSupportedException(); + + public Task DeleteConnectionWebhookAsync(ProviderContext context, string ownerPath, string externalWebhookId, CancellationToken cancellationToken) + { + Deletes.Add((ownerPath, externalWebhookId, context.Credential.EncryptedPayload)); + + return Refuse ? throw new HttpRequestException("stub provider: group hook delete refused") : Task.CompletedTask; + } + } + private async Task SeedRepositoryAsync(Guid teamId, Guid instanceId, Guid credentialId, string name = "api") { using var scope = _fixture.BeginScope(); diff --git a/backend/tests/CodeSpace.IntegrationTests/Variables/VariableShredFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/Variables/VariableShredFlowTests.cs new file mode 100644 index 000000000..2bf96daf4 --- /dev/null +++ b/backend/tests/CodeSpace.IntegrationTests/Variables/VariableShredFlowTests.cs @@ -0,0 +1,95 @@ +using System.Text.Json; +using Autofac; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Services.Variables; +using CodeSpace.IntegrationTests.Infrastructure; +using CodeSpace.IntegrationTests.Workflows.Infrastructure; +using CodeSpace.Messages.Commands.Projects; +using CodeSpace.Messages.Commands.Variables; +using CodeSpace.Messages.Constants; +using CodeSpace.Messages.Enums; +using MediatR; +using Microsoft.EntityFrameworkCore; +using Npgsql; +using Shouldly; + +namespace CodeSpace.IntegrationTests.Variables; + +/// +/// A deleted variable keeps its row for the audit trail and nothing a secret could be recovered from. Every read of a +/// variable filters on deleted_date IS NULL and nothing restores one, so a deleted Secret's ciphertext could only +/// ever be opened by someone holding a copy of the database and the master key. Both ways a variable is deleted — on its +/// own, and with the project it belongs to — drop it, and 0249's check admits a NULL ciphertext on a deleted Secret only. +/// +[Collection(PostgresCollection.Name)] +[Trait("Category", "Integration")] +public sealed class VariableShredFlowTests +{ + private readonly PostgresFixture _fixture; + + public VariableShredFlowTests(PostgresFixture fixture) { _fixture = fixture; } + + [Theory] + [InlineData(VariableValueType.Secret, "\"CANARY-variable-secret\"")] + [InlineData(VariableValueType.String, "\"plain-kept\"")] + public async Task Deleting_a_variable_drops_a_secrets_ciphertext_and_the_name_is_free_again(VariableValueType valueType, string value) + { + var (teamId, userId) = await WorkflowsTestSeed.SeedTeamAsync(_fixture); + using var scope = _fixture.BeginScope(); + var variables = scope.Resolve(); + + await variables.SetAsync(VariableScope.Team, teamId, teamId, "DEPLOY_TOKEN", valueType, Json(value), null, userId, CancellationToken.None); + await variables.DeleteAsync(VariableScope.Team, teamId, teamId, "DEPLOY_TOKEN", userId, CancellationToken.None); + + var deleted = await DeletedRowAsync(VariableScope.Team, teamId, "DEPLOY_TOKEN"); + deleted.ValueEncrypted.ShouldBeNull("a deleted variable must hold no ciphertext anyone could open later"); + deleted.ValuePlain.ShouldBe(valueType == VariableValueType.Secret ? null : value, "a plain value is no secret and stays as the record of what the variable held"); + + await variables.SetAsync(VariableScope.Team, teamId, teamId, "DEPLOY_TOKEN", valueType, Json(value), null, userId, CancellationToken.None); + (await variables.GetAllForEngineAsync(VariableScope.Team, teamId, CancellationToken.None)).Single(v => v.Name == "DEPLOY_TOKEN").Value.GetRawText().ShouldBe(value, "the name is free again and the new row reads back"); + } + + [Fact] + public async Task Deleting_a_project_drops_the_ciphertext_of_its_secret_variables() + { + var (teamId, userId) = await WorkflowsTestSeed.SeedTeamAsync(_fixture); + Guid projectId; + + using (var scope = _fixture.BeginScopeAs(userId, teamId, Roles.Admin)) + { + var mediator = scope.Resolve(); + projectId = await mediator.Send(new CreateProjectCommand { Name = $"Shred {Guid.NewGuid():N}" }); + await mediator.Send(new SetProjectVariableCommand { ProjectId = projectId, Name = "API_KEY", ValueType = VariableValueType.Secret, Value = Json("\"CANARY-project-secret\"") }); + } + + using (var scope = _fixture.BeginScopeAs(userId, teamId, Roles.Admin)) + await scope.Resolve().Send(new DeleteProjectCommand { ProjectId = projectId }); + + (await DeletedRowAsync(VariableScope.Project, projectId, "API_KEY")).ValueEncrypted.ShouldBeNull("the project cascade retires its variables the same way a delete does"); + } + + [Fact] + public async Task A_live_secret_still_must_carry_its_ciphertext() + { + // 0249 relaxed the value check for deleted rows only. A live Secret with no ciphertext is still a programmer + // error the database refuses — otherwise the engine would read NULL where a secret should be. + var (teamId, userId) = await WorkflowsTestSeed.SeedTeamAsync(_fixture); + using (var scope = _fixture.BeginScope()) + await scope.Resolve().SetAsync(VariableScope.Team, teamId, teamId, "LIVE_SECRET", VariableValueType.Secret, Json("\"CANARY-live\""), null, userId, CancellationToken.None); + + using var verify = _fixture.BeginScope(); + var db = verify.Resolve(); + var refused = await Should.ThrowAsync(() => db.Database.ExecuteSqlInterpolatedAsync($"UPDATE variable SET value_encrypted = NULL WHERE team_id = {teamId} AND name = 'LIVE_SECRET' AND deleted_date IS NULL")); + + refused.ConstraintName.ShouldBe("chk_variable_value_exclusive"); + } + + private async Task DeletedRowAsync(VariableScope scope, Guid scopeId, string name) + { + using var verify = _fixture.BeginScope(); + + return await verify.Resolve().Variable.AsNoTracking().SingleAsync(v => v.Scope == scope && v.ScopeId == scopeId && v.Name == name && v.DeletedDate != null); + } + + private static JsonElement Json(string raw) => JsonDocument.Parse(raw).RootElement.Clone(); +} diff --git a/backend/tests/CodeSpace.IntegrationTests/Workflows/Artifacts/Credentials/StorageCredentialShredTests.cs b/backend/tests/CodeSpace.IntegrationTests/Workflows/Artifacts/Credentials/StorageCredentialShredTests.cs new file mode 100644 index 000000000..d7c9aa4dd --- /dev/null +++ b/backend/tests/CodeSpace.IntegrationTests/Workflows/Artifacts/Credentials/StorageCredentialShredTests.cs @@ -0,0 +1,153 @@ +using Autofac; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Workflows.Artifacts.Credentials; +using CodeSpace.Core.Services.Workflows.Artifacts.Providers.Local; +using CodeSpace.IntegrationTests.Infrastructure; +using CodeSpace.Messages.Commands.Storage; +using CodeSpace.Messages.Enums; +using Microsoft.EntityFrameworkCore; +using Npgsql; +using Shouldly; + +namespace CodeSpace.IntegrationTests.Workflows.Artifacts.Credentials; + +/// +/// Revoking a storage credential blanks the encrypted payload of every one of its revisions, and only a revoked +/// credential's revisions may ever be blanked. +/// +/// Why revoked and not merely superseded: the secret resolver reads the exact revision a storage profile pins +/// (db:{id}:{revision}), so while a credential is Active every revision of it is a live key — a profile revision +/// that still holds stored bytes resolves through it, and a new profile revision may pin it +/// (StorageCredentialSecretResolverTests.Exact_old_revision_never_falls_forward_to_the_current_secret). Blanking +/// one would make routed bytes permanently unreadable. A Revoked credential resolves for nobody: the resolver answers +/// NotActive before it reads any envelope, revocation is terminal, and it is refused while a stored location or a +/// live profile still resolves through the credential. So its ciphertext is dead weight, recoverable only by someone +/// holding a database copy and the key ring. +/// +[Collection(PostgresCollection.Name)] +[Trait("Category", "Integration")] +public sealed class StorageCredentialShredTests +{ + private readonly PostgresFixture _fixture; + + public StorageCredentialShredTests(PostgresFixture fixture) => _fixture = fixture; + + [Fact] + public async Task Revoking_blanks_every_revisions_payload_and_keeps_what_the_audit_trail_reads() + { + var world = await SeedRotatedCredentialAsync(); + var before = await RevisionsAsync(world); + + await RevokeAsync(world); + + var after = await RevisionsAsync(world); + after.ShouldAllBe(r => r.EncryptedPayload == string.Empty, "a revoked credential's revisions must not stay decryptable"); + after.Select(Audit).ShouldBe(before.Select(Audit), "revision, provider, hint, envelope fingerprint and authorship stay for the audit trail"); + + using var scope = _fixture.BeginScope(); + var resolver = scope.Resolve(); + foreach (var revision in new[] { 1, 2 }) + (await resolver.ResolveAsync(new StorageCredentialSecretRequest(world.TeamId, world.CredentialId, revision, LocalRwxArtifactStorageDriverFactory.TypeKey), CancellationToken.None)) + .ShouldBe(new StorageCredentialSecretResolution.NotActive(StorageCredentialState.Revoked), "a revoked credential resolves for nobody, so nothing reads the blanked envelope"); + } + + [Fact] + public async Task A_superseded_revision_of_an_active_credential_keeps_its_payload() + { + var world = await SeedRotatedCredentialAsync(); + + var refused = await Should.ThrowAsync(() => ExecuteAsync($"UPDATE storage_credential_revision SET encrypted_payload = '' WHERE storage_credential_id = '{world.CredentialId}' AND revision = 1")); + + refused.MessageText.ShouldContain("Revoked", Case.Sensitive, "a profile may still pin revision 1 of an Active credential; blanking it would strand the bytes stored through it"); + (await RevisionsAsync(world)).ShouldAllBe(r => r.EncryptedPayload != string.Empty); + } + + [Theory] + [InlineData("UPDATE storage_credential_revision SET envelope_fingerprint = 'sha256:" + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff" + "' WHERE storage_credential_id = '{0}'")] + [InlineData("UPDATE storage_credential_revision SET encrypted_payload = 'CfDJ8-restored' WHERE storage_credential_id = '{0}'")] + [InlineData("UPDATE storage_credential_revision SET encrypted_payload = '' WHERE storage_credential_id = '{0}'")] + [InlineData("DELETE FROM storage_credential_revision WHERE storage_credential_id = '{0}'")] + public async Task On_a_revoked_credential_the_one_blanking_update_is_all_the_guard_ever_admits(string statement) + { + var world = await SeedRotatedCredentialAsync(); + await RevokeAsync(world); + + var refused = await Should.ThrowAsync(() => ExecuteAsync(string.Format(statement, world.CredentialId))); + + refused.MessageText.ShouldContain("immutable", Case.Sensitive, "a blanked revision is final — no second blanking, no restore, no other edit, no delete"); + } + + [Fact] + public async Task A_revision_never_arrives_blank() + { + var world = await SeedRotatedCredentialAsync(); + + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + var credential = await db.StorageCredential.SingleAsync(c => c.Id == world.CredentialId); + db.StorageCredentialRevision.Add(Revision(world, 3, string.Empty)); + credential.CurrentRevision = 3; + + var refused = await Should.ThrowAsync(() => db.SaveChangesAsync()); + + refused.InnerException.ShouldBeOfType().MessageText.ShouldContain("payload is required", Case.Sensitive); + } + + private static object Audit(StorageCredentialRevision r) => new { r.Id, r.Revision, r.ProviderTypeKey, r.SafeHint, r.EnvelopeFingerprint, r.CreatedDate, r.CreatedBy }; + + private async Task RevokeAsync(World world) + { + using var scope = _fixture.BeginScope(); + var credential = await scope.Resolve().StorageCredential.AsNoTracking().SingleAsync(c => c.Id == world.CredentialId); + + await scope.Resolve().RevokeAsync(world.TeamId, world.ActorId, new RevokeStorageCredentialCommand { CredentialId = world.CredentialId, ExpectedXmin = credential.Xmin, ExpectedCurrentRevision = credential.CurrentRevision }, CancellationToken.None); + } + + private async Task> RevisionsAsync(World world) + { + using var scope = _fixture.BeginScope(); + + return await scope.Resolve().StorageCredentialRevision.AsNoTracking().Where(r => r.StorageCredentialId == world.CredentialId).OrderBy(r => r.Revision).ToListAsync(); + } + + private async Task ExecuteAsync(string sql) + { + await using var connection = new NpgsqlConnection(_fixture.ConnectionString); + await connection.OpenAsync(); + await using var command = new NpgsqlCommand(sql, connection); + await command.ExecuteNonQueryAsync(); + } + + /// An Active credential rotated once — revision 1 superseded, revision 2 current — that no profile references. + private async Task SeedRotatedCredentialAsync() + { + var world = new World(Guid.NewGuid(), Guid.NewGuid(), Guid.NewGuid()); + + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + db.User.Add(new User { Id = world.ActorId, Email = $"shred-{world.ActorId:N}@test.local", Name = $"shred-{world.ActorId:N}" }); + db.Team.Add(new Team { Id = world.TeamId, Slug = $"shred-{world.TeamId:N}", Name = "Storage Shred Team", Kind = TeamKind.Workspace }); + db.TeamMembership.Add(new TeamMembership { Id = Guid.NewGuid(), TeamId = world.TeamId, UserId = world.ActorId, Role = TeamRole.Owner }); + + var credential = new StorageCredential { Id = world.CredentialId, TeamId = world.TeamId, StableName = $"shred-{Guid.NewGuid():N}", CurrentRevision = 1, State = StorageCredentialState.Active, CreatedDate = DateTimeOffset.UtcNow, CreatedBy = world.ActorId }; + credential.Revisions.Add(Revision(world, 1, "CfDJ8-CANARY-storage-revision-1")); + db.StorageCredential.Add(credential); + await db.SaveChangesAsync(); + + db.StorageCredentialRevision.Add(Revision(world, 2, "CfDJ8-CANARY-storage-revision-2")); + credential.CurrentRevision = 2; + await db.SaveChangesAsync(); + + return world; + } + + private static StorageCredentialRevision Revision(World world, int revision, string encryptedPayload) => new() + { + Id = Guid.NewGuid(), TeamId = world.TeamId, StorageCredentialId = world.CredentialId, Revision = revision, + ProviderTypeKey = LocalRwxArtifactStorageDriverFactory.TypeKey, EncryptedPayload = encryptedPayload, SafeHint = $"rev-{revision}", + EnvelopeFingerprint = $"sha256:{new string((char)('a' + revision), 64)}", CreatedDate = DateTimeOffset.UtcNow, CreatedBy = world.ActorId, + }; + + private sealed record World(Guid TeamId, Guid ActorId, Guid CredentialId); +} diff --git a/backend/tests/CodeSpace.IntegrationTests/Workflows/Artifacts/Defaults/StorageDefaultMaterializerFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/Workflows/Artifacts/Defaults/StorageDefaultMaterializerFlowTests.cs index 4d7c535ac..87068e9f3 100644 --- a/backend/tests/CodeSpace.IntegrationTests/Workflows/Artifacts/Defaults/StorageDefaultMaterializerFlowTests.cs +++ b/backend/tests/CodeSpace.IntegrationTests/Workflows/Artifacts/Defaults/StorageDefaultMaterializerFlowTests.cs @@ -226,8 +226,66 @@ public async Task An_unknown_team_is_refused_by_name_rather_than_read_as_a_missi outcome.ShouldBeOfType("authoring a template and fixing the caller are opposite responses"); } + /// + /// Rotating the template's secret appends a new envelope and blanks the one it replaced; clearing it blanks the one it + /// named. The materializer decrypts only the envelope its template points at, so neither old one has a reader left, + /// and a deployment-wide cloud key kept decryptable forever is what rotating it was meant to end. A team materialized + /// after the rotation is the proof the blanking took nothing a reader needs. + /// + [Fact] + public async Task Rotating_or_clearing_the_templates_secret_blanks_the_envelope_it_replaced() + { + await AuthorTemplateAsync(NewRoot()); + var replaced = await SetTemplateSecretAsync(clear: false); + var current = await SetTemplateSecretAsync(clear: false); + + current.ShouldNotBe(replaced, "fixture check: a supplied secret appends a new envelope"); + (await EnvelopeAsync(replaced!.Value)).ShouldBe(string.Empty, "the rotated-out envelope has no reader left and must not stay decryptable"); + (await EnvelopeAsync(current!.Value)).ShouldNotBeNullOrEmpty("the envelope the template names is the one the materializer decrypts"); + + (await MaterializeAsync(await SeedTeamAsync(), automatic: true)).ShouldBeOfType("a team materialized after a rotation reads the new envelope; a reader of the blanked one would halt as DestinationUnusable"); + + (await SetTemplateSecretAsync(clear: true)).ShouldBeNull("fixture check: the credential was cleared"); + (await EnvelopeAsync(current.Value)).ShouldBe(string.Empty, "a cleared envelope has no reader left either"); + } + // ─── World + helpers ───────────────────────────────────────────────────── + /// + /// Supplies a new secret to the shared template through its own service — or clears the one it has — keeping the rest + /// of it as it is, and returns the envelope it names afterwards. The local provider takes an empty secret object. + /// + private async Task SetTemplateSecretAsync(bool clear) + { + using var scope = _fixture.BeginScope(); + var service = scope.Resolve(); + var actorId = await SeedOperatorAsync(scope); + var existing = (await service.ListAsync(CancellationToken.None)).Single(row => row.DataClassTypeKey == AgentRunLogClass); + var template = (await service.GetAsync(existing.Id, CancellationToken.None))!; + + await service.UpdateAsync(actorId, new UpdateStorageDefaultCommand + { + DefaultId = template.Id, + ExpectedXmin = template.Xmin, + ExpectedRevision = template.Revision, + ProviderTypeKey = template.ProviderTypeKey, + NonSecretConfig = template.NonSecretConfig, + NamespaceRoot = template.NamespaceRoot, + AdoptionPolicy = template.AdoptionPolicy, + Secret = clear ? null : JsonDocument.Parse("{}").RootElement, + ClearCredential = clear, + }, CancellationToken.None); + + return await scope.Resolve().StorageDefault.AsNoTracking().Where(row => row.Id == template.Id).Select(row => row.CredentialId).SingleAsync(); + } + + private async Task EnvelopeAsync(Guid credentialId) + { + using var scope = _fixture.BeginScope(); + + return await scope.Resolve().StorageDefaultCredential.AsNoTracking().Where(row => row.Id == credentialId).Select(row => row.EncryptedPayload).SingleAsync(); + } + private async Task MaterializeAsync(World world, bool automatic, string? dataClass = null) { using var scope = _fixture.BeginScope(); diff --git a/backend/tests/CodeSpace.IntegrationTests/Workflows/SweepCommandTransactionFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/Workflows/SweepCommandTransactionFlowTests.cs index eeb5b3dce..e2795d7a7 100644 --- a/backend/tests/CodeSpace.IntegrationTests/Workflows/SweepCommandTransactionFlowTests.cs +++ b/backend/tests/CodeSpace.IntegrationTests/Workflows/SweepCommandTransactionFlowTests.cs @@ -140,6 +140,7 @@ public async Task Budget_settlement_releases_the_terminal_map_branch_reservation [InlineData(typeof(RepairDisconnectedCredentialBindingsCommand))] [InlineData(typeof(ResumeAbandonedArtifactTransfersCommand))] [InlineData(typeof(RetireDepartedMembersCredentialsCommand))] + [InlineData(typeof(ShredRetiredSecretsCommand))] [InlineData(typeof(SweepBudgetSettlementCommand))] [InlineData(typeof(SweepCompletionShadowCommand))] [InlineData(typeof(TierStaleModelCapabilitiesCommand))] diff --git a/backend/tests/CodeSpace.UnitTests/Architecture/CredentialSpendGateTests.cs b/backend/tests/CodeSpace.UnitTests/Architecture/CredentialSpendGateTests.cs new file mode 100644 index 000000000..6e29a8b2f --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Architecture/CredentialSpendGateTests.cs @@ -0,0 +1,51 @@ +using System.IO; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Messages.Authorization; +using CodeSpace.Messages.Queries.Credentials; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Metadata; +using Shouldly; + +namespace CodeSpace.UnitTests.Architecture; + +/// +/// Requests that present one credential to its provider on the caller's behalf. The credential marker alone proves the +/// credential is the caller's team's and the caller a member, and no more — so on membership alone a Viewer could spend +/// another member's personal token to enumerate everything that person's account can see at the provider, private +/// repositories outside the team included. Listing what a credential reaches is now its owner's (a Personal credential) +/// or a repos.manage holder's, the bar of the bind the listing feeds. +/// +[Trait("Category", "Unit")] +public sealed class CredentialSpendGateTests +{ + [Fact] + public void Listing_what_a_credential_reaches_takes_the_spend_gate() + { + typeof(IRequireCredentialSpend).IsAssignableFrom(typeof(ListAccessibleRepositoriesQuery)).ShouldBeTrue("listing what a credential reaches presents it to the provider — gate it on whose it is, not on membership alone"); + } + + [Fact] + public void The_spend_gate_runs_on_a_credential_already_proven_to_be_the_callers_teams() + { + typeof(IRequireCredentialAccess).IsAssignableFrom(typeof(IRequireCredentialSpend)).ShouldBeTrue("the spend tier decides only whether membership is enough; the team match must already hold"); + } + + /// + /// DbUp owns the schema; the EF model only describes it. 0249 relaxed each payload check so a revoked credential's + /// revision, and a deployment-default envelope no template names, can hold an empty payload, and the model's copy of + /// each check must say the same thing. + /// + [Theory] + [InlineData(typeof(StorageCredentialRevision), "ck_storage_credential_revision_encrypted_payload")] + [InlineData(typeof(StorageDefaultCredential), "ck_storage_default_credential_encrypted_payload")] + public void The_models_payload_check_matches_the_one_0249_installed(Type entity, string constraint) + { + using var db = new CodeSpaceDbContext(new DbContextOptionsBuilder().UseNpgsql("Host=127.0.0.1;Port=1;Database=unused;Username=unused;Password=unused").UseSnakeCaseNamingConvention().Options); + var check = db.GetService().Model.FindEntityType(entity)!.GetCheckConstraints().Single(c => c.Name == constraint); + var migration = File.ReadAllText(Path.Combine(AppContext.BaseDirectory, "Persistence", "DbUpFiles", "0249_shred_retired_secrets.sql")); + + migration.ShouldContain($"ADD CONSTRAINT {constraint} CHECK ({check.Sql})", Case.Sensitive); + } +} diff --git a/backend/tests/CodeSpace.UnitTests/Architecture/RequestAuthorizationInventoryTests.cs b/backend/tests/CodeSpace.UnitTests/Architecture/RequestAuthorizationInventoryTests.cs index c32a9fbd1..a7c09b8bc 100644 --- a/backend/tests/CodeSpace.UnitTests/Architecture/RequestAuthorizationInventoryTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Architecture/RequestAuthorizationInventoryTests.cs @@ -79,6 +79,7 @@ public class RequestAuthorizationInventoryTests ["BackfillPackCloneUrlsCommand"] = "seals the clone URL of pack rows that still hold a pasted token in plaintext, across every team, in bounded batches; it changes where a pack's existing credential is kept, never which source a pack clones or what any user can reach, and a sealed row is no longer a candidate.", ["RepairDisconnectedCredentialBindingsCommand"] = "moves repositories and group hooks still bound to a disconnected credential onto the credential that credential's own owner connected afterwards on the same provider instance and team, across every team, in bounded batches; it never moves a binding off a live credential or onto another person's, so it only completes a reconnect the owner already made, and a moved binding is no longer a candidate.", ["RetireDepartedMembersCredentialsCommand"] = "retires the personal tokens whose owner — or, for one stored without an owner, whose creator account — has no standing in the team the token was lent to (no membership there and not an instance Admin), across every team, in bounded batches; it ends access the person no longer has standing to lend, exactly as their departure does, never touches a team-service token, the token of anyone who still stands in its team, or one it cannot attribute to an account, and a retired token is no longer a candidate.", + ["ShredRetiredSecretsCommand"] = "blanks the ciphertext of secrets that are already retired — deleted variables, revoked or soft-deleted git tokens, deleted connections' client and hook secrets, revoked storage credentials' revisions, deployment-default envelopes no template names — across every team; it never touches a live secret, ends no access anyone still has, and a blanked row is no longer a candidate, so it only finishes a retirement an older build left decryptable.", ["BackfillRunScorecardsCommand"] = "projects the observation-only north-star row for terminal runs that terminalized before the table existed, across every team, in bounded batches; a run that already has a row is not a candidate, so it can only ever add a measurement of a settled run — it authors nothing a user owns and changes no run's outcome.", ["SweepStaleAgentWorkspacesCommand"] = "sweep", ["TierStaleModelCapabilitiesCommand"] = "sweep", diff --git a/backend/tests/CodeSpace.UnitTests/Jobs/RetiredSecretShredDispatchTests.cs b/backend/tests/CodeSpace.UnitTests/Jobs/RetiredSecretShredDispatchTests.cs new file mode 100644 index 000000000..5b1861342 --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Jobs/RetiredSecretShredDispatchTests.cs @@ -0,0 +1,93 @@ +using CodeSpace.Core.Handlers.CommandHandlers.Credentials; +using CodeSpace.Core.Jobs.RecurringJobs; +using CodeSpace.Core.Services.Credentials; +using CodeSpace.Messages.Commands.Credentials; +using CodeSpace.Messages.Mediation; +using MediatR; +using Shouldly; + +namespace CodeSpace.UnitTests.Jobs; + +/// +/// 🟢 Unit: shredding the secrets an older build retired without blanking them is the standard job → command → service +/// chain (Rule 14 + Rule 16) — the job holds no query, the handler holds no logic. Hand-rolled recording doubles, matching +/// the codebase convention. +/// +[Trait("Category", "Unit")] +public class RetiredSecretShredDispatchTests +{ + [Fact] + public async Task The_shred_job_dispatches_its_command_every_hour_and_holds_no_logic() + { + var mediator = new RecordingMediator(); + var job = new RetiredSecretShredRecurringJob(mediator); + + job.JobId.ShouldBe(nameof(RetiredSecretShredRecurringJob), "Hangfire indexes the schedule by this id — a rename strands the old one"); + job.CronExpression.ShouldBe("0 * * * *", "a committed cadence: a secret an older pod retires during a rollout stays decryptable at most an hour after the new build takes over"); + + await job.Execute(); + + mediator.Sent.ShouldHaveSingleItem().ShouldBeOfType("the job is a thin dispatcher — it only sends the command"); + } + + [Fact] + public async Task The_shred_handler_forwards_to_the_service_and_returns_its_count() + { + var shred = new RecordingShred { ToReturn = 4 }; + var handler = new ShredRetiredSecretsCommandHandler(shred); + + var shredded = await handler.Handle(new ShredRetiredSecretsCommand(), CancellationToken.None); + + shred.Calls.ShouldBe(1, "the handler delegates the whole pass to the service (Rule 16)"); + shredded.ShouldBe(4, "the handler surfaces the service's count verbatim"); + } + + [Fact] + public void The_shred_command_settles_each_kind_of_secret_on_its_own() + { + typeof(INonTransactionalCommand).IsAssignableFrom(typeof(ShredRetiredSecretsCommand)) + .ShouldBeTrue("each kind is blanked by one statement that commits on its own; one transaction around the pass would hold every candidate row's lock until the last statement and let one failing statement undo the rest"); + } + + /// Records the requests sent through the mediator; the rest of the surface is unreachable in these tests. + private sealed class RecordingMediator : IMediator + { + public List Sent { get; } = new(); + + public Task Send(IRequest request, CancellationToken cancellationToken = default) + { + Sent.Add(request); + return Task.FromResult(default(TResponse)!); + } + + public Task Send(object request, CancellationToken cancellationToken = default) + { + Sent.Add(request); + return Task.FromResult(null); + } + + public Task Send(TRequest request, CancellationToken cancellationToken = default) where TRequest : IRequest + { + Sent.Add(request!); + return Task.CompletedTask; + } + + public IAsyncEnumerable CreateStream(IStreamRequest request, CancellationToken cancellationToken = default) => throw new NotSupportedException(); + public IAsyncEnumerable CreateStream(object request, CancellationToken cancellationToken = default) => throw new NotSupportedException(); + public Task Publish(object notification, CancellationToken cancellationToken = default) => throw new NotSupportedException(); + public Task Publish(TNotification notification, CancellationToken cancellationToken = default) where TNotification : INotification => throw new NotSupportedException(); + } + + /// Counts the passes asked for + returns a canned count, so the handler test asserts pure delegation. + private sealed class RecordingShred : IRetiredSecretShredService + { + public int Calls; + public int ToReturn; + + public Task ShredAsync(CancellationToken cancellationToken) + { + Calls++; + return Task.FromResult(ToReturn); + } + } +} diff --git a/backend/tests/CodeSpace.UnitTests/OAuth/OAuthClientRevokeTests.cs b/backend/tests/CodeSpace.UnitTests/OAuth/OAuthClientRevokeTests.cs new file mode 100644 index 000000000..c831034f3 --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/OAuth/OAuthClientRevokeTests.cs @@ -0,0 +1,139 @@ +using System.Net; +using System.Text; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.OAuth; +using CodeSpace.Messages.Enums; +using Shouldly; + +namespace CodeSpace.UnitTests.OAuth; + +/// +/// What each provider's client sends when CodeSpace ends a token. GitHub revokes at two endpoints: the grant delete ends +/// every token GitHub issued the user under the OAuth App — the ones another team connected through the same app holds +/// too — so it is sent only for a whole-grant revoke (a Disconnect); otherwise the token delete ends the one token. +/// GitLab revokes per token (RFC 7009) and sends the same request either way. The URL, method, body and client +/// authentication are asserted as they leave the client, through a recording . +/// +[Trait("Category", "Unit")] +public sealed class OAuthClientRevokeTests +{ + private static readonly ProviderInstance GitHubInstance = new() { Id = Guid.NewGuid(), TeamId = Guid.NewGuid(), Provider = ProviderKind.GitHub, DisplayName = "github", BaseUrl = "https://github.com", OauthClientId = "shared-app" }; + + private static readonly ProviderInstance GitLabInstance = new() { Id = Guid.NewGuid(), TeamId = Guid.NewGuid(), Provider = ProviderKind.GitLab, DisplayName = "gitlab", BaseUrl = "https://gitlab.example", OauthClientId = "shared-app" }; + + [Fact] + public void A_revoke_ends_the_whole_grant_unless_told_otherwise() + { + var input = new OAuthRevokeInput { Instance = GitHubInstance, ClientId = "shared-app", ClientSecret = "app-secret", Token = "tok" }; + + input.WholeGrant.ShouldBeTrue("a Disconnect, and every caller that does not say, keeps the grant-level revoke it always sent"); + } + + [Theory] + [InlineData(true, "access_token", "/applications/shared-app/grant")] // a Disconnect: the user's whole grant to the app + [InlineData(true, "refresh_token", "/applications/shared-app/grant")] // unchanged: a Disconnect sends both tokens to the grant delete + [InlineData(false, "access_token", "/applications/shared-app/token")] // a connection delete: this access token only + public async Task GitHub_deletes_the_grant_only_for_a_whole_grant_revoke_and_otherwise_the_one_token(bool wholeGrant, string tokenTypeHint, string expectedPath) + { + var handler = new RecordingHandler(HttpStatusCode.NoContent); + + await GitHub(handler).RevokeAsync(Input(GitHubInstance, wholeGrant, "tok-CANARY", tokenTypeHint), CancellationToken.None); + + var request = handler.Requests.ShouldHaveSingleItem(); + request.Method.ShouldBe(HttpMethod.Delete); + request.Uri.ShouldBe(new Uri("https://api.github.com" + expectedPath)); + request.Body.ShouldBe("{\"access_token\":\"tok-CANARY\"}"); + request.Authorization.ShouldBe("Basic " + Convert.ToBase64String(Encoding.ASCII.GetBytes("shared-app:app-secret")), "both endpoints authenticate as the OAuth App"); + } + + [Fact] + public async Task GitHub_sends_nothing_for_a_refresh_token_outside_a_whole_grant_revoke() + { + var handler = new RecordingHandler(HttpStatusCode.NoContent); + + await GitHub(handler).RevokeAsync(Input(GitHubInstance, wholeGrant: false, "rt-CANARY", "refresh_token"), CancellationToken.None); + + handler.Requests.ShouldBeEmpty("GitHub has no endpoint that ends a refresh token on its own; the only one that does is the grant delete, which would end the user's tokens in every other team on the same OAuth App"); + } + + [Theory] + [InlineData(HttpStatusCode.NoContent)] // revoked + [InlineData(HttpStatusCode.NotFound)] // already gone + [InlineData(HttpStatusCode.UnprocessableContent)] // not a token GitHub knows + public async Task GitHub_treats_a_token_revoke_that_finds_nothing_left_to_end_as_done(HttpStatusCode status) + { + var handler = new RecordingHandler(status); + + await Should.NotThrowAsync(() => GitHub(handler).RevokeAsync(Input(GitHubInstance, wholeGrant: false, "tok", "access_token"), CancellationToken.None)); + } + + [Fact] + public async Task GitHub_surfaces_a_token_revoke_the_provider_failed() + { + var handler = new RecordingHandler(HttpStatusCode.InternalServerError); + + var failure = await Should.ThrowAsync(() => GitHub(handler).RevokeAsync(Input(GitHubInstance, wholeGrant: false, "tok", "access_token"), CancellationToken.None)); + + failure.Error.ShouldBe("http_500", "the caller logs a failed revoke as best effort; it must not read as done"); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task GitLab_revokes_the_one_token_whether_or_not_the_whole_grant_is_asked_for(bool wholeGrant) + { + var handler = new RecordingHandler(HttpStatusCode.OK); + + await GitLab(handler).RevokeAsync(Input(GitLabInstance, wholeGrant, "rt-CANARY", "refresh_token"), CancellationToken.None); + + var request = handler.Requests.ShouldHaveSingleItem(); + request.Method.ShouldBe(HttpMethod.Post); + request.Uri.ShouldBe(new Uri("https://gitlab.example/oauth/revoke")); + request.Body.ShouldBe("client_id=shared-app&client_secret=app-secret&token=rt-CANARY&token_type_hint=refresh_token", "RFC 7009 ends exactly the token presented"); + } + + private static OAuthRevokeInput Input(ProviderInstance instance, bool wholeGrant, string token, string tokenTypeHint) => new() + { + Instance = instance, + ClientId = "shared-app", + ClientSecret = "app-secret", + Token = token, + TokenTypeHint = tokenTypeHint, + WholeGrant = wholeGrant + }; + + private static GitHubOAuthClient GitHub(RecordingHandler handler) => new(new SingleClientFactory(handler), TimeProvider.System); + + private static GitLabOAuthClient GitLab(RecordingHandler handler) => new(new SingleClientFactory(handler), TimeProvider.System); + + private sealed record RecordedRequest(HttpMethod Method, Uri Uri, string Body, string? Authorization); + + /// Answers every request with one status and records it whole, as the provider would receive it. + private sealed class RecordingHandler : HttpMessageHandler + { + private readonly HttpStatusCode _status; + + public RecordingHandler(HttpStatusCode status) { _status = status; } + + public List Requests { get; } = new(); + + protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + var body = request.Content == null ? string.Empty : await request.Content.ReadAsStringAsync(cancellationToken); + + Requests.Add(new RecordedRequest(request.Method, request.RequestUri!, body, request.Headers.Authorization?.ToString())); + + return new HttpResponseMessage(_status) { Content = new StringContent("{}") }; + } + } + + /// Hands out clients over the recording handler; the handler outlives each client the provider client disposes. + private sealed class SingleClientFactory : IHttpClientFactory + { + private readonly HttpMessageHandler _handler; + + public SingleClientFactory(HttpMessageHandler handler) { _handler = handler; } + + public HttpClient CreateClient(string name) => new(_handler, disposeHandler: false); + } +} diff --git a/backend/tests/CodeSpace.UnitTests/OAuth/OAuthGrantRevokerTests.cs b/backend/tests/CodeSpace.UnitTests/OAuth/OAuthGrantRevokerTests.cs new file mode 100644 index 000000000..cd93888d6 --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/OAuth/OAuthGrantRevokerTests.cs @@ -0,0 +1,119 @@ +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Credentials; +using CodeSpace.Core.Services.OAuth; +using CodeSpace.Messages.Credentials; +using CodeSpace.Messages.Enums; +using Shouldly; + +namespace CodeSpace.UnitTests.OAuth; + +/// +/// The provider side of ending an OAuth credential, shared by a Disconnect and by deleting the connection the credential +/// was made through. The refresh token goes first — on most providers it is the grant — then the access token. The +/// client secret is the one it is handed rather than one it reads off the connection: a delete reads the secret before +/// it blanks it, and revokes only after its transaction commits, when the connection no longer holds it. Whether the +/// provider is asked to end the user's whole grant or only these tokens is the caller's to say, on every request. +/// +[Trait("Category", "Unit")] +public sealed class OAuthGrantRevokerTests +{ + private static readonly ProviderInstance Instance = new() { Id = Guid.NewGuid(), TeamId = Guid.NewGuid(), Provider = ProviderKind.GitLab, DisplayName = "gitlab", BaseUrl = "https://gitlab.example", OauthClientId = "client-1" }; + + [Fact] + public async Task Revokes_the_refresh_token_then_the_access_token_with_the_client_secret_it_is_handed() + { + var client = new RecordingOAuthClient(); + + await Revoker(client).RevokeAsync(Instance, "client-secret-CANARY", new OAuthPayload { AccessToken = "at-CANARY", RefreshToken = "rt-CANARY" }, wholeGrant: true, CancellationToken.None); + + client.Revokes.Select(r => (r.Token, r.TokenTypeHint)).ShouldBe(new[] { ("rt-CANARY", "refresh_token"), ("at-CANARY", "access_token") }); + client.Revokes.ShouldAllBe(r => r.ClientId == "client-1" && r.ClientSecret == "client-secret-CANARY" && r.Instance == Instance); + } + + [Fact] + public async Task Revokes_only_the_access_token_of_a_grant_without_a_refresh_token() + { + var client = new RecordingOAuthClient(); + + await Revoker(client).RevokeAsync(Instance, "client-secret", new OAuthPayload { AccessToken = "at-only" }, wholeGrant: true, CancellationToken.None); + + client.Revokes.Select(r => r.Token).ShouldBe(new[] { "at-only" }); + } + + [Theory] + [InlineData(true)] // a Disconnect: the user's whole grant to the OAuth App + [InlineData(false)] // a connection delete: only the tokens this credential held + public async Task Asks_for_the_scope_it_is_told_on_every_token(bool wholeGrant) + { + var client = new RecordingOAuthClient(); + + await Revoker(client).RevokeAsync(Instance, "client-secret", new OAuthPayload { AccessToken = "at", RefreshToken = "rt" }, wholeGrant, CancellationToken.None); + + client.Revokes.Count.ShouldBe(2, "fixture check: both tokens were revoked"); + client.Revokes.ShouldAllBe(r => r.WholeGrant == wholeGrant, "the provider client decides what each scope means at its endpoints; the revoker must pass the caller's choice through untouched"); + } + + [Theory] + [InlineData(true)] // the stored envelope opens and holds OAuth tokens + [InlineData(false)] // it does not open — a rotated key ring, a blanked row + public void Reads_the_tokens_of_a_readable_OAuth_payload_and_nothing_from_an_unreadable_one(bool readable) + { + var serializer = new CredentialPayloadSerializer(); + var envelope = readable ? PrefixEncryptor.Seal(serializer.Serialize(new OAuthPayload { AccessToken = "at-read", RefreshToken = "rt-read" })) : "not-an-envelope"; + var credential = new Credential { Id = Guid.NewGuid(), AuthType = AuthType.OAuth, DisplayName = "oauth", EncryptedPayload = envelope }; + + var payload = Revoker(new RecordingOAuthClient()).ReadPayload(credential); + + if (!readable) + { + payload.ShouldBeNull(); + return; + } + + payload.ShouldNotBeNull(); + payload.AccessToken.ShouldBe("at-read"); + payload.RefreshToken.ShouldBe("rt-read"); + } + + private static OAuthGrantRevoker Revoker(RecordingOAuthClient client) => new(new SingleClientRegistry(client), new PrefixEncryptor(), new CredentialPayloadSerializer()); + + private sealed class RecordingOAuthClient : IOAuthClient + { + public List Revokes { get; } = new(); + + public ProviderKind Kind => ProviderKind.GitLab; + + public Uri BuildAuthorizeUrl(OAuthAuthorizeInput input) => throw new NotSupportedException(); + + public Task ExchangeCodeAsync(OAuthCodeExchangeInput input, CancellationToken cancellationToken) => throw new NotSupportedException(); + + public Task RefreshAsync(OAuthRefreshInput input, CancellationToken cancellationToken) => throw new NotSupportedException(); + + public Task RevokeAsync(OAuthRevokeInput input, CancellationToken cancellationToken) + { + Revokes.Add(input); + return Task.CompletedTask; + } + } + + private sealed class SingleClientRegistry : IOAuthClientRegistry + { + private readonly IOAuthClient _client; + + public SingleClientRegistry(IOAuthClient client) { _client = client; } + + public IOAuthClient Get(ProviderKind kind) => kind == _client.Kind ? _client : throw new NotSupportedException($"No OAuth client registered for provider {kind}"); + } + + /// An envelope that opens only when it carries the prefix it was sealed with, as a real key ring opens only its own. + private sealed class PrefixEncryptor : IPayloadEncryptor + { + private const string Prefix = "sealed:"; + + public static string Seal(string plaintext) => Prefix + plaintext; + + public string Encrypt(string plaintext) => Seal(plaintext); + + public string Decrypt(string ciphertext) => ciphertext.StartsWith(Prefix, StringComparison.Ordinal) ? ciphertext[Prefix.Length..] : throw new InvalidOperationException("the envelope does not open"); + } +} diff --git a/backend/tests/CodeSpace.UnitTests/OAuth/OAuthTokenRefresherTests.cs b/backend/tests/CodeSpace.UnitTests/OAuth/OAuthTokenRefresherTests.cs index 12c8ad438..de07ad6af 100644 --- a/backend/tests/CodeSpace.UnitTests/OAuth/OAuthTokenRefresherTests.cs +++ b/backend/tests/CodeSpace.UnitTests/OAuth/OAuthTokenRefresherTests.cs @@ -172,21 +172,29 @@ public async Task A_refused_refresh_with_nothing_newer_stored_rethrows_the_refus refusal.Error.ShouldBe("invalid_grant"); } - [Fact] - public async Task A_credential_revoked_while_the_provider_rotated_is_not_written_back() + /// + /// Revocation empties the row without taking the refresh lock — here while the provider answers the refresh. The pair + /// it issued must not land in the revoked row, and since nothing stores it, it is revoked at the provider — the way the + /// write that emptied the row ends tokens. A Disconnect ends the user's grant, so the pair goes with the grant. A + /// retirement (connection delete, departure, relink) also soft-deletes the row and ends tokens, never the grant, which + /// on GitHub also carries the user's tokens in every other team connected through the same OAuth App. + /// + [Theory] + [InlineData(false, true)] // Disconnect: blanked, still listed → the whole grant + [InlineData(true, false)] // retirement: blanked and soft-deleted → only the pair + public async Task A_credential_revoked_while_the_provider_rotated_is_not_written_back(bool retired, bool expectedWholeGrant) { - // Revocation empties the row without taking the refresh lock — here while the provider answers the refresh. The - // pair it issued must not land in the revoked row, and since nothing stores it, it is revoked at the provider. var ctx = BuildContext(); var stale = new OAuthPayload { AccessToken = "old", RefreshToken = "r-0", ExpiresAt = Now.AddSeconds(-1) }; var row = new StoredRow(stale); - var provider = new RecordingOAuthClient("new", "r-1", Now.AddHours(2), onRefreshing: row.Revoke); + var provider = new RecordingOAuthClient("new", "r-1", Now.AddHours(2), onRefreshing: retired ? row.Retire : row.Revoke); var failure = await Should.ThrowAsync(() => RunAsync(stale, ctx, row, provider)); failure.Message.ShouldContain("was revoked"); row.IsEmpty.ShouldBeTrue(); provider.RevokedTokens.ShouldBe(new[] { "r-1", "new" }); + provider.WholeGrants.ShouldBe(new[] { expectedWholeGrant, expectedWholeGrant }, ignoreOrder: false, customMessage: "the issued pair ends the way the write that emptied the row ends tokens — check OAuthTokenRefresher.RevokeIssuedTokensAsync"); } [Fact] @@ -283,14 +291,26 @@ private sealed class StoredRow : ICredentialPayloadReader public void Store(CredentialPayload payload) => _ciphertext = Serializer.Serialize(payload); + private bool _retired; + + /// A Disconnect: the token blanked, the row still in the team's list. public void Revoke() => _ciphertext = string.Empty; + /// A retirement — connection delete, departure, relink: the token blanked and the row soft-deleted. + public void Retire() + { + Revoke(); + _retired = true; + } + public bool IsEmpty => _ciphertext.Length == 0; public OAuthPayload Read() => (OAuthPayload)Serializer.Deserialize(AuthType.OAuth, _ciphertext); public Task ReadEncryptedPayloadAsync(Guid credentialId, CancellationToken cancellationToken) => Task.FromResult(_ciphertext); + public Task IsRetiredAsync(Guid credentialId, CancellationToken cancellationToken) => Task.FromResult(_retired); + /// The writer's compare-and-swap: stores only if the row still holds . public bool StoreIfUnchanged(string expected, CredentialPayload payload) { @@ -336,6 +356,9 @@ public RecordingOAuthClient(string accessToken, string? refreshToken, DateTimeOf public List RevokedTokens { get; } = new(); + /// of each revoke, in the order of . + public List WholeGrants { get; } = new(); + public ProviderKind Kind => ProviderKind.GitLab; public Uri BuildAuthorizeUrl(OAuthAuthorizeInput input) => throw new NotSupportedException(); @@ -366,6 +389,7 @@ public Task RefreshAsync(OAuthRefreshInput input, Cancellati public Task RevokeAsync(OAuthRevokeInput input, CancellationToken cancellationToken) { RevokedTokens.Add(input.Token); + WholeGrants.Add(input.WholeGrant); return Task.CompletedTask; } } diff --git a/backend/tests/CodeSpace.UnitTests/Variables/VariableRetirementTests.cs b/backend/tests/CodeSpace.UnitTests/Variables/VariableRetirementTests.cs new file mode 100644 index 000000000..9d00cca42 --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Variables/VariableRetirementTests.cs @@ -0,0 +1,40 @@ +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Variables; +using CodeSpace.Messages.Enums; +using Shouldly; + +namespace CodeSpace.UnitTests.Variables; + +/// +/// Deleting a variable keeps its row for the audit trail and drops the only thing in it that is still a secret: a +/// Secret's ciphertext. Nothing reads a deleted row's value — every read filters on deleted_date IS NULL and +/// nothing restores one — so the envelope could only ever be opened by someone holding the database and the key. A +/// plain value is not a secret and stays, as the history of what the variable was. +/// +[Trait("Category", "Unit")] +public sealed class VariableRetirementTests +{ + private static readonly DateTimeOffset Now = new(2030, 1, 1, 12, 0, 0, TimeSpan.Zero); + + [Theory] + [InlineData(VariableValueType.Secret)] + [InlineData(VariableValueType.String)] + public void Retiring_stamps_the_deletion_and_drops_a_secrets_ciphertext_only(VariableValueType valueType) + { + var actor = Guid.NewGuid(); + var row = new Variable + { + Id = Guid.NewGuid(), Name = "TOKEN", ValueType = valueType, + ValueEncrypted = valueType == VariableValueType.Secret ? new byte[] { 1, 2, 3 } : null, + ValuePlain = valueType == VariableValueType.Secret ? null : "\"kept\"", + }; + + VariableRetirement.Retire(row, Now, actor); + + row.DeletedDate.ShouldBe(Now); + row.LastModifiedDate.ShouldBe(Now); + row.LastModifiedBy.ShouldBe(actor); + row.ValueEncrypted.ShouldBeNull("a deleted variable holds no ciphertext anyone could open later"); + row.ValuePlain.ShouldBe(valueType == VariableValueType.Secret ? null : "\"kept\"", "a plain value is no secret and stays as the record of what the variable held"); + } +}