You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "kunwp1 (via GitHub)" <gi...@apache.org> on 2026/10/20 19:31:56 UTC
Re: [PR] fix(resource, computing-unit): keep owners' full access to their datasets, models and computing units [texera]
kunwp1 commented on code in PR #8890:
URL: https://github.com/apache/texera/pull/8890#discussion_r4328882193
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitAccessResource.scala:
##########
@@ -139,6 +139,7 @@ class ComputingUnitAccessResource {
computingUnitUserAccessDao
.fetchByCuid(cuid)
.asScala
+ .filterNot(access => isOwner(cuid, access.getUid))
Review Comment:
This method never checks that the caller can see the unit. Any logged-in user can list the collaborators (emails, names, privileges) of any cuid, whereas the dataset and model access lists require read access first. Also, `isOwner` runs a separate query for every row here. Loading the unit once (for both the read check and the owner uid) would fix both.
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitManagingResource.scala:
##########
@@ -884,22 +884,15 @@ class ComputingUnitManagingResource {
statusReason = if (isOwner) statusReason else None,
metrics = ComputingUnitHelpers.getComputingUnitMetrics(unit),
isOwner = isOwner,
- accessPrivilege = {
- val cuAccessDao = new ComputingUnitUserAccessDao(context.configuration())
- val access = cuAccessDao
- .fetchByUid(user.getUid)
- .asScala
- .find(access => access.getCuid.equals(cuid))
-
- if (access.isDefined) {
- access.get.getPrivilege
- } else if (unit.getUid.equals(user.getUid)) {
- PrivilegeEnum.WRITE
- } else {
- // Default privilege for non-owners without explicit access
- PrivilegeEnum.NONE
- }
- },
+ accessPrivilege =
+ if (isOwner) PrivilegeEnum.WRITE
+ else
+ new ComputingUnitUserAccessDao(context.configuration())
+ .fetchByUid(user.getUid)
+ .asScala
+ .find(access => access.getCuid.equals(cuid))
+ .map(_.getPrivilege)
+ .getOrElse(PrivilegeEnum.NONE),
Review Comment:
`ComputingUnitAccess.getComputingUnitAccess(cuid, uid)` in `common/auth` already applies exactly this rule (owner → WRITE, else the grant, else NONE) in a single query, and this service already depends on `Auth`. Reusing it would keep the owner rule in one place for computing units.
##########
computing-unit-managing-service/src/main/scala/org/apache/texera/service/resource/ComputingUnitAccessResource.scala:
##########
@@ -165,6 +166,9 @@ class ComputingUnitAccessResource {
}
val granteeId = resolveUidByEmail(email)
+ if (isOwner(cuid, granteeId)) {
+ throw new ForbiddenException("You cannot modify the owner's permissions!")
+ }
Review Comment:
This guard is repeated in `revokeAccess`, and the message now appears in four places (here, revoke, `ResourceAccess.requireNotOwner`, and `WorkflowAccessResource`). Can you add a small helper?
##########
common/resource/src/main/scala/org/apache/texera/service/resource/ResourceAccess.scala:
##########
@@ -69,32 +69,34 @@ object ResourceAccess {
).flatMap(record => Option(record.value1()))
.contains(uid)
+ /** The owner always has WRITE, whatever their own access row says. */
def privilegeOf[R <: Record, A <: Record](
ctx: DSLContext,
resource: ResourceTables[R, A],
id: Integer,
uid: Integer
): PrivilegeEnum =
- Option(
- ctx
- .select(resource.privilegeField)
- .from(resource.accessTable)
- .where(
- resource.accessIdField
- .eq(id)
- .and(resource.accessUidField.eq(uid))
- )
- .fetchOneInto(classOf[PrivilegeEnum])
- ).getOrElse(PrivilegeEnum.NONE)
+ if (userOwns(ctx, resource, id, uid)) PrivilegeEnum.WRITE
+ else
+ Option(
+ ctx
+ .select(resource.privilegeField)
+ .from(resource.accessTable)
+ .where(
+ resource.accessIdField
+ .eq(id)
+ .and(resource.accessUidField.eq(uid))
+ )
+ .fetchOneInto(classOf[PrivilegeEnum])
+ ).getOrElse(PrivilegeEnum.NONE)
def userHasWriteAccess[R <: Record, A <: Record](
ctx: DSLContext,
resource: ResourceTables[R, A],
id: Integer,
uid: Integer
): Boolean =
- userOwns(ctx, resource, id, uid) ||
- privilegeOf(ctx, resource, id, uid) == PrivilegeEnum.WRITE
+ privilegeOf(ctx, resource, id, uid) == PrivilegeEnum.WRITE
def userHasReadAccess[R <: Record, A <: Record](
Review Comment:
This method runs it twice: once through `userHasWriteAccess` and once directly. Computing the privilege once (`isPublic || privilegeOf != NONE`) would be both simpler and cheaper.
##########
common/resource/src/main/scala/org/apache/texera/service/resource/ResourceAccess.scala:
##########
@@ -286,6 +292,7 @@ object ResourceAccess {
): Response = {
requireWriteAccess(ctx, resource, id, requesterUid)
val granteeUid = new UserDao(ctx.configuration()).fetchOneByEmail(email).getUid
Review Comment:
An unknown email makes `fetchOneByEmail` return null here, so revoke fails with a 500 while `grant` returns a 400 for the same input. Reusing the same check as `grant` would make the two endpoints behave alike.
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/VersionedResourceTables.scala:
##########
@@ -90,13 +90,16 @@ trait VersionedResourceTables[Rec <: Record, P] extends BaseEntityTable with Laz
uid: Integer
): Option[(Integer, DashboardClickableFileEntry)] = {
val resource = pojo(record)
+ val isOwner = ownerUidOf(resource) == uid
repositorySize(resource).map { size =>
idOf(resource) -> entry(
resource,
record.into(USER).into(classOf[User]).getEmail,
- Option(record.get(access.privilegeColumn, classOf[PrivilegeEnum]))
- .getOrElse(PrivilegeEnum.NONE),
- ownerUidOf(resource) == uid,
+ if (isOwner) PrivilegeEnum.WRITE
+ else
+ Option(record.get(access.privilegeColumn, classOf[PrivilegeEnum]))
+ .getOrElse(PrivilegeEnum.NONE),
Review Comment:
Codecov still reports 2 uncovered lines here.
##########
common/resource/src/main/scala/org/apache/texera/service/resource/ResourceAccess.scala:
##########
@@ -69,32 +69,34 @@ object ResourceAccess {
).flatMap(record => Option(record.value1()))
.contains(uid)
+ /** The owner always has WRITE, whatever their own access row says. */
def privilegeOf[R <: Record, A <: Record](
ctx: DSLContext,
resource: ResourceTables[R, A],
id: Integer,
uid: Integer
): PrivilegeEnum =
- Option(
- ctx
- .select(resource.privilegeField)
- .from(resource.accessTable)
- .where(
- resource.accessIdField
- .eq(id)
- .and(resource.accessUidField.eq(uid))
- )
- .fetchOneInto(classOf[PrivilegeEnum])
- ).getOrElse(PrivilegeEnum.NONE)
+ if (userOwns(ctx, resource, id, uid)) PrivilegeEnum.WRITE
+ else
Review Comment:
A tiny shared helper would keep the "owner gets WRITE, otherwise use the grant" rule in one place if it ever changes (e.g. admins).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org