Conversation
…, backend see apps/verifier/urls.py, apps/verifier/views.py, currently use ngrok for getting webhook from sjtu wj platform
|
log:
|
…imestamp, answerid, verification code check and create user session
A-lexisL
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR introduces OAuth authentication functionality with SJTU university credentials, replacing the previous email-based authentication system. It implements a secure multi-step authentication flow using Turnstile verification, questionnaire-based identity validation, and session management with Redis caching.
- Adds comprehensive OAuth backend API with secure token management and user verification
- Integrates frontend Auth component with Turnstile security verification and authentication flow
- Replaces email-based authentication with university credential system
Reviewed Changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| website/urls.py | Adds OAuth API endpoints and reorganizes authentication routes |
| website/settings.py | Configures Redis caching, OAuth environment variables, and session management |
| pyproject.toml | Adds httpx and django-redis dependencies for OAuth functionality |
| frontend/src/main.js | Adds new Auth route for OAuth flow |
| frontend/src/components/Auth.vue | Complete OAuth frontend component with Turnstile integration |
| apps/web/views.py | Updates authentication decorators and removes deprecated auth views |
| apps/web/models/review.py | Handles edge case of multiple reviews per user-course pair |
| apps/auth/views.py | Comprehensive OAuth backend implementation with secure token management |
| apps/auth/* | New OAuth app structure with proper Django configuration |
Comments suppressed due to low confidence (1)
website/settings.py:1
- Extra closing parenthesis in comment. Should be 'password resets(deprecated)' without the double closing parenthesis.
import os
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 30 out of 32 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| for key, value in config.items(): | ||
| globals()[key] = value |
There was a problem hiding this comment.
Using globals()[key] = value to dynamically set configuration variables can be dangerous as it allows arbitrary code execution if the YAML file is compromised. Consider using a whitelist of allowed configuration keys or a more secure configuration loading mechanism.
| for key, value in config.items(): | |
| globals()[key] = value | |
| # Only allow specific keys from the YAML config to be set as globals | |
| ALLOWED_CONFIG_KEYS = { | |
| "EXAMPLE_SETTING", # Replace/add with actual allowed keys | |
| "ANOTHER_SETTING", # Replace/add with actual allowed keys | |
| # Add all expected config keys here | |
| } | |
| for key, value in config.items(): | |
| if key in ALLOWED_CONFIG_KEYS: | |
| globals()[key] = value | |
| else: | |
| # Optionally, log or warn about unexpected keys | |
| pass |
| latest_answer, error_response = asyncio.run( | ||
| utils.get_latest_answer(action=mapped_action, account=account), | ||
| ) |
There was a problem hiding this comment.
Using asyncio.run() in a synchronous Django view creates overhead by starting a new event loop for each request. Consider using async views or making the HTTP calls synchronous to improve performance.
| latest_answer, error_response = asyncio.run( | |
| utils.get_latest_answer(action=mapped_action, account=account), | |
| ) | |
| latest_answer, error_response = utils.get_latest_answer_sync(action=mapped_action, account=account) |
|
The component has some redundancy in the watch logic. The two watchers for reviews and totalPages both try to adjust the currentPage when it exceeds totalPages, which could be simplified into a single watcher. CourseReview/frontend/src/components/ReviewPagination.vue Lines 250 to 264 in 745cc06 |
|
Signup.vue and ResetPassword.vue are largely duplicated. Refactor to extract shared template and logic into a reusable component. |
| status=500, | ||
| ) | ||
|
|
||
| # Build the 'params' and 'sort' dictionaries |
apps/auth/utils.py
Outdated
| full_url_path = f"{QUEST_BASE_URL}/{quest_api}/json" | ||
|
|
||
| try: | ||
| async with httpx.AsyncClient() as client: |
There was a problem hiding this comment.
TODO: Currently using async function in sync django so the waiting is still blocking. Requires migrating whole project to async.
apps/auth/utils.py
Outdated
| latest_answer = full_data["data"]["rows"][0] # Get the first (latest) row | ||
|
|
||
| # Find the verification code by matching the question ID | ||
| verification_code = None |
apps/auth/utils.py
Outdated
| } | ||
|
|
||
| # Single regex pattern to check all character requirements at once | ||
| pattern = r"^(?=.*[a-z])(?=.*[A-Z])(?=.*\d).*$" |
There was a problem hiding this comment.
Currently a very strong password like KalwJ&!)@& cannot pass this test, and any passphrase (e.g. subscript-mardi-distract) cannot. Passphrases are common, easy to remember and very secure.
I think the rule should be like the sum of scores of length, character of different cases, number, symbols (i.e. `~!@#$%^&*()-=_+,./;'[]<>?:"{}|) must pass a threshold.
Update:
See frontend password check logic (add requirement of lengh >= 10 && length < 32). Make frontend and backend consistent, except that backend also need to use django password validator to ensure not common passwords (as you have already done)
| from apps.web.models import Student | ||
|
|
||
|
|
||
| class CsrfExemptSessionAuthentication(SessionAuthentication): |
There was a problem hiding this comment.
This seems to disable anti CSRF check. This is generally dangerous but is safe here due to httponly temp_token check.
There was a problem hiding this comment.
Maybe unauthorized user doesn't have csrf token? (I know nothing
frontend/src/components/Auth.vue
Outdated
| }; | ||
|
|
||
| // Get cookie value by name | ||
| function getCookie(name) { |
There was a problem hiding this comment.
I remember there are multiple getCookie() in frontend, is there? Confirm not.
frontend/src/components/Auth.vue
Outdated
| return cookieValue; | ||
| } | ||
|
|
||
| // Reset authentication state |
frontend/src/views/Login.vue
Outdated
| @@ -1,92 +1,137 @@ | |||
| <template> | |||
| <!-- | |||
| for key, value in config.items(): | ||
| globals()[key] = value |
| countdown.value = 1; | ||
|
|
||
| countdownInterval = setInterval(() => { | ||
| countdown.value--; |
There was a problem hiding this comment.
You setInterval 1 second, decrease by 1 every second which will run only once, after one second you clear the interval and jump. WTF???
| } | ||
| }, 1000); | ||
| } catch (err) { | ||
| console.error("Failed to copy to clipboard:", err); |
There was a problem hiding this comment.
Duplicated code. The try-catch error handling is for copy failure. So display copy failure message on error, and wait and redirect go anyhow.
| const hasOtpHint = urlParams.has("otp_hint"); | ||
| const referrer = document.referrer; | ||
| const isFromQuestionnaire = | ||
| referrer.includes("questionnaire") || |
There was a problem hiding this comment.
WTF is "questionnaire"?
Obviously Copilot hallucination.
|
|
||
| // Prevent sending development/mock tokens to backend which will fail verification. | ||
| // Mock tokens are generated by Turnstile.vue when VITE_TURNSTILE_MOCK=true or on localhost. | ||
| const mockPrefix = "dev-turnstile-token-"; |
| return `${minutes}:${seconds.toString().padStart(2, "0")}`; | ||
| }; | ||
|
|
||
| // getCookie imported from utils/cookies.js |
There was a problem hiding this comment.
stop fucking adding so many useless comments
| if (redirectTime) { | ||
| const timeSinceRedirect = now - parseInt(redirectTime); | ||
| // If user returned within 60 seconds, likely verification failed | ||
| if (timeSinceRedirect < 60 * 1000 && timeSinceRedirect > 5 * 1000) { |
There was a problem hiding this comment.
Remove this. If failed then use has to wait for the two minutes pass.
What if there is a network change? What if user session is somehow suspended? What if user closes his laptop and open again? This code will clear the OTP and break valid auth flow on these interruptions.
| } | ||
| } | ||
|
|
||
| // If OTP exists for more than 30 seconds when user returns, likely verification failed |
|
|
||
| // If user is on signup, reset_password, or login page and has stored auth data, restart verification flow | ||
| const isOnSignupWithStoredAuth = | ||
| props.action === "signup" && localStorage.getItem("auth_otp"); |
There was a problem hiding this comment.
I think there is no point switching cases of action
| } | ||
|
|
||
| // Add visibility change listener to detect when user returns from questionnaire | ||
| const handleVisibilityChange = () => { |
There was a problem hiding this comment.
Explain this. What is this for?
| let turnstileWidget = null; | ||
|
|
||
| // Generate unique container ID for this instance | ||
| const containerId = `turnstile-widget-${Math.random() |
There was a problem hiding this comment.
There may be collisions. Is it possible to detect if there exist a widget with id_1, if there is than create one with id_2?
wj OAuth and account-password login
4 participants
No description provided.