-
Notifications
You must be signed in to change notification settings - Fork 4
111 refactor google auth to delete authentication if already exists #287
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
base: master
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR adds Google OAuth support by introducing a GoogleProvider for token management, integrates token revocation into the authentication flow, and renames the standard provider class.
- Define a shared
Credentialsinterface and relocateAUTH_COOKIE_NAMEto the schemas module. - Implement
GoogleProvider(token fetch/revoke) and invoke revocation inAuthDataService#createAuth. - Rename
StandardSecuritytoStandardProviderand update related imports/tests.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/types/auth.ts | Added Credentials interface; removed old cookie constant. |
| packages/shared/src/schemas/auth.ts | Added AUTH_COOKIE_NAME constant. |
| apps/backend/src/data-layer/services/AuthDataService.ts | Integrated token revocation when existing auth is detected. |
| apps/backend/src/business-layer/provider/standard.ts | Renamed StandardSecurity to StandardProvider. |
| apps/backend/src/business-layer/provider/standard.test.ts | Updated tests to use StandardProvider. |
| apps/backend/src/business-layer/provider/google.ts | Introduced GoogleProvider with fetchTokens and revokeToken. |
| apps/backend/src/app/api/auth/google/callback/route.ts | Switched to GoogleProvider.fetchTokens; cleaned up token logic. |
Comments suppressed due to low confidence (3)
apps/backend/src/data-layer/services/AuthDataService.ts:13
- The JSDoc above still states this method returns an
Authenticationdocument, but the signature now allowsundefined; update the comment or ensure the method always returns a value.
public async createAuth(newAuth: CreateAuthenticationData): Promise<Authentication | undefined> {
apps/backend/src/business-layer/provider/google.ts:26
- [nitpick] Consider adding unit tests for
fetchTokensandrevokeTokento validate token handling and error paths.
static async fetchTokens(code: string): Promise<Credentials | undefined> {
packages/shared/src/schemas/auth.ts:101
- Duplicated definition of AUTH_COOKIE_NAME in different modules can lead to inconsistencies; consider centralizing this constant in one shared location.
export const AUTH_COOKIE_NAME = "auth_token"
| const usersToCreate = [casualUserMock, memberUserMock, adminUserMock] | ||
| await Promise.all( | ||
| usersToCreate.map((user) => | ||
| payload.create({ | ||
| collection: "user", | ||
| data: user, | ||
| }), | ||
| ), | ||
| ) | ||
| await Promise.all(usersToCreate.map(new UserDataService().createUser)) |
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.
just a note, i'm reverting this because it doesn't properly set the IDs, UserDataService.createUser omits the id
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Authenticationcollection is createdGoogleProviderclass to handle token managementStandardSecuritytoStandardProviderFixes #111
Type of change
How Has This Been Tested?
Checklist before requesting a review