refactor: refactor admin config to global #1818
Conversation
|
@codex review |
Signed-off-by: allure <[email protected]>
AllureCurtain
force-pushed
the
feat/cedar-admin
branch
from
63985d5 to
f422dd8
Compare
There was a problem hiding this comment.
Pull request overview
This pull request refactors the admin configuration system from a path-based approach (where each root directory had its own .mega_cedar.json file) to a global approach (with a single .mega_cedar.json file at the repository root). This change aligns with issue #1755's goal to unify admin permission handling across the monorepo.
Changes:
- Simplified admin permission checking by removing path-based context
- Consolidated admin configuration into a single global
.mega_cedar.jsonfile at root directory - Streamlined Redis caching with a single global cache key instead of per-directory keys
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| saturn/src/admin_resolver.rs | Updated documentation comment to reflect new global config location at root directory |
| mono/src/api/router/user_router.rs | Removed obsolete test for path traversal logic that's no longer needed |
| mono/src/api/router/admin_router.rs | Removed path query parameters and root_dir response fields from admin API endpoints |
| jupiter/src/utils/converter.rs | Changed initialization to create single global .mega_cedar.json at root with .gitkeep files for subdirectories |
| ceres/src/api_service/mono_api_service.rs | Simplified cache invalidation logic to use global cache key |
| ceres/src/api_service/admin_ops.rs | Refactored to remove path-based logic, simplified EntityStore loading and cache key management |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63985d57a3
ℹ️ 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".
| let blob_item = root_tree | ||
| .tree_items | ||
| .iter() | ||
| .find(|item| item.name == ADMIN_FILE) | ||
| .ok_or_else(|| { |
There was a problem hiding this comment.
This lookup now only searches the root tree for /.mega_cedar.json, so repositories created before this change (which only wrote .mega_cedar.json into each root directory) will fail admin checks with a “not found in root directory” error until a migration adds the new root file. That means /api/v1/admin/me and /api/v1/admin/list will start returning 500s after upgrade for existing repos. Consider a backward-compatible fallback (e.g., probe legacy locations when root lookup fails) or ensure a migration creates the root file before enabling this code path.
Useful? React with 👍 / 👎.
| #[derive(Serialize, ToSchema)] | ||
| pub struct IsAdminResponse { | ||
| pub is_admin: bool, | ||
| pub root_dir: String, | ||
| } | ||
|
|
||
| #[derive(Serialize, ToSchema)] | ||
| pub struct AdminListResponse { | ||
| pub admins: Vec<String>, | ||
| pub root_dir: String, | ||
| } |
There was a problem hiding this comment.
Removing the root_dir field from IsAdminResponse and AdminListResponse is a breaking API change. This will cause clients relying on this field to fail. Consider versioning the API endpoint or providing a migration path for existing clients.
| async fn is_admin_me( | ||
| user: LoginUser, | ||
| Query(params): Query<AdminQueryParams>, | ||
| State(state): State<MonoApiServiceState>, | ||
| ) -> Result<Json<CommonResult<IsAdminResponse>>, ApiError> { |
There was a problem hiding this comment.
The removal of the path query parameter from the admin endpoints is a breaking API change. Existing API consumers that pass this parameter will need to update their code. This should be documented in release notes or migration guides.
| } | ||
| let admin_file_modified = files.iter().any(|file| { | ||
| let normalized = file.replace('\\', "/"); | ||
| normalized.ends_with(crate::api_service::admin_ops::ADMIN_FILE) |
There was a problem hiding this comment.
The cache invalidation logic uses ends_with to check for .mega_cedar.json modifications. However, this will match any file ending with .mega_cedar.json regardless of its location in the tree. Since the admin file is now specifically at the root (/.mega_cedar.json), consider checking for the exact path or ensuring the normalized path equals .mega_cedar.json to avoid false positives from files like foo/.mega_cedar.json in subdirectories.
| normalized.ends_with(crate::api_service::admin_ops::ADMIN_FILE) | |
| let admin_file_path = crate::api_service::admin_ops::ADMIN_FILE; | |
| normalized == admin_file_path || normalized == format!("/{}", admin_file_path) |
| let mut trees = Vec::new(); | ||
| let mut blobs = Vec::new(); | ||
|
|
||
| // Create unique .gitkeep for each root directory to ensure different tree hashes |
There was a problem hiding this comment.
The comment describes the tree item as .gitkeep, but the actual blob content format "Placeholder file for /{} directory" is specific to each root directory. While this is intentional to ensure different tree hashes (as noted in the comment on line 421), consider making the comment more explicit about why unique content is needed, mentioning that identical content would result in identical tree hashes which could cause issues.
| // Create unique .gitkeep for each root directory to ensure different tree hashes | |
| // Create a .gitkeep for each root directory with directory-specific content. | |
| // This ensures that each root gets a distinct blob ID and thus a distinct tree hash. | |
| // If all .gitkeep files had identical content, their blobs (and the trees containing | |
| // them) would be identical, which could cause different roots to share the same | |
| // tree hash and lead to confusing or incorrect behavior when working with Git trees. |
cbcd638
3 participants
link #1755