You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "hongkunxu (via GitHub)" <gi...@apache.org> on 2025/11/20 07:29:43 UTC
[PR] [Enhance] Optimize user login [pinot]
hongkunxu opened a new pull request, #17242:
URL: https://github.com/apache/pinot/pull/17242
1. allow common user to login
2. Deny regular users access to the ZooKeeper browser.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "hongkunxu (via GitHub)" <gi...@apache.org>.
hongkunxu commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2544767780
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -61,7 +62,7 @@ public String getToken() {
}
public boolean hasTable(String tableName) {
- return isTableIncluded(tableName) && isTableNotExcluded(tableName);
+ return StringUtils.isEmpty(tableName) || (isTableIncluded(tableName) && isTableNotExcluded(tableName));
Review Comment:
When a user logs in through the UI, this method is called with an empty table name. This means that unless the user has permissions for all tables, the method will fail. @xiangfu0
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "hongkunxu (via GitHub)" <gi...@apache.org>.
hongkunxu commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2544767780
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -61,7 +62,7 @@ public String getToken() {
}
public boolean hasTable(String tableName) {
- return isTableIncluded(tableName) && isTableNotExcluded(tableName);
+ return StringUtils.isEmpty(tableName) || (isTableIncluded(tableName) && isTableNotExcluded(tableName));
Review Comment:
When a user logs in through the UI, this method is called with an empty table name. This means that unless the user has permissions for all tables, the method will fail.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2643153962
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -122,24 +122,39 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
private Optional<ZkBasicAuthPrincipal> getPrincipalAuth(RequesterIdentity requesterIdentity) {
Collection<String> tokens = extractAuthorizationTokens(requesterIdentity);
- if (tokens.isEmpty()) {
+ if (tokens == null || tokens.isEmpty()) {
return Optional.empty();
}
- _name2principal = BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
- .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
+ Map<String, ZkBasicAuthPrincipal> name2principal =
+ BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
+ .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
- Map<String, String> name2password = tokens.stream().collect(
- Collectors.toMap(
- org.apache.pinot.common.auth.BasicAuthUtils::extractUsername,
- org.apache.pinot.common.auth.BasicAuthUtils::extractPassword,
- (v1, v2) -> v2));
- Map<String, ZkBasicAuthPrincipal> password2principal =
- name2password.keySet().stream().collect(Collectors.toMap(name2password::get, _name2principal::get));
+ for (String token : tokens) {
+ String username = org.apache.pinot.common.auth.BasicAuthUtils.extractUsername(token);
Review Comment:
import class `org.apache.pinot.common.auth.BasicAuthUtils` and just do
```
String username = BasicAuthUtils.extractUsername(token);
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -122,24 +122,39 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
private Optional<ZkBasicAuthPrincipal> getPrincipalAuth(RequesterIdentity requesterIdentity) {
Collection<String> tokens = extractAuthorizationTokens(requesterIdentity);
- if (tokens.isEmpty()) {
+ if (tokens == null || tokens.isEmpty()) {
return Optional.empty();
}
- _name2principal = BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
- .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
+ Map<String, ZkBasicAuthPrincipal> name2principal =
+ BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
+ .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
- Map<String, String> name2password = tokens.stream().collect(
- Collectors.toMap(
- org.apache.pinot.common.auth.BasicAuthUtils::extractUsername,
- org.apache.pinot.common.auth.BasicAuthUtils::extractPassword,
- (v1, v2) -> v2));
- Map<String, ZkBasicAuthPrincipal> password2principal =
- name2password.keySet().stream().collect(Collectors.toMap(name2password::get, _name2principal::get));
+ for (String token : tokens) {
+ String username = org.apache.pinot.common.auth.BasicAuthUtils.extractUsername(token);
+ String password = org.apache.pinot.common.auth.BasicAuthUtils.extractPassword(token);
Review Comment:
``` String password = BasicAuthUtils.extractPassword(token);```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java:
##########
@@ -81,7 +81,7 @@ public boolean verify(@ApiParam(value = "Table name without type") @QueryParam("
@ApiParam(value = "API access type") @DefaultValue("READ") @QueryParam("accessType") AccessType accessType,
@ApiParam(value = "Endpoint URL") @QueryParam("endpointUrl") String endpointUrl) {
AccessControl accessControl = _accessControlFactory.create();
- return accessControl.hasAccess(tableName, accessType, _httpHeaders, endpointUrl);
+ return accessControl.hasAccess(_httpHeaders, TargetType.CLUSTER);
Review Comment:
In this case, I feel we should just mark this API deprecated and create new APIs for this.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "hongkunxu (via GitHub)" <gi...@apache.org>.
hongkunxu commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2642566487
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -22,11 +22,11 @@
import java.util.Collections;
import java.util.HashSet;
import java.util.Map;
-import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import javax.ws.rs.NotAuthorizedException;
Review Comment:
Since I removed Objects::nonNull, the Objects reference is no longer used.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2544710006
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -61,7 +62,7 @@ public String getToken() {
}
public boolean hasTable(String tableName) {
- return isTableIncluded(tableName) && isTableNotExcluded(tableName);
+ return StringUtils.isEmpty(tableName) || (isTableIncluded(tableName) && isTableNotExcluded(tableName));
Review Comment:
why check for empty table name?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "shauryachats (via GitHub)" <gi...@apache.org>.
shauryachats commented on PR #17242:
URL: https://github.com/apache/pinot/pull/17242#issuecomment-3726448132
@hongkunxu @xiangfu0 is there a feature flag which enables the cluster to always be in superuser mode? At Uber, we restrict the access of the Controller to be solely for Pinot engineers, hence we do not require an additional auth login page. cc: @ankitsultana
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "hongkunxu (via GitHub)" <gi...@apache.org>.
hongkunxu commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2642637080
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -122,24 +122,39 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
private Optional<ZkBasicAuthPrincipal> getPrincipalAuth(RequesterIdentity requesterIdentity) {
Collection<String> tokens = extractAuthorizationTokens(requesterIdentity);
- if (tokens.isEmpty()) {
+ if (tokens == null || tokens.isEmpty()) {
return Optional.empty();
}
- _name2principal = BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
- .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
+ Map<String, ZkBasicAuthPrincipal> name2principal =
+ BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
+ .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
Review Comment:
This behavior is consistent with the previous logic, and it has been running in the production environment without any issues, even under peak traffic of up to 300 QPS and daily traffic exceeding 1 million requests.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "hongkunxu (via GitHub)" <gi...@apache.org>.
hongkunxu commented on PR #17242:
URL: https://github.com/apache/pinot/pull/17242#issuecomment-3726780249
> Pinot
Hi @shauryachats , I think the current design is more aligned with how mainstream OLAP engines work, and there isn’t a strong reason to restrict normal users from accessing the UI. I understand the supermode you mentioned, but that should be a special case. I’ll discuss this with @xiangfu0 and see how we should handle that scenario.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2642366916
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java:
##########
@@ -81,7 +81,7 @@ public boolean verify(@ApiParam(value = "Table name without type") @QueryParam("
@ApiParam(value = "API access type") @DefaultValue("READ") @QueryParam("accessType") AccessType accessType,
@ApiParam(value = "Endpoint URL") @QueryParam("endpointUrl") String endpointUrl) {
AccessControl accessControl = _accessControlFactory.create();
- return accessControl.hasAccess(tableName, accessType, _httpHeaders, endpointUrl);
+ return accessControl.hasAccess(_httpHeaders, TargetType.CLUSTER);
Review Comment:
The verify endpoint now ignores all its parameters (tableName, accessType, endpointUrl) and only checks cluster-level access. This breaks the documented API contract where callers expect table-specific and access-type-specific verification. Either update the endpoint's signature to remove unused parameters or implement the verification logic to actually use them.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -22,11 +22,11 @@
import java.util.Collections;
import java.util.HashSet;
import java.util.Map;
-import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import javax.ws.rs.NotAuthorizedException;
Review Comment:
The removed import `java.util.Objects` appears to still be needed, as the code likely uses Objects.nonNull or similar elsewhere in this file. Verify that this import removal doesn't break compilation.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -122,24 +122,39 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
private Optional<ZkBasicAuthPrincipal> getPrincipalAuth(RequesterIdentity requesterIdentity) {
Collection<String> tokens = extractAuthorizationTokens(requesterIdentity);
- if (tokens.isEmpty()) {
+ if (tokens == null || tokens.isEmpty()) {
return Optional.empty();
}
- _name2principal = BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
- .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
+ Map<String, ZkBasicAuthPrincipal> name2principal =
+ BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
+ .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
Review Comment:
The name2principal map is reconstructed on every authentication attempt by calling getAllBrokerUserConfig() and processing it. This is inefficient for high-frequency authentication requests. Consider caching this map and only refreshing it when user configuration changes, similar to how the controller version uses the _name2principal field.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "hongkunxu (via GitHub)" <gi...@apache.org>.
hongkunxu commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2643574566
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -122,24 +122,39 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
private Optional<ZkBasicAuthPrincipal> getPrincipalAuth(RequesterIdentity requesterIdentity) {
Collection<String> tokens = extractAuthorizationTokens(requesterIdentity);
- if (tokens.isEmpty()) {
+ if (tokens == null || tokens.isEmpty()) {
return Optional.empty();
}
- _name2principal = BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
- .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
+ Map<String, ZkBasicAuthPrincipal> name2principal =
+ BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
+ .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
- Map<String, String> name2password = tokens.stream().collect(
- Collectors.toMap(
- org.apache.pinot.common.auth.BasicAuthUtils::extractUsername,
- org.apache.pinot.common.auth.BasicAuthUtils::extractPassword,
- (v1, v2) -> v2));
- Map<String, ZkBasicAuthPrincipal> password2principal =
- name2password.keySet().stream().collect(Collectors.toMap(name2password::get, _name2principal::get));
+ for (String token : tokens) {
+ String username = org.apache.pinot.common.auth.BasicAuthUtils.extractUsername(token);
Review Comment:
<img width="484" height="71" alt="image" src="https://github.com/user-attachments/assets/39cb5f36-0691-4ab2-83aa-a172f3c2622c" />
Hi @xiangfu0 Unfortunately, we cannot do this at the moment, because both pinot-common and pinot-core define a BasicAuthUtils class with the same name, and both classes are used in this method.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17242:
URL: https://github.com/apache/pinot/pull/17242#issuecomment-3556493335
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17242?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.74%. Comparing base ([`325446c`](https://app.codecov.io/gh/apache/pinot/commit/325446cabb5c2cca2ecbcbd9893913eebd66720f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`18dc74a`](https://app.codecov.io/gh/apache/pinot/commit/18dc74a3fe184a55dab7692c0cf83704de65b0b9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17242?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...org/apache/pinot/core/auth/BasicAuthPrincipal.java](https://app.codecov.io/gh/apache/pinot/pull/17242?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fauth%2FBasicAuthPrincipal.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9hdXRoL0Jhc2ljQXV0aFByaW5jaXBhbC5qYXZh) | 0.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17242?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (325446c) and HEAD (18dc74a). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (325446c) | HEAD (18dc74a) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|0|
>|unittests2|2|0|
>|integration|6|0|
>|integration2|2|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17242 +/- ##
============================================
- Coverage 63.20% 55.74% -7.47%
+ Complexity 1433 701 -732
============================================
Files 3124 2436 -688
Lines 185374 137115 -48259
Branches 28335 21829 -6506
============================================
- Hits 117169 76435 -40734
+ Misses 59169 54261 -4908
+ Partials 9036 6419 -2617
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17242/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/17242/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/17242/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/17242/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/17242/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/17242/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/17242/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.74% <0.00%> (-7.41%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17242/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.74% <0.00%> (-7.47%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17242/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.74% <0.00%> (-7.46%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17242/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.74% <0.00%> (-0.03%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17242/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/17242?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "hongkunxu (via GitHub)" <gi...@apache.org>.
hongkunxu commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2643573242
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java:
##########
@@ -81,7 +81,7 @@ public boolean verify(@ApiParam(value = "Table name without type") @QueryParam("
@ApiParam(value = "API access type") @DefaultValue("READ") @QueryParam("accessType") AccessType accessType,
@ApiParam(value = "Endpoint URL") @QueryParam("endpointUrl") String endpointUrl) {
AccessControl accessControl = _accessControlFactory.create();
- return accessControl.hasAccess(tableName, accessType, _httpHeaders, endpointUrl);
+ return accessControl.hasAccess(_httpHeaders, TargetType.CLUSTER);
Review Comment:
Good advice. I will create new API and make original API deprecated.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2548956591
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -61,7 +62,7 @@ public String getToken() {
}
public boolean hasTable(String tableName) {
- return isTableIncluded(tableName) && isTableNotExcluded(tableName);
+ return StringUtils.isEmpty(tableName) || (isTableIncluded(tableName) && isTableNotExcluded(tableName));
Review Comment:
i see, who is calling this api, shall we change the UI to call other methods like hasClusterAccess etc?
We can add those apis if 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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "hongkunxu (via GitHub)" <gi...@apache.org>.
hongkunxu commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2549042691
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -61,7 +62,7 @@ public String getToken() {
}
public boolean hasTable(String tableName) {
- return isTableIncluded(tableName) && isTableNotExcluded(tableName);
+ return StringUtils.isEmpty(tableName) || (isTableIncluded(tableName) && isTableNotExcluded(tableName));
Review Comment:
Thanks for your suggestion. And I will modify it 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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "hongkunxu (via GitHub)" <gi...@apache.org>.
hongkunxu commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2642561630
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java:
##########
@@ -81,7 +81,7 @@ public boolean verify(@ApiParam(value = "Table name without type") @QueryParam("
@ApiParam(value = "API access type") @DefaultValue("READ") @QueryParam("accessType") AccessType accessType,
@ApiParam(value = "Endpoint URL") @QueryParam("endpointUrl") String endpointUrl) {
AccessControl accessControl = _accessControlFactory.create();
- return accessControl.hasAccess(tableName, accessType, _httpHeaders, endpointUrl);
+ return accessControl.hasAccess(_httpHeaders, TargetType.CLUSTER);
Review Comment:
The other parameters of this interface are always empty and will not be used going forward.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "hongkunxu (via GitHub)" <gi...@apache.org>.
hongkunxu commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2643574566
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -122,24 +122,39 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
private Optional<ZkBasicAuthPrincipal> getPrincipalAuth(RequesterIdentity requesterIdentity) {
Collection<String> tokens = extractAuthorizationTokens(requesterIdentity);
- if (tokens.isEmpty()) {
+ if (tokens == null || tokens.isEmpty()) {
return Optional.empty();
}
- _name2principal = BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
- .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
+ Map<String, ZkBasicAuthPrincipal> name2principal =
+ BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
+ .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
- Map<String, String> name2password = tokens.stream().collect(
- Collectors.toMap(
- org.apache.pinot.common.auth.BasicAuthUtils::extractUsername,
- org.apache.pinot.common.auth.BasicAuthUtils::extractPassword,
- (v1, v2) -> v2));
- Map<String, ZkBasicAuthPrincipal> password2principal =
- name2password.keySet().stream().collect(Collectors.toMap(name2password::get, _name2principal::get));
+ for (String token : tokens) {
+ String username = org.apache.pinot.common.auth.BasicAuthUtils.extractUsername(token);
Review Comment:
<img width="484" height="71" alt="image" src="https://github.com/user-attachments/assets/39cb5f36-0691-4ab2-83aa-a172f3c2622c" />
Hi @xiangfu0 Unfortunately, we cannot do this at the moment, because both pinot-common and pinot-core define a BasicAuthUtils class with the same name, and both classes are used in this method. Supporting this change would require merging the two classes. Do you want me to merge these two classes with the same name?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "hongkunxu (via GitHub)" <gi...@apache.org>.
hongkunxu commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2642637080
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -122,24 +122,39 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
private Optional<ZkBasicAuthPrincipal> getPrincipalAuth(RequesterIdentity requesterIdentity) {
Collection<String> tokens = extractAuthorizationTokens(requesterIdentity);
- if (tokens.isEmpty()) {
+ if (tokens == null || tokens.isEmpty()) {
return Optional.empty();
}
- _name2principal = BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
- .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
+ Map<String, ZkBasicAuthPrincipal> name2principal =
+ BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
+ .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
Review Comment:
This behavior is consistent with the previous logic, and it has been running in the production environment without any issues, even under peak traffic of up to 300 QPS and daily traffic exceeding 1 million requests.
Although this tip has limited practical impact from an engineering perspective, it still makes sense to apply it from a code-quality standpoint.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #17242:
URL: https://github.com/apache/pinot/pull/17242
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "hongkunxu (via GitHub)" <gi...@apache.org>.
hongkunxu commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2643574566
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -122,24 +122,39 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
private Optional<ZkBasicAuthPrincipal> getPrincipalAuth(RequesterIdentity requesterIdentity) {
Collection<String> tokens = extractAuthorizationTokens(requesterIdentity);
- if (tokens.isEmpty()) {
+ if (tokens == null || tokens.isEmpty()) {
return Optional.empty();
}
- _name2principal = BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
- .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
+ Map<String, ZkBasicAuthPrincipal> name2principal =
+ BasicAuthUtils.extractBasicAuthPrincipals(_userCache.getAllBrokerUserConfig()).stream()
+ .collect(Collectors.toMap(BasicAuthPrincipal::getName, p -> p));
- Map<String, String> name2password = tokens.stream().collect(
- Collectors.toMap(
- org.apache.pinot.common.auth.BasicAuthUtils::extractUsername,
- org.apache.pinot.common.auth.BasicAuthUtils::extractPassword,
- (v1, v2) -> v2));
- Map<String, ZkBasicAuthPrincipal> password2principal =
- name2password.keySet().stream().collect(Collectors.toMap(name2password::get, _name2principal::get));
+ for (String token : tokens) {
+ String username = org.apache.pinot.common.auth.BasicAuthUtils.extractUsername(token);
Review Comment:
<img width="484" height="71" alt="image" src="https://github.com/user-attachments/assets/39cb5f36-0691-4ab2-83aa-a172f3c2622c" />
Hi @xiangfu0 Unfortunately, we cannot do this at the moment, because both pinot-common and pinot-core define a BasicAuthUtils class with the same name, and both classes are used in this method. Supporting this change would require merging the two classes.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [Enhance] Optimize user login [pinot]
Posted by "hongkunxu (via GitHub)" <gi...@apache.org>.
hongkunxu commented on code in PR #17242:
URL: https://github.com/apache/pinot/pull/17242#discussion_r2643573242
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java:
##########
@@ -81,7 +81,7 @@ public boolean verify(@ApiParam(value = "Table name without type") @QueryParam("
@ApiParam(value = "API access type") @DefaultValue("READ") @QueryParam("accessType") AccessType accessType,
@ApiParam(value = "Endpoint URL") @QueryParam("endpointUrl") String endpointUrl) {
AccessControl accessControl = _accessControlFactory.create();
- return accessControl.hasAccess(tableName, accessType, _httpHeaders, endpointUrl);
+ return accessControl.hasAccess(_httpHeaders, TargetType.CLUSTER);
Review Comment:
<img width="484" height="71" alt="image" src="https://github.com/user-attachments/assets/39cb5f36-0691-4ab2-83aa-a172f3c2622c" />
Hi @xiangfu0 Unfortunately, we cannot do this at the moment, because both pinot-common and pinot-core define a BasicAuthUtils class with the same name, and both classes are used in this method. Supporting this change would require merging the two classes.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerAuthResource.java:
##########
@@ -81,7 +81,7 @@ public boolean verify(@ApiParam(value = "Table name without type") @QueryParam("
@ApiParam(value = "API access type") @DefaultValue("READ") @QueryParam("accessType") AccessType accessType,
@ApiParam(value = "Endpoint URL") @QueryParam("endpointUrl") String endpointUrl) {
AccessControl accessControl = _accessControlFactory.create();
- return accessControl.hasAccess(tableName, accessType, _httpHeaders, endpointUrl);
+ return accessControl.hasAccess(_httpHeaders, TargetType.CLUSTER);
Review Comment:
<img width="484" height="71" alt="image" src="https://github.com/user-attachments/assets/39cb5f36-0691-4ab2-83aa-a172f3c2622c" />
Hi @xiangfu0 Unfortunately, we cannot do this at the moment, because both pinot-common and pinot-core define a BasicAuthUtils class with the same name, and both classes are used in this method. Supporting this change would require merging the two classes.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org