You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/03/08 10:07:17 UTC
[GitHub] [pinot] INNOCENT-BOY opened a new pull request #8314: Access Control management Development
INNOCENT-BOY opened a new pull request #8314:
URL: https://github.com/apache/pinot/pull/8314
## Description
<!-- Add a description of your PR here.
A good description should include pointers to an issue or design document, etc.
-->
Our team has developed a User console tab in the pinot controller page, to have better access control management. By adopting the user console, the admin can:
1. ADD/DELETE/UPDATE a user
2. ADD/DELETE/UPDATE permissions of tables.
The screenshot below demonstrates how it looks.
Under the current pinot version, the user access control configuration is stored in the config file(such as pinot-controller. properties), and when you try to add or modify users to Pinot. This led to a bad experience because you have to rewrite the config file and restart the controller/broker/server to make this adjustment work.
That is why in our design, we store user config info to Zookeeper (in Helix PropertyStore). More specifically:
- User Configuration store in ZK and encrypted user password via *AES Encryption Algorithm;*
- *The user role has been distinguished by **user** and **admin.** Only admin can have access to the user console page in pinot controller.*
- *You can change on user role without restarting your pinot clusters, and this change happens immediately (*We design a UserCache class based on the watcher mechanism to monitor user config in ZK.*)*
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] NO (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] NO (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
<!-- If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release. -->
<!-- If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text.
-->
## Documentation
<!-- If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
-->
--
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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r822645050
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/AccessControlFactory.java
##########
@@ -29,10 +31,13 @@
public static final String ACCESS_CONTROL_CLASS_CONFIG = "class";
public abstract void init(PinotConfiguration confguration);
+ public abstract void init(ZkHelixPropertyStore<ZNRecord> propertyStore);
+
public abstract AccessControl create();
- public static AccessControlFactory loadFactory(PinotConfiguration configuration) {
+ public static AccessControlFactory loadFactory(PinotConfiguration configuration,
+ ZkHelixPropertyStore<ZNRecord> propertyStore) {
Review comment:
@apucher Thanks agian, To keep backward compatibility with existing auth implementations, I have reconstructed relevant classes in controller, broker, and server. Please check the new design 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: 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
[GitHub] [pinot] yuxuejiao commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
yuxuejiao commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r826531459
##########
File path: pinot-controller/src/main/resources/app/pages/UserPage.tsx
##########
@@ -0,0 +1,301 @@
+/* eslint-disable no-nested-ternary */
+/**
+ * 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.
+ */
+
+import React, {useEffect, useState} from 'react';
+import {Button, Paper, Table, TableBody, TableCell, TableContainer, TableHead, TableRow, Dialog, DialogActions, Snackbar, DialogTitle} from '@material-ui/core';
+import Alert from '@material-ui/lab/Alert';
+import { makeStyles } from '@material-ui/core/styles';
+
+import 'codemirror/lib/codemirror.css';
+import 'codemirror/theme/material.css';
+import 'codemirror/mode/javascript/javascript';
+import 'codemirror/mode/sql/sql';
+import 'codemirror/addon/hint/show-hint';
+import 'codemirror/addon/hint/sql-hint';
+import 'codemirror/addon/hint/show-hint.css';
Review comment:
Thanks for your review, I will remove them 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
[GitHub] [pinot] joshigaurava commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
joshigaurava commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r826182444
##########
File path: pinot-controller/src/main/resources/app/pages/UserPage.tsx
##########
@@ -0,0 +1,301 @@
+/* eslint-disable no-nested-ternary */
+/**
+ * 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.
+ */
+
+import React, {useEffect, useState} from 'react';
+import {Button, Paper, Table, TableBody, TableCell, TableContainer, TableHead, TableRow, Dialog, DialogActions, Snackbar, DialogTitle} from '@material-ui/core';
+import Alert from '@material-ui/lab/Alert';
+import { makeStyles } from '@material-ui/core/styles';
+
+import 'codemirror/lib/codemirror.css';
+import 'codemirror/theme/material.css';
+import 'codemirror/mode/javascript/javascript';
+import 'codemirror/mode/sql/sql';
+import 'codemirror/addon/hint/show-hint';
+import 'codemirror/addon/hint/sql-hint';
+import 'codemirror/addon/hint/show-hint.css';
Review comment:
If I am not mistaken, these codemirror imports are unused? Can we please remove them?
--
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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r827826870
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
##########
@@ -93,4 +93,21 @@ public void validatePermission(Optional<String> tableNameOpt, AccessType accessT
Response.Status.FORBIDDEN);
}
}
+
+ public void validatePermission(HttpHeaders httpHeaders, String endpointUrl,
+ AccessControl accessControl) {
+ validatePermission("you", httpHeaders, endpointUrl, accessControl);
Review comment:
@apucher The original idea is to reuse the same name method. Now I have integrated the two methods into one.
--
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
[GitHub] [pinot] apucher commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r822179052
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/Constants.java
##########
@@ -59,6 +61,18 @@ public static TableType validateTableType(String tableTypeStr) {
}
}
+ public static ComponentType validateComponentType(String componentTypeStr) {
+ if (componentTypeStr == null || componentTypeStr.isEmpty()) {
+ return null;
+ }
Review comment:
you could us `StringUtils.isBlank` here
--
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
[GitHub] [pinot] yuxuejiao commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
yuxuejiao commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r826532763
##########
File path: pinot-controller/src/main/resources/app/components/User/UpdateUser.tsx
##########
@@ -0,0 +1,235 @@
+/**
+ * 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.
+ */
+
+import React, { useState } from 'react';
+import {
+ Checkbox,
+ DialogContent,
+ FormControl,
+ IconButton,
+ Input, InputAdornment,
+ InputLabel, ListItemText,
+ makeStyles,
+ MenuItem,
+ Select,
+} from '@material-ui/core';
+import Visibility from "@material-ui/icons/Visibility";
+import VisibilityOff from "@material-ui/icons/VisibilityOff";
+
+type Props = {
+ tableList: Array<{
+ name: string,
+ checked: boolean,
+ disabled: boolean
+ }>,
+ setUserInfo: Function,
+ userProp?: {
+ username: string,
+ password: string,
+ component: string,
+ role: string,
+ tables: Array<string>,
+ permissions: Array<string>
+ }
+}
+
+export default function UpdateUser({
+ tableList,
+ setUserInfo,
+ userProp
+}: Props) {
+
+ const PERMISSIONS = ["READ", "CREATE", "UPDATE", "DELETE"];
+ const [user, setUser] = useState(userProp);
+ const transferTable = tableList.map(item=>{
+ return {
+ ...item,
+ checked: !!userProp.tables.includes(item.name)
+ }
+ })
+ const [tables, setTables] = useState(transferTable);
+ const [showPassword, setShowPassword] = useState(false);
+ const handleClickShowPassword = () => {
+ setShowPassword(!showPassword);
+ };
+ const handleMouseDownPassword = (event) => {
+ event.preventDefault();
+ };
+ const changeHandler = (field, e)=>{
+ let val = e.target.value;
+ let userCopy = {...user};
Review comment:
You are right. Thanks for your suggestion. I will fix 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
[GitHub] [pinot] apucher commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r822176863
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/provider/UserCache.java
##########
@@ -0,0 +1,176 @@
+/**
+ * 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.pinot.common.config.provider;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.stream.Collectors;
+import javax.annotation.Nullable;
+import org.I0Itec.zkclient.IZkChildListener;
+import org.I0Itec.zkclient.IZkDataListener;
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.common.utils.config.UserConfigUtils;
+import org.apache.pinot.spi.config.user.ComponentType;
+import org.apache.pinot.spi.config.user.UserConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class UserCache {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(UserCache.class);
+ private static final String USER_CONFIG_PARENT_PATH = "/CONFIGS/USER";
+ private static final String USER_CONFIG_PATH_PREFIX = "/CONFIGS/USER/";
+
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+
+ private final UserConfigChangeListener _userConfigChangeListener = new UserConfigChangeListener();
+
+ private final Map<String, UserConfig> _userConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userControllerConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userBrokerConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userServerConfigMap = new ConcurrentHashMap<>();
Review comment:
if you intend to configure all components dynamically, you'll need to track `minion` as well.
@xiangfu0 should we track dedicated permissions for the upcoming pinot proxy as well?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthUtils.java
##########
@@ -56,26 +58,29 @@ private BasicAuthUtils() {
* my.prefix.access.control.principals.user456.permissions=read,update
* </pre>
*
- * @param configuration pinot configuration
- * @param prefix configuration namespace
+ * @param userConfigList user configuration list
* @return list of BasicAuthPrincipals
*/
- public static List<BasicAuthPrincipal> extractBasicAuthPrincipals(PinotConfiguration configuration, String prefix) {
- String principalNames = configuration.getProperty(prefix);
- Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals");
+ public static List<BasicAuthPrincipal> extractBasicAuthPrincipals(List<UserConfig> userConfigList) {
+ return userConfigList.stream()
+ .map(user -> {
+ String name = user.getUserName().trim();
+ Preconditions.checkArgument(StringUtils.isNoneBlank(name), "%s is not a valid username", name);
+ String password = user.getPassword().trim();
+ Preconditions.checkArgument(StringUtils.isNoneBlank(password), "must provide a password for %s", name);
Review comment:
should this be `isNotBlank`?
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/user/UserConfig.java
##########
@@ -0,0 +1,143 @@
+/**
+ * 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.pinot.spi.config.user;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonPropertyDescription;
+import com.google.common.base.Preconditions;
+import java.util.List;
+import java.util.Objects;
+import javax.annotation.Nullable;
+import org.apache.pinot.spi.config.BaseJsonConfig;
+
+
+public class UserConfig extends BaseJsonConfig {
+ public static final String USERNAME_KEY = "username";
+ public static final String PASSWORD_KEY = "password";
+ public static final String COMPONET_KEY = "component";
+ public static final String ROLE_KEY = "role";
+ public static final String AUTH_TOKEN_KEY = "authToken";
+ public static final String TABLES_KEY = "tables";
+ public static final String PERMISSIONS_KEY = "permissions";
+
+ @JsonPropertyDescription("The name of User")
+ private String _username;
+
+ @JsonPropertyDescription("The password of User")
+ private String _password;
+
+ @JsonPropertyDescription("The name of Component")
+ private ComponentType _componentType;
+
+ @JsonPropertyDescription("The role of user")
+ private RoleType _roleType;
+
+ @JsonPropertyDescription("The tables owned of User")
+ private List<String> _tables;
+
+ @JsonPropertyDescription("The table permission of User")
+ private List<AccessType> _permissions;
+
+ @JsonCreator
+ public UserConfig(@JsonProperty(value = USERNAME_KEY, required = true) String username,
+ @JsonProperty(value = PASSWORD_KEY, required = true) String password,
+ @JsonProperty(value = COMPONET_KEY, required = true) String component,
+ @JsonProperty(value = ROLE_KEY, required = true) String role,
+ @JsonProperty(value = TABLES_KEY) @Nullable List<String> tableList,
+ @JsonProperty(value = PERMISSIONS_KEY) @Nullable List<AccessType> permissionList
+ ) {
+ Preconditions.checkArgument(username != null, "'username' must be configured");
+ Preconditions.checkArgument(password != null, "'password' must be configured");
+
+ // NOTE: Handle lower case table type and raw table name for backward-compatibility
+ _username = username;
+ _password = password;
+ _componentType = ComponentType.valueOf(component.toUpperCase());
+ _roleType = RoleType.valueOf(role.toUpperCase());
+ _tables = tableList;
+ _permissions = permissionList;
+ }
+
+ @JsonProperty(USERNAME_KEY)
+ public String getUserName() {
+ return _username;
+ }
+
+ public String getUsernameWithComponent() {
+ return getUserName() + "_" + getComponentType().toString();
+ }
+
+ public boolean isExist(String username, ComponentType component) {
+ return _username.equals(username) && _componentType.equals(component);
+ }
+
+ @JsonProperty(PASSWORD_KEY)
+ public String getPassword() {
+ return _password;
+ }
+
+ @JsonProperty(TABLES_KEY)
+ public List<String> getTables() {
+ return _tables;
+ }
+
+ @JsonProperty(PERMISSIONS_KEY)
+ public List<AccessType> getPermissios() {
+ return _permissions;
+ }
+
+ @JsonProperty(COMPONET_KEY)
+ public ComponentType getComponentType() {
+ return _componentType;
+ }
+
+ @JsonProperty(ROLE_KEY)
+ public RoleType getRoleType() {
+ return _roleType;
+ }
+
+ public void setRole(String roleTypeStr) {
+ _roleType = RoleType.valueOf(roleTypeStr);
+ }
+
+ public void setPassword(String password) {
+ _password = password;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ if (!super.equals(o)) {
+ return false;
+ }
+ UserConfig that = (UserConfig) o;
+ return _username.equals(that._username) && _componentType == that._componentType;
+ }
Review comment:
this might lead to unexpected behavior. what's the rationale for using `username` and `component` only?
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -55,8 +58,13 @@ public BasicAuthAccessControlFactory() {
// left blank
}
Review comment:
This whole class should be factored out into a separate implementation. We have to ensure backwards-compatibility for the existing BasicAuth implementation. You could move your implementation to a new `ZkBasicAuth` class or similar.
This has the further benefit that your whole PR may become fully backwards compatible.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlFactory.java
##########
@@ -30,5 +32,8 @@ default void init(PinotConfiguration pinotConfiguration) {
// left blank
}
+ default void init(ZkHelixPropertyStore<ZNRecord> zkHelixPropertyStore) {
+ }
+
Review comment:
similar to broker, backwards-compatibility is key. I suggest a `init(ControllerConf, ZkHelixPropertyStore<ZNRecord>)` here that calls `init(ControllerConf)` by default
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/auth/BasicAuthTest.java
##########
@@ -28,24 +30,30 @@
@Test
Review comment:
move implementation to new class, retain backward-compatibility.
##########
File path: pinot-core/src/main/java/org/apache/pinot/server/access/BasicAuthAccessFactory.java
##########
@@ -37,10 +40,16 @@
private AccessControl _accessControl;
+ @Override
public void init(PinotConfiguration configuration) {
- _accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
+// _accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
}
+ public void init(ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ _accessControl = new BasicAuthAccessControl(new UserCache(propertyStore));
+ }
Review comment:
same as broker and controller. extend the interface and default to legacy behavior. also, move the implementation to its own class.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/EncryptUtils.java
##########
@@ -0,0 +1,180 @@
+/**
+ * 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.pinot.common.utils;
+
+import java.util.Base64;
+import javax.crypto.Cipher;
+import javax.crypto.spec.SecretKeySpec;
+import org.apache.commons.lang3.StringUtils;
+
+/**
+ * AES Encryption Algorithm
+ *
+ */
+public class EncryptUtils {
+
+ /**
+ * encryption and decryption keys, even external
+ */
+ public static final String AES_DATA_SECURITY_KEY = "4%YkW!@g5LGcf9Ut";
Review comment:
+1 for storing passwords in encrypted form. I suggest having a look at `bcrypt` as an alternative since a hard-coded password is easily reverse-engineered. Chances are, you'll be able to replace this entire util with a native implementation.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/provider/UserCache.java
##########
@@ -0,0 +1,176 @@
+/**
+ * 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.pinot.common.config.provider;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.stream.Collectors;
+import javax.annotation.Nullable;
+import org.I0Itec.zkclient.IZkChildListener;
+import org.I0Itec.zkclient.IZkDataListener;
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.common.utils.config.UserConfigUtils;
+import org.apache.pinot.spi.config.user.ComponentType;
+import org.apache.pinot.spi.config.user.UserConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class UserCache {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(UserCache.class);
+ private static final String USER_CONFIG_PARENT_PATH = "/CONFIGS/USER";
+ private static final String USER_CONFIG_PATH_PREFIX = "/CONFIGS/USER/";
+
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+
+ private final UserConfigChangeListener _userConfigChangeListener = new UserConfigChangeListener();
+
+ private final Map<String, UserConfig> _userConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userControllerConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userBrokerConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userServerConfigMap = new ConcurrentHashMap<>();
+
+ public UserCache(ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ _propertyStore = propertyStore;
+ synchronized (_userConfigChangeListener) {
Review comment:
what's the rationale behind `synchronized` here?
##########
File path: pinot-controller/src/main/resources/app/App.tsx
##########
@@ -52,6 +52,29 @@ const App = () => {
const [authorizationEndpoint, setAuthorizationEndpoint] = React.useState(
null
);
+ const [role, setRole] = React.useState('');
Review comment:
@joshigaurava could you have a look here?
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/user/ComponentType.java
##########
@@ -0,0 +1,23 @@
+/**
+ * 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.pinot.spi.config.user;
+
+public enum ComponentType {
+ CONTROLLER, BROKER, SERVER
Review comment:
+ minion, proxy
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/AccessControlFactory.java
##########
@@ -29,10 +31,13 @@
public static final String ACCESS_CONTROL_CLASS_CONFIG = "class";
public abstract void init(PinotConfiguration confguration);
+ public abstract void init(ZkHelixPropertyStore<ZNRecord> propertyStore);
+
public abstract AccessControl create();
- public static AccessControlFactory loadFactory(PinotConfiguration configuration) {
+ public static AccessControlFactory loadFactory(PinotConfiguration configuration,
+ ZkHelixPropertyStore<ZNRecord> propertyStore) {
Review comment:
We can't break backwards-compatibility with existing auth implementations - however, we can add an interface here that receives both `PinotConfiguration` and `ZkHelixPropertyStore` and has a default implementation that simply calls the legacy `init(PinotConfiguration)`
The BrokerStarter can then call the enhance interface. Existing AuthFactories will simply default to the existing behavior, while your new implementation can override and use the enhanced one.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/provider/UserCache.java
##########
@@ -0,0 +1,176 @@
+/**
+ * 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.pinot.common.config.provider;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.stream.Collectors;
+import javax.annotation.Nullable;
+import org.I0Itec.zkclient.IZkChildListener;
+import org.I0Itec.zkclient.IZkDataListener;
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.common.utils.config.UserConfigUtils;
+import org.apache.pinot.spi.config.user.ComponentType;
+import org.apache.pinot.spi.config.user.UserConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class UserCache {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(UserCache.class);
+ private static final String USER_CONFIG_PARENT_PATH = "/CONFIGS/USER";
+ private static final String USER_CONFIG_PATH_PREFIX = "/CONFIGS/USER/";
Review comment:
I like the overall approach here. @Jackie-Jiang @xiangfu0 may have some thoughts on ZK paths here and below
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/user/AccessType.java
##########
@@ -0,0 +1,23 @@
+/**
+ * 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.pinot.spi.config.user;
+
+public enum AccessType {
+ CREATE, READ, UPDATE, DELETE
+}
Review comment:
@Jackie-Jiang @xiangfu0 should the SPI changes here go into a different package?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -50,8 +53,9 @@
Review comment:
Similar to Broker auth, this should be in its own class for backwards-compatibility.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
##########
@@ -93,4 +93,21 @@ public void validatePermission(Optional<String> tableNameOpt, AccessType accessT
Response.Status.FORBIDDEN);
}
}
+
+ public void validatePermission(HttpHeaders httpHeaders, String endpointUrl,
+ AccessControl accessControl) {
+ validatePermission("you", httpHeaders, endpointUrl, accessControl);
Review comment:
what's the rationale behind a hard-coded `you` user here?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/Constants.java
##########
@@ -59,6 +61,18 @@ public static TableType validateTableType(String tableTypeStr) {
}
}
+ public static ComponentType validateComponentType(String componentTypeStr) {
+ if (componentTypeStr == null || componentTypeStr.isEmpty()) {
+ return null;
+ }
Review comment:
you could us `StringUtils.isNotNull` here
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1268,6 +1274,32 @@ public Schema getSchemaForTableConfig(TableConfig tableConfig) {
AccessOption.PERSISTENT);
}
+ public void initUserACLConfig() throws IOException {
+ if (Optional.ofNullable(ZKMetadataProvider.getAllUserName(_propertyStore)).isEmpty()) {
+ addUser(new UserConfig("admin", "Pda@wap%admin", ComponentType.CONTROLLER.name(),
+ RoleType.ADMIN.name(), null, null));
+ addUser(new UserConfig("admin", "Pda@wap%admin", ComponentType.BROKER.name(),
+ RoleType.ADMIN.name(), null, null));
+ addUser(new UserConfig("admin", "Pda@wap%admin", ComponentType.SERVER.name(),
+ RoleType.ADMIN.name(), null, null));
+ }
Review comment:
I suggest factoring-out the bootstrapping to plain old properties injection. That way, anyone using pinot can bootstrap the cluster with a set of defined yet deployment-specific credentials and then mutate in zookeeper as 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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r822673606
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/user/UserConfig.java
##########
@@ -0,0 +1,143 @@
+/**
+ * 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.pinot.spi.config.user;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonPropertyDescription;
+import com.google.common.base.Preconditions;
+import java.util.List;
+import java.util.Objects;
+import javax.annotation.Nullable;
+import org.apache.pinot.spi.config.BaseJsonConfig;
+
+
+public class UserConfig extends BaseJsonConfig {
+ public static final String USERNAME_KEY = "username";
+ public static final String PASSWORD_KEY = "password";
+ public static final String COMPONET_KEY = "component";
+ public static final String ROLE_KEY = "role";
+ public static final String AUTH_TOKEN_KEY = "authToken";
+ public static final String TABLES_KEY = "tables";
+ public static final String PERMISSIONS_KEY = "permissions";
+
+ @JsonPropertyDescription("The name of User")
+ private String _username;
+
+ @JsonPropertyDescription("The password of User")
+ private String _password;
+
+ @JsonPropertyDescription("The name of Component")
+ private ComponentType _componentType;
+
+ @JsonPropertyDescription("The role of user")
+ private RoleType _roleType;
+
+ @JsonPropertyDescription("The tables owned of User")
+ private List<String> _tables;
+
+ @JsonPropertyDescription("The table permission of User")
+ private List<AccessType> _permissions;
+
+ @JsonCreator
+ public UserConfig(@JsonProperty(value = USERNAME_KEY, required = true) String username,
+ @JsonProperty(value = PASSWORD_KEY, required = true) String password,
+ @JsonProperty(value = COMPONET_KEY, required = true) String component,
+ @JsonProperty(value = ROLE_KEY, required = true) String role,
+ @JsonProperty(value = TABLES_KEY) @Nullable List<String> tableList,
+ @JsonProperty(value = PERMISSIONS_KEY) @Nullable List<AccessType> permissionList
+ ) {
+ Preconditions.checkArgument(username != null, "'username' must be configured");
+ Preconditions.checkArgument(password != null, "'password' must be configured");
+
+ // NOTE: Handle lower case table type and raw table name for backward-compatibility
+ _username = username;
+ _password = password;
+ _componentType = ComponentType.valueOf(component.toUpperCase());
+ _roleType = RoleType.valueOf(role.toUpperCase());
+ _tables = tableList;
+ _permissions = permissionList;
+ }
+
+ @JsonProperty(USERNAME_KEY)
+ public String getUserName() {
+ return _username;
+ }
+
+ public String getUsernameWithComponent() {
+ return getUserName() + "_" + getComponentType().toString();
+ }
+
+ public boolean isExist(String username, ComponentType component) {
+ return _username.equals(username) && _componentType.equals(component);
+ }
+
+ @JsonProperty(PASSWORD_KEY)
+ public String getPassword() {
+ return _password;
+ }
+
+ @JsonProperty(TABLES_KEY)
+ public List<String> getTables() {
+ return _tables;
+ }
+
+ @JsonProperty(PERMISSIONS_KEY)
+ public List<AccessType> getPermissios() {
+ return _permissions;
+ }
+
+ @JsonProperty(COMPONET_KEY)
+ public ComponentType getComponentType() {
+ return _componentType;
+ }
+
+ @JsonProperty(ROLE_KEY)
+ public RoleType getRoleType() {
+ return _roleType;
+ }
+
+ public void setRole(String roleTypeStr) {
+ _roleType = RoleType.valueOf(roleTypeStr);
+ }
+
+ public void setPassword(String password) {
+ _password = password;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ if (!super.equals(o)) {
+ return false;
+ }
+ UserConfig that = (UserConfig) o;
+ return _username.equals(that._username) && _componentType == that._componentType;
+ }
Review comment:
@apucher Thanks for your considerable consider, but in our implementation, this Class UserConfig represents Auth User Entity that is stored in ZK. And both username and component fields could enough identify unique user entity.
--
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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r828006135
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1268,6 +1274,32 @@ public Schema getSchemaForTableConfig(TableConfig tableConfig) {
AccessOption.PERSISTENT);
}
+ public void initUserACLConfig() throws IOException {
+ if (Optional.ofNullable(ZKMetadataProvider.getAllUserName(_propertyStore)).isEmpty()) {
+ addUser(new UserConfig("admin", "Pda@wap%admin", ComponentType.CONTROLLER.name(),
+ RoleType.ADMIN.name(), null, null));
+ addUser(new UserConfig("admin", "Pda@wap%admin", ComponentType.BROKER.name(),
+ RoleType.ADMIN.name(), null, null));
+ addUser(new UserConfig("admin", "Pda@wap%admin", ComponentType.SERVER.name(),
+ RoleType.ADMIN.name(), null, null));
+ }
Review comment:
@apucher You are right. I have set DEFAULT_ACCESS_CONTROL_USERNAME and DEFAULT_ACCESS_CONTROL_P
ASSWORD. These two variable has default value. And user can reset these two value via controller configuration file.
--
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
[GitHub] [pinot] apucher commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r836093058
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/AllowAllAccessControlFactory.java
##########
@@ -34,6 +36,10 @@ public AllowAllAccessControlFactory() {
public void init(PinotConfiguration configuration) {
}
+ public void init(ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ }
+
+ @Override
Review comment:
see above. if there's any change in this file, it's not backwards-compatible
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/AccessControlFactory.java
##########
@@ -29,6 +31,7 @@
public static final String ACCESS_CONTROL_CLASS_CONFIG = "class";
public abstract void init(PinotConfiguration confguration);
+ public abstract void init(ZkHelixPropertyStore<ZNRecord> propertyStore);
Review comment:
See suggestion below. I'd make this a non-abstract method that takes both `PinotConfiguration` and `ZkHelixPropertyStore` and calls the legacy `init(PinotConfiguration)`. Then, add override this method in your new code while old code remains unaffected. You'll also want to document this intended use in the javadoc
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/AccessControlFactory.java
##########
@@ -29,10 +31,13 @@
public static final String ACCESS_CONTROL_CLASS_CONFIG = "class";
public abstract void init(PinotConfiguration confguration);
+ public abstract void init(ZkHelixPropertyStore<ZNRecord> propertyStore);
+
public abstract AccessControl create();
- public static AccessControlFactory loadFactory(PinotConfiguration configuration) {
+ public static AccessControlFactory loadFactory(PinotConfiguration configuration,
+ ZkHelixPropertyStore<ZNRecord> propertyStore) {
Review comment:
Great. IMO you can change the signature of the static `loadFactory` to include both configuration and property store. Since it's static it's not really part of the interface and there's no need to duplicate the code.
Rather that using `instanceof` I'd suggest to introduce an `init(PinotConfiguration, ZkHelixPropertyStore)` that defaults to invoking the legacy `init(PinotConfiguration)` method. That way, you achieve backwards compatibility and your new code can override the expanded method and take advantage of ZK access
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/BcryptUtils.java
##########
@@ -0,0 +1,50 @@
+/**
+ * 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.pinot.common.utils;
+
+import org.mindrot.jbcrypt.BCrypt;
+
+public class BcryptUtils {
Review comment:
awesome.
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/provider/UserCache.java
##########
@@ -0,0 +1,176 @@
+/**
+ * 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.pinot.common.config.provider;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.stream.Collectors;
+import javax.annotation.Nullable;
+import org.I0Itec.zkclient.IZkChildListener;
+import org.I0Itec.zkclient.IZkDataListener;
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.common.utils.config.UserConfigUtils;
+import org.apache.pinot.spi.config.user.ComponentType;
+import org.apache.pinot.spi.config.user.UserConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class UserCache {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(UserCache.class);
+ private static final String USER_CONFIG_PARENT_PATH = "/CONFIGS/USER";
+ private static final String USER_CONFIG_PATH_PREFIX = "/CONFIGS/USER/";
+
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+
+ private final UserConfigChangeListener _userConfigChangeListener = new UserConfigChangeListener();
+
+ private final Map<String, UserConfig> _userConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userControllerConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userBrokerConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userServerConfigMap = new ConcurrentHashMap<>();
Review comment:
it's fine. we'll extend this whenever we need any properties for minion or proxy
##########
File path: pinot-common/pom.xml
##########
@@ -301,6 +301,11 @@
<groupId>org.apache.yetus</groupId>
<artifactId>audience-annotations</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.mindrot</groupId>
+ <artifactId>jbcrypt</artifactId>
+ <version>0.4</version>
+ </dependency>
Review comment:
@Jackie-Jiang do we have bcrypt available somewhere in the existing dependencies?
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import java.util.Collection;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.config.provider.UserCache;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.utils.BcryptUtils;
+import org.apache.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.core.auth.ZkBasicAuthPrincipal;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ * pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
Review comment:
would you mind updating the javadoc?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -239,7 +241,10 @@ private static long getRandomInitialDelayInSeconds() {
private static final boolean DEFAULT_ENABLE_SPLIT_COMMIT = true;
private static final int DEFAULT_JERSEY_ADMIN_PORT = 21000;
private static final String DEFAULT_ACCESS_CONTROL_FACTORY_CLASS =
- "org.apache.pinot.controller.api.access.AllowAllAccessFactory";
+ "org.apache.pinot.controller.api.access.ZkBasicAuthAccessControlFactory";
+// "org.apache.pinot.controller.api.access.AllowAllAccessFactory";
Review comment:
change to legacy behavior?
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -55,10 +57,16 @@ public BasicAuthAccessControlFactory() {
// left blank
}
+ @Override
public void init(PinotConfiguration configuration) {
_accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
}
+ @Override
+ public void init(ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ }
+
+ @Override
Review comment:
same here.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/ZkBasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,132 @@
+/**
+ * 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.pinot.controller.api.access;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import javax.ws.rs.core.HttpHeaders;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.common.config.provider.UserCache;
+import org.apache.pinot.common.utils.BcryptUtils;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.core.auth.ZkBasicAuthPrincipal;
+import org.apache.pinot.spi.config.user.ComponentType;
+import org.apache.pinot.spi.config.user.RoleType;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "controller.admin.access.control" family of
+ * properties.
+ *
+ * <pre>
+ * Example:
+ * controller.admin.access.control.principals=admin123,user456
+ * controller.admin.access.control.principals.admin123.password=verysecret
+ * controller.admin.access.control.principals.user456.password=kindasecret
+ * controller.admin.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * controller.admin.access.control.principals.user456.permissions=read,update
+ * </pre>
+ */
Review comment:
javadoc
--
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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r838634927
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/AccessControlFactory.java
##########
@@ -29,10 +31,13 @@
public static final String ACCESS_CONTROL_CLASS_CONFIG = "class";
public abstract void init(PinotConfiguration confguration);
+ public abstract void init(ZkHelixPropertyStore<ZNRecord> propertyStore);
+
public abstract AccessControl create();
- public static AccessControlFactory loadFactory(PinotConfiguration configuration) {
+ public static AccessControlFactory loadFactory(PinotConfiguration configuration,
+ ZkHelixPropertyStore<ZNRecord> propertyStore) {
Review comment:
@apucher Thanks for your suggestions. I have changed the signature of the static loadFactory to include both configuration and property store.
Also I have reconstructed AccessControlFactory and relevant class about Controller/Broker/Server. And introduce an init(PinotConfiguration, ZkHelixPropertyStore) that defaults to invoking the legacy init(PinotConfiguration) method Rather that using instanceof. Looking forward your review 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: 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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r838642628
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
##########
@@ -239,7 +241,10 @@ private static long getRandomInitialDelayInSeconds() {
private static final boolean DEFAULT_ENABLE_SPLIT_COMMIT = true;
private static final int DEFAULT_JERSEY_ADMIN_PORT = 21000;
private static final String DEFAULT_ACCESS_CONTROL_FACTORY_CLASS =
- "org.apache.pinot.controller.api.access.AllowAllAccessFactory";
+ "org.apache.pinot.controller.api.access.ZkBasicAuthAccessControlFactory";
+// "org.apache.pinot.controller.api.access.AllowAllAccessFactory";
Review comment:
Sure.
--
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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r838641620
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import java.util.Collection;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.config.provider.UserCache;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.utils.BcryptUtils;
+import org.apache.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.core.auth.ZkBasicAuthPrincipal;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ * pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
Review comment:
Yap, I have updated this javadoc in this class.
--
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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r838643020
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/ZkBasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,132 @@
+/**
+ * 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.pinot.controller.api.access;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import javax.ws.rs.core.HttpHeaders;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.common.config.provider.UserCache;
+import org.apache.pinot.common.utils.BcryptUtils;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.core.auth.ZkBasicAuthPrincipal;
+import org.apache.pinot.spi.config.user.ComponentType;
+import org.apache.pinot.spi.config.user.RoleType;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "controller.admin.access.control" family of
+ * properties.
+ *
+ * <pre>
+ * Example:
+ * controller.admin.access.control.principals=admin123,user456
+ * controller.admin.access.control.principals.admin123.password=verysecret
+ * controller.admin.access.control.principals.user456.password=kindasecret
+ * controller.admin.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * controller.admin.access.control.principals.user456.permissions=read,update
+ * </pre>
+ */
Review comment:
Sure.
--
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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r838639689
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/AllowAllAccessControlFactory.java
##########
@@ -34,6 +36,10 @@ public AllowAllAccessControlFactory() {
public void init(PinotConfiguration configuration) {
}
+ public void init(ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ }
+
+ @Override
Review comment:
@apucher Thanks for your suggestions. I have changed the signature of the static loadFactory to include both configuration and property store.
Also I have reconstructed AccessControlFactory and relevant class about Controller/Broker/Server. And introduce an init(PinotConfiguration, ZkHelixPropertyStore) that defaults to invoking the legacy init(PinotConfiguration) method Rather that using instanceof. Looking forward your review 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: 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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r838637904
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlFactory.java
##########
@@ -30,5 +32,8 @@ default void init(PinotConfiguration pinotConfiguration) {
// left blank
}
+ default void init(ZkHelixPropertyStore<ZNRecord> zkHelixPropertyStore) {
+ }
+
Review comment:
@apucher Thanks for your suggestions. I introduce an init(PinotConfiguration, ZkHelixPropertyStore) that defaults to invoking the legacy init(PinotConfiguration) method Rather that using instanceof. Looking forward your review 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: 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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r828006135
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1268,6 +1274,32 @@ public Schema getSchemaForTableConfig(TableConfig tableConfig) {
AccessOption.PERSISTENT);
}
+ public void initUserACLConfig() throws IOException {
+ if (Optional.ofNullable(ZKMetadataProvider.getAllUserName(_propertyStore)).isEmpty()) {
+ addUser(new UserConfig("admin", "Pda@wap%admin", ComponentType.CONTROLLER.name(),
+ RoleType.ADMIN.name(), null, null));
+ addUser(new UserConfig("admin", "Pda@wap%admin", ComponentType.BROKER.name(),
+ RoleType.ADMIN.name(), null, null));
+ addUser(new UserConfig("admin", "Pda@wap%admin", ComponentType.SERVER.name(),
+ RoleType.ADMIN.name(), null, null));
+ }
Review comment:
@apucher You are right. I have set DEFAULT_ACCESS_CONTROL_USERNAME and DEFAULT_ACCESS_CONTROL_PASSWORD. These two variables have default value. And user can set these two value via controller configuration file.
--
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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r827826870
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
##########
@@ -93,4 +93,21 @@ public void validatePermission(Optional<String> tableNameOpt, AccessType accessT
Response.Status.FORBIDDEN);
}
}
+
+ public void validatePermission(HttpHeaders httpHeaders, String endpointUrl,
+ AccessControl accessControl) {
+ validatePermission("you", httpHeaders, endpointUrl, accessControl);
Review comment:
@apucher The original idea is to reuse the same name method. Now I have integrated the two methods into one. I have deleted this abrupt word "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: 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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r827844941
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/Constants.java
##########
@@ -59,6 +61,18 @@ public static TableType validateTableType(String tableTypeStr) {
}
}
+ public static ComponentType validateComponentType(String componentTypeStr) {
+ if (componentTypeStr == null || componentTypeStr.isEmpty()) {
+ return null;
+ }
Review comment:
Thx， I have accepted your advice. @apucher
--
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
[GitHub] [pinot] apucher commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r822179052
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/Constants.java
##########
@@ -59,6 +61,18 @@ public static TableType validateTableType(String tableTypeStr) {
}
}
+ public static ComponentType validateComponentType(String componentTypeStr) {
+ if (componentTypeStr == null || componentTypeStr.isEmpty()) {
+ return null;
+ }
Review comment:
you could us `StringUtils.isNotBlank` here
--
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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r833049859
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/EncryptUtils.java
##########
@@ -0,0 +1,180 @@
+/**
+ * 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.pinot.common.utils;
+
+import java.util.Base64;
+import javax.crypto.Cipher;
+import javax.crypto.spec.SecretKeySpec;
+import org.apache.commons.lang3.StringUtils;
+
+/**
+ * AES Encryption Algorithm
+ *
+ */
+public class EncryptUtils {
+
+ /**
+ * encryption and decryption keys, even external
+ */
+ public static final String AES_DATA_SECURITY_KEY = "4%YkW!@g5LGcf9Ut";
Review comment:
@apucher I have already give a bcrypt implementation. Please to check 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: 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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r822639887
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthUtils.java
##########
@@ -56,26 +58,29 @@ private BasicAuthUtils() {
* my.prefix.access.control.principals.user456.permissions=read,update
* </pre>
*
- * @param configuration pinot configuration
- * @param prefix configuration namespace
+ * @param userConfigList user configuration list
* @return list of BasicAuthPrincipals
*/
- public static List<BasicAuthPrincipal> extractBasicAuthPrincipals(PinotConfiguration configuration, String prefix) {
- String principalNames = configuration.getProperty(prefix);
- Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals");
+ public static List<BasicAuthPrincipal> extractBasicAuthPrincipals(List<UserConfig> userConfigList) {
+ return userConfigList.stream()
+ .map(user -> {
+ String name = user.getUserName().trim();
+ Preconditions.checkArgument(StringUtils.isNoneBlank(name), "%s is not a valid username", name);
+ String password = user.getPassword().trim();
+ Preconditions.checkArgument(StringUtils.isNoneBlank(password), "must provide a password for %s", name);
Review comment:
@apucher Thanks for checking, I have corrected 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: 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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r822653032
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/user/ComponentType.java
##########
@@ -0,0 +1,23 @@
+/**
+ * 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.pinot.spi.config.user;
+
+public enum ComponentType {
+ CONTROLLER, BROKER, SERVER
Review comment:
@apucher Currently, Only Controller、broker, and server components need Auth Access Control. I don't think it appends minion and proxy.
--
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
[GitHub] [pinot] yuxuejiao commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
yuxuejiao commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r826532837
##########
File path: pinot-controller/src/main/resources/app/components/User/AddUser.tsx
##########
@@ -0,0 +1,242 @@
+/**
+ * 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.
+ */
+
+import React, { useState } from 'react';
+import {
+ Checkbox,
+ DialogContent,
+ FormControl,
+ Input,
+ InputLabel, ListItemText,
+ MenuItem,
+ Select,
+ TextField,
+ InputAdornment,
+ IconButton,
+} from '@material-ui/core';
+import Visibility from '@material-ui/icons/Visibility';
+import VisibilityOff from '@material-ui/icons/VisibilityOff';
+type Props = {
+ tableList: Array<{
+ name: string,
+ checked: boolean,
+ disabled: boolean
+ }>,
+ setUserInfo: Function,
+ userProp?: {
+ username: string,
+ password: string,
+ component: string,
+ role: string,
+ tables: Array<string>,
+ permissions: Array<string>
+ }
+}
+
+export default function AddUser({
+ tableList,
+ setUserInfo,
+ userProp
+}: Props) {
+ const PERMISSIONS = ["READ", "CREATE", "UPDATE", "DELETE"];
+ const [user, setUser] = useState({
+ ...userProp,
+ tables: ["DUAL"]
+ });
+ const [tables, setTables] = useState(tableList);
+ const [showPassword, setShowPassword] = useState(false);
+ const handleClickShowPassword = () => {
+ setShowPassword(!showPassword);
+ };
+ const handleMouseDownPassword = (event) => {
+ event.preventDefault();
+ };
+ const changeHandler = (field, e)=>{
+ let val = e.target.value;
+ let userCopy = {...user};
Review comment:
You are right. Thanks for your suggestion. I will fix 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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r822653032
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/user/ComponentType.java
##########
@@ -0,0 +1,23 @@
+/**
+ * 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.pinot.spi.config.user;
+
+public enum ComponentType {
+ CONTROLLER, BROKER, SERVER
Review comment:
@apucher Currently, Only Controller、broker, and server components need Auth Access Control. I haven't got the point of appending minion and proxy at here.
--
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
[GitHub] [pinot] joshigaurava commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
joshigaurava commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r826186236
##########
File path: pinot-controller/src/main/resources/app/components/User/AddUser.tsx
##########
@@ -0,0 +1,242 @@
+/**
+ * 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.
+ */
+
+import React, { useState } from 'react';
+import {
+ Checkbox,
+ DialogContent,
+ FormControl,
+ Input,
+ InputLabel, ListItemText,
+ MenuItem,
+ Select,
+ TextField,
+ InputAdornment,
+ IconButton,
+} from '@material-ui/core';
+import Visibility from '@material-ui/icons/Visibility';
+import VisibilityOff from '@material-ui/icons/VisibilityOff';
+type Props = {
+ tableList: Array<{
+ name: string,
+ checked: boolean,
+ disabled: boolean
+ }>,
+ setUserInfo: Function,
+ userProp?: {
+ username: string,
+ password: string,
+ component: string,
+ role: string,
+ tables: Array<string>,
+ permissions: Array<string>
+ }
+}
+
+export default function AddUser({
+ tableList,
+ setUserInfo,
+ userProp
+}: Props) {
+ const PERMISSIONS = ["READ", "CREATE", "UPDATE", "DELETE"];
+ const [user, setUser] = useState({
+ ...userProp,
+ tables: ["DUAL"]
+ });
+ const [tables, setTables] = useState(tableList);
+ const [showPassword, setShowPassword] = useState(false);
+ const handleClickShowPassword = () => {
+ setShowPassword(!showPassword);
+ };
+ const handleMouseDownPassword = (event) => {
+ event.preventDefault();
+ };
+ const changeHandler = (field, e)=>{
+ let val = e.target.value;
+ let userCopy = {...user};
Review comment:
Spread operator usually creates a shallow copy which creates problems down the road. can we please use lodash deepClone?
--
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
[GitHub] [pinot] Jackie-Jiang commented on pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#issuecomment-1062043466
@apucher Can you please help take a look at this PR?
--
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
[GitHub] [pinot] apucher commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r822179052
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/Constants.java
##########
@@ -59,6 +61,18 @@ public static TableType validateTableType(String tableTypeStr) {
}
}
+ public static ComponentType validateComponentType(String componentTypeStr) {
+ if (componentTypeStr == null || componentTypeStr.isEmpty()) {
+ return null;
+ }
Review comment:
you could use `StringUtils.isBlank` here
--
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
[GitHub] [pinot] apucher commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r841183636
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/AccessControlFactory.java
##########
@@ -29,10 +31,13 @@
public static final String ACCESS_CONTROL_CLASS_CONFIG = "class";
public abstract void init(PinotConfiguration confguration);
+ public abstract void init(ZkHelixPropertyStore<ZNRecord> propertyStore);
+
public abstract AccessControl create();
- public static AccessControlFactory loadFactory(PinotConfiguration configuration) {
+ public static AccessControlFactory loadFactory(PinotConfiguration configuration,
+ ZkHelixPropertyStore<ZNRecord> propertyStore) {
Review comment:
see comments above. I'd go as far as marking both init methods non-abstract, and by default implement the new one to call the old one.
--
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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r838640855
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -55,10 +57,16 @@ public BasicAuthAccessControlFactory() {
// left blank
}
+ @Override
public void init(PinotConfiguration configuration) {
_accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
}
+ @Override
+ public void init(ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ }
+
+ @Override
Review comment:
I have restructed this method as you mentioned before. @apucher
--
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
[GitHub] [pinot] joshigaurava commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
joshigaurava commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r826185382
##########
File path: pinot-controller/src/main/resources/app/components/User/UpdateUser.tsx
##########
@@ -0,0 +1,235 @@
+/**
+ * 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.
+ */
+
+import React, { useState } from 'react';
+import {
+ Checkbox,
+ DialogContent,
+ FormControl,
+ IconButton,
+ Input, InputAdornment,
+ InputLabel, ListItemText,
+ makeStyles,
+ MenuItem,
+ Select,
+} from '@material-ui/core';
+import Visibility from "@material-ui/icons/Visibility";
+import VisibilityOff from "@material-ui/icons/VisibilityOff";
+
+type Props = {
+ tableList: Array<{
+ name: string,
+ checked: boolean,
+ disabled: boolean
+ }>,
+ setUserInfo: Function,
+ userProp?: {
+ username: string,
+ password: string,
+ component: string,
+ role: string,
+ tables: Array<string>,
+ permissions: Array<string>
+ }
+}
+
+export default function UpdateUser({
+ tableList,
+ setUserInfo,
+ userProp
+}: Props) {
+
+ const PERMISSIONS = ["READ", "CREATE", "UPDATE", "DELETE"];
+ const [user, setUser] = useState(userProp);
+ const transferTable = tableList.map(item=>{
+ return {
+ ...item,
+ checked: !!userProp.tables.includes(item.name)
+ }
+ })
+ const [tables, setTables] = useState(transferTable);
+ const [showPassword, setShowPassword] = useState(false);
+ const handleClickShowPassword = () => {
+ setShowPassword(!showPassword);
+ };
+ const handleMouseDownPassword = (event) => {
+ event.preventDefault();
+ };
+ const changeHandler = (field, e)=>{
+ let val = e.target.value;
+ let userCopy = {...user};
Review comment:
Spread operator usually creates a shallow copy which creates problems down the road. can we please use lodash `deepClone`?
--
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
[GitHub] [pinot] yuxuejiao commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
yuxuejiao commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r826531459
##########
File path: pinot-controller/src/main/resources/app/pages/UserPage.tsx
##########
@@ -0,0 +1,301 @@
+/* eslint-disable no-nested-ternary */
+/**
+ * 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.
+ */
+
+import React, {useEffect, useState} from 'react';
+import {Button, Paper, Table, TableBody, TableCell, TableContainer, TableHead, TableRow, Dialog, DialogActions, Snackbar, DialogTitle} from '@material-ui/core';
+import Alert from '@material-ui/lab/Alert';
+import { makeStyles } from '@material-ui/core/styles';
+
+import 'codemirror/lib/codemirror.css';
+import 'codemirror/theme/material.css';
+import 'codemirror/mode/javascript/javascript';
+import 'codemirror/mode/sql/sql';
+import 'codemirror/addon/hint/show-hint';
+import 'codemirror/addon/hint/sql-hint';
+import 'codemirror/addon/hint/show-hint.css';
Review comment:
Tanks for your review, I will remove them 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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r829641115
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/provider/UserCache.java
##########
@@ -0,0 +1,176 @@
+/**
+ * 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.pinot.common.config.provider;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.stream.Collectors;
+import javax.annotation.Nullable;
+import org.I0Itec.zkclient.IZkChildListener;
+import org.I0Itec.zkclient.IZkDataListener;
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.common.utils.config.UserConfigUtils;
+import org.apache.pinot.spi.config.user.ComponentType;
+import org.apache.pinot.spi.config.user.UserConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class UserCache {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(UserCache.class);
+ private static final String USER_CONFIG_PARENT_PATH = "/CONFIGS/USER";
+ private static final String USER_CONFIG_PATH_PREFIX = "/CONFIGS/USER/";
+
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+
+ private final UserConfigChangeListener _userConfigChangeListener = new UserConfigChangeListener();
+
+ private final Map<String, UserConfig> _userConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userControllerConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userBrokerConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userServerConfigMap = new ConcurrentHashMap<>();
Review comment:
@apucher I think your suggestion is more reasonable，but now Pinot haven't added access control to minion and proxy. We would get nothing information about minion and proxy. Maybe I didn't understand your point. I look forward to your reply. Thx
--
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
[GitHub] [pinot] apucher commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r841184410
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
##########
@@ -93,4 +93,17 @@ public void validatePermission(Optional<String> tableNameOpt, AccessType accessT
Response.Status.FORBIDDEN);
}
}
+
+
+ public void validatePermission(HttpHeaders httpHeaders, String endpointUrl,
+ AccessControl accessControl) {
+ boolean hasPermission;
+ String accessTypeToEndpintMsg =
+ String.format("request to the endpioint '%s'", endpointUrl);
Review comment:
delete `accessTypeToEndpintMsg` and merge if & declaration of `hasPermission`?
##########
File path: pinot-core/src/main/java/org/apache/pinot/server/access/ZkBasicAuthAccessFactory.java
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pinot.server.access;
+
+import io.netty.channel.ChannelHandlerContext;
+import java.util.Collection;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+import org.apache.commons.lang.StringUtils;
+import org.apache.helix.HelixManager;
+import org.apache.pinot.common.config.provider.UserCache;
+import org.apache.pinot.common.utils.BcryptUtils;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.core.auth.ZkBasicAuthPrincipal;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+
+/**
+ * Zookeeper Basic Authentication based on either http headers or Controller UI. Configured user properties
+ * via json format.
+ *
+ * <pre>
+ * Http headers Json Example:
+ * {
+ * "username": "user1",
+ * "password": "user1@passwd",
+ * "component": "SERVER",
+ * "role" : "USER",
+ * "tables": ["jock_pinot_poc2", "jocktest"],
+ * "permissions": ["READ"]
+ * }
+ * </pre>
+ */
Review comment:
same story with the auth header
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/config/UserConfigUtils.java
##########
@@ -0,0 +1,83 @@
+/**
+ * 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.pinot.common.utils.config;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+import org.apache.helix.ZNRecord;
+import org.apache.pinot.spi.config.user.AccessType;
+import org.apache.pinot.spi.config.user.UserConfig;
+
+public class UserConfigUtils {
Review comment:
would you mind adding a short javadoc that describes what `UserConfig` is actually being used for?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/ZkBasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,132 @@
+/**
+ * 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.pinot.controller.api.access;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import javax.ws.rs.core.HttpHeaders;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.common.config.provider.UserCache;
+import org.apache.pinot.common.utils.BcryptUtils;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.core.auth.ZkBasicAuthPrincipal;
+import org.apache.pinot.spi.config.user.ComponentType;
+import org.apache.pinot.spi.config.user.RoleType;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "controller.admin.access.control" family of
+ * properties.
+ *
+ * <pre>
+ * Example:
+ * controller.admin.access.control.principals=admin123,user456
+ * controller.admin.access.control.principals.admin123.password=verysecret
+ * controller.admin.access.control.principals.user456.password=kindasecret
+ * controller.admin.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * controller.admin.access.control.principals.user456.permissions=read,update
+ * </pre>
+ */
Review comment:
same as above. these aren't the authorization headers you're expecting, right?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/BasicAuthAccessControlFactory.java
##########
@@ -50,10 +51,16 @@
private AccessControl _accessControl;
+ @Override
public void init(PinotConfiguration configuration) {
_accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
}
+ @Override
+ public void init(PinotConfiguration pinotConfiguration, PinotHelixResourceManager pinotHelixResourceManager) {
+ init(pinotConfiguration);
+ }
+
Review comment:
same as above. no changes should happen to this file
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1268,6 +1274,32 @@ public Schema getSchemaForTableConfig(TableConfig tableConfig) {
AccessOption.PERSISTENT);
}
+ public void initUserACLConfig() throws IOException {
+ if (Optional.ofNullable(ZKMetadataProvider.getAllUserName(_propertyStore)).isEmpty()) {
+ addUser(new UserConfig("admin", "Pda@wap%admin", ComponentType.CONTROLLER.name(),
+ RoleType.ADMIN.name(), null, null));
+ addUser(new UserConfig("admin", "Pda@wap%admin", ComponentType.BROKER.name(),
+ RoleType.ADMIN.name(), null, null));
+ addUser(new UserConfig("admin", "Pda@wap%admin", ComponentType.SERVER.name(),
+ RoleType.ADMIN.name(), null, null));
+ }
Review comment:
awesome
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/AccessControlFactory.java
##########
@@ -29,10 +31,13 @@
public static final String ACCESS_CONTROL_CLASS_CONFIG = "class";
public abstract void init(PinotConfiguration confguration);
+ public abstract void init(ZkHelixPropertyStore<ZNRecord> propertyStore);
+
public abstract AccessControl create();
- public static AccessControlFactory loadFactory(PinotConfiguration configuration) {
+ public static AccessControlFactory loadFactory(PinotConfiguration configuration,
+ ZkHelixPropertyStore<ZNRecord> propertyStore) {
Review comment:
see comments below. I'd go as far as marking both init methods non-abstract, and by default implement the new one to call the old one.
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/AllowAllAccessControlFactory.java
##########
@@ -34,6 +36,10 @@ public AllowAllAccessControlFactory() {
public void init(PinotConfiguration configuration) {
}
+ public void init(ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ }
+
+ @Override
Review comment:
you're still breaking backwards-compatibility. if you have to make changes to this file, you equally affect 3rd party implementations which aren't in the open source repository.
you can simply move the implementation of `init(PinotConfiguration, ZkHelixPropertyStore)` to the AccessControlFactory parent class and declare it non-abstract. then, override the method in you new implementation while this class (and similar ones) can remain unchanged
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java
##########
@@ -55,10 +57,16 @@ public BasicAuthAccessControlFactory() {
// left blank
}
+ @Override
public void init(PinotConfiguration configuration) {
_accessControl = new BasicAuthAccessControl(BasicAuthUtils.extractBasicAuthPrincipals(configuration, PREFIX));
}
+ @Override
+ public void init(ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ }
+
+ @Override
Review comment:
same
##########
File path: pinot-common/pom.xml
##########
@@ -301,6 +301,11 @@
<groupId>org.apache.yetus</groupId>
<artifactId>audience-annotations</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.mindrot</groupId>
+ <artifactId>jbcrypt</artifactId>
+ <version>0.4</version>
+ </dependency>
Review comment:
@mayankshriv is bcrypt available already? otherwise is the license compatible? https://github.com/jeremyh/jBCrypt/blob/master/LICENSE
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java
##########
@@ -0,0 +1,130 @@
+/**
+ * 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.pinot.broker.broker;
+
+import com.google.common.base.Preconditions;
+import java.util.Collection;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.broker.api.AccessControl;
+import org.apache.pinot.broker.api.HttpRequesterIdentity;
+import org.apache.pinot.broker.api.RequesterIdentity;
+import org.apache.pinot.common.config.provider.UserCache;
+import org.apache.pinot.common.request.BrokerRequest;
+import org.apache.pinot.common.utils.BcryptUtils;
+import org.apache.pinot.core.auth.BasicAuthPrincipal;
+import org.apache.pinot.core.auth.BasicAuthUtils;
+import org.apache.pinot.core.auth.ZkBasicAuthPrincipal;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * Basic Authentication based on http headers. Configured via the "pinot.broker.access.control" family of properties.
+ *
+ * <pre>
+ * Example:
+ * pinot.broker.access.control.principals=admin123,user456
+ * pinot.broker.access.control.principals.admin123.password=verysecret
+ * pinot.broker.access.control.principals.user456.password=kindasecret
+ * pinot.broker.access.control.principals.user456.tables=stuff,lessImportantStuff
+ * </pre>
+ */
Review comment:
could you check this? are these the JSON header you're expecting for authorization?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotUserRestletResource.java
##########
@@ -0,0 +1,244 @@
+/**
+ * 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.pinot.controller.api.resources;
+
+import io.swagger.annotations.Api;
+import io.swagger.annotations.ApiOperation;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.Executor;
+import java.util.concurrent.ExecutorService;
+import javax.inject.Inject;
+import javax.ws.rs.DELETE;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.PUT;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+import org.apache.commons.httpclient.HttpConnectionManager;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+import org.apache.pinot.common.metrics.ControllerMetrics;
+import org.apache.pinot.common.utils.BcryptUtils;
+import org.apache.pinot.controller.ControllerConf;
+import org.apache.pinot.controller.api.access.AccessControlFactory;
+import org.apache.pinot.controller.api.access.AccessControlUtils;
+import org.apache.pinot.controller.api.access.AccessType;
+import org.apache.pinot.controller.api.access.Authenticate;
+import org.apache.pinot.controller.api.exception.ControllerApplicationException;
+import org.apache.pinot.controller.api.exception.UserAlreadyExistsException;
+import org.apache.pinot.controller.helix.core.PinotHelixResourceManager;
+import org.apache.pinot.controller.helix.core.minion.PinotHelixTaskResourceManager;
+import org.apache.pinot.spi.config.user.ComponentType;
+import org.apache.pinot.spi.config.user.UserConfig;
+import org.apache.pinot.spi.utils.JsonUtils;
+import org.glassfish.grizzly.http.server.Request;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Api(tags = Constants.USER_TAG)
+@Path("/")
+public class PinotUserRestletResource {
Review comment:
move the header javadoc here?
--
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
[GitHub] [pinot] apucher commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
apucher commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r841183251
##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/AllowAllAccessControlFactory.java
##########
@@ -34,6 +36,10 @@ public AllowAllAccessControlFactory() {
public void init(PinotConfiguration configuration) {
}
+ public void init(ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ }
+
+ @Override
Review comment:
you're still breaking backwards-compatibility. if you have to make changes to this file, you equally affect 3rd party implementations which aren't in the open source repository.
you can simply move the implementation of `init(PinotConfiguration, ZkHelixPropertyStore)` to the AccessControlFactory parent class and declare it non-abstract. then, override the method in your new implementation while this class (and similar ones) can remain unchanged
--
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
[GitHub] [pinot] mcvsubbu commented on pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#issuecomment-1062014617
A few things.
- Mark this backward-incompat since you are changing the interfaces for auth. Installations using their own auth mechanisms will need to re-compile (at the least)
- What is the behavior if the controller APIs are invoked on an installation that uses its own auth mechanisms?
--
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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r829641115
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/provider/UserCache.java
##########
@@ -0,0 +1,176 @@
+/**
+ * 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.pinot.common.config.provider;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.stream.Collectors;
+import javax.annotation.Nullable;
+import org.I0Itec.zkclient.IZkChildListener;
+import org.I0Itec.zkclient.IZkDataListener;
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.common.utils.config.UserConfigUtils;
+import org.apache.pinot.spi.config.user.ComponentType;
+import org.apache.pinot.spi.config.user.UserConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class UserCache {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(UserCache.class);
+ private static final String USER_CONFIG_PARENT_PATH = "/CONFIGS/USER";
+ private static final String USER_CONFIG_PATH_PREFIX = "/CONFIGS/USER/";
+
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+
+ private final UserConfigChangeListener _userConfigChangeListener = new UserConfigChangeListener();
+
+ private final Map<String, UserConfig> _userConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userControllerConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userBrokerConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userServerConfigMap = new ConcurrentHashMap<>();
Review comment:
@apucher I think your suggestion is more reasonable，but now Pinot haven't added access control to minion and proxy. We would get nothing information about minion and proxy. Maybe I didn't understand your point. I look forward to your reply. Thx
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
##########
@@ -93,4 +93,21 @@ public void validatePermission(Optional<String> tableNameOpt, AccessType accessT
Response.Status.FORBIDDEN);
}
}
+
+ public void validatePermission(HttpHeaders httpHeaders, String endpointUrl,
+ AccessControl accessControl) {
+ validatePermission("you", httpHeaders, endpointUrl, accessControl);
Review comment:
@apucher The original idea is to reuse the same name method. Now I have integrated the two methods into one. I have deleted this abrupt word "you".
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1268,6 +1274,32 @@ public Schema getSchemaForTableConfig(TableConfig tableConfig) {
AccessOption.PERSISTENT);
}
+ public void initUserACLConfig() throws IOException {
+ if (Optional.ofNullable(ZKMetadataProvider.getAllUserName(_propertyStore)).isEmpty()) {
+ addUser(new UserConfig("admin", "Pda@wap%admin", ComponentType.CONTROLLER.name(),
+ RoleType.ADMIN.name(), null, null));
+ addUser(new UserConfig("admin", "Pda@wap%admin", ComponentType.BROKER.name(),
+ RoleType.ADMIN.name(), null, null));
+ addUser(new UserConfig("admin", "Pda@wap%admin", ComponentType.SERVER.name(),
+ RoleType.ADMIN.name(), null, null));
+ }
Review comment:
@apucher You are right. I have set DEFAULT_ACCESS_CONTROL_USERNAME and DEFAULT_ACCESS_CONTROL_PASSWORD. These two variables have default value. And user can set these two value via controller configuration file.
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/user/ComponentType.java
##########
@@ -0,0 +1,23 @@
+/**
+ * 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.pinot.spi.config.user;
+
+public enum ComponentType {
+ CONTROLLER, BROKER, SERVER
Review comment:
@apucher Currently, Only Controller、broker, and server components need Auth Access Control. I haven't got the point of appending minion and proxy at here.
--
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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r822650377
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/config/provider/UserCache.java
##########
@@ -0,0 +1,176 @@
+/**
+ * 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.pinot.common.config.provider;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.stream.Collectors;
+import javax.annotation.Nullable;
+import org.I0Itec.zkclient.IZkChildListener;
+import org.I0Itec.zkclient.IZkDataListener;
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.helix.AccessOption;
+import org.apache.helix.ZNRecord;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.pinot.common.utils.config.UserConfigUtils;
+import org.apache.pinot.spi.config.user.ComponentType;
+import org.apache.pinot.spi.config.user.UserConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class UserCache {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(UserCache.class);
+ private static final String USER_CONFIG_PARENT_PATH = "/CONFIGS/USER";
+ private static final String USER_CONFIG_PATH_PREFIX = "/CONFIGS/USER/";
+
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+
+ private final UserConfigChangeListener _userConfigChangeListener = new UserConfigChangeListener();
+
+ private final Map<String, UserConfig> _userConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userControllerConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userBrokerConfigMap = new ConcurrentHashMap<>();
+ private final Map<String, UserConfig> _userServerConfigMap = new ConcurrentHashMap<>();
+
+ public UserCache(ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ _propertyStore = propertyStore;
+ synchronized (_userConfigChangeListener) {
Review comment:
@apucher I referenced the TableCache class implementation that uses keyword synchronized. In this constructor method, Read happens only， so it won't happen race competition. I have deleted the keyword synchronized.
--
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
[GitHub] [pinot] INNOCENT-BOY commented on a change in pull request #8314: Access Control management Development
Posted by GitBox <gi...@apache.org>.
INNOCENT-BOY commented on a change in pull request #8314:
URL: https://github.com/apache/pinot/pull/8314#discussion_r827826870
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
##########
@@ -93,4 +93,21 @@ public void validatePermission(Optional<String> tableNameOpt, AccessType accessT
Response.Status.FORBIDDEN);
}
}
+
+ public void validatePermission(HttpHeaders httpHeaders, String endpointUrl,
+ AccessControl accessControl) {
+ validatePermission("you", httpHeaders, endpointUrl, accessControl);
Review comment:
@apucher The original idea is to reuse the same name method. Now I have integrated the two methods in one.
--
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