Skip to content

Fix: Document/File set user permission only if not privileged user#11026

Merged
lohanidamodar merged 3 commits into1.8.xfrom
fix-file-permissions
Dec 28, 2025
Merged

Fix: Document/File set user permission only if not privileged user#11026
lohanidamodar merged 3 commits into1.8.xfrom
fix-file-permissions

Conversation

@lohanidamodar
Copy link
Copy Markdown
Member

@lohanidamodar lohanidamodar commented Dec 28, 2025 •
edited
Loading

…ed user

What does this PR do?

  • Set user permission only if not privileged user

Test Plan

  • Add new tests to StorageConsoleClientTest.php

Related PRs and Issues

  • (Related PR or issue)

Checklist

  • Have you read the Contributing Guidelines on issues?
  • If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Dec 28, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

The changes modify permission handling logic across storage and document creation systems. In storage bucket creation and file authorization, the code now checks the isPrivilegedUser flag before automatically granting current-user permissions. Similarly, document creation default permissions now skip assignment for privileged users. A new test validates that file permissions are not automatically set for console uploads.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: preventing automatic user permission assignment for privileged users on documents/files.
Description check ✅ Passed The description is related to the changeset and explains the core change and test additions, though it could be more detailed.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-file-permissions
📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 69b861c and 3a98361.

📒 Files selected for processing (3)
  • app/controllers/api/storage.php
  • src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php
  • tests/e2e/Services/Storage/StorageConsoleClientTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/Services/Storage/StorageConsoleClientTest.php (2)
tests/e2e/Client.php (1)
  • Client (8-342)
src/Appwrite/Utopia/Response.php (1)
  • file (841-848)
⏰ 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). (20)
  • GitHub Check: E2E Service Test (Proxy)
  • GitHub Check: E2E Service Test (Functions)
  • GitHub Check: E2E Service Test (Users)
  • GitHub Check: E2E Service Test (Migrations)
  • GitHub Check: E2E Service Test (Locale)
  • GitHub Check: E2E Service Test (FunctionsSchedule)
  • GitHub Check: E2E Service Test (Teams)
  • GitHub Check: E2E Service Test (Realtime)
  • GitHub Check: E2E Service Test (Webhooks)
  • GitHub Check: E2E Service Test (Databases/Legacy)
  • GitHub Check: E2E Service Test (Messaging)
  • GitHub Check: E2E Service Test (GraphQL)
  • GitHub Check: E2E Service Test (Databases/TablesDB)
  • GitHub Check: E2E Service Test (Account)
  • GitHub Check: E2E Service Test (Console)
  • GitHub Check: E2E General Test
  • GitHub Check: E2E Service Test (Dev Keys)
  • GitHub Check: Unit Test
  • GitHub Check: E2E Service Test (Site Screenshots)
  • GitHub Check: scan
🔇 Additional comments (4)
app/controllers/api/storage.php (2)

462-469: LGTM: Privileged users correctly excluded from auto-permission assignment.

The added !$isPrivilegedUser check ensures that console users and API keys don't receive automatic user-scoped permissions when none are explicitly provided. This is correct behavior since privileged users already have elevated access and don't need individual user permissions.

471-490: LGTM: Consistent use of pre-computed privilege flags.

Good refactoring to use the $isAPIKey and $isPrivilegedUser flags (set at lines 440-441) instead of calling User::isApp() and User::isPrivileged() directly. This improves consistency and ensures the same privilege determination is used throughout the permission logic.

src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)

228-235: LGTM: Document permissions correctly exclude privileged users.

The addition of !$isPrivilegedUser check at line 230 mirrors the storage file permission logic and ensures privileged users (console, API keys) don't receive automatic user-scoped permissions when creating documents. This maintains consistency across the document and storage APIs.

tests/e2e/Services/Storage/StorageConsoleClientTest.php (1)

164-198: LGTM: Well-structured test validates the new behavior.

The new test method effectively verifies that console users (privileged users) don't receive automatic permission assignment when uploading files. The test:

  • Creates a bucket with fileSecurity: true to enable per-file permissions
  • Uploads a file without specifying permissions
  • Asserts that the returned permissions array is empty
  • Properly cleans up resources

The test structure follows existing patterns in the file and provides good coverage for the new behavior.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 28, 2025 •
edited
Loading

Security Scan Results for PR

Docker Image Scan Results

Package Version Vulnerability Severity
libpng 1.6.51-r0 CVE-2025-66293 HIGH
libpng-dev 1.6.51-r0 CVE-2025-66293 HIGH

Source Code Scan Results

🎉 No vulnerabilities found!

@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 28, 2025 •
edited
Loading

✨ Benchmark results

  • Requests per second: 1,178
  • Requests with 200 status code: 212,062
  • P99 latency: 0.163860492

⚡ Benchmark Comparison

Metric This PR Latest version
RPS 1,178 1,256
200 212,062 226,166
P99 0.163860492 0.162039417

@lohanidamodar lohanidamodar marked this pull request as ready for review December 28, 2025 02:08
@lohanidamodar lohanidamodar merged commit 6d02554 into 1.8.x Dec 28, 2025
40 checks passed
@lohanidamodar lohanidamodar deleted the fix-file-permissions branch December 28, 2025 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eldadfux eldadfux eldadfux approved these changes

@stnguyen90 stnguyen90 Awaiting requested review from stnguyen90

@abnegate abnegate Awaiting requested review from abnegate

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lohanidamodar @eldadfux