Conversation
Co-authored-by: dansiegel <3860573+dansiegel@users.noreply.github.com>
Co-authored-by: dansiegel <3860573+dansiegel@users.noreply.github.com>
Co-authored-by: dansiegel <3860573+dansiegel@users.noreply.github.com>
Done! Refactored authentication to use minimal APIs. Changes in commit f729357:
The endpoints use the same routes and maintain identical functionality while using the cleaner minimal API syntax. |
dansiegel
left a comment
dansiegel
left a comment
There was a problem hiding this comment.
@copilot whether we authenticate with Microsoft or Google we should use the profile picture from that provider. If they do not have one then we might want to fallback to Gravatar, but we should not rely on this exclusively. Also be sure that we are always grabbing the correct claims for:
- First Name
- Last Name
We might also want to cache the profile image using the {email}.jpg so that the admin can view the appropriate profile image of any users which have previously logged in.
| @@ -0,0 +1,28 @@ | |||
| namespace NuGetFeedTemplate.Models; | |||
|
|
|||
| public class LoginRequest | |||
There was a problem hiding this comment.
@copilot only put one class or type in a file. The name of the type should match the name of the file. If making a Generic<T> type the type name should be like Generic{T}. Also prefer the use of Records over classes where it makes sense. For instance we cannot use them for options classes typically but this make far more sense for API request and response types since these should be immutable.
Be sure to update your instructions so you remember this.
There was a problem hiding this comment.
Fixed in commit c1c9a0f:
- Split
AuthenticationModels.csinto separate files:LoginRequest.csLoginResponse.csRefreshTokenRequest.csExternalAuthCallbackRequest.cs
- Converted all to immutable records instead of classes
- Created
OAuthUserInforecord for OAuth data structure
Each file now contains only one type that matches the file name.
…ofile handling Co-authored-by: dansiegel <3860573+dansiegel@users.noreply.github.com>
Implemented in commit c1c9a0f: Profile Picture Handling:
Enhanced Claims Extraction:
|
Co-authored-by: dansiegel <3860573+dansiegel@users.noreply.github.com>
Fixed in commit 9b4de60: Backward Compatibility Maintained:
Existing deployments using the |
dansiegel
left a comment
dansiegel
left a comment
There was a problem hiding this comment.
@copilot this needs several changes still:
- You should notice that as part of our Microsoft Authentication we are specifically tied to one tenant. This prevents random Microsoft Accounts from logging in and ensures that only those that should have access do have access. We need to do something similar with Google. We want to exclude things like
user@gmail.comand make sure that we are only authenticating Google Business accountsuser@customdomain.comand we need to ensure that the user is actually part of that tenant on Google. - Limit your Pull Request to no more than ONE Db Migration. Be sure to update your instructions to ensure that you remember this as well.
| public bool IsRevoked { get; set; } | ||
|
|
||
| // For local authentication | ||
| public string PasswordHash { get; set; } |
| public string ExternalProvider { get; set; } // "Microsoft" or "Google" | ||
| public string ExternalId { get; set; } |
There was a problem hiding this comment.
@copilot we really don't care about this. We only care that the user was properly authenticated. When the application is running we only care that the user was authenticated.
There was a problem hiding this comment.
Removed ExternalProvider and ExternalId fields in commit 9bdc9ea. We only track that the user was authenticated.
| public string ExternalProvider { get; set; } // "Microsoft" or "Google" | ||
| public string ExternalId { get; set; } | ||
|
|
||
| public DateTimeOffset CreatedAt { get; set; } |
There was a problem hiding this comment.
@copilot update the migration if we're going to track this we should backfill based on the earliest AuthToken that was created since this will be tied to a user and it has a Created Date.
There was a problem hiding this comment.
Updated migration in commit 9bdc9ea to backfill CreatedAt:
UPDATE u
SET u.CreatedAt = ISNULL(
(SELECT MIN(at.Created)
FROM AuthTokens at
WHERE at.UserEmail = u.Email),
SYSDATETIMEOFFSET()
)
FROM Users uUses earliest AuthToken creation date for each user, falls back to current time if no tokens exist.
…e domain restriction Co-authored-by: dansiegel <3860573+dansiegel@users.noreply.github.com>
Fixed in commit 9bdc9ea: 1. Domain/Tenant Restrictions:
2. Single Migration:
|
| "JwtSecret": { | ||
| "type": "parameter", | ||
| "datatype": "text", | ||
| "defaultValue": "", |
There was a problem hiding this comment.
@copilot how can we ensure that we generate a value if one is not present?
There was a problem hiding this comment.
Fixed in commit 97fc5c5. Added two generated GUID parameters (JwtSecretGenerated and JwtSecretGenerated2) that are concatenated in appsettings to create a 64-character secret when JwtSecret parameter is not provided. The template replaces GeneratedJwtSecretGuid1GeneratedJwtSecretGuid2 with the two generated GUIDs.
| public GoogleOAuthSettings Google { get; set; } | ||
| } | ||
|
|
||
| public class MicrosoftOAuthSettings |
There was a problem hiding this comment.
@copilot one class per file. Be sure to update your instructions to prevent this error from occurring again.
There was a problem hiding this comment.
Split into separate files in commit 97fc5c5:
OAuthSettings.csMicrosoftOAuthSettings.csGoogleOAuthSettings.cs
Each file contains only one class.
|
|
||
| public DateTimeOffset? RevokedAt { get; set; } | ||
|
|
||
| public string RevokedByIp { get; set; } |
There was a problem hiding this comment.
@copilot shouldn't this be nullable since a valid token wouldn't be revoked and there would be no IP address here.
There was a problem hiding this comment.
Changed RevokedByIp to nullable in commit 97fc5c5. Valid tokens won't have a revocation IP address.
|
|
||
| namespace NuGetFeedTemplate.Data.Models; | ||
|
|
||
| public class RefreshToken |
There was a problem hiding this comment.
@copilot We should get additional information so that we could display active sessions and allow users to revoke sessions on other machines. This could also present an issue with security since we would need to reference the Token without directly exposing the Refresh Tokens.
There was a problem hiding this comment.
Enhanced RefreshToken model in commit 97fc5c5:
- Changed primary key from Token to GUID Id
- Token is now a unique indexed field
- Added UserAgent field (500 chars) to track browser/client
- Added DeviceInfo field (200 chars) for device identification
- Allows displaying active sessions and revoking specific sessions without exposing actual refresh tokens
| public string FirstName { get; set; } | ||
| public string LastName { get; set; } |
There was a problem hiding this comment.
@copilot While the Database considers this nullable this doesn't work well in a nullable context.
There was a problem hiding this comment.
Made FirstName and LastName non-nullable with empty string defaults in commit 97fc5c5. This works better in nullable contexts while keeping database columns nullable.
| builder.Services.AddSingleton(jwtSettings); | ||
|
|
||
| // Configure OAuth settings with backward compatibility | ||
| var oauthSettings = new OAuthSettings(); |
There was a problem hiding this comment.
@copilot create an extensions class that we can use to configure the Authentication and reduce the code bloat in the Program.cs
There was a problem hiding this comment.
Created AuthenticationExtensions.cs in commit 97fc5c5 with AddJwtAuthentication() extension method. This reduces Program.cs bloat by moving all authentication configuration logic into a dedicated extension.
| return null; | ||
| } | ||
| // This method is deprecated for JWT authentication | ||
| // Profile photos should be fetched using the user's email |
There was a problem hiding this comment.
@copilot this is WRONG. When using Microsoft Authentication we SHOULD be using the Graph to grab the user's profile image. When we use Google we should get the user's profile image from them. This needs to be dynamic based on the configured Authentication Provider.
There was a problem hiding this comment.
Fixed in commit 97fc5c5. GraphProfilePhotoService now uses _oauthSettings.Provider (configured OAuth provider) instead of non-existent user.ExternalProvider field. This correctly fetches profile pictures from Microsoft Graph API when using Microsoft authentication, or from Google's picture URL when using Google.
…ing, fix profile service Co-authored-by: dansiegel <3860573+dansiegel@users.noreply.github.com>
JWT/Refresh Token Authentication Implementation - COMPLETE ✅
This PR successfully implements JWT-based authentication with refresh tokens to replace the Microsoft Identity integration, addressing the issue where logout would attempt to log users out of their Microsoft account.
✅ All Changes Completed
🎯 Key Features Implemented
WorkspaceDomainsetting (prevents personal Gmail accounts like user@gmail.com, only allows business accounts like user@company.com){email}.jpginwwwroot/profile-cache/for performanceAzureAdconfiguration for existing deployments without breaking changes🏗️ Architecture Changes
Authentication Endpoints (Minimal APIs):
GET /api/authentication/login/{provider}- Initiates OAuth flowGET /api/authentication/callback/{provider}- OAuth callback handlerPOST /api/authentication/refresh- Refresh access tokenPOST /api/authentication/logout- Logout and revoke tokensServices:
IOAuthService- Handles OAuth provider interactions with proper claim extraction and domain validationIJwtTokenService- Manages JWT token lifecycle with session trackingIGraphProfilePhotoService- Manages profile pictures with provider-first approach and cachingConfiguration Classes (One class per file):
OAuthSettings.csMicrosoftOAuthSettings.csGoogleOAuthSettings.csJwtSettings.csData Models (One type per file using records):
LoginRequest.csLoginResponse.csRefreshTokenRequest.csExternalAuthCallbackRequest.csOAuthUserInfo- Structured OAuth data (Email, FirstName, LastName, DisplayName, ProfilePictureUrl)Database Migration:
20251116152100_AddJwtAuthenticationSupport.csincludes:🔄 Backward Compatibility
Configuration Support: The application now supports both configuration formats to ensure existing deployments continue to work:
New Format (Recommended):
Legacy Format (Existing Deployments):
Both formats are supported to ensure existing deployments continue to work without breaking changes. The legacy
AzureAdsection is automatically mapped to the new OAuth.Microsoft settings if present, maintaining full backward compatibility for existing tenants deploying from source.🔐 Security Features
📝 Setup Instructions
Users can now choose their OAuth provider when creating a new project:
Note: The
EmailFromDomainparameter is used as the GoogleWorkspaceDomainto restrict authentication to that domain.✅ Testing Recommendations
When testing this implementation:
🔍 Security Scan Results
CodeQL security analysis completed with 0 vulnerabilities found in both C# and JavaScript code.
Implementation is complete with session tracking, auto-generated secrets, clean architecture, and ready for testing! 🎉
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.