Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RW-12581 Remove user enabled check when redundant #4534

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

RW-12581 Remove user enabled check when redundant #4534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb5925a
more
Qi77Qi May 13, 2024
948c845
improve where we check user enabled
Qi77Qi May 13, 2024
3ef3060
Merge branch 'develop' into listDisks
Qi77Qi May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions ...ain/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterp.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,13 +202,22 @@
ctx <- as.ask

// throw 403 if user doesn't have project permission
hasProjectPermission <- cloudContext.traverse(cc =>
authProvider.isUserProjectReader(
cc,
userInfo
)
)
_ <- F.raiseWhen(!hasProjectPermission.getOrElse(true))(ForbiddenError(userInfo.userEmail, Some(ctx.traceId)))
_ <- cloudContext match {
case Some(cc) =>
for {
hasProjectPermission <- authProvider.isUserProjectReader(
cc,
userInfo
)
_ <- F.raiseWhen(!hasProjectPermission)(
ForbiddenError(userInfo.userEmail, Some(ctx.traceId))

Check warning on line 213 in http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterp.scala

View check run for this annotation

Codecov / codecov/patch

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/DiskServiceInterp.scala#L213 

Added line #L213 was not covered by tests
)
} yield ()
case None =>
authProvider.checkUserEnabled(
userInfo
) // when request doesn't have cloudContext defined, we check if user is enabled
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving up the check if cloudContext is not defined; otherwise, remove this redundant check


paramMap <- F.fromEither(processListParameters(params))
creatorOnly <- F.fromEither(processCreatorOnlyParameter(userInfo.userEmail, params, ctx.traceId))
Expand Down Expand Up @@ -262,9 +271,6 @@
)
.toVector
}
// We authenticate actions on resources. If there are no visible disks,
// we need to check if user should be able to see the empty list.
_ <- if (res.isEmpty) authProvider.checkUserEnabled(userInfo) else F.unit
} yield res

override def deleteDisk(userInfo: UserInfo, googleProject: GoogleProject, diskName: DiskName)(implicit
Expand Down
31 changes: 18 additions & 13 deletions .../scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterp.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,19 +268,24 @@
for {
ctx <- as.ask

// Authorize: user has an active account and has accepted terms of service
_ <- authProvider.checkUserEnabled(userInfo)

// throw 403 if user doesn't have project permission
hasProjectPermission <- cloudContext.traverse(cc =>
authProvider.isUserProjectReader(
cc,
userInfo
)
)
_ <- ctx.span.traverse(s => F.delay(s.addAnnotation("Done checking project permission with Sam")))

_ <- F.raiseWhen(!hasProjectPermission.getOrElse(true))(ForbiddenError(userInfo.userEmail, Some(ctx.traceId)))
_ <- cloudContext match {
case Some(cc) =>
// throw 403 if user doesn't have project permission
for {
hasProjectPermission <- authProvider.isUserProjectReader(
cc,
userInfo
)
_ <- ctx.span.traverse(s => F.delay(s.addAnnotation("Done checking project permission with Sam")))
_ <- F.raiseWhen(!hasProjectPermission)(
ForbiddenError(userInfo.userEmail, Some(ctx.traceId))

Check warning on line 281 in http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterp.scala

View check run for this annotation

Codecov / codecov/patch

http/src/main/scala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeServiceInterp.scala#L281 

Added line #L281 was not covered by tests
)
} yield ()
case None =>
authProvider.checkUserEnabled(
userInfo
) // when request doesn't have cloudContext defined, we check if user is enabled
}

(labelMap, includeDeleted, _) <- F.fromEither(processListParameters(params))
excludeStatuses = if (includeDeleted) List.empty else List(RuntimeStatus.Deleted)
Expand Down
11 changes: 5 additions & 6 deletions ...cala/org/broadinstitute/dsde/workbench/leonardo/http/service/RuntimeV2ServiceInterp.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,12 +778,11 @@ class RuntimeV2ServiceInterp[F[_]: Parallel](

}

final case class AuthorizedIds(
val ownerGoogleProjectIds: Set[ProjectSamResourceId],
val ownerWorkspaceIds: Set[WorkspaceResourceSamResourceId],
val readerGoogleProjectIds: Set[ProjectSamResourceId],
val readerRuntimeIds: Set[SamResourceId],
val readerWorkspaceIds: Set[WorkspaceResourceSamResourceId]
final case class AuthorizedIds(ownerGoogleProjectIds: Set[ProjectSamResourceId],
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these vals are redundant

ownerWorkspaceIds: Set[WorkspaceResourceSamResourceId],
readerGoogleProjectIds: Set[ProjectSamResourceId],
readerRuntimeIds: Set[SamResourceId],
readerWorkspaceIds: Set[WorkspaceResourceSamResourceId]
)

final case class WorkspaceNotFoundException(workspaceId: WorkspaceId, traceId: TraceId)
Expand Down
Loading