-
Notifications
You must be signed in to change notification settings - Fork 2
Integration #329
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
Integration #329
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 modifies the user registration flow by adding authentication requirements and changing the default activation status. The signup endpoint is uncommented and now requires authentication, while new users are created in an inactive state by default.
Key changes:
- New users are now created with
active = Falseinstead ofactive = True - The
/signupendpoint now requires authentication via JWT token withLLEIDAHACKERuser type - Email notification system for new user creation is re-enabled
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/impl/LleidaHacker/service.py | Changed default user activation status to inactive with warning comment |
| src/impl/LleidaHacker/router_v1.py | Uncommented and modified signup endpoint to require authentication and send verification emails |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| def signup(payload: LleidaHackerCreate, token: BaseToken = Depends(JWTBearer())): | ||
| if not token.check([UserType.LLEIDAHACKER]): | ||
| raise AuthenticationException("Not authorized") | ||
|
|
Copilot
AI
Oct 21, 2025
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.
The signup endpoint should not require authentication. Requiring a JWT token to create a new user account creates a chicken-and-egg problem where users cannot sign up without already being authenticated. This breaks the typical user registration flow.
| def signup(payload: LleidaHackerCreate, token: BaseToken = Depends(JWTBearer())): | |
| if not token.check([UserType.LLEIDAHACKER]): | |
| raise AuthenticationException("Not authorized") | |
| def signup(payload: LleidaHackerCreate): |
| ) | ||
| new_lleidahacker.password = get_password_hash(payload.password) | ||
| new_lleidahacker.active = True | ||
| new_lleidahacker.active = False # IMPORTANT DO NOT ACTIVATE USER AUTOMATICALLY !!!!!!!!!!!!! @Big_Lolo |
Copilot
AI
Oct 21, 2025
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.
[nitpick] The comment uses excessive punctuation and mentions a specific person. Comments should be professional and explain the reasoning rather than using emphatic styling. Consider: '# Users must be manually activated by administrators for security reasons' or reference a configuration setting/documentation.
| new_lleidahacker.active = False # IMPORTANT DO NOT ACTIVATE USER AUTOMATICALLY !!!!!!!!!!!!! @Big_Lolo | |
| new_lleidahacker.active = False # Users must be manually activated by administrators for security reasons |
No description provided.