fix(project): correct access privilege lookup#5156
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5156 +/- ##
============================================
+ Coverage 43.39% 43.42% +0.02%
+ Complexity 2215 2214 -1
============================================
Files 1049 1049
Lines 40567 40567
Branches 4321 4321
============================================
+ Hits 17606 17617 +11
+ Misses 21871 21859 -12
- Partials 1090 1091 +1
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thank you for contributing. Please use the |
|
/request-review @Yicong-Huang |
|
I remember someone told me projects feature was deprecated, maybe @mengw15 can confirm? |
|
even if the project is deprecated, we still need to maintain the codebase. unless we decide to remove project completely, then we should remove the codebase. |
|
In that case, I would say in case it is indeed deprecated, lets discuss the removal of that feature before reviewing this PR, is that ok? |
I feel those two are orthogonal, I hope to encourage @fallintoplace to finish this PR, regardless if we decided to drop support of project eventually or not. Let's discuss removal separately, which is a much larger scope. |
|
Got it, I will review it. Thanks. |
| assert(!ProjectAccessResource.userHasWriteAccess(project.getPid, readerUid)) | ||
| } | ||
|
|
||
| it should "return NONE if the user has no project access row" in { |
There was a problem hiding this comment.
Could we make this negative test cover the (pid, uid) bug more directly?
Right now noAccessUid has no rows in PROJECT_USER_ACCESS, so the test only proves that a missing user row returns NONE. Since this PR specifically fixes lookup by both pid and uid, please add or adjust this case so the user has access to a different project, then assert that lookup on this project still returns NONE.
For example: create two projects, grant readerUid READ on project A only, then assert getProjectAccessPrivilege(projectB.getPid, readerUid) == PrivilegeEnum.NONE. That would catch a regression where the query accidentally filters only by uid and ignores pid.
There was a problem hiding this comment.
Thanks, updated the negative case to cover this more directly.
The test now creates two projects, grants readerUid READ access only on the first project, and asserts that getProjectAccessPrivilege returns NONE for the second project with the same readerUid. This should catch a regression where the lookup filters by uid but not pid.
fallintoplace
force-pushed
the
fix/project-access-lookup
branch
from
f7188d3 to
a159bf7
Compare
|
LGTM |
6 participants
What changes were proposed in this PR?
Fixes the project access privilege lookup to query
PROJECT_USER_ACCESSby(pid, uid)instead of mixing workflow/dataset access tables. Adds regression coverage for WRITE, READ, and NONE project access rows.Any related issues, documentation, discussions?
Closes #5155
How was this PR tested?
git diff --checkgit diff --cached --checkJAVA_HOME=/opt/homebrew/opt/openjdk@17/libexec/openjdk.jdk/Contents/Home PATH=/opt/homebrew/opt/openjdk@17/bin:$PATH /tmp/texera-sbt "WorkflowExecutionService / Test / testOnly org.apache.texera.web.resource.dashboard.user.project.ProjectAccessResourceSpec"FATAL: password authentication failed for user "postgres", causing generated DAO classes to be unavailable.Was this PR authored or co-authored using generative AI tooling?
No