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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
