-
Notifications
You must be signed in to change notification settings - Fork 0
Feature: Public Register #9
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: server-pro
Are you sure you want to change the base?
Conversation
|
@codex Check this PR? |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6582f6dc0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
services/web/app/src/router.mjs
Outdated
| if(!Boolean(process.env.OVERLEAF_ALLOW_PUBLIC_REGISTRATION)) | ||
| webRouter.get('/register', UserPagesController.registerPage) |
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.
Compare OVERLEAF_ALLOW_PUBLIC_REGISTRATION explicitly
The new guard uses Boolean(process.env.OVERLEAF_ALLOW_PUBLIC_REGISTRATION), which treats any non-empty string (e.g., "false", "0") as true. In that scenario the legacy /register route is skipped, while the new registration module is still disabled because it checks process.env.OVERLEAF_ALLOW_PUBLIC_REGISTRATION === 'true' (services/web/modules/registration/index.mjs:4), so /register becomes a 404 and any flows that redirect to registration break. This only happens when the env var is set to a non-empty falsey string, so it’s easy to hit in production configs that use "false"; consider matching the module’s strict string check here too.
Useful? React with 👍 / 👎.
- if OVERLEAF_ALLOW_PUBLIC_REGISTRATION is set to `true`, allow public register - if set to `@example.com`, we will do email check and validation
|
@codex, what about now |
|
Summary
Testing
|
Description
Imply public register router. I think we need to add some limit. For instance, we can config a env var, to allow public register or only allow a specific domain ended email to register.
Here are the todos:
Related issues / Pull Requests
Contributor Agreement