-
Notifications
You must be signed in to change notification settings - Fork 0
Feature: Introduce Template Manage to Overleaf-Pro #6 (from CEP) #7
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
Conversation
- filestore, add a DEL router for delete templates files - modules/template: add templates system for overleaf - Todo: translation update. We don't want to change i18 files, so we should keep them English.
|
Just wait before auto CI finished. |
|
We have intergrated all template system files in webmodules, without any any other files. Also we add a public download router, from which we can download zip templates from overleaf pro cloud system. Next, we need to finish some i18 chores, we want to remove no needed i18 UI, for we want to keep it simple for i18 resource translations update from upstream in the future. |
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: 0bbe53fdf3
ℹ️ 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/modules/template-gallery/app/src/TemplateGalleryManager.mjs
Show resolved
Hide resolved
| async function deleteTemplate({ templateId, version }) { | ||
| await deleteTemplateAssets(templateId, version, true) | ||
| } |
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.
Enforce ownership/admin check before deleting templates
Deletion also accepts only templateId/version and performs the delete without any ownership/admin verification. Combined with the route’s login-only guard, any authenticated user can delete any template if they know the id. Please check that the requester is the owner or an admin before calling deleteTemplateAssets.
Useful? React with 👍 / 👎.
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.
Fixed
| try { | ||
| userIsAdmin = (await UserGetter.promises.getUser(userId, { isAdmin: 1 })).isAdmin | ||
| } catch { | ||
| logger.error({ error, userId }, 'Logged in user does not exist, strange...') | ||
| userIsAdmin = false |
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.
Avoid ReferenceError in canUserOverrideTemplate catch
The catch { ... } block references error, but no binding is declared in the catch clause. In runtimes that support optional catch bindings, error will be undefined here, causing a ReferenceError and masking the original UserGetter failure. This means a missing user record will crash the override check instead of falling back to userIsAdmin = false.
Useful? React with 👍 / 👎.
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.
fixed
|
I will do a check for buggy display in Menu (user setting/templates), I will test this on overleaf-v6.1.0 ce. |
- permission check while edit templates - catch error in try block (TemplateGalleryHelper.mjs) - some comments in router.js (templates)
|
@codex please review again. |
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: 75417b3eb8
ℹ️ 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".
| async function ensureUserCanManageTemplates(req, res, next) { | ||
| const user = SessionManager.getSessionUser(req.session) | ||
| const userId = SessionManager.getLoggedInUserId(req.session) |
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.
Import SessionManager before using it
ensureUserCanManageTemplates calls SessionManager.getSessionUser/getLoggedInUserId, but SessionManager is never imported in this module. At runtime, any request that hits this middleware will throw a ReferenceError before the permission check runs, so template management endpoints will 500 for all users. Please add the missing import (or pass the session helper in) so the middleware can execute.
Useful? React with 👍 / 👎.
|
|
||
| req.requestLogger.addFields({ key, bucket }) | ||
| req.requestLogger.setMessage('deleting file') | ||
|
|
||
| FileHandler.deleteFile(bucket, key, function (err) { |
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.
Avoid calling non-existent FileHandler.deleteFile
FileController.deleteFile invokes FileHandler.deleteFile, but FileHandler only exports insertFile, getFile, getRedirectUrl, and getFileSize (see services/filestore/app/js/FileHandler.js). Because deleteFile is not defined, the new DELETE endpoint will throw TypeError: FileHandler.deleteFile is not a function on every request. Add a delete implementation to FileHandler or call the correct helper to prevent the endpoint from crashing.
Useful? React with 👍 / 👎.
|
@codex I have fixed, what about now? |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex, I have fixed, what about now? |
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: 3fc26ee583
ℹ️ 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".
| const user = SessionManager.getSessionUser(req.session) | ||
| const userId = SessionManager.getLoggedInUserId(req.session) | ||
| const isAdminOrTemplateOwner = hasAdminAccess(user) || Settings.templates?.user_id === userId | ||
|
|
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.
Honor nonAdminCanManage in template permissions
The permissions check only allows admins or the configured Settings.templates.user_id. Because Settings.templates.nonAdminCanManage (set from OVERLEAF_NON_ADMIN_CAN_PUBLISH_TEMPLATES) is never consulted here, enabling that env flag still returns 403 for non‑admin users who should be allowed to manage templates. This makes the deployment toggle ineffective and blocks intended users.
Useful? React with 👍 / 👎.
| try { | ||
| await Template.deleteOne({ _id: templateId }).exec() | ||
| } catch (error) { | ||
| logger.error({ err, templateId }, 'Failed to delete template from MongoDB') |
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.
Avoid ReferenceError when template delete fails
In the MongoDB delete error path, the logger references err, but the caught variable is error. If the delete fails (e.g., transient DB issue), this throws a ReferenceError and masks the real failure, returning an unexpected 500 instead of the underlying error. Logging error avoids introducing a new exception.
Useful? React with 👍 / 👎.
| export function updateTemplate({ | ||
| editedTemplate, | ||
| template | ||
| }: UpdateTemplateOptions): Promise<Template | null> { |
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.
Fix updateTemplate options type mismatch
updateTemplate destructures editedTemplate, but UpdateTemplateOptions does not define that property. This produces a TypeScript error at the function signature and makes it impossible for callers to satisfy the declared type, breaking builds that run type checking.
Useful? React with 👍 / 👎.
|
Ok, I will stop ask for codex, Now test with cloud VM. We will get ready for Merge this feature! |





In this pr we will Introduce Template Manage to Overleaf-Pro #6 (from CEP)
Todo
Description
Related issues / Pull Requests
Contributor Agreement