walterddr commented on code in PR #8314: URL: https://github.com/apache/pinot/pull/8314#discussion_r863216579
########## 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: what is the relationship between AccessType and RoleType? ########## 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 { Review Comment: this and below: could you add some detail Javadoc for these enum/classes. ########## pinot-spi/src/main/java/org/apache/pinot/spi/config/user/RoleType.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 RoleType { + ADMIN, USER +} Review Comment: are these pre-set AccessType / ComponentType pair or can the RoleType be extended going into the future? if so I would foresee this enum to change a lot and might not be a good idea to keep it as enum, since the role basically is an ask to implement: ``` public interface RoleType { List<AccessType> getAccessTypes(ComponentType componentType); } ``` ########## pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthUtils.java: ########## @@ -61,21 +64,43 @@ private BasicAuthUtils() { * @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"); + String principalNames = configuration.getProperty(prefix); + Preconditions.checkArgument(StringUtils.isNotBlank(principalNames), "must provide principals"); - return Arrays.stream(principalNames.split(",")).map(rawName -> { - String name = rawName.trim(); - Preconditions.checkArgument(StringUtils.isNotBlank(name), "%s is not a valid name", name); + return Arrays.stream(principalNames.split(",")).map(rawName -> { + String name = rawName.trim(); + Preconditions.checkArgument(StringUtils.isNotBlank(name), "%s is not a valid name", name); - String password = configuration.getProperty(String.format("%s.%s.%s", prefix, name, PASSWORD)); - Preconditions.checkArgument(StringUtils.isNotBlank(password), "must provide a password for %s", name); + String password = configuration.getProperty(String.format("%s.%s.%s", prefix, name, PASSWORD)); + Preconditions.checkArgument(StringUtils.isNotBlank(password), "must provide a password for %s", name); - Set<String> tables = extractSet(configuration, String.format("%s.%s.%s", prefix, name, TABLES)); - Set<String> permissions = extractSet(configuration, String.format("%s.%s.%s", prefix, name, PERMISSIONS)); + Set<String> tables = extractSet(configuration, String.format("%s.%s.%s", prefix, name, TABLES)); + Set<String> permissions = extractSet(configuration, String.format("%s.%s.%s", prefix, name, PERMISSIONS)); - return new BasicAuthPrincipal(name, toBasicAuthToken(name, password), tables, permissions); - }).collect(Collectors.toList()); + return new BasicAuthPrincipal(name, toBasicAuthToken(name, password), tables, permissions); + }).collect(Collectors.toList()); + } + + public static List<ZkBasicAuthPrincipal> extractBasicAuthPrincipals(List<UserConfig> userConfigList) { + return userConfigList.stream() + .map(user -> { + String name = user.getUserName().trim(); + Preconditions.checkArgument(StringUtils.isNotBlank(name), "%s is not a valid username", name); + String password = user.getPassword().trim(); Review Comment: why do we need to trim password? is whitespace char not allowed? ########## pinot-broker/src/main/java/org/apache/pinot/broker/broker/AccessControlFactory.java: ########## @@ -28,11 +30,24 @@ public abstract class AccessControlFactory { public static final Logger LOGGER = LoggerFactory.getLogger(AccessControlFactory.class); public static final String ACCESS_CONTROL_CLASS_CONFIG = "class"; - public abstract void init(PinotConfiguration confguration); + public void init(PinotConfiguration configuration) { + }; + + /** + * Extend original init method inorder to support Zookeeper BasicAuthAccessControlFactory Review Comment: +1 either use PinotHelixResourceManager or ZkHelixPropertyStore since you used PinotHelixResourceManager in the init method of `ZkBasicAuthAccessControlFactory` ########## pinot-spi/src/main/java/org/apache/pinot/spi/config/user/UserConfig.java: ########## @@ -0,0 +1,140 @@ +/** + * 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 { Review Comment: assuming this is going to be one payload setting per `componentType/user` combination? one userConfig can be: user: `foo` to access `pinot-server` with `read/write` access to `bar1, bar2` table. this could be very straightforward to start but I think username/password/role can be registered separately from component type / table / token / permission (this can be address in follow up PR before next release) ########## pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthUtils.java: ########## @@ -61,21 +64,43 @@ private BasicAuthUtils() { * @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"); + String principalNames = configuration.getProperty(prefix); Review Comment: could we add test to BasicAuthUtils for the 2 new encode/decode method? ########## pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java: ########## @@ -240,6 +242,8 @@ private static long getRandomInitialDelayInSeconds() { 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"; + private static final String DEFAULT_ACCESS_CONTROL_USERNAME = "admin"; + private static final String DEFAULT_ACCESS_CONTROL_PASSWORD = "admin"; Review Comment: ```suggestion private static final String DEFAULT_ACCESS_CONTROL_PASSWORD = "passwd"; ``` ########## pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java: ########## @@ -240,6 +242,8 @@ private static long getRandomInitialDelayInSeconds() { 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"; + private static final String DEFAULT_ACCESS_CONTROL_USERNAME = "admin"; + private static final String DEFAULT_ACCESS_CONTROL_PASSWORD = "admin"; Review Comment: althought I dont know if we should have a default --> if not configured we should simply throw exception if a secure auth enabled cluster -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
