vicennial commented on code in PR #49880:
URL: https://github.com/apache/spark/pull/49880#discussion_r1950851809
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectAddArtifactsHandler.scala:
##########
@@ -52,6 +53,8 @@ class SparkConnectAddArtifactsHandler(val responseObserver:
StreamObserver[AddAr
private var holder: SessionHolder = _
override def onNext(req: AddArtifactsRequest): Unit = try {
+ ConnectCommon.CONNECT_LOCAL_AUTH_TOKEN.foreach(k =>
Review Comment:
Nit: would prefer this check to be centralised in some companion object
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectAddArtifactsHandler.scala:
##########
@@ -52,6 +53,8 @@ class SparkConnectAddArtifactsHandler(val responseObserver:
StreamObserver[AddAr
private var holder: SessionHolder = _
override def onNext(req: AddArtifactsRequest): Unit = try {
+ ConnectCommon.CONNECT_LOCAL_AUTH_TOKEN.foreach(k =>
+ assert(k == req.getUserContext.getLocalAuthToken))
Review Comment:
Do we want to throw a more descriptive message in these cases of auth
failures?
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectService.scala:
##########
@@ -366,6 +385,8 @@ object SparkConnectService extends Logging {
val debugMode =
SparkEnv.get.conf.getBoolean("spark.connect.grpc.debug.enabled", true)
val bindAddress = SparkEnv.get.conf.get(CONNECT_GRPC_BINDING_ADDRESS)
val startPort = SparkEnv.get.conf.get(CONNECT_GRPC_BINDING_PORT)
+ ConnectCommon.CONNECT_LOCAL_AUTH_TOKEN = Option(
+ System.getenv(ConnectCommon.CONNECT_LOCAL_AUTH_TOKEN_ENV_NAME))
Review Comment:
Would we need to do any special handling when the env var is set to `""` i.e
empty string?
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectAddArtifactsHandler.scala:
##########
@@ -52,6 +53,8 @@ class SparkConnectAddArtifactsHandler(val responseObserver:
StreamObserver[AddAr
private var holder: SessionHolder = _
override def onNext(req: AddArtifactsRequest): Unit = try {
+ ConnectCommon.CONNECT_LOCAL_AUTH_TOKEN.foreach(k =>
+ assert(k == req.getUserContext.getLocalAuthToken))
Review Comment:
Further, assertions can technically be disabled in the JVM. Doing so might
lead to accidental removal of auth
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectAddArtifactsHandler.scala:
##########
@@ -52,6 +53,8 @@ class SparkConnectAddArtifactsHandler(val responseObserver:
StreamObserver[AddAr
private var holder: SessionHolder = _
override def onNext(req: AddArtifactsRequest): Unit = try {
+ ConnectCommon.CONNECT_LOCAL_AUTH_TOKEN.foreach(k =>
Review Comment:
We can aslo log any auth failure
--
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: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
