Fix permisson error in listGrouped in list datasets route #8249
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning Rate limit exceeded@MichaelBuessemeyer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/controllers/DatasetController.scala (1)
239-241: Document the access control patternConsider adding documentation to explain the access control pattern, particularly why
GlobalAccessContextis used for organization lookup while maintaining regular access control for datasets.Add a comment like:
private def listGrouped(datasets: List[Dataset], requestingUser: Option[User])( implicit ctx: DBAccessContext, m: MessagesProvider): Fox[List[JsObject]] = + // Note: GlobalAccessContext is used for organization lookup as we need to access + // organization information for public datasets, even when the user doesn't have + // direct access to the organization. Dataset access is still controlled via the + // regular context. for {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/controllers/DatasetController.scala(1 hunks)
🔇 Additional comments (3)
app/controllers/DatasetController.scala (3)
240-240: LGTM! Using GlobalAccessContext for organization lookup
The change to use GlobalAccessContext for organization lookup is correct and addresses the permission error when accessing organizations associated with public datasets.
Line range hint 312-313: LGTM! Consistent use of GlobalAccessContext
The use of GlobalAccessContext for both organization and dataStore lookups is consistent with the fix pattern and maintains proper access control at the dataset level.
Line range hint 240-313: Verify consistent usage of GlobalAccessContext
Let's verify that the GlobalAccessContext pattern is consistently applied across the codebase for organization and datastore access.
✅ Verification successful
GlobalAccessContext usage is consistent across organization and datastore access
The verification results show that GlobalAccessContext is consistently used across the codebase for both organizationDAO.findOne and dataStoreDAO.findOne operations. The pattern in the reviewed code matches the global pattern, where:
- All organization lookups use
organizationDAO.findOne(...)(GlobalAccessContext) - All datastore lookups use
dataStoreDAO.findOne[ByName](...)(GlobalAccessContext)
This consistent usage indicates that the code under review follows the established pattern for accessing these resources.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for organization and datastore access patterns
echo "Checking organization access patterns..."
rg "organizationDAO\.findOne.*GlobalAccessContext" -A 2
echo "Checking datastore access patterns..."
rg "dataStoreDAO\.findOne.*GlobalAccessContext" -A 2
Length of output: 13376
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/controllers/DatasetController.scala (2)
Line range hint
15-17: Fix indentation in elif block.The indentation is incorrect and inconsistent with Scala style guidelines. The
elifblock should be aligned with theifstatement.Apply this diff to fix the indentation:
if loyalty_years >= 5: discount = 0.15 - elif loyalty_years >= 3: - discount = 0.1 + elif loyalty_years >= 3: + discount = 0.1
Line range hint
12-24: Review the discount and fee structure.The current implementation could result in customers paying more after a discount due to the flat $20 fee. For example:
- Purchase of $100 with 10% discount = $90
- After $20 fee = $110 (more than original price)
Consider one of these alternatives:
- Apply the fee before the discount
- Scale the fee based on the purchase amount
- Only apply the fee above a certain purchase threshold
Would you like me to propose a revised implementation that addresses these concerns?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/controllers/DatasetController.scala(1 hunks)
🔇 Additional comments (2)
app/controllers/DatasetController.scala (2)
226-226: LGTM! This change correctly fixes the permission error.
The use of GlobalAccessContext here is appropriate as it allows fetching organization metadata for public datasets without requiring organization-level permissions, preventing the crash described in the PR objectives.
226-226: Consistent use of GlobalAccessContext across methods.
The same fix has been correctly applied to both listGrouped and read methods, ensuring consistent handling of organization metadata access throughout the codebase.
Also applies to: 264-264
This pr fixes permission errors for listing datasets without
compact=true. The error results from the access query including all public datasets and the code that groups the datasets by organization before serialization to json requesting the organization without a global access context. This causes the code not to find organizations that are included in the list due to public DS in their organizations. This makes the code crash.For reference: Here the DSes are retrieved (including public DS):
webknossos/app/controllers/DatasetController.scala
Lines 189 to 199 in 24e4981
And here the orga of each DS (they are grouped by orga id at this point) is accessed:
webknossos/app/controllers/DatasetController.scala
Lines 239 to 241 in 24e4981
=> This request fails if the previous ds listing includes datasets which are public and of an organization the user has no access to.
To fix this, I suggest making this request in a
GlobalAccessContextas the previous DS listing takes care of only loading DS the user is allowed to see and the serialization of the DS does not include any sensitive information of the organization.URL of deployed dev instance (used for testing):
Steps to test:
<wk-url>/api/datasetsroute worksIssues:
(Please delete unneeded items, merge only when none are left open)