morrySnow commented on code in PR #66205:
URL: https://github.com/apache/doris/pull/66205#discussion_r3773571424
##########
fe/fe-core/src/main/java/org/apache/doris/httpv2/restv2/MetaInfoActionV2.java:
##########
@@ -117,7 +120,9 @@ public Object getAllCatalogs(
public Object getAllDatabases(
@PathVariable(value = NS_KEY) String ns,
HttpServletRequest request, HttpServletResponse response) {
- checkWithCookie(request, response, false);
+ // Authenticate; the per-object SHOW filters below authorize. See
checkInstanceOverdueIfCloud.
+ ActionAuthorizationInfo authInfo = checkWithCookie(request, response,
false);
+ checkInstanceOverdueIfCloud(authInfo.userIdentity);
Review Comment:
Same catalog-name privilege-check bug remains in this V2 controller. This PR
fixes it in the legacy `MetaInfoAction.getAllDatabases` by passing `ns` to
`checkDbPriv` (instead of hardcoding `InternalCatalog.INTERNAL_CATALOG_NAME`),
but the identical bug is still present in this file, which this PR also
modifies. In `getAllDatabases` (line 139) and `getTables` (line 192) below, the
SHOW privilege is checked against `InternalCatalog.INTERNAL_CATALOG_NAME` even
though `catalogName` may be an external catalog.
`getAccessControllerOrDefault(ctl)` routes on that catalog name, so a non-admin
account listing an external catalog's databases/tables gets filtered against
the *internal* catalog and sees nothing (or wrong results) for external
catalogs. `getTableSchema` (~line 245) already uses `catalogName` correctly.
Suggest applying the same `catalogName` fix here.
##########
fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/MetaInfoAction.java:
##########
@@ -105,22 +106,25 @@ public Object getAllDatabases(
if (catalog == null) {
return ResponseEntityBuilder.badRequest("Unknown catalog " + ns);
}
- List<String> dbNames = new ArrayList<>(catalog.getDbNames());
- List<String> dbNameSet = Lists.newArrayList();
+ // getDbNames() may return an immutable view (see the external meta
cache refactor); this
Review Comment:
Minor / docs-only: the comment's rationale is inaccurate. It claims
`getDbNames()` "may return an immutable view", but the current implementations
all return a fresh mutable copy: `InternalCatalog.getDbNames()` returns
`Lists.newArrayList(fullNameToDb.keySet())`, and `ExternalCatalog.getDbNames()`
returns `metaCache.listNames()` which is
`...stream().map(Pair::value).collect(Collectors.toList())` — a new
`ArrayList`, not an immutable view. The code is safe either way (it iterates
`dbNames` and sorts its own `visibleDbNames` copy), but the stated
justification is wrong and could mislead a future reader into relying on
snapshot semantics that don't actually hold.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
