-
Notifications
You must be signed in to change notification settings - Fork 0
CLD-03 follow-up: DB-backed auth codes, PKCE, account linking #812
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2a9cf64
67c66e7
740d9f9
ff6e8f0
29f6eca
bb92247
99f5305
8b20dbd
57ed8c4
1ec777f
5457479
8fb927a
2232bb1
b2720fc
9a8ab0e
0b99ab2
4ea2875
3164429
4743422
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| using Taskdeck.Domain.Entities; | ||
|
|
||
| namespace Taskdeck.Application.Interfaces; | ||
|
|
||
| public interface IOAuthAuthCodeRepository : IRepository<OAuthAuthCode> | ||
| { | ||
| /// <summary> | ||
| /// Finds an auth code by its code string. Returns null if not found. | ||
| /// </summary> | ||
| Task<OAuthAuthCode?> GetByCodeAsync(string code, CancellationToken cancellationToken = default); | ||
|
|
||
| /// <summary> | ||
| /// Atomically consumes an auth code by setting IsConsumed = true where IsConsumed = false. | ||
| /// Returns true if the code was consumed (affected 1 row), false if already consumed or not found. | ||
| /// This provides single-use semantics safe for concurrent requests. | ||
| /// </summary> | ||
| Task<bool> TryConsumeAtomicAsync(string code, CancellationToken cancellationToken = default); | ||
|
|
||
| /// <summary> | ||
| /// Deletes all auth codes that have expired before the specified cutoff time. | ||
| /// Returns the number of codes removed. | ||
| /// </summary> | ||
| Task<int> DeleteExpiredAsync(DateTimeOffset cutoff, CancellationToken cancellationToken = default); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -209,6 +209,83 @@ public async Task<Result<AuthResultDto>> ExternalLoginAsync(ExternalLoginDto dto | |
| } | ||
| } | ||
|
|
||
| public async Task<Result<LinkedAccountDto>> CompleteAccountLinkAsync(Guid userId, string provider, string providerUserId, string? displayName, string? avatarUrl) | ||
| { | ||
| try | ||
| { | ||
| if (userId == Guid.Empty) | ||
| return Result.Failure<LinkedAccountDto>(ErrorCodes.ValidationError, "User ID is required"); | ||
|
|
||
| var user = await _unitOfWork.Users.GetByIdAsync(userId); | ||
| if (user == null) | ||
| return Result.Failure<LinkedAccountDto>(ErrorCodes.NotFound, "User not found"); | ||
|
|
||
| if (!user.IsActive) | ||
| return Result.Failure<LinkedAccountDto>(ErrorCodes.Forbidden, "User account is inactive"); | ||
|
|
||
| // Check if this provider+userId combo is already linked to a different user | ||
| var existingLogin = await _unitOfWork.ExternalLogins.GetByProviderAsync(provider, providerUserId); | ||
| if (existingLogin != null) | ||
| { | ||
| if (existingLogin.UserId == userId) | ||
| return Result.Failure<LinkedAccountDto>(ErrorCodes.Conflict, $"This {provider} account is already linked to your account"); | ||
|
|
||
| return Result.Failure<LinkedAccountDto>(ErrorCodes.Conflict, $"This {provider} account is already linked to a different user"); | ||
| } | ||
|
|
||
| // Check if user already has a linked account for this provider | ||
| var userLogins = await _unitOfWork.ExternalLogins.GetByUserIdAsync(userId); | ||
| if (userLogins.Any(l => l.Provider == provider)) | ||
| return Result.Failure<LinkedAccountDto>(ErrorCodes.Conflict, $"Your account is already linked to a {provider} account"); | ||
|
|
||
| var newLogin = new ExternalLogin(userId, provider, providerUserId, displayName, avatarUrl); | ||
| await _unitOfWork.ExternalLogins.AddAsync(newLogin); | ||
| await _unitOfWork.SaveChangesAsync(); | ||
|
|
||
| return Result.Success(new LinkedAccountDto( | ||
| newLogin.Provider, | ||
| newLogin.ProviderUserId, | ||
| newLogin.ProviderDisplayName, | ||
| newLogin.AvatarUrl, | ||
| newLogin.CreatedAt)); | ||
| } | ||
| catch (DomainException ex) | ||
| { | ||
| return Result.Failure<LinkedAccountDto>(ex.ErrorCode, ex.Message); | ||
| } | ||
| catch (Exception) | ||
| { | ||
| return Result.Failure<LinkedAccountDto>(ErrorCodes.UnexpectedError, "Account linking failed due to an unexpected error"); | ||
| } | ||
| } | ||
|
|
||
| public async Task<Result> UnlinkExternalLoginAsync(Guid userId, string provider) | ||
| { | ||
| try | ||
| { | ||
| if (userId == Guid.Empty) | ||
| return Result.Failure(ErrorCodes.ValidationError, "User ID is required"); | ||
|
|
||
| var user = await _unitOfWork.Users.GetByIdAsync(userId); | ||
| if (user == null) | ||
| return Result.Failure(ErrorCodes.NotFound, "User not found"); | ||
|
|
||
| var logins = await _unitOfWork.ExternalLogins.GetByUserIdAsync(userId); | ||
| var loginToRemove = logins.FirstOrDefault(l => l.Provider == provider); | ||
| if (loginToRemove == null) | ||
| return Result.Failure(ErrorCodes.NotFound, $"No {provider} account is linked"); | ||
|
|
||
| await _unitOfWork.ExternalLogins.DeleteAsync(loginToRemove); | ||
| await _unitOfWork.SaveChangesAsync(); | ||
|
Comment on lines
+278
to
+279
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This path removes the external login unconditionally, but OAuth-created users are given a random unknown password hash and cannot set a new password without the current one, so deleting their only GitHub login can permanently lock them out of the account. Add a guard that blocks unlink when no other sign-in method exists (or require setting a password first). Useful? React with 👍 / 👎. |
||
|
|
||
| return Result.Success(); | ||
| } | ||
| catch (DomainException ex) | ||
| { | ||
| return Result.Failure(ex.ErrorCode, ex.Message); | ||
| } | ||
| } | ||
|
|
||
| public async Task<Result> ChangePasswordAsync(Guid userId, string currentPassword, string newPassword) | ||
| { | ||
| try | ||
|
|
@@ -290,7 +367,7 @@ public async Task<Result<UserDto>> ValidateTokenAsync(string token) | |
| } | ||
| } | ||
|
|
||
| private string GenerateJwtToken(User user) | ||
| public string GenerateJwtToken(User user) | ||
| { | ||
| if (!TryValidateJwtSettings(out var jwtValidationError)) | ||
| throw new DomainException(ErrorCodes.UnexpectedError, jwtValidationError); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,144 @@ | ||
| using Taskdeck.Domain.Common; | ||
| using Taskdeck.Domain.Exceptions; | ||
|
|
||
| namespace Taskdeck.Domain.Entities; | ||
|
|
||
| /// <summary> | ||
| /// A short-lived, single-use authorization code issued after OAuth callback. | ||
| /// Stored in the database to survive restarts and support multi-instance deployments. | ||
| /// Supports both login (Token populated) and account-linking (ProviderData populated) flows. | ||
| /// </summary> | ||
| public class OAuthAuthCode : Entity | ||
| { | ||
| private string _code = string.Empty; | ||
|
|
||
| public string Code | ||
| { | ||
| get => _code; | ||
| private set | ||
| { | ||
| if (string.IsNullOrWhiteSpace(value)) | ||
| throw new DomainException(ErrorCodes.ValidationError, "Auth code cannot be empty"); | ||
|
|
||
| if (value.Length > 512) | ||
| throw new DomainException(ErrorCodes.ValidationError, "Auth code cannot exceed 512 characters"); | ||
|
|
||
| _code = value; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// The user ID this code authenticates (login flow) or Guid.Empty (link flow). | ||
| /// </summary> | ||
| public Guid UserId { get; private set; } | ||
|
|
||
| /// <summary> | ||
| /// Legacy field kept for schema compatibility. No longer populated with real JWTs -- | ||
| /// tokens are re-issued at exchange time from the stored UserId. | ||
| /// </summary> | ||
| public string Token { get; private set; } = string.Empty; | ||
|
|
||
| /// <summary> | ||
| /// The purpose of this code: "login" or "link". | ||
| /// </summary> | ||
| public string Purpose { get; private set; } = "login"; | ||
|
|
||
| /// <summary> | ||
| /// JSON-serialized provider identity data for account linking flows. | ||
| /// Contains provider, providerUserId, displayName, avatarUrl. | ||
| /// </summary> | ||
| public string? ProviderData { get; private set; } | ||
|
|
||
| /// <summary> | ||
| /// When this code expires and can no longer be exchanged. | ||
| /// </summary> | ||
| public DateTimeOffset ExpiresAt { get; private set; } | ||
|
|
||
| /// <summary> | ||
| /// Whether this code has been consumed (exchanged for a token). | ||
| /// </summary> | ||
| public bool IsConsumed { get; private set; } | ||
|
|
||
| /// <summary> | ||
| /// When the code was consumed, if applicable. | ||
| /// </summary> | ||
| public DateTimeOffset? ConsumedAt { get; private set; } | ||
|
|
||
| private OAuthAuthCode() : base() { } | ||
|
|
||
| /// <summary> | ||
| /// Creates an auth code for the login flow (token exchange). | ||
| /// Token parameter is accepted for backward compatibility but is no longer stored; | ||
| /// JWTs are re-issued at exchange time from the UserId. | ||
| /// </summary> | ||
| public OAuthAuthCode(string code, Guid userId, string token, DateTimeOffset expiresAt) | ||
| : base() | ||
| { | ||
| if (userId == Guid.Empty) | ||
| throw new DomainException(ErrorCodes.ValidationError, "User ID cannot be empty"); | ||
|
|
||
| if (string.IsNullOrWhiteSpace(token)) | ||
| throw new DomainException(ErrorCodes.ValidationError, "Token cannot be empty"); | ||
|
|
||
| if (expiresAt <= DateTimeOffset.UtcNow) | ||
| throw new DomainException(ErrorCodes.ValidationError, "Expiry must be in the future"); | ||
|
|
||
| Code = code; | ||
| UserId = userId; | ||
| Token = string.Empty; // Never store actual JWT in DB — re-issue at exchange time | ||
| Purpose = "login"; | ||
| ExpiresAt = expiresAt; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates an auth code for the account linking flow (provider identity exchange). | ||
| /// The initiatingUserId binds this code to the user who started the link flow, | ||
| /// preventing CSRF attacks where an attacker's GitHub is linked to a victim's account. | ||
| /// </summary> | ||
| public static OAuthAuthCode CreateForLinking(string code, Guid initiatingUserId, string providerData, DateTimeOffset expiresAt) | ||
| { | ||
| if (initiatingUserId == Guid.Empty) | ||
| throw new DomainException(ErrorCodes.ValidationError, "Initiating user ID is required for linking"); | ||
|
|
||
| if (string.IsNullOrWhiteSpace(providerData)) | ||
| throw new DomainException(ErrorCodes.ValidationError, "Provider data cannot be empty for linking"); | ||
|
|
||
| if (expiresAt <= DateTimeOffset.UtcNow) | ||
| throw new DomainException(ErrorCodes.ValidationError, "Expiry must be in the future"); | ||
|
|
||
| var entity = new OAuthAuthCode | ||
| { | ||
| Code = code, | ||
| UserId = initiatingUserId, | ||
| Token = string.Empty, | ||
| Purpose = "link", | ||
| ProviderData = providerData, | ||
| ExpiresAt = expiresAt | ||
| }; | ||
| return entity; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Returns true if this code has expired. | ||
| /// </summary> | ||
| public bool IsExpired => DateTimeOffset.UtcNow > ExpiresAt; | ||
|
|
||
| /// <summary> | ||
| /// Returns true if this is a linking code (not a login code). | ||
| /// </summary> | ||
| public bool IsLinkingCode => Purpose == "link"; | ||
|
|
||
| /// <summary> | ||
| /// Attempts to consume this code. Returns false if already consumed or expired. | ||
| /// </summary> | ||
| public bool TryConsume() | ||
| { | ||
| if (IsConsumed || IsExpired) | ||
| return false; | ||
|
|
||
| IsConsumed = true; | ||
| ConsumedAt = DateTimeOffset.UtcNow; | ||
| Touch(); | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If two link completions race for the same
(provider, providerUserId), the second insert will hit theExternalLoginsunique constraint atSaveChangesAsync(). Thiscatch (Exception)path currently maps that expected collision toUnexpectedError, so callers get a 500 instead of a 409 conflict even though the account was simply linked first by another request/tab. Catch uniqueness violations explicitly and returnErrorCodes.Conflictso retry/double-submit races don’t surface as server faults.Useful? React with 👍 / 👎.