You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "turboFei (via GitHub)" <gi...@apache.org> on 2024/07/05 00:25:18 UTC

[PR] [CELEBORN-1318][FOLLOWUP] Transfer properties for token authentication [celeborn]

turboFei opened a new pull request, #2604:
URL: https://github.com/apache/celeborn/pull/2604

   <!--
   Thanks for sending a pull request!  Here are some tips for you:
     - Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
     - Be sure to keep the PR description updated to reflect all changes.
     - Please write your PR title to summarize what this PR proposes.
     - If possible, provide a concise example to reproduce the issue for a faster review.
   -->
   
   ### What changes were proposed in this pull request?
   
   
   
   ### Why are the changes needed?
   
   
   
   ### Does this PR introduce _any_ user-facing change?
   
   
   
   ### How was this patch tested?
   
   


-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer extraInfo for http authentication providers [celeborn]

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#issuecomment-2211162179

   Thanks, merged to main


-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer properties for token authentication [celeborn]

Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#discussion_r1666940680


##########
common/src/main/scala/org/apache/celeborn/common/authentication/Credential.scala:
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.authentication
+
+/**
+ * The credential object that is passed to the token authentication provider.
+ *
+ * @param token The token that is used for authentication
+ * @param props The properties that associated with the token
+ */
+case class Credential(token: String, props: Map[String, String] = Map.empty)

Review Comment:
   do you mean this? @pan3793 



-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer extraInfo for http authentication providers [celeborn]

Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#discussion_r1673234579


