bobbai00 commented on code in PR #3598:
URL: https://github.com/apache/texera/pull/3598#discussion_r2311640746
##########
core/amber/src/main/scala/edu/uci/ics/texera/web/ServletAwareConfigurator.scala:
##########
@@ -29,46 +31,78 @@ import java.nio.charset.Charset
import javax.websocket.HandshakeResponse
import javax.websocket.server.{HandshakeRequest, ServerEndpointConfig}
import scala.jdk.CollectionConverters.ListHasAsScala
+import scala.jdk.CollectionConverters._
/**
- * This configurator extracts HTTPSession and associates it to
ServerEndpointConfig,
- * allow it to be accessed by Websocket connections.
- * <pre>
- * See <a
href="https://stackoverflow.com/questions/17936440/accessing-httpsession-
- * from-httpservletrequest-in-a-web-socket-serverendpoint"></a>
- * </pre>
- */
+ * This configurator extracts HTTPSession and associates it to
ServerEndpointConfig,
+ * allow it to be accessed by Websocket connections.
+ * <pre>
+ * See <a
href="https://stackoverflow.com/questions/17936440/accessing-httpsession-
+ * from-httpservletrequest-in-a-web-socket-serverendpoint"></a>
+ * </pre>
+ */
class ServletAwareConfigurator extends ServerEndpointConfig.Configurator with
LazyLogging {
override def modifyHandshake(
- config: ServerEndpointConfig,
- request: HandshakeRequest,
- response: HandshakeResponse
- ): Unit = {
+ config: ServerEndpointConfig,
+ request: HandshakeRequest,
+ response: HandshakeResponse
+ ): Unit = {
try {
- val params =
- URLEncodedUtils.parse(new URI("?" + request.getQueryString),
Charset.defaultCharset())
- params.asScala
- .map(pair => pair.getName -> pair.getValue)
- .toMap
- .get("access-token")
- .map(token => {
- val claims = jwtConsumer.process(token).getJwtClaims
- config.getUserProperties.put(
- classOf[User].getName,
- new User(
- claims.getClaimValue("userId").asInstanceOf[Long].toInt,
- claims.getSubject,
-
String.valueOf(claims.getClaimValue("email").asInstanceOf[String]),
- null,
- null,
- null,
- null,
- null
- )
+ val headers =
request.getHeaders.asScala.view.mapValues(_.asScala.headOption).toMap
+ if (headers.contains(HeaderField.UserComputingUnitAccess)) {
+ // KUBERNETES MODE: Construct the User object from trusted headers
Review Comment:
I think here you should check if HeaderField.UserId & HeaderField.UserName &
HeaderField.UserComputingUnitAccess & HeaderField.UserComputingUnitAccess all
exist
##########
core/gui/src/app/workspace/component/menu/menu.component.ts:
##########
@@ -720,4 +733,6 @@ export class MenuComponent implements OnInit, OnDestroy {
this.config.env.workflowEmailNotificationEnabled &&
this.config.env.userSystemEnabled
);
}
+
+ protected readonly Privilege = Privilege;
Review Comment:
How is this variable used?
##########
core/amber/src/main/scala/edu/uci/ics/texera/web/resource/WorkflowWebsocketResource.scala:
##########
@@ -94,6 +102,9 @@ class WorkflowWebsocketResource extends LazyLogging {
sessionState.send(modifyLogicResponse)
}
case workflowExecuteRequest: WorkflowExecuteRequest =>
+ if (KubernetesConfig.kubernetesComputingUnitEnabled &&
sessionState.getUserComputingUnitAccess != PrivilegeEnum.WRITE) {
Review Comment:
Ditto. Please remove the `KubernetesConfig.kubernetesComputingUnitEnabled`.
##########
core/amber/src/main/scala/edu/uci/ics/texera/web/ServletAwareConfigurator.scala:
##########
@@ -29,46 +31,78 @@ import java.nio.charset.Charset
import javax.websocket.HandshakeResponse
import javax.websocket.server.{HandshakeRequest, ServerEndpointConfig}
import scala.jdk.CollectionConverters.ListHasAsScala
+import scala.jdk.CollectionConverters._
/**
- * This configurator extracts HTTPSession and associates it to
ServerEndpointConfig,
- * allow it to be accessed by Websocket connections.
- * <pre>
- * See <a
href="https://stackoverflow.com/questions/17936440/accessing-httpsession-
- * from-httpservletrequest-in-a-web-socket-serverendpoint"></a>
- * </pre>
- */
+ * This configurator extracts HTTPSession and associates it to
ServerEndpointConfig,
+ * allow it to be accessed by Websocket connections.
+ * <pre>
+ * See <a
href="https://stackoverflow.com/questions/17936440/accessing-httpsession-
+ * from-httpservletrequest-in-a-web-socket-serverendpoint"></a>
+ * </pre>
+ */
class ServletAwareConfigurator extends ServerEndpointConfig.Configurator with
LazyLogging {
override def modifyHandshake(
- config: ServerEndpointConfig,
- request: HandshakeRequest,
- response: HandshakeResponse
- ): Unit = {
+ config: ServerEndpointConfig,
+ request: HandshakeRequest,
+ response: HandshakeResponse
+ ): Unit = {
try {
- val params =
- URLEncodedUtils.parse(new URI("?" + request.getQueryString),
Charset.defaultCharset())
- params.asScala
- .map(pair => pair.getName -> pair.getValue)
- .toMap
- .get("access-token")
- .map(token => {
- val claims = jwtConsumer.process(token).getJwtClaims
- config.getUserProperties.put(
- classOf[User].getName,
- new User(
- claims.getClaimValue("userId").asInstanceOf[Long].toInt,
- claims.getSubject,
-
String.valueOf(claims.getClaimValue("email").asInstanceOf[String]),
- null,
- null,
- null,
- null,
- null
- )
+ val headers =
request.getHeaders.asScala.view.mapValues(_.asScala.headOption).toMap
+ if (headers.contains(HeaderField.UserComputingUnitAccess)) {
+ // KUBERNETES MODE: Construct the User object from trusted headers
Review Comment:
Change the comment to be something like: The user is already being
authorized by the `AccessControlService`
##########
core/computing-unit-managing-service/src/main/scala/edu/uci/ics/texera/service/resource/ComputingUnitManagingResource.scala:
##########
@@ -96,7 +90,8 @@ object ComputingUnitManagingResource {
.get,
EnvironmentalVariable.ENV_MAX_WORKFLOW_WEBSOCKET_REQUEST_PAYLOAD_SIZE_KB
-> EnvironmentalVariable
.get(EnvironmentalVariable.ENV_MAX_WORKFLOW_WEBSOCKET_REQUEST_PAYLOAD_SIZE_KB)
- .get
+ .get,
+ EnvironmentalVariable.ENV_KUBERNETES_COMPUTING_UNIT_ENABLED ->
KubernetesConfig.kubernetesComputingUnitEnabled,
Review Comment:
This should be removed after the above changes
##########
core/amber/src/main/scala/edu/uci/ics/texera/web/resource/WorkflowWebsocketResource.scala:
##########
@@ -51,13 +54,18 @@ class WorkflowWebsocketResource extends LazyLogging {
SessionState.setState(session.getId, sessionState)
val wid = session.getRequestParameterMap.get("wid").get(0).toLong
val cuid = session.getRequestParameterMap.get("cuid").get(0).toInt
+ var cuAccessEnum: PrivilegeEnum = null
+ if (KubernetesConfig.kubernetesComputingUnitEnabled) {
Review Comment:
can you change this logic to be:
- remove the if (KubernetesConfig.kubernetesComputingUnitEnabled)
-
PrivilegeEnum.valueOf(session.getUserProperties.getOrElse(HeaderField.UserComputingUnitAccess,
{// call the getComputingUnitAccess to query the database for the cu access}).
KubernetesConfig.kubernetesComputingUnitEnabled this flag should just be a
switch of having k8s pod as CU or no. It should NOT be used to determine the
way of verifying CU access.
##########
core/gui/src/app/workspace/component/menu/menu.component.ts:
##########
@@ -202,6 +206,15 @@ export class MenuComponent implements OnInit, OnDestroy {
this.computingUnitStatusSubscription.unsubscribe();
}
+ private subscribeToComputingUnitSelection(): void {
+ this.computingUnitStatusService
+ .getSelectedComputingUnit()
+ .pipe(untilDestroyed(this))
+ .subscribe(unit => {
+ this.selectedComputingUnit = unit;
+ });
+ }
+
Review Comment:
why these changes are needed?
--
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]
