up

[emrgnt-cmplxty]

Important

This pull request adds OAuth support for Google and GitHub, updates user account management, and modifies the database schema and SDK accordingly.

• OAuth Support:
  • Added oauthGoogleAuthorize and oauthGithubAuthorize methods in users.ts and users.py for OAuth authorization.
  • Implemented Google and GitHub OAuth callback handlers in users_router.py.
• Database Changes:
  • Added account_type, google_id, and github_id columns to the users table in users.py.
  • Created indexes for google_id and github_id in users.py.
• User Management:
  • Updated create_user and update_user methods in users.py to handle OAuth accounts.
  • Modified register and login methods in r2r_auth.py to support OAuth.
• SDK Updates:
  • Added OAuth methods to UsersSDK in users.py.
• Dependency Updates:
  • Added google-auth and google-auth-oauthlib to pyproject.toml.

^{This description was created by for d44ea19. It will automatically update as commits are pushed.}

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to d44ea19 in 2 minutes and 39 seconds

More details

• Looked at 884 lines of code in 10 files
• Skipped 1 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. py/core/database/users.py:301

• Draft comment:
  Setting hashed_password to an empty string if None can lead to confusion or errors. Consider handling this more explicitly to ensure clarity and correctness.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The code is handling a valid use case - OAuth users don't have passwords, so hashed_password needs some default value. Empty string is a reasonable choice since: 1. The code validates password requirements for password accounts 2. OAuth accounts are clearly marked with account_type 3. Empty string is a clear signal that there is no password 4. The database column is NOT NULL so we need some value
  Could storing NULL be better than empty string to make the lack of password more explicit? Could this cause issues if the code assumes hashed_password is always valid?
  The NOT NULL constraint on the column means NULL isn't an option. The account_type field already makes it explicit when a user has no password. The code checks account_type before using the password.
  The comment should be deleted. The current approach of using empty string is reasonable given the database constraints and the code already handles the OAuth vs password distinction properly through account_type.

2. py/core/providers/auth/r2r_auth.py:583

• Draft comment:
  Marking the user as verified based on the OAuth provider's email assumes the email is trustworthy. Ensure this assumption is documented and validated to avoid security issues.
• Reason this comment was not posted:
  Comment did not seem useful.

3. py/core/main/api/v3/users_router.py:1853

• Draft comment:
  The state parameter in github_callback is not validated. Ensure it is checked against a stored value to prevent CSRF attacks.
• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_zPA7gJvmE628Kw36

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

The handling for the GitHub provider in oauth_callback_handler is incomplete. Implement the logic similar to the Google provider to ensure correct functionality.

[ellipsis-dev]

The state parameter in google_callback is not validated. Ensure it is checked against a stored value to prevent CSRF attacks.