winterhazel commented on code in PR #9504: URL: https://github.com/apache/cloudstack/pull/9504#discussion_r1967628994
########## engine/schema/src/main/java/org/apache/cloudstack/acl/dao/ApiKeyPairPermissionsDaoImpl.java: ########## @@ -0,0 +1,67 @@ +// 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.cloudstack.acl.dao; + +import com.cloud.utils.db.Filter; +import com.cloud.utils.db.GenericDaoBase; +import com.cloud.utils.db.SearchBuilder; +import com.cloud.utils.db.SearchCriteria; +import java.util.Collections; +import java.util.Objects; +import org.apache.cloudstack.acl.ApiKeyPairPermissionVO; +import org.springframework.stereotype.Component; + +import java.util.List; + +@Component +public class ApiKeyPairPermissionsDaoImpl extends GenericDaoBase<ApiKeyPairPermissionVO, Long> implements ApiKeyPairPermissionsDao { + private final SearchBuilder<ApiKeyPairPermissionVO> permissionByApiKeyPairIdSearch; + + public ApiKeyPairPermissionsDaoImpl() { + super(); + + permissionByApiKeyPairIdSearch = createSearchBuilder(); + permissionByApiKeyPairIdSearch.and("apiKeyPairId", permissionByApiKeyPairIdSearch.entity().getApiKeyPairId(), SearchCriteria.Op.EQ); + permissionByApiKeyPairIdSearch.done(); + } + + public List<ApiKeyPairPermissionVO> findAllByApiKeyPairId(Long apiKeyPairId) { + SearchCriteria<ApiKeyPairPermissionVO> sc = permissionByApiKeyPairIdSearch.create(); + sc.setParameters("apiKeyPairId", String.valueOf(apiKeyPairId)); + return listBy(sc); + } + + @Override + public ApiKeyPairPermissionVO persist(final ApiKeyPairPermissionVO item) { + item.setSortOrder(0); + final List<ApiKeyPairPermissionVO> permissionsList = findAllByKeyPairIdSorted(item.getApiKeyPairId()); + if (permissionsList != null && !permissionsList.isEmpty()) { Review Comment: ```suggestion if (!CollectionUtils.isEmpty(permissionsList)) { ``` ########## engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade42010to42100.java: ########## @@ -57,9 +60,52 @@ public InputStream[] getPrepareScripts() { return new InputStream[] {script}; } + protected void performKeyPairMigration(Connection conn) throws SQLException { Review Comment: You can do this operation via SQL directly ########## api/src/main/java/org/apache/cloudstack/query/QueryService.java: ########## @@ -135,6 +135,8 @@ public interface QueryService { ListResponse<UserResponse> searchForUsers(Long domainId, boolean recursive) throws PermissionDeniedException; + List<Long> searchForAccessableUsers(); Review Comment: ```suggestion List<Long> searchForAccessibleUsers(); ``` ########## engine/schema/src/main/java/org/apache/cloudstack/acl/dao/ApiKeyPairDaoImpl.java: ########## @@ -0,0 +1,117 @@ +// 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.cloudstack.acl.dao; + +import com.cloud.utils.Pair; +import com.cloud.utils.db.Filter; +import com.cloud.utils.db.GenericDaoBase; +import com.cloud.utils.db.SearchBuilder; +import com.cloud.utils.db.SearchCriteria; +import java.util.List; +import org.apache.cloudstack.acl.ApiKeyPairVO; +import org.apache.cloudstack.api.command.admin.user.ListUserKeysCmd; +import org.apache.commons.collections.CollectionUtils; +import org.springframework.stereotype.Component; + +@Component +public class ApiKeyPairDaoImpl extends GenericDaoBase<ApiKeyPairVO, Long> implements ApiKeyPairDao { + + private final SearchBuilder<ApiKeyPairVO> keyPairSearch; + + ApiKeyPairDaoImpl() { + super(); + + keyPairSearch = createSearchBuilder(); + keyPairSearch.and("apiKey", keyPairSearch.entity().getApiKey(), SearchCriteria.Op.EQ); + keyPairSearch.and("secretKey", keyPairSearch.entity().getSecretKey(), SearchCriteria.Op.EQ); + keyPairSearch.and("id", keyPairSearch.entity().getId(), SearchCriteria.Op.EQ); + keyPairSearch.and("userId", keyPairSearch.entity().getUserId(), SearchCriteria.Op.IN); + keyPairSearch.done(); + } + + @Override + public ApiKeyPairVO findByApiKey(String apiKey) { + SearchCriteria<ApiKeyPairVO> sc = keyPairSearch.create(); + sc.setParameters("apiKey", apiKey); + return findOneBy(sc); + } + + public ApiKeyPairVO findBySecretKey(String secretKey) { + SearchCriteria<ApiKeyPairVO> sc = keyPairSearch.create(); + sc.setParameters("secretKey", secretKey); + return findOneBy(sc); + } + + public Pair<List<ApiKeyPairVO>, Integer> listApiKeysByUserOrApiKeyId(Long userId, Long apiKeyId) { + SearchCriteria<ApiKeyPairVO> sc = keyPairSearch.create(); + if (userId != null) { + sc.setParametersIfNotNull("userId", String.valueOf(userId)); + } + sc.setParametersIfNotNull("id", apiKeyId); + final Filter searchFilter = new Filter(100); + return searchAndCount(sc, searchFilter); + } + + public ApiKeyPairVO getLastApiKeyCreatedByUser(Long userId) { + final SearchCriteria<ApiKeyPairVO> sc = keyPairSearch.create(); + if (userId != null) { + sc.setParameters("userId", String.valueOf(userId)); + } + final Filter searchBySorted = new Filter(ApiKeyPairVO.class, "id", false, null, null); + final List<ApiKeyPairVO> apiKeyPairVOList = listBy(sc, searchBySorted); Review Comment: Maybe `findOneBy` instead of `listBy`? ########## server/src/main/java/com/cloud/servlet/ConsoleProxyServlet.java: ########## @@ -538,24 +539,23 @@ private boolean verifyRequest(Map<String, Object[]> requestParameters) { txn.close(); User user = null; // verify there is a user with this api key - Pair<User, Account> userAcctPair = _accountMgr.findUserByApiKey(apiKey); - if (userAcctPair == null) { + Ternary<User, Account, ApiKeyPair> keyPairTernary = _accountMgr.findUserByApiKey(apiKey); + if (keyPairTernary == null) { LOGGER.debug("apiKey does not map to a valid user -- ignoring request, apiKey: " + apiKey); return false; } - user = userAcctPair.first(); - Account account = userAcctPair.second(); + user = keyPairTernary.first(); + Account account = keyPairTernary.second(); + ApiKeyPair keyPair = keyPairTernary.third(); if (!user.getState().equals(Account.State.ENABLED) || !account.getState().equals(Account.State.ENABLED)) { LOGGER.debug("disabled or locked user accessing the api, user: {}; state: {}; accountState: {}", user, user.getState(), account.getState()); return false; } - // verify secret key exists - secretKey = user.getSecretKey(); - if (secretKey == null) { - LOGGER.debug("User does not have a secret key associated with the account -- ignoring request, user: {}", user); + if (keyPair == null) { + LOGGER.debug("User does not have a keypair associated with the account -- ignoring request, username: " + user.getUsername()); Review Comment: ```suggestion LOGGER.debug("User does not have a keypair associated with the account -- ignoring request, username: {}", user.getUsername()); ``` ########## engine/schema/src/main/java/org/apache/cloudstack/acl/dao/ApiKeyPairDaoImpl.java: ########## @@ -0,0 +1,117 @@ +// 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.cloudstack.acl.dao; + +import com.cloud.utils.Pair; +import com.cloud.utils.db.Filter; +import com.cloud.utils.db.GenericDaoBase; +import com.cloud.utils.db.SearchBuilder; +import com.cloud.utils.db.SearchCriteria; +import java.util.List; +import org.apache.cloudstack.acl.ApiKeyPairVO; +import org.apache.cloudstack.api.command.admin.user.ListUserKeysCmd; +import org.apache.commons.collections.CollectionUtils; +import org.springframework.stereotype.Component; + +@Component +public class ApiKeyPairDaoImpl extends GenericDaoBase<ApiKeyPairVO, Long> implements ApiKeyPairDao { + + private final SearchBuilder<ApiKeyPairVO> keyPairSearch; + + ApiKeyPairDaoImpl() { + super(); + + keyPairSearch = createSearchBuilder(); + keyPairSearch.and("apiKey", keyPairSearch.entity().getApiKey(), SearchCriteria.Op.EQ); + keyPairSearch.and("secretKey", keyPairSearch.entity().getSecretKey(), SearchCriteria.Op.EQ); + keyPairSearch.and("id", keyPairSearch.entity().getId(), SearchCriteria.Op.EQ); + keyPairSearch.and("userId", keyPairSearch.entity().getUserId(), SearchCriteria.Op.IN); + keyPairSearch.done(); + } + + @Override + public ApiKeyPairVO findByApiKey(String apiKey) { + SearchCriteria<ApiKeyPairVO> sc = keyPairSearch.create(); + sc.setParameters("apiKey", apiKey); + return findOneBy(sc); + } + + public ApiKeyPairVO findBySecretKey(String secretKey) { + SearchCriteria<ApiKeyPairVO> sc = keyPairSearch.create(); + sc.setParameters("secretKey", secretKey); + return findOneBy(sc); + } + + public Pair<List<ApiKeyPairVO>, Integer> listApiKeysByUserOrApiKeyId(Long userId, Long apiKeyId) { + SearchCriteria<ApiKeyPairVO> sc = keyPairSearch.create(); + if (userId != null) { + sc.setParametersIfNotNull("userId", String.valueOf(userId)); + } + sc.setParametersIfNotNull("id", apiKeyId); + final Filter searchFilter = new Filter(100); + return searchAndCount(sc, searchFilter); + } + + public ApiKeyPairVO getLastApiKeyCreatedByUser(Long userId) { + final SearchCriteria<ApiKeyPairVO> sc = keyPairSearch.create(); + if (userId != null) { + sc.setParameters("userId", String.valueOf(userId)); + } Review Comment: ```suggestion sc.setParametersIfNotNull("userId", userId); ``` ########## engine/schema/src/main/java/com/cloud/upgrade/dao/Upgrade42010to42100.java: ########## @@ -57,9 +60,52 @@ public InputStream[] getPrepareScripts() { return new InputStream[] {script}; } + protected void performKeyPairMigration(Connection conn) throws SQLException { + try { + logger.debug("Performing keypair migration from user table to api_keypair table."); + PreparedStatement pstmt = conn.prepareStatement("SELECT u.id, u.api_key, u.secret_key, a.domain_id, u.id FROM `cloud`.`user` AS u JOIN `cloud`.`account` AS a " + Review Comment: ```suggestion PreparedStatement pstmt = conn.prepareStatement("SELECT u.id, u.api_key, u.secret_key, a.domain_id, a.id FROM `cloud`.`user` AS u JOIN `cloud`.`account` AS a " + ``` Need to fix this. The migration will fail if the account_id does not match the user_id ########## engine/schema/src/main/java/org/apache/cloudstack/acl/dao/ApiKeyPairDaoImpl.java: ########## @@ -0,0 +1,117 @@ +// 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.cloudstack.acl.dao; + +import com.cloud.utils.Pair; +import com.cloud.utils.db.Filter; +import com.cloud.utils.db.GenericDaoBase; +import com.cloud.utils.db.SearchBuilder; +import com.cloud.utils.db.SearchCriteria; +import java.util.List; +import org.apache.cloudstack.acl.ApiKeyPairVO; +import org.apache.cloudstack.api.command.admin.user.ListUserKeysCmd; +import org.apache.commons.collections.CollectionUtils; +import org.springframework.stereotype.Component; + +@Component +public class ApiKeyPairDaoImpl extends GenericDaoBase<ApiKeyPairVO, Long> implements ApiKeyPairDao { + + private final SearchBuilder<ApiKeyPairVO> keyPairSearch; + + ApiKeyPairDaoImpl() { + super(); + + keyPairSearch = createSearchBuilder(); + keyPairSearch.and("apiKey", keyPairSearch.entity().getApiKey(), SearchCriteria.Op.EQ); + keyPairSearch.and("secretKey", keyPairSearch.entity().getSecretKey(), SearchCriteria.Op.EQ); + keyPairSearch.and("id", keyPairSearch.entity().getId(), SearchCriteria.Op.EQ); + keyPairSearch.and("userId", keyPairSearch.entity().getUserId(), SearchCriteria.Op.IN); + keyPairSearch.done(); + } + + @Override + public ApiKeyPairVO findByApiKey(String apiKey) { + SearchCriteria<ApiKeyPairVO> sc = keyPairSearch.create(); + sc.setParameters("apiKey", apiKey); + return findOneBy(sc); + } + + public ApiKeyPairVO findBySecretKey(String secretKey) { + SearchCriteria<ApiKeyPairVO> sc = keyPairSearch.create(); + sc.setParameters("secretKey", secretKey); + return findOneBy(sc); + } + + public Pair<List<ApiKeyPairVO>, Integer> listApiKeysByUserOrApiKeyId(Long userId, Long apiKeyId) { + SearchCriteria<ApiKeyPairVO> sc = keyPairSearch.create(); + if (userId != null) { + sc.setParametersIfNotNull("userId", String.valueOf(userId)); + } Review Comment: ```suggestion sc.setParametersIfNotNull("userId", userId); ``` I think that the conversion to string is not 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