##########
common/src/main/scala/org/apache/celeborn/common/authentication/Credential.scala:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.authentication
+
+trait PasswordCredential {
+  def username: String
+  def password: String
+  def extraInfo: Map[String, String] = Map.empty
+}
+
+case class DefaultPasswordCredential(
+    username: String,
+    password: String,
+    override val extraInfo: Map[String, String] = Map.empty) extends PasswordCredential
+
+trait TokenCredential {
+  def token: String
+  def extraInfo: Map[String, String] = Map.empty
+}
+
+case class DefaultTokenCredential(
+    token: String,
+    override val extraInfo: Map[String, String] = Map.empty) extends TokenCredential
+
+object Credential {
+  val CLIENT_IP_KEY = "clientIp"

Review Comment:
   ```
   @javax.annotation.Generated(value = "org.openapitools.codegen.languages.JavaClientCodegen", date = "2024-07-10T15:33:39.089-07:00[America/Los_Angeles]")
   public class WorkersResponse {
     public static final String JSON_PROPERTY_WORKERS = "workers";
     private List<WorkerData> workers = new ArrayList<>();
   
     public static final String JSON_PROPERTY_LOST_WORKERS = "lostWorkers";
     private List<WorkerTimestampData> lostWorkers;
   ```
   
   We can keep it as it is now.



-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer properties for token authentication [celeborn]

Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#issuecomment-2209670206

   cc @pan3793 


-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer extraInfo for token authentication [celeborn]

Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#discussion_r1666980409


##########
common/src/main/scala/org/apache/celeborn/common/authentication/TokenAuthenticationProvider.scala:
##########
@@ -23,15 +23,19 @@ trait TokenAuthenticationProvider {
 
   /**
    * The authenticate method is called by the celeborn authentication layer
-   * to authenticate token for their requests.
-   * If the token is to be granted, return nothing/throw nothing.
-   * When the token is to be disallowed, throw an appropriate [[SecurityException]].
+   * to authenticate credential for their requests.
+   * If the credential is to be granted, return nothing/throw nothing.
+   * When the credential is to be disallowed, throw an appropriate [[SecurityException]].
    *
-   * @param token The token received over the connection request.
+   * @param credential The credential received over the connection request
    * @return The identifier associated with the token
    *
-   * @throws SecurityException When the token is found to be invalid by the implementation
+   * @throws SecurityException When the credential is found to be invalid by the implementation
    */
   @throws[SecurityException]
-  def authenticate(token: String): Principal
+  def authenticate(credential: TokenCredential): Principal
+}
+
+object TokenAuthenticationProvider {
+  val CLIENT_IP_PROPERTY = "CLIENT_IP"

Review Comment:
   done



-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer extraInfo for token authentication [celeborn]

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#discussion_r1666963234


##########
common/src/main/scala/org/apache/celeborn/common/authentication/Credential.scala:
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.authentication
+
+/**
+ * The credential object that is passed to the token authentication provider.
+ *
+ * @param token The token that is used for authentication
+ * @param props The properties that associated with the token
+ */
+case class Credential(token: String, props: Map[String, String] = Map.empty)

Review Comment:
   second thought, check client ip is also applicable for password authN cases, and user may want to have more custom rules, we should make the API likes
   
   ```
   trait PasswordCredential {
     def username: String
     def password: String
     def extraInfo: Map[String, String] = Map.empty
   }
   
   case class DefaultPasswordCredential(
     username: String,
     password: String,
     extraInfo: Map[String, String] = Map.empty) implement PasswordCredential
   ```



-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer extraInfo for http authentication providers [celeborn]

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#discussion_r1666987866


##########
service/src/main/scala/org/apache/celeborn/server/common/http/HttpAuthUtils.scala:
##########
@@ -17,9 +17,19 @@
 
 package org.apache.celeborn.server.common.http
 
+import org.apache.celeborn.common.authentication.Credential
+import org.apache.celeborn.server.common.http.authentication.AuthenticationFilter
+
 object HttpAuthUtils {
   // HTTP header used by the server endpoint during an authentication sequence.
   val WWW_AUTHENTICATE_HEADER = "WWW-Authenticate"
   // HTTP header used by the client endpoint during an authentication sequence.
   val AUTHORIZATION_HEADER = "Authorization"
+
+  def getCredentialExtraInfo: Map[String, String] = {
+    Map(Credential.CLIENT_IP_PROPERTY ->

Review Comment:
   ```suggestion
       Map(Credential.CLIENT_IP_KEY ->
   ```



##########
service/src/test/scala/org/apache/celeborn/server/common/http/authentication/UserDefineTokenAuthenticationProviderImpl.scala:
##########
@@ -20,14 +20,16 @@ package org.apache.celeborn.server.common.http.authentication
 import java.security.Principal
 import javax.security.sasl.AuthenticationException
 
-import org.apache.celeborn.common.authentication.{BasicPrincipal, TokenAuthenticationProvider}
+import org.apache.celeborn.common.authentication.{BasicPrincipal, Credential, TokenAuthenticationProvider, TokenCredential}
 import org.apache.celeborn.common.internal.Logging
 import org.apache.celeborn.server.common.http.authentication.UserDefineTokenAuthenticationProviderImpl.VALID_TOKEN
 
 class UserDefineTokenAuthenticationProviderImpl extends TokenAuthenticationProvider with Logging {
-  override def authenticate(token: String): Principal = {
-    if (token == VALID_TOKEN) {
-      logInfo(s"Success log in of token: $token")
+  override def authenticate(credential: TokenCredential): Principal = {
+    val clientIp =
+      credential.extraInfo.getOrElse(Credential.CLIENT_IP_PROPERTY, null)

Review Comment:
   ```suggestion
         credential.extraInfo.getOrElse(Credential.CLIENT_IP_KEY, null)
   ```



-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer extraInfo for http authentication providers [celeborn]

Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#discussion_r1669847101


##########
common/src/main/scala/org/apache/celeborn/common/authentication/Credential.scala:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.authentication
+
+trait PasswordCredential {
+  def username: String
+  def password: String
+  def extraInfo: Map[String, String] = Map.empty
+}
+
+case class DefaultPasswordCredential(
+    username: String,
+    password: String,
+    override val extraInfo: Map[String, String] = Map.empty) extends PasswordCredential
+
+trait TokenCredential {
+  def token: String
+  def extraInfo: Map[String, String] = Map.empty
+}
+
+case class DefaultTokenCredential(
+    token: String,
+    override val extraInfo: Map[String, String] = Map.empty) extends TokenCredential
+
+object Credential {
+  val CLIENT_IP_KEY = "clientIp"

Review Comment:
   mistake, should I rename it to `CLIENT_IP`? @pan3793 



-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer properties for token authentication [celeborn]

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#discussion_r1666533035


##########
common/src/main/scala/org/apache/celeborn/common/authentication/TokenAuthenticationProvider.scala:
##########
@@ -28,10 +28,15 @@ trait TokenAuthenticationProvider {
    * When the token is to be disallowed, throw an appropriate [[SecurityException]].
    *
    * @param token The token received over the connection request.
+   * @param props The properties associated with the token
    * @return The identifier associated with the token
    *
    * @throws SecurityException When the token is found to be invalid by the implementation
    */
   @throws[SecurityException]
-  def authenticate(token: String): Principal
+  def authenticate(token: String, props: Map[String, String]): Principal

Review Comment:
   To avoid breaking API again, maybe we can introduce a `Credential` interface?



-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer extraInfo for http authentication providers [celeborn]

Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#discussion_r1669847101


##########
common/src/main/scala/org/apache/celeborn/common/authentication/Credential.scala:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.authentication
+
+trait PasswordCredential {
+  def username: String
+  def password: String
+  def extraInfo: Map[String, String] = Map.empty
+}
+
+case class DefaultPasswordCredential(
+    username: String,
+    password: String,
+    override val extraInfo: Map[String, String] = Map.empty) extends PasswordCredential
+
+trait TokenCredential {
+  def token: String
+  def extraInfo: Map[String, String] = Map.empty
+}
+
+case class DefaultTokenCredential(
+    token: String,
+    override val extraInfo: Map[String, String] = Map.empty) extends TokenCredential
+
+object Credential {
+  val CLIENT_IP_KEY = "clientIp"

Review Comment:
   mistake, should I raise a followup pr to rename it to `CLIENT_IP`? @pan3793 



-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer extraInfo for token authentication [celeborn]

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#discussion_r1666972942


##########
common/src/main/scala/org/apache/celeborn/common/authentication/TokenAuthenticationProvider.scala:
##########
@@ -23,15 +23,19 @@ trait TokenAuthenticationProvider {
 
   /**
    * The authenticate method is called by the celeborn authentication layer
-   * to authenticate token for their requests.
-   * If the token is to be granted, return nothing/throw nothing.
-   * When the token is to be disallowed, throw an appropriate [[SecurityException]].
+   * to authenticate credential for their requests.
+   * If the credential is to be granted, return nothing/throw nothing.
+   * When the credential is to be disallowed, throw an appropriate [[SecurityException]].
    *
-   * @param token The token received over the connection request.
+   * @param credential The credential received over the connection request
    * @return The identifier associated with the token
    *
-   * @throws SecurityException When the token is found to be invalid by the implementation
+   * @throws SecurityException When the credential is found to be invalid by the implementation
    */
   @throws[SecurityException]
-  def authenticate(token: String): Principal
+  def authenticate(credential: TokenCredential): Principal
+}
+
+object TokenAuthenticationProvider {
+  val CLIENT_IP_PROPERTY = "CLIENT_IP"

Review Comment:
   move it to
   ```
   object Credential {
     val CLIENT_IP: "CLIENT_IP"
   }
   ```



-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer extraInfo for http authentication providers [celeborn]

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 closed pull request #2604: [CELEBORN-1318][FOLLOWUP] Transfer extraInfo for http authentication providers
URL: https://github.com/apache/celeborn/pull/2604


-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer extraInfo for token authentication [celeborn]

Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#discussion_r1666979616


##########
common/src/main/scala/org/apache/celeborn/common/authentication/TokenAuthenticationProvider.scala:
##########
@@ -23,15 +23,19 @@ trait TokenAuthenticationProvider {
 
   /**
    * The authenticate method is called by the celeborn authentication layer
-   * to authenticate token for their requests.
-   * If the token is to be granted, return nothing/throw nothing.
-   * When the token is to be disallowed, throw an appropriate [[SecurityException]].
+   * to authenticate credential for their requests.
+   * If the credential is to be granted, return nothing/throw nothing.
+   * When the credential is to be disallowed, throw an appropriate [[SecurityException]].
    *
-   * @param token The token received over the connection request.
+   * @param credential The credential received over the connection request
    * @return The identifier associated with the token
    *
-   * @throws SecurityException When the token is found to be invalid by the implementation
+   * @throws SecurityException When the credential is found to be invalid by the implementation
    */
   @throws[SecurityException]
-  def authenticate(token: String): Principal
+  def authenticate(credential: TokenCredential): Principal
+}
+
+object TokenAuthenticationProvider {
+  val CLIENT_IP_PROPERTY = "CLIENT_IP"

Review Comment:
   ok



-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer extraInfo for http authentication providers [celeborn]

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#discussion_r1671765092


##########
common/src/main/scala/org/apache/celeborn/common/authentication/Credential.scala:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.authentication
+
+trait PasswordCredential {
+  def username: String
+  def password: String
+  def extraInfo: Map[String, String] = Map.empty
+}
+
+case class DefaultPasswordCredential(
+    username: String,
+    password: String,
+    override val extraInfo: Map[String, String] = Map.empty) extends PasswordCredential
+
+trait TokenCredential {
+  def token: String
+  def extraInfo: Map[String, String] = Map.empty
+}
+
+case class DefaultTokenCredential(
+    token: String,
+    override val extraInfo: Map[String, String] = Map.empty) extends TokenCredential
+
+object Credential {
+  val CLIENT_IP_KEY = "clientIp"

Review Comment:
   Okay. BTW, what's the open api generated style for parameter name? "clientIp" or "client_ip"? let's sync them too.



-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer properties for token authentication [celeborn]

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#discussion_r1666947561


##########
common/src/main/scala/org/apache/celeborn/common/authentication/Credential.scala:
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.authentication
+
+/**
+ * The credential object that is passed to the token authentication provider.
+ *
+ * @param token The token that is used for authentication
+ * @param props The properties that associated with the token
+ */
+case class Credential(token: String, props: Map[String, String] = Map.empty)

Review Comment:
   almost, but maybe
   ```
   case class PasswordCredential(username: String, password: String)
   case class TokenCredential(token: String, properties: Map[String, String])
   ``



##########
common/src/main/scala/org/apache/celeborn/common/authentication/Credential.scala:
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.authentication
+
+/**
+ * The credential object that is passed to the token authentication provider.
+ *
+ * @param token The token that is used for authentication
+ * @param props The properties that associated with the token
+ */
+case class Credential(token: String, props: Map[String, String] = Map.empty)

Review Comment:
   almost, but maybe
   ```
   case class PasswordCredential(username: String, password: String)
   case class TokenCredential(token: String, properties: Map[String, String])
   ```



-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer properties for token authentication [celeborn]

Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#discussion_r1666954336


##########
common/src/main/scala/org/apache/celeborn/common/authentication/Credential.scala:
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.authentication
+
+/**
+ * The credential object that is passed to the token authentication provider.
+ *
+ * @param token The token that is used for authentication
+ * @param props The properties that associated with the token
+ */
+case class Credential(token: String, props: Map[String, String] = Map.empty)

Review Comment:
   done



-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer extraInfo for http authentication providers [celeborn]

Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#discussion_r1666987775


##########
service/src/test/scala/org/apache/celeborn/server/common/http/authentication/UserDefinePasswordAuthenticationProviderImpl.scala:
##########
@@ -20,16 +20,18 @@ package org.apache.celeborn.server.common.http.authentication
 import java.security.Principal
 import javax.security.sasl.AuthenticationException
 
-import org.apache.celeborn.common.authentication.{BasicPrincipal, PasswdAuthenticationProvider}
+import org.apache.celeborn.common.authentication.{BasicPrincipal, Credential, PasswdAuthenticationProvider, PasswordCredential}
 import org.apache.celeborn.common.internal.Logging
 import org.apache.celeborn.server.common.http.authentication.UserDefinePasswordAuthenticationProviderImpl.VALID_PASSWORD
 
 class UserDefinePasswordAuthenticationProviderImpl
   extends PasswdAuthenticationProvider with Logging {
-  override def authenticate(user: String, password: String): Principal = {
-    if (password == VALID_PASSWORD) {
-      logInfo(s"Success log in of user: $user")
-      new BasicPrincipal(user)
+  override def authenticate(credential: PasswordCredential): Principal = {
+    val clientIp =
+      credential.extraInfo.getOrElse(Credential.CLIENT_IP_PROPERTY, null)

Review Comment:
   ```suggestion
         credential.extraInfo.getOrElse(Credential.CLIENT_IP_KEY, null)
   ```



##########
common/src/main/scala/org/apache/celeborn/common/authentication/Credential.scala:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.authentication
+
+trait PasswordCredential {
+  def username: String
+  def password: String
+  def extraInfo: Map[String, String] = Map.empty
+}
+
+case class DefaultPasswordCredential(
+    username: String,
+    password: String,
+    override val extraInfo: Map[String, String] = Map.empty) extends PasswordCredential
+
+trait TokenCredential {
+  def token: String
+  def extraInfo: Map[String, String] = Map.empty
+}
+
+case class DefaultTokenCredential(
+    token: String,
+    override val extraInfo: Map[String, String] = Map.empty) extends TokenCredential
+
+object Credential {
+  val CLIENT_IP_PROPERTY = "clientIp"

Review Comment:
   ```suggestion
     val CLIENT_IP_KEY = "clientIp"
   ```



-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [CELEBORN-1318][FOLLOWUP] Transfer extraInfo for http authentication providers [celeborn]

Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #2604:
URL: https://github.com/apache/celeborn/pull/2604#issuecomment-2211154894

   thanks for review and suggestions @pan3793 


-- 
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: issues-unsubscribe@celeborn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org