Allow admins (site or org) to download projects by code#1829
Conversation
Site admins and org admins can download arbitrary projects without needing to be a member (projects owned by their org, or any project at all in the case of site admins), so let's return a boolean flag for the frontend to enable UI for that. The UI will still need to handle "Hey, you're not actually authorized to download that project" because it will be used by org admins, not only by site admins who have all access.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change introduces the ability for admins to download any FW Lite project by entering a project code, regardless of membership, with appropriate error handling for invalid or non-CRDT projects. It adds new API endpoints, result types, permissions checks, and UI elements to support this workflow, spanning backend and frontend. Changes
Estimated code review effort3 (~45 minutes) Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
Bot
added
💻 FW Lite
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
Dialog has no content yet
Instead of downloading a CRDT project with no commits, which leads to errors in FW Lite when it can't find a default writing system, we'll check first whether the Lexbox project is a CRDT project, and refuse to download it if it isn't. Currently this just displays an error notification, but soon we'll display this in the dialog box instead.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Errors now show in dialog, dialog doesn't close until download completed, dialog doesn't close if any errors.
Copilot AI suggests that projectCode comes from user input and could therefore contain newlines. That's not really likely given how the input dialog is written, but it's theoretically possible. So to avoid newlines in the middle of a log message, which could confuse some logging software, we'll make this extra, not really needed, check.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
backend/FwLite/FwLiteShared/Projects/LexboxProjectService.cs (1)
86-86: Sanitize user input in log messages to prevent log injection.The
projectCodeparameter comes from user input and should be sanitized before logging to prevent log injection attacks.Consider using structured logging with parameters:
-logger.LogError(e, $"Error checking if lexbox project {projectCode} is a CRDT project"); +logger.LogError(e, "Error checking if lexbox project {ProjectCode} is a CRDT project", projectCode);
🧹 Nitpick comments (2)
backend/LexCore/Entities/ListProjectsResult.cs (1)
3-3: Address the TODO comment regarding record placement.Consider moving this record to a more appropriate location that can be easily referenced from
FwLiteSharedto avoid cross-assembly reference issues. Perhaps a shared contracts or models assembly would be more suitable.backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs (1)
40-41: Address TODO comments during code review.There are two TODO comments that need team discussion:
- Line 40-41: Bikeshed the
CanDownloadProjectsWithoutMembershipproperty name- Line 94-96: Consider returning a proper object type instead of nullable
ServerProjectsWould you like me to suggest alternative names for the property or help design a better return type structure?
Also applies to: 94-96
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs(7 hunks)backend/FwLite/FwLiteShared/Projects/LexboxProjectService.cs(1 hunks)backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs(1 hunks)backend/FwLite/FwLiteWeb/Routes/ProjectRoutes.cs(1 hunks)backend/LexBoxApi/Controllers/CrdtController.cs(2 hunks)backend/LexBoxApi/GraphQL/LexAuthUserCanDownloadProjectsExtensions.cs(1 hunks)backend/LexBoxApi/Services/ProjectService.cs(1 hunks)backend/LexCore/Entities/ListProjectsResult.cs(1 hunks)frontend/viewer/src/home/Server.svelte(6 hunks)frontend/viewer/src/home/ServersList.svelte(3 hunks)frontend/viewer/src/lib/admin-dialogs/GetProjectByCodeDialog.svelte(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/DownloadProjectByCodeResult.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/ICombinedProjectsService.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/IServerProjects.ts(1 hunks)frontend/viewer/src/lib/services/projects-service.ts(2 hunks)
🧠 Learnings (6)
📓 Common learnings
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1753
File: backend/FwHeadless/Services/SyncHostedService.cs:212-235
Timestamp: 2025-06-16T04:10:03.163Z
Learning: In the FwHeadless project, using hard-coded "crdt" strings for CRDT project names is acceptable and doesn't cause issues with project name collisions in their current architecture, though the team acknowledges that using constants instead of magic strings would be a better practice.
backend/LexBoxApi/Services/ProjectService.cs (1)
Learnt from: hahn-kev
PR: #1804
File: backend/FwLite/LcmCrdt/CurrentProjectService.cs:18-19
Timestamp: 2025-07-07T06:02:41.194Z
Learning: In the CurrentProjectService class, the service locator pattern is intentionally used to retrieve IDbContextFactory and EntrySearchServiceFactory because these services indirectly depend on CurrentProjectService to have the current project set, creating a circular dependency. This is an acceptable use of service locator to break the circular dependency while keeping project context responsibility consolidated.
backend/LexBoxApi/Controllers/CrdtController.cs (1)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
frontend/viewer/src/lib/admin-dialogs/GetProjectByCodeDialog.svelte (1)
Learnt from: hahn-kev
PR: #1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
frontend/viewer/src/home/Server.svelte (1)
Learnt from: hahn-kev
PR: #1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
backend/FwLite/FwLiteShared/Projects/LexboxProjectService.cs (1)
Learnt from: hahn-kev
PR: #1753
File: backend/FwHeadless/Services/SyncHostedService.cs:212-235
Timestamp: 2025-06-16T04:10:03.163Z
Learning: In the FwHeadless project, using hard-coded "crdt" strings for CRDT project names is acceptable and doesn't cause issues with project name collisions in their current architecture, though the team acknowledges that using constants instead of magic strings would be a better practice.
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1753
File: backend/FwHeadless/Services/SyncHostedService.cs:212-235
Timestamp: 2025-06-16T04:10:03.163Z
Learning: In the FwHeadless project, using hard-coded "crdt" strings for CRDT project names is acceptable and doesn't cause issues with project name collisions in their current architecture, though the team acknowledges that using constants instead of magic strings would be a better practice.
backend/LexBoxApi/Services/ProjectService.cs (1)
Learnt from: hahn-kev
PR: #1804
File: backend/FwLite/LcmCrdt/CurrentProjectService.cs:18-19
Timestamp: 2025-07-07T06:02:41.194Z
Learning: In the CurrentProjectService class, the service locator pattern is intentionally used to retrieve IDbContextFactory and EntrySearchServiceFactory because these services indirectly depend on CurrentProjectService to have the current project set, creating a circular dependency. This is an acceptable use of service locator to break the circular dependency while keeping project context responsibility consolidated.
backend/LexBoxApi/Controllers/CrdtController.cs (1)
Learnt from: hahn-kev
PR: #1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
frontend/viewer/src/lib/admin-dialogs/GetProjectByCodeDialog.svelte (1)
Learnt from: hahn-kev
PR: #1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
frontend/viewer/src/home/Server.svelte (1)
Learnt from: hahn-kev
PR: #1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
backend/FwLite/FwLiteShared/Projects/LexboxProjectService.cs (1)
Learnt from: hahn-kev
PR: #1753
File: backend/FwHeadless/Services/SyncHostedService.cs:212-235
Timestamp: 2025-06-16T04:10:03.163Z
Learning: In the FwHeadless project, using hard-coded "crdt" strings for CRDT project names is acceptable and doesn't cause issues with project name collisions in their current architecture, though the team acknowledges that using constants instead of magic strings would be a better practice.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build UI / publish-ui
- GitHub Check: Build FwHeadless / publish-fw-headless
- GitHub Check: check-and-lint
- GitHub Check: Analyze (csharp)
- GitHub Check: Build FW Lite and run tests
🔇 Additional comments (17)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/IServerProjects.ts (1)
13-13: LGTM! Clean interface extension.The new boolean property correctly follows TypeScript naming conventions and logically extends the interface to support admin download permissions.
backend/LexBoxApi/Services/ProjectService.cs (1)
368-371: LGTM! Clean CRDT project verification method.The method correctly implements CRDT project detection using the existing
CrdtCommitsextension. The synchronous approach is appropriate for this simple existence check.backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (1)
134-134: LGTM! Consistent enum export configuration.The new enum export follows the established pattern and correctly uses string literals for TypeScript consumption.
backend/LexBoxApi/GraphQL/LexAuthUserCanDownloadProjectsExtensions.cs (1)
8-13: LGTM! Clear permission logic with broad admin access.The extension method correctly implements the admin permission check. Note that organization admins can download any project by code (not just their org's projects), which aligns with the PR description that the frontend will handle authorization errors for unauthorized access.
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/DownloadProjectByCodeResult.ts (1)
6-10: LGTM! Well-defined result enum.The enum clearly defines the three possible outcomes for project download operations with descriptive names that align with the expected backend behavior.
backend/LexCore/Entities/ListProjectsResult.cs (1)
5-7: LGTM - Clean record definition.The record structure is appropriate for encapsulating project listing results with the membership permission flag, aligning well with the PR's admin download functionality.
backend/LexBoxApi/Controllers/CrdtController.cs (2)
78-94: LGTM - Proper wrapping of existing functionality.The change to return
ListProjectsResultmaintains the existing logic while adding the permission flag through the extension method. The user refresh logic is preserved correctly.
96-105: LGTM - Well-implemented CRDT check endpoint.The endpoint properly:
- Validates view permissions
- Handles project lookup with appropriate 404 response
- Returns boolean CRDT status with correct HTTP status codes
backend/FwLite/FwLiteWeb/Routes/ProjectRoutes.cs (1)
52-63: LGTM - Proper integration of role parameter.The route correctly:
- Adds the optional
rolequery parameter with proper nullable typing- Passes all required parameters to the renamed service method
- Maintains the existing endpoint behavior while extending functionality
frontend/viewer/src/lib/services/projects-service.ts (3)
1-2: LGTM - Proper import additions.The imports correctly add the required types for the enhanced functionality.
10-12: LGTM - Correct return type update.The method signature properly reflects the backend change from returning a simple array to the richer
IServerProjectsstructure.
22-24: LGTM - Proper method signature for new functionality.The method signature correctly includes all required parameters (
code,server,userRole) and returns the appropriate result type for the download by code functionality.frontend/viewer/src/home/ServersList.svelte (4)
2-2: LGTM - Appropriate type import addition.The import correctly adds the
IServerProjectstype needed for the enhanced data structure.
13-13: LGTM - Correct state type update.The state type properly reflects the backend change to return richer server project metadata.
20-22: LGTM - Proper data structure handling.The code correctly stores the full
serverProjectsobject rather than just the projects array, enabling access to the additional metadata.
60-64: LGTM - Correct property access and flag passing.The template properly:
- Accesses the nested
projectsarray with appropriate filtering- Extracts the
canDownloadProjectsWithoutMembershipflag- Passes the flag to the
Servercomponent ascanDowloadByCodefrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Projects/ICombinedProjectsService.ts (1)
1-23: Skip review of generated file.This file is automatically generated by Reinforced.Typings tool. Any manual changes would be lost on regeneration.
rmunn
changed the title
…wnload-any-crdt-project Fixes merge conflict in ReinforcedFwLiteTypingConfig.cs
|
@hahn-kev - The new API ... I imagine the answer is that we shouldn't change Edit: Just had another idea. We could let |
|
@rmunn you're right, let's just modify |
Since lookupProjectId is not used by anything else, we can just make that the endpoint name. It now looks up project ID and also verifies necessary permissions to download CRDT projects, all in one call.
hahn-kev
left a comment
hahn-kev
left a comment
There was a problem hiding this comment.
left some comments I'd like dealt with before we merge.
I also made a couple tweaks myself, fixed the dialog height issue.
Now returns appropriate HTTP error codes for 403, 404, etc.
hahn-kev
left a comment
hahn-kev
left a comment
There was a problem hiding this comment.
Looks good to me, hopefully those merge conflicts aren't too big
Resolves merge conflicts
Fix merge conflict again
3 participants
Fixes #1707.
Backend now gives frontend a project list for each server and whether the user is allowed to download arbitrary projects (by project code) from that server without being a member. This applies to site admins (who have rights to all projects) and org admins (who have rights to projects owned by their org). The latter case means the frontend still needs to handle "Well, turns out you're not authorized to download that project code" errors.
Screenshot when you aren't a member of any projects on Lexbox:
Screenshot when you are a member of some projects on Lexbox, but can also download any project by code:
Screenshot of download dialog (TODO: Tighten up that unneeded vertical space a little):
Screenshot of download dialog when you try to download a project you already have:
Screenshot of download dialog when you try to download a project that doesn't exist on the server: