Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
yuqi1129 merged PR #4160: URL: https://github.com/apache/gravitino/pull/4160 -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
lw-yang commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2258135908 @jerqi please help to check it again -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
lw-yang commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2258109687 got, I will complete doc in another PR later -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
jerqi commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2258100174 You can refer to https://github.com/apache/gravitino/pull/2592/files . You should add the `DeprecatedConfig` and `alternatives`. And you should add the document, 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
lw-yang commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2257650692 I think it doesn't break compatibility. i think backward compatible means that application for previous versions still run on the new version without modification Add a new config option is ok for me and maybe is more appropriate, i will add a new config later -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
jerqi commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2257585566 > Yes, `add a new config option` is also a solution. I think if we can maintain backward compatibility, we can reuse the previous configuration, so I didn't introduce a new configuration item `Add a new config option` won't break the backward compatibility. -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
jerqi commented on code in PR #4160:
URL: https://github.com/apache/gravitino/pull/4160#discussion_r1696391669
##
server/src/main/java/org/apache/gravitino/server/web/ConfigServlet.java:
##
@@ -51,7 +55,9 @@ public ConfigServlet(ServerConfig serverConfig) {
String config = String.valueOf(serverConfig.get(key));
configs.put(key.getKey(), config);
}
-if
(serverConfig.get(Configs.AUTHENTICATOR).equalsIgnoreCase(AuthenticatorType.OAUTH.name()))
{
+
+List authenticators =
COMMA.splitToList(serverConfig.get(Configs.AUTHENTICATOR));
Review Comment:
This code isn't easy to maintain.
--
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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
jerqi commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2257551301 > Why can't we guarantee compatibility, if upgrade to 0.6.0, the previous config can also work Config interface provides different results. Before 0.6.0, it's a string. After 0.6.0, it's a string split by `.`. -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
lw-yang commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2257409543 Why can't we guarantee compatibility, if upgrade to 0.6.0, the previous config can also work -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
jerqi commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2257404495 > Yes, `add a new config option` is also a solution. I think if we can maintain backward compatibility, we can reuse the previous configuration, so I didn't introduce a new configuration item You actually change the config type. It can't guarantee the compatibility. -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
lw-yang commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2257398933 Yes, `add a new config option` is also a solution. I think if we can maintain backward compatibility, we can reuse the previous configuration, so I didn't introduce a new configuration item -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
jerqi commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2257390314 > If the `list` is used, the `/config` interface will return `['xx']`, which includes an additional square bracket, instead of just a string. it require additional processing for the previous callers. > > If multiple authentication methods are configured on the server side, the frontend simply select the first one now. So if you insist on this, you should deprecate the config option and add a new config option. -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
lw-yang commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2257386067 If the `list` is used, the `/config` interface will return `['xx']`, which includes an additional square bracket, instead of just a string. it require additional processing for the previous callers. If multiple authentication methods are configured on the server side, the frontend simply select the first one 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
jerqi commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2257379824 > We should maintain backward compatibility, so revert the changes to keep `string` type First, It won't break backward compatiblity using sequeuece. Second, how to handle the string `oauth,simple` for front end? -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
lw-yang commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2257377248 We should maintain backward compatibility, so revert the changes to keep `string` type -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
jerqi commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2257374838 > @jerqi @lw-yang has reverted the changes about the type of authenticator from `List` to `String`, Is it okay for you? Not ok for me. It's weird. -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
yuqi1129 commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2257360487 @jerqi @lw-yang has reverted the changes about the type of authenticator from `List` to `String`, Is it okay for you? -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
yuqi1129 commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2252701529 > > @lw-yang @jerqi The reason front-end tests fail is that as we have supported multiple authentications in the server side, front-end page can't get right `gravitino.authenticator` and is unable to decide which authentication is going to use: > > Before modification: gravitino.authenticator=simple gravitino.authenticator=oauth > > After:  https://private-user-images.githubusercontent.com/15794564/352055995-a380d7f6-b64d-417e-abeb-a06b97849ca5.png?jwt=eyJhbGc iOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjE5MTE1OTYsIm5iZiI6MTcyMTkxMTI5NiwicGF0aCI6Ii8xNTc5NDU2NC8zNTIwNTU5OTUtYTM4MGQ3ZjYtYjY0ZC00MTdlLWFiZWItYTA2Yjk3ODQ5Y2E1LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA3MjUlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNzI1VDEyNDEzNlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTY3OWM0MjM2ZDFmOWY4OTk1YzA5ZTNmODM1MjVmOTRhY2I4NDc5ZjM1OGNkZjU0YzUyMGY5ZTM0ZWJkMzNjMGYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.z3RL0lh44WQrXRFDHjf5EK5tB6gPeRfMS4hViGLO7Sw"> > > Note: Please pay attention to the bracket in the value. > > Another issue: How will the front-end page know which authentication type to use and then load the corresponding login page if the server has set multiple authentications? > > Front end should use the first authenticator in my opinion. I concur with it. To make it work, some changes need to be made. -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
jerqi commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2250233395 > @lw-yang @jerqi The reason front-end tests fail is that as we have supported multiple authentications in the server side, front-end page can't get right `gravitino.authenticator` and is unable to decide which authentication is going to use: > > Before modification: gravitino.authenticator=simple gravitino.authenticator=oauth > > After:  https://private-user-images.githubusercontent.com/15794564/352055995-a380d7f6-b64d-417e-abeb-a06b97849ca5.png?jwt=eyJhbGciO iJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjE5MTE1OTYsIm5iZiI6MTcyMTkxMTI5NiwicGF0aCI6Ii8xNTc5NDU2NC8zNTIwNTU5OTUtYTM4MGQ3ZjYtYjY0ZC00MTdlLWFiZWItYTA2Yjk3ODQ5Y2E1LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA3MjUlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNzI1VDEyNDEzNlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTY3OWM0MjM2ZDFmOWY4OTk1YzA5ZTNmODM1MjVmOTRhY2I4NDc5ZjM1OGNkZjU0YzUyMGY5ZTM0ZWJkMzNjMGYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.z3RL0lh44WQrXRFDHjf5EK5tB6gPeRfMS4hViGLO7Sw"> > > Note: Please pay attention to the bracket in the value. > > Another issue: How will the front-end page know which authentication type to use and then load the corresponding login page if the server has set multiple authentications? Front end should use the first authenticator in my opinion. -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
yuqi1129 commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2249911313 @lw-yang @jerqi The reason front-end tests fail is that as we have supported multiple authentications in the server side, front-end page can't get right `gravitino.authenticator` and is unable to decide which authentication is going to use: Before modification: gravitino.authenticator=simple gravitino.authenticator=oauth After:  https://github.com/user-attachments/assets/a380d7f6-b64d-417e-abeb-a06b97849ca5";> Another problem: If the server has set multiple authentications, how will the front-end page know which authentication type to use and then load the corresponding login page? -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
lw-yang commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-220893 > Could you modify the document of this config option? If there are some authenticators using the same header, what will it happen? We should tell the user the risk by the document, too. I will complete this in another PR，and update some outdated doc -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
jerqi commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2244393409 Could you modify the document of this config option? If there are some authenticators using the same header, what will it happen? We should tell the user the risk by the document, 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
yuqi1129 closed pull request #4160: [#4056] improvement(server-common): Support multiple authentication at the same time on the server side URL: https://github.com/apache/gravitino/pull/4160 -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
lw-yang commented on code in PR #4160:
URL: https://github.com/apache/gravitino/pull/4160#discussion_r1686148329
##
server-common/src/main/java/org/apache/gravitino/server/authentication/Authenticator.java:
##
@@ -55,4 +55,19 @@ default Principal authenticateToken(byte[] tokenData) {
* @throws RuntimeException if the initialization fails
*/
void initialize(Config config) throws RuntimeException;
+
+ /**
+ * Determines if the provided token data is supported by this authenticator
+ *
+ * This method checks if the given token data can be processed by this
authenticator.
+ * Implementations should override this method to provide specific logic for
determining if the
+ * token data format or content is recognized and can be authenticated.
+ *
+ * @param tokenData The byte array containing the token data to be checked.
+ * @return true if the token data is supported and can be authenticated by
this authenticator;
+ * false otherwise.
+ */
+ default boolean supportsAuthentication(byte[] tokenData) {
Review Comment:
right
--
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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
jerqi commented on code in PR #4160:
URL: https://github.com/apache/gravitino/pull/4160#discussion_r1686127051
##
server-common/src/main/java/org/apache/gravitino/server/authentication/Authenticator.java:
##
@@ -55,4 +55,19 @@ default Principal authenticateToken(byte[] tokenData) {
* @throws RuntimeException if the initialization fails
*/
void initialize(Config config) throws RuntimeException;
+
+ /**
+ * Determines if the provided token data is supported by this authenticator
+ *
+ * This method checks if the given token data can be processed by this
authenticator.
+ * Implementations should override this method to provide specific logic for
determining if the
+ * token data format or content is recognized and can be authenticated.
+ *
+ * @param tokenData The byte array containing the token data to be checked.
+ * @return true if the token data is supported and can be authenticated by
this authenticator;
+ * false otherwise.
+ */
+ default boolean supportsAuthentication(byte[] tokenData) {
Review Comment:
`supportsToken` may be better?
--
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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
lw-yang commented on code in PR #4160:
URL: https://github.com/apache/gravitino/pull/4160#discussion_r1685931868
##
server-common/src/main/java/org/apache/gravitino/server/authentication/OAuth2TokenAuthenticator.java:
##
@@ -126,6 +126,13 @@ public void initialize(Config config) throws
RuntimeException {
this.defaultSigningKey =
decodeSignKey(Base64.getDecoder().decode(configuredSignKey), algType);
}
+ @Override
+ public boolean supports(byte[] tokenData) {
Review Comment:
ok, i will change it
--
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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
jerqi commented on code in PR #4160:
URL: https://github.com/apache/gravitino/pull/4160#discussion_r1685930178
##
server-common/src/main/java/org/apache/gravitino/server/authentication/OAuth2TokenAuthenticator.java:
##
@@ -126,6 +126,13 @@ public void initialize(Config config) throws
RuntimeException {
this.defaultSigningKey =
decodeSignKey(Base64.getDecoder().decode(configuredSignKey), algType);
}
+ @Override
+ public boolean supports(byte[] tokenData) {
Review Comment:
Supports will be better. Because we may add other SupportsXXX methods.
--
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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
lw-yang commented on code in PR #4160:
URL: https://github.com/apache/gravitino/pull/4160#discussion_r1685929292
##
server-common/src/main/java/org/apache/gravitino/server/authentication/OAuth2TokenAuthenticator.java:
##
@@ -126,6 +126,13 @@ public void initialize(Config config) throws
RuntimeException {
this.defaultSigningKey =
decodeSignKey(Base64.getDecoder().decode(configuredSignKey), algType);
}
+ @Override
+ public boolean supports(byte[] tokenData) {
Review Comment:
I think `supports` is enough based on the given context.
like
https://github.com/spring-projects/spring-security/blob/480c84a6e1e0f16aa5fbb0b1fc5373640d6c9a1a/core/src/main/java/org/springframework/security/authentication/AuthenticationProvider.java#L63
--
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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
lw-yang commented on code in PR #4160:
URL: https://github.com/apache/gravitino/pull/4160#discussion_r1685926516
##
core/src/main/java/org/apache/gravitino/Configs.java:
##
@@ -199,7 +199,9 @@ private Configs() {}
public static final ConfigEntry AUTHENTICATOR =
new ConfigBuilder("gravitino.authenticator")
- .doc("The authenticator which Gravitino uses")
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
jerqi commented on code in PR #4160:
URL: https://github.com/apache/gravitino/pull/4160#discussion_r1682668999
##
server-common/src/main/java/org/apache/gravitino/server/authentication/OAuth2TokenAuthenticator.java:
##
@@ -126,6 +126,13 @@ public void initialize(Config config) throws
RuntimeException {
this.defaultSigningKey =
decodeSignKey(Base64.getDecoder().decode(configuredSignKey), algType);
}
+ @Override
+ public boolean supports(byte[] tokenData) {
Review Comment:
Could you have a better name?What does this authenticator support?
##
server-common/src/main/java/org/apache/gravitino/server/authentication/OAuth2TokenAuthenticator.java:
##
@@ -126,6 +126,13 @@ public void initialize(Config config) throws
RuntimeException {
this.defaultSigningKey =
decodeSignKey(Base64.getDecoder().decode(configuredSignKey), algType);
}
+ @Override
+ public boolean supports(byte[] tokenData) {
Review Comment:
Could you have a better name? What does this authenticator support?
--
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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
jerqi commented on code in PR #4160:
URL: https://github.com/apache/gravitino/pull/4160#discussion_r1682667135
##
core/src/main/java/org/apache/gravitino/Configs.java:
##
@@ -199,7 +199,9 @@ private Configs() {}
public static final ConfigEntry AUTHENTICATOR =
new ConfigBuilder("gravitino.authenticator")
- .doc("The authenticator which Gravitino uses")
Review Comment:
Could you reuse `toSequence`?
--
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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
lw-yang commented on code in PR #4160:
URL: https://github.com/apache/gravitino/pull/4160#discussion_r1682662637
##
integration-test-common/src/test/java/org/apache/gravitino/integration/test/MiniGravitino.java:
##
@@ -115,19 +117,17 @@ public void start() throws Exception {
this.host = jettyServerConfig.getHost();
this.port = jettyServerConfig.getHttpPort();
String URI = String.format("http://%s:%d";, host, port);
-if (AuthenticatorType.OAUTH
-.name()
-.toLowerCase()
-.equals(context.customConfig.get(Configs.AUTHENTICATOR.getKey( {
+if (COMMA
Review Comment:
yes, any of them is ok
##
server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticationFilter.java:
##
@@ -54,22 +55,36 @@ public void init(FilterConfig filterConfig) throws
ServletException {}
public void doFilter(ServletRequest request, ServletResponse response,
FilterChain chain)
throws IOException, ServletException {
try {
- Authenticator authenticator;
- if (filterAuthenticator == null) {
-authenticator = ServerAuthenticator.getInstance().authenticator();
+ List authenticators;
+ if (filterAuthenticators == null || filterAuthenticators.isEmpty()) {
+authenticators = ServerAuthenticator.getInstance().authenticators();
} else {
-authenticator = filterAuthenticator;
+authenticators = filterAuthenticators;
}
HttpServletRequest req = (HttpServletRequest) request;
Enumeration headerData =
req.getHeaders(AuthConstants.HTTP_HEADER_AUTHORIZATION);
byte[] authData = null;
if (headerData.hasMoreElements()) {
authData = headerData.nextElement().getBytes(StandardCharsets.UTF_8);
}
- if (authenticator.isDataFromToken()) {
-Principal principal = authenticator.authenticateToken(authData);
-
request.setAttribute(AuthConstants.AUTHENTICATED_PRINCIPAL_ATTRIBUTE_NAME,
principal);
+
+ Principal principal = null;
+ for (Authenticator authenticator : authenticators) {
+if (!authenticator.supports(authData)) {
+ continue;
+}
+if (authenticator.isDataFromToken()) {
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
yuqi1129 commented on code in PR #4160:
URL: https://github.com/apache/gravitino/pull/4160#discussion_r1682377681
##
integration-test-common/src/test/java/org/apache/gravitino/integration/test/MiniGravitino.java:
##
@@ -115,19 +117,17 @@ public void start() throws Exception {
this.host = jettyServerConfig.getHost();
this.port = jettyServerConfig.getHttpPort();
String URI = String.format("http://%s:%d";, host, port);
-if (AuthenticatorType.OAUTH
-.name()
-.toLowerCase()
-.equals(context.customConfig.get(Configs.AUTHENTICATOR.getKey( {
+if (COMMA
Review Comment:
So, If the server supports `oauth` and `Kerberos` authentication at the same
time, client will use `aouth` to connect to it?
##
server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticationFilter.java:
##
@@ -54,22 +55,36 @@ public void init(FilterConfig filterConfig) throws
ServletException {}
public void doFilter(ServletRequest request, ServletResponse response,
FilterChain chain)
throws IOException, ServletException {
try {
- Authenticator authenticator;
- if (filterAuthenticator == null) {
-authenticator = ServerAuthenticator.getInstance().authenticator();
+ List authenticators;
+ if (filterAuthenticators == null || filterAuthenticators.isEmpty()) {
+authenticators = ServerAuthenticator.getInstance().authenticators();
} else {
-authenticator = filterAuthenticator;
+authenticators = filterAuthenticators;
}
HttpServletRequest req = (HttpServletRequest) request;
Enumeration headerData =
req.getHeaders(AuthConstants.HTTP_HEADER_AUTHORIZATION);
byte[] authData = null;
if (headerData.hasMoreElements()) {
authData = headerData.nextElement().getBytes(StandardCharsets.UTF_8);
}
- if (authenticator.isDataFromToken()) {
-Principal principal = authenticator.authenticateToken(authData);
-
request.setAttribute(AuthConstants.AUTHENTICATED_PRINCIPAL_ATTRIBUTE_NAME,
principal);
+
+ Principal principal = null;
+ for (Authenticator authenticator : authenticators) {
+if (!authenticator.supports(authData)) {
+ continue;
+}
+if (authenticator.isDataFromToken()) {
Review Comment:
if (authenticator.supports(authData) && authenticator.isDataFromToken())
--
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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
jerryshao commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2235803724 @jerqi @yuqi1129 please help to review. -- 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]
Re: [PR] [#4056] improvement(server-common): Support multiple authentication at the same time on the server side [gravitino]
lw-yang commented on PR #4160: URL: https://github.com/apache/gravitino/pull/4160#issuecomment-2228374171 @yuqi1129 could you please help to review it when you free -- 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]
