-
-
Notifications
You must be signed in to change notification settings - Fork 1
refactor: social login cleanup #36
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: main
Are you sure you want to change the base?
refactor: social login cleanup #36
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors social login flows by extracting the common OAuth2 completion steps into a shared helper, removing outdated role-assignment code, and standardizing client typing and logging for both GitHub and Google providers. Sequence diagram for unified OAuth2 social login completionsequenceDiagram
actor User
participant "Request"
participant "RegistrationController"
participant "OAuth2 Client (GitHub/Google)"
participant "UserService"
participant "Dashboard"
User->>Request: Initiate social login (GitHub/Google)
Request->>RegistrationController: Complete login
RegistrationController->>"OAuth2 Client (GitHub/Google)": get_id_email(token)
"OAuth2 Client (GitHub/Google)"-->>RegistrationController: id, email
RegistrationController->>UserService: get_or_upsert(email)
UserService-->>RegistrationController: user, created
RegistrationController->>Request: set_session(user_id)
RegistrationController->>Request: log auth request
alt New user
RegistrationController->>Request: log user creation
RegistrationController->>Request: flash welcome message
end
RegistrationController->>Dashboard: Redirect to dashboard
Class diagram for updated RegistrationController social login methodsclassDiagram
class RegistrationController {
+github_complete(request, access_token_state, users_service) : InertiaRedirect
+google_complete(request, access_token_state, users_service) : InertiaRedirect
+_auth_complete(request, access_token_state, users_service, oauth_client) : InertiaRedirect
}
class BaseOAuth2 {
+get_id_email(token)
+name
}
class UserService {
+get_or_upsert(match_fields, email, is_verified, is_active)
+to_schema(user, schema_type)
}
RegistrationController --> BaseOAuth2 : uses
RegistrationController --> UserService : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- The removal of roles_service means you’ve dropped default role assignment—ensure this logic is intentionally moved or reimplemented in the new _auth_complete flow.
- By removing oauth_account_service, the OAuth account linking step is lost—confirm that users_service.get_or_upsert now handles linking or re-add that logic.
- You’re logging with oauth_client.name—verify that BaseOAuth2.client provides a
nameproperty, or explicitly pass the provider identifier to avoid undefined values.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The removal of roles_service means you’ve dropped default role assignment—ensure this logic is intentionally moved or reimplemented in the new _auth_complete flow.
- By removing oauth_account_service, the OAuth account linking step is lost—confirm that users_service.get_or_upsert now handles linking or re-add that logic.
- You’re logging with oauth_client.name—verify that BaseOAuth2.client provides a `name` property, or explicitly pass the provider identifier to avoid undefined values.
## Individual Comments
### Comment 1
<location> `app/domain/accounts/controllers.py:198` </location>
<code_context>
)
request.set_session({"user_id": user.email})
- request.logger.info("google auth request", id=_id, email=email)
+ request.logger.info("auth request complete", id=id_, email=email, provider=oauth_client.name)
if created:
request.logger.info("created a new user", id=user.id)
</code_context>
<issue_to_address>
**issue (bug_risk):** Provider name is logged using oauth_client.name, which may not be present on all BaseOAuth2 implementations.
Verify that all BaseOAuth2 subclasses define the 'name' attribute, or use getattr with a default value to prevent runtime errors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
I'd be happy to add oauth2 state + PKCE - it's simple enough with a session, but it's out of scope in a cleanup PR and I don't really want to set up GCP to test it (GitHub would be simpler) |
Description
Small cleanup of social login, so that it's easier to understand and port to litestar-fullstack
Summary by Sourcery
Refactor social login flows to eliminate duplicate code and unify GitHub and Google login completion by introducing a shared helper that handles the OAuth2 flow and user registration.
Enhancements: