Skip to content
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

[#11018] Implement Google OAuth 2.0 to replace Users API #11073

Merged
merged 13 commits into from
Apr 13, 2021

Conversation

wkurniawan07
Copy link
Member

@wkurniawan07 wkurniawan07 commented Mar 30, 2021

Fixes #11018

This is security-related which I do not have much expertise on, so I'd appreciate all inputs.

List of things done:

  • Standardized the login/logout URLs such that no more generation is necessary.
    • Two login URLs: dev server login and Google OAuth login. Both login URLs will be deployed, with redirect added for the one that should not be used in the specific environment.
    • Logout URL is the same no matter the environment.
  • Designed a user cookie format to capture the user information: user email and a flag indicating whether user is admin.
  • Implemented the dev server login page exactly how App Engine SDK did it behind the screen.
  • Implemented OAuth 2.0 flow with configuration from Google Cloud IAM.
  • Removed the admin constraint in web.xml as this "admin" privilege is coupled with the privilege in the Google Cloud IAM.
    • This is not necessarily what an "admin" means for the system, so decoupling this will also restore some business logic for us.
    • The system is not any less secure as the usual admin check (either login credential or App Engine queue name header) is still present.
  • For E2E tests against production server, we do not need pre-login and using user data folder anymore. Instead we inject the user cookie (whose format we have full control) to the browser instance.
    • This is not vulnerable as the cookie is encrypted and can only be decrypted with the correct key.

Some implementation details:

  • The cookie is generated by serializing a standard JSON data structure, then encrypted.
    • Cookie forgery is not possible unless the person knows the encryption key.
    • Tampering by manually changing values will not work because it will cause the cookie value decryption to fail.
    • The cookie has HttpOnly flag, and additional Secure flag for production server, and is persisted for 7 days.
  • Standard OAuth 2.0 flow is used, with the state parameter containing an encrypted data structure.
    • The data structure contains redirect URL (after callback) and session ID (as a form of random string).
    • The encryption will ensure that the parameter cannot be forged or tampered.
    • Session ID will be used to confirm that the login comes from the exact same session.
  • "Admin" privilege is now configured via build.properties file.
    • Only users with login credential matching the property will have admin privilege.

@wkurniawan07 wkurniawan07 marked this pull request as ready for review March 30, 2021 13:32
@wkurniawan07 wkurniawan07 added the s.ToReview The PR is waiting for review(s) label Mar 30, 2021
@wkurniawan07 wkurniawan07 added this to the V8.0.0 milestone Mar 30, 2021
Copy link
Contributor

@madanalogy madanalogy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't take a close look at E2E changes as I'm not very familiar with them. Changes otherwise looks good, just one suggestion below

Copy link
Contributor

@madanalogy madanalogy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@madanalogy madanalogy self-assigned this Apr 13, 2021
@madanalogy madanalogy added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Apr 13, 2021
@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Apr 13, 2021
@wkurniawan07 wkurniawan07 merged commit 63f1ca0 into TEAMMATES:teammatesv8 Apr 13, 2021
@wkurniawan07 wkurniawan07 deleted the runtime-upgrade/auth branch April 13, 2021 10:57
@wkurniawan07 wkurniawan07 added c.Task Other non-user-facing works, e.g. refactoring, adding tests c.DevOps Process-related or build-related improvement and addition and removed c.Task Other non-user-facing works, e.g. refactoring, adding tests labels Jun 6, 2021
daongochieu2810 pushed a commit to daongochieu2810/teammates that referenced this pull request Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.DevOps Process-related or build-related improvement and addition s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants