You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/12/13 13:00:49 UTC
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5769: New feature: give access permission of networks to other accounts in same domain
DaanHoogland commented on a change in pull request #5769:
URL: https://github.com/apache/cloudstack/pull/5769#discussion_r767695890
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkPermissionsCmd.java
##########
@@ -0,0 +1,133 @@
+// 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.api.command.user.network;
+
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.AccountResponse;
+import org.apache.cloudstack.api.response.NetworkResponse;
+import org.apache.cloudstack.api.response.ProjectResponse;
+import org.apache.cloudstack.api.response.SuccessResponse;
+import org.apache.log4j.Logger;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.network.Network;
+import com.cloud.user.Account;
+
+import java.util.List;
+
+@APICommand(name = "createNetworkPermissions", description = "Updates network permissions.",
+ responseObject = SuccessResponse.class,
+ entityType = {Network.class},
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = false,
+ since = "4.17.0",
+ authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User})
+public class CreateNetworkPermissionsCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(CreateNetworkPermissionsCmd.class.getName());
Review comment:
```suggestion
public static final Logger LOGGER = Logger.getLogger(CreateNetworkPermissionsCmd.class.getName());
```
as the convention for static finals
##########
File path: engine/schema/src/main/java/org/apache/cloudstack/network/dao/NetworkPermissionDaoImpl.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.cloudstack.network.dao;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.log4j.Logger;
+import org.springframework.stereotype.Component;
+
+import org.apache.cloudstack.network.NetworkPermissionVO;
+
+import com.cloud.user.AccountVO;
+import com.cloud.user.dao.AccountDao;
+import com.cloud.utils.db.JoinBuilder;
+import com.cloud.utils.db.GenericDaoBase;
+import com.cloud.utils.db.GenericSearchBuilder;
+import com.cloud.utils.db.SearchBuilder;
+import com.cloud.utils.db.SearchCriteria;
+
+import javax.annotation.PostConstruct;
+import javax.inject.Inject;
+
+
+@Component
+public class NetworkPermissionDaoImpl extends GenericDaoBase<NetworkPermissionVO, Long> implements NetworkPermissionDao {
+ private static final Logger s_logger = Logger.getLogger(NetworkPermissionDaoImpl.class);
+
+ private SearchBuilder<NetworkPermissionVO> NetworkAndAccountSearch;
+ private SearchBuilder<NetworkPermissionVO> NetworkIdSearch;
+ private GenericSearchBuilder<NetworkPermissionVO, Long> FindNetworkIdsByAccount;
+ private GenericSearchBuilder<NetworkPermissionVO, Long> FindNetworkIdsByDomain;
+
+ @Inject
+ AccountDao _accountDao;
Review comment:
is it wise to have inter-dao dependencies, other than hierarchical?
I guess using this in the `FindNetworkIdsByDomain` is actually the whole jist of the functionality in this PR and maybe we already employ these tactics elsewhere, but it triggers brain alerts. No -1 on this point, just want to make sure we implement the best practice here.
##########
File path: tools/marvin/marvin/lib/base.py
##########
@@ -5275,7 +5275,7 @@ def removeIp(cls, apiclient, ipaddressid):
"""Remove secondary Ip from NIC"""
cmd = removeIpFromNic.removeIpFromNicCmd()
cmd.id = ipaddressid
- return (apiclient.addIpToNic(cmd))
+ return (apiclient.removeIpFromNic(cmd))
Review comment:
:) oops detected
##########
File path: api/src/main/java/com/cloud/network/Network.java
##########
@@ -332,6 +332,14 @@ private State(String description) {
}
}
+ public enum NetworkFilter {
+ account, // return account networks that have been registered for or created by the calling user
+ domain, // return domain networks that have been registered for or created by the calling user
+ accountdomain, // return account and domain networks that have been registered for or created by the calling user
+ shared, // including networks that have been granted to the calling user by another user
+ all // all networks (account, domain and shared)
+ }
Review comment:
convention is to use all capitals for enum values (I think)
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkPermissionsCmd.java
##########
@@ -0,0 +1,133 @@
+// 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.api.command.user.network;
+
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.AccountResponse;
+import org.apache.cloudstack.api.response.NetworkResponse;
+import org.apache.cloudstack.api.response.ProjectResponse;
+import org.apache.cloudstack.api.response.SuccessResponse;
+import org.apache.log4j.Logger;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.network.Network;
+import com.cloud.user.Account;
+
+import java.util.List;
+
+@APICommand(name = "createNetworkPermissions", description = "Updates network permissions.",
Review comment:
```suggestion
@APICommand(name = CreateNetworkPermissionsCmd.APINAME, description = "Updates network permissions.",
```
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkPermissionsCmd.java
##########
@@ -0,0 +1,133 @@
+// 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.api.command.user.network;
+
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.AccountResponse;
+import org.apache.cloudstack.api.response.NetworkResponse;
+import org.apache.cloudstack.api.response.ProjectResponse;
+import org.apache.cloudstack.api.response.SuccessResponse;
+import org.apache.log4j.Logger;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.network.Network;
+import com.cloud.user.Account;
+
+import java.util.List;
+
+@APICommand(name = "createNetworkPermissions", description = "Updates network permissions.",
+ responseObject = SuccessResponse.class,
+ entityType = {Network.class},
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = false,
+ since = "4.17.0",
+ authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User})
+public class CreateNetworkPermissionsCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(CreateNetworkPermissionsCmd.class.getName());
+
+ private static final String s_name = "createnetworkpermissionsresponse";
Review comment:
the standard construct is to use .APINAME and use that in the getCommandName() method (and on the @APICommand annotation
```suggestion
public static final String APINAME = "createnetworkpermissionsresponse";
```
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/network/CreateNetworkPermissionsCmd.java
##########
@@ -0,0 +1,133 @@
+// 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.api.command.user.network;
+
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.AccountResponse;
+import org.apache.cloudstack.api.response.NetworkResponse;
+import org.apache.cloudstack.api.response.ProjectResponse;
+import org.apache.cloudstack.api.response.SuccessResponse;
+import org.apache.log4j.Logger;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.network.Network;
+import com.cloud.user.Account;
+
+import java.util.List;
+
+@APICommand(name = "createNetworkPermissions", description = "Updates network permissions.",
+ responseObject = SuccessResponse.class,
+ entityType = {Network.class},
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = false,
+ since = "4.17.0",
+ authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User})
+public class CreateNetworkPermissionsCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(CreateNetworkPermissionsCmd.class.getName());
+
+ private static final String s_name = "createnetworkpermissionsresponse";
+
+ // ///////////////////////////////////////////////////
+ // ////////////// API parameters /////////////////////
+ // ///////////////////////////////////////////////////
+
+ @Parameter(name = ApiConstants.ACCOUNTS,
+ type = CommandType.LIST,
+ collectionType = CommandType.STRING,
+ description = "a comma delimited list of accounts within owner's domain. If specified, \"op\" parameter has to be passed in.")
+ private List<String> accountNames;
+
+ @Parameter(name = ApiConstants.ACCOUNT_IDS,
+ type = CommandType.LIST,
+ collectionType = CommandType.UUID,
+ entityType = AccountResponse.class,
+ description = "a comma delimited list of account IDs within owner's domain. If specified, \"op\" parameter has to be passed in.")
+ private List<Long> accountIds;
+
+ @Parameter(name = ApiConstants.NETWORK_ID, type = CommandType.UUID, entityType = NetworkResponse.class, required = true, description = "the network ID")
+ private Long networkId;
+
+ @Parameter(name = ApiConstants.PROJECT_IDS,
+ type = CommandType.LIST,
+ collectionType = CommandType.UUID,
+ entityType = ProjectResponse.class,
+ description = "a comma delimited list of projects within owner's domain. If specified, \"op\" parameter has to be passed in.")
+ private List<Long> projectIds;
+
+ // ///////////////////////////////////////////////////
+ // ///////////////// Accessors ///////////////////////
+ // ///////////////////////////////////////////////////
+
+
+ public List<String> getAccountNames() {
+ if (accountIds != null && accountNames != null) {
+ throw new InvalidParameterValueException("Accounts and accountNames can't be specified together");
+ }
+ return accountNames;
+ }
+
+ public List<Long> getAccountIds() {
+ if (accountIds != null && accountNames != null) {
+ throw new InvalidParameterValueException("Accounts and accountNames can't be specified together");
+ }
+ return accountIds;
+ }
+
+ public Long getNetworkId() {
+ return networkId;
+ }
+
+ public List<Long> getProjectIds() {
+ return projectIds;
+ }
+
+ // ///////////////////////////////////////////////////
+ // ///////////// API Implementation///////////////////
+ // ///////////////////////////////////////////////////
+
+ @Override
+ public String getCommandName() {
+ return s_name;
+ }
Review comment:
```suggestion
public String getCommandName() {
return APINAME.toLowerCase() + BaseCmd.RESPONSE_SUFFIX;
}
```
We should actually move that ^^ method to BaseCmd.
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/network/RemoveNetworkPermissionsCmd.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.cloudstack.api.command.user.network;
+
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.AccountResponse;
+import org.apache.cloudstack.api.response.NetworkResponse;
+import org.apache.cloudstack.api.response.ProjectResponse;
+import org.apache.cloudstack.api.response.SuccessResponse;
+import org.apache.log4j.Logger;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.network.Network;
+import com.cloud.user.Account;
+
+import java.util.List;
+
+@APICommand(name = "removeNetworkPermissions", description = "Removes network permissions.",
+ responseObject = SuccessResponse.class,
+ entityType = {Network.class},
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = false,
+ since = "4.17.0",
+ authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User})
+public class RemoveNetworkPermissionsCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(RemoveNetworkPermissionsCmd.class.getName());
+
+ private static final String s_name = "removenetworkpermissionsresponse";
Review comment:
idem
##########
File path: server/src/main/java/com/cloud/server/ManagementServerImpl.java
##########
@@ -3228,6 +3239,10 @@ public long getMemoryOrCpuCapacityByHost(final Long hostId, final short capacity
cmdList.add(ListNetworksCmd.class);
cmdList.add(RestartNetworkCmd.class);
cmdList.add(UpdateNetworkCmd.class);
+ cmdList.add(CreateNetworkPermissionsCmd.class);
+ cmdList.add(ListNetworkPermissionsCmd.class);
+ cmdList.add(RemoveNetworkPermissionsCmd.class);
+ cmdList.add(ResetNetworkPermissionsCmd.class);
Review comment:
do these belong here or in `NetworkServiceImpl` or maybe in `NetworkACLManager` or `NetworkUsageManager` or something?
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/network/RemoveNetworkPermissionsCmd.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.cloudstack.api.command.user.network;
+
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.AccountResponse;
+import org.apache.cloudstack.api.response.NetworkResponse;
+import org.apache.cloudstack.api.response.ProjectResponse;
+import org.apache.cloudstack.api.response.SuccessResponse;
+import org.apache.log4j.Logger;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.network.Network;
+import com.cloud.user.Account;
+
+import java.util.List;
+
+@APICommand(name = "removeNetworkPermissions", description = "Removes network permissions.",
+ responseObject = SuccessResponse.class,
+ entityType = {Network.class},
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = false,
+ since = "4.17.0",
+ authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User})
+public class RemoveNetworkPermissionsCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(RemoveNetworkPermissionsCmd.class.getName());
+
+ private static final String s_name = "removenetworkpermissionsresponse";
+
+ // ///////////////////////////////////////////////////
+ // ////////////// API parameters /////////////////////
+ // ///////////////////////////////////////////////////
+
+ @Parameter(name = ApiConstants.ACCOUNTS,
+ type = CommandType.LIST,
+ collectionType = CommandType.STRING,
+ description = "a comma delimited list of accounts within owner's domain. If specified, \"op\" parameter has to be passed in.")
+ private List<String> accountNames;
+
+ @Parameter(name = ApiConstants.ACCOUNT_IDS,
+ type = CommandType.LIST,
+ collectionType = CommandType.UUID,
+ entityType = AccountResponse.class,
+ description = "a comma delimited list of account IDs within owner's domain. If specified, \"op\" parameter has to be passed in.")
+ private List<Long> accountIds;
+
+ @Parameter(name = ApiConstants.NETWORK_ID, type = CommandType.UUID, entityType = NetworkResponse.class, required = true, description = "the network ID")
+ private Long networkId;
+
+ @Parameter(name = ApiConstants.PROJECT_IDS,
+ type = CommandType.LIST,
+ collectionType = CommandType.UUID,
+ entityType = ProjectResponse.class,
+ description = "a comma delimited list of projects within owner's domain. If specified, \"op\" parameter has to be passed in.")
+ private List<Long> projectIds;
+
+ // ///////////////////////////////////////////////////
+ // ///////////////// Accessors ///////////////////////
+ // ///////////////////////////////////////////////////
+
+ public List<String> getAccountNames() {
+ if (accountIds != null && accountNames != null) {
+ throw new InvalidParameterValueException("Accounts and accountNames can't be specified together");
+ }
+ return accountNames;
+ }
+
+ public List<Long> getAccountIds() {
+ if (accountIds != null && accountNames != null) {
+ throw new InvalidParameterValueException("Accounts and accountNames can't be specified together");
+ }
+ return accountIds;
+ }
Review comment:
as we have multiple validations, does it make sense to add a `validation()` and call it at the start of `execute()`
##########
File path: test/integration/smoke/test_network_permissions.py
##########
@@ -0,0 +1,500 @@
+# 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.
+
+"""
+Tests of network permissions
+"""
+
+import logging
+
+from nose.plugins.attrib import attr
+from marvin.cloudstackTestCase import cloudstackTestCase
+
+from marvin.lib.base import (Account,
+ Configurations,
+ Domain,
+ Project,
+ ServiceOffering,
+ VirtualMachine,
+ Zone,
+ Network,
+ NetworkOffering,
+ NetworkPermission,
+ NIC,
+ SSHKeyPair)
+
+from marvin.lib.common import (get_domain,
+ get_zone,
+ get_template)
+
+NETWORK_FILTER_ACCOUNT = 'account'
+NETWORK_FILTER_DOMAIN = 'domain'
+NETWORK_FILTER_ACCOUNT_DOMAIN = 'accountdomain'
+NETWORK_FILTER_SHARED = 'shared'
+NETWORK_FILTER_ALL = 'all'
+
+class TestNetworkPermissions(cloudstackTestCase):
+ """
+ Test user-shared networks
+ """
+ @classmethod
+ def setUpClass(cls):
+ cls.testClient = super(
+ TestNetworkPermissions,
+ cls).getClsTestClient()
+ cls.apiclient = cls.testClient.getApiClient()
+ cls.services = cls.testClient.getParsedTestDataConfig()
+
+ zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests())
+ cls.zone = Zone(zone.__dict__)
+ cls.template = get_template(cls.apiclient, cls.zone.id)
+ cls._cleanup = []
+
+ cls.logger = logging.getLogger("TestNetworkPermissions")
+ cls.stream_handler = logging.StreamHandler()
+ cls.logger.setLevel(logging.DEBUG)
+ cls.logger.addHandler(cls.stream_handler)
+
+ cls.domain = get_domain(cls.apiclient)
+
+ # Create sub-domain
+ cls.sub_domain = Domain.create(
+ cls.apiclient,
+ cls.services["acl"]["domain1"]
+ )
+
+ # Create domain admin and normal user
+ cls.domain_admin = Account.create(
+ cls.apiclient,
+ cls.services["acl"]["accountD1A"],
+ admin=True,
+ domainid=cls.sub_domain.id
+ )
+ cls.network_owner = Account.create(
+ cls.apiclient,
+ cls.services["acl"]["accountD11A"],
+ domainid=cls.sub_domain.id
+ )
+ cls.other_user = Account.create(
+ cls.apiclient,
+ cls.services["acl"]["accountD11B"],
+ domainid=cls.sub_domain.id
+ )
+ # Create project
+ cls.project = Project.create(
+ cls.apiclient,
+ cls.services["project"],
+ account=cls.domain_admin.name,
+ domainid=cls.domain_admin.domainid
+ )
+
+ # Create small service offering
+ cls.service_offering = ServiceOffering.create(
+ cls.apiclient,
+ cls.services["service_offerings"]["small"]
+ )
+
+ # Create network offering for isolated networks
+ cls.network_offering_isolated = NetworkOffering.create(
+ cls.apiclient,
+ cls.services["isolated_network_offering"]
+ )
+ cls.network_offering_isolated.update(cls.apiclient, state='Enabled')
+
+ # Create api clients for domain admin and normal user
+ cls.domainadmin_user = cls.domain_admin.user[0]
+ cls.domainadmin_apiclient = cls.testClient.getUserApiClient(
+ cls.domainadmin_user.username, cls.sub_domain.name
+ )
+ cls.networkowner_user = cls.network_owner.user[0]
+ cls.user_apiclient = cls.testClient.getUserApiClient(
+ cls.networkowner_user.username, cls.sub_domain.name
+ )
+
+ cls.otheruser_user = cls.other_user.user[0]
+ cls.otheruser_apiclient = cls.testClient.getUserApiClient(
+ cls.otheruser_user.username, cls.sub_domain.name
+ )
+
+ # Create networks for domain admin, normal user and project
+ cls.services["network"]["name"] = "Test Network Isolated - Project"
+ cls.project_network = Network.create(
+ cls.apiclient,
+ cls.services["network"],
+ networkofferingid=cls.network_offering_isolated.id,
+ domainid=cls.sub_domain.id,
+ projectid=cls.project.id,
+ zoneid=cls.zone.id
+ )
+
+ cls.services["network"]["name"] = "Test Network Isolated - Domain admin"
+ cls.domainadmin_network = Network.create(
+ cls.apiclient,
+ cls.services["network"],
+ networkofferingid=cls.network_offering_isolated.id,
+ domainid=cls.sub_domain.id,
+ accountid=cls.domain_admin.name,
+ zoneid=cls.zone.id
+ )
+
+ cls.services["network"]["name"] = "Test Network Isolated - Normal user"
+ cls.user_network = Network.create(
+ cls.apiclient,
+ cls.services["network"],
+ networkofferingid=cls.network_offering_isolated.id,
+ domainid=cls.sub_domain.id,
+ accountid=cls.network_owner.name,
+ zoneid=cls.zone.id
+ )
+
+ cls._cleanup.append(cls.service_offering)
+ cls._cleanup.append(cls.network_offering_isolated)
+
+ cls._cleanup.append(cls.sub_domain)
+ cls._cleanup.append(cls.network_owner)
+ cls._cleanup.append(cls.other_user)
+ cls._cleanup.append(cls.domain_admin)
+ cls._cleanup.append(cls.project)
Review comment:
I'd rather do these appends immediatly after the respective creates, just in case an exeption happens in the next create for some reason.
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/network/ResetNetworkPermissionsCmd.java
##########
@@ -0,0 +1,89 @@
+// 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.api.command.user.network;
+
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.NetworkResponse;
+import org.apache.cloudstack.api.response.SuccessResponse;
+import org.apache.log4j.Logger;
+
+import com.cloud.network.Network;
+import com.cloud.user.Account;
+
+@APICommand(name = "resetNetworkPermissions", description = "Resets network permissions.",
+ responseObject = SuccessResponse.class,
+ entityType = {Network.class},
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = false,
+ since = "4.17.0",
+ authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User})
+public class ResetNetworkPermissionsCmd extends BaseCmd {
+ public static final Logger s_logger = Logger.getLogger(ResetNetworkPermissionsCmd.class.getName());
+
+ private static final String s_name = "resetnetworkpermissionsresponse";
Review comment:
idem
##########
File path: server/src/main/java/com/cloud/network/NetworkServiceImpl.java
##########
@@ -1853,25 +1871,55 @@ public Network doInTransaction(TransactionStatus status) throws InsufficientCapa
if (isSystem == null || !isSystem) {
if (!permittedAccounts.isEmpty()) {
- //get account level networks
- networksToReturn.addAll(listAccountSpecificNetworks(buildNetworkSearchCriteria(sb, keyword, id, isSystem, zoneId, guestIpType, trafficType, physicalNetworkId, networkOfferingId,
- aclType, skipProjectNetworks, restartRequired, specifyIpRanges, vpcId, tags, display, vlanId, associatedNetworkId), searchFilter, permittedAccounts));
- //get domain level networks
- if (domainId != null) {
+ if (Network.NetworkFilter.account.equals(networkFilter) || Network.NetworkFilter.accountdomain.equals(networkFilter) || Network.NetworkFilter.all.equals(networkFilter)) {
+ //get account level networks
Review comment:
looks like we want an `EnumUtils` method `isAnyOf(value, ...)`
```
public static boolean isAnyOf(<T extends Enum<T>> checkVal, <T extends Enum<T>> val...) {
```
???
##########
File path: server/src/main/java/com/cloud/network/NetworkServiceImpl.java
##########
@@ -4908,6 +4988,162 @@ public boolean releasePodIp(ReleasePodIpCmdByAdmin ip) throws CloudRuntimeExcept
return new Pair<List<? extends GuestVlan>, Integer>(vlans.first(), vlans.second());
}
+ @Override
+ public List<? extends NetworkPermission> listNetworkPermissions(ListNetworkPermissionsCmd cmd) {
+ final Long networkId = cmd.getNetworkId();
+ NetworkVO network = _networksDao.findById(networkId);
+ if (network == null) {
+ throw new InvalidParameterValueException("unable to find network with id " + networkId);
+ }
+ final Account caller = CallContext.current().getCallingAccount();
+ _accountMgr.checkAccess(caller, AccessType.OperateEntry, true, network);
+
+ List<String> accountNames = new ArrayList<String>();
+ List<NetworkPermissionVO> permissions = _networkPermissionDao.findByNetwork(networkId);
+ return permissions;
+ }
+
+ @Override
+ public boolean createNetworkPermissions(CreateNetworkPermissionsCmd cmd) {
+ final Long id = cmd.getNetworkId();
+ List<String> accountNames = cmd.getAccountNames();
+ List<Long> accountIds = cmd.getAccountIds();
+ List<Long> projectIds = cmd.getProjectIds();
+
+ final Account caller = CallContext.current().getCallingAccount();
+ NetworkVO network = validateNetworkPermissionParameters(caller, id);
+
+ accountIds = populateAccounts(caller, accountIds, network.getDomainId(), accountNames, projectIds);
+
+ final List<Long> accountIdsFinal = accountIds;
+ final Account owner = _accountMgr.getAccount(network.getAccountId());
+ Transaction.execute(new TransactionCallbackNoReturn() {
+ @Override
+ public void doInTransactionWithoutResult(TransactionStatus status) {
+ for (Long accountId : accountIdsFinal) {
+ Account permittedAccount = _accountDao.findActiveAccountById(accountId, network.getDomainId());
+ if (permittedAccount != null) {
+ if (permittedAccount.getId() == owner.getId()) {
+ continue; // don't grant permission to the network owner, they implicitly have permission
+ }
+ NetworkPermissionVO existingPermission = _networkPermissionDao.findByNetworkAndAccount(id, permittedAccount.getId());
+ if (existingPermission == null) {
+ NetworkPermissionVO networkPermission = new NetworkPermissionVO(id, permittedAccount.getId());
+ _networkPermissionDao.persist(networkPermission);
+ }
+ } else {
+ throw new InvalidParameterValueException("Unable to find account " + accountId + " in the domain of network " + network + ". No permissions is added");
+ }
+ }
+ }
+ });
+
+ return true;
+ }
+
+ @Override
+ public boolean removeNetworkPermissions(RemoveNetworkPermissionsCmd cmd) {
+ final Long id = cmd.getNetworkId();
+ List<String> accountNames = cmd.getAccountNames();
+ List<Long> accountIds = cmd.getAccountIds();
+ List<Long> projectIds = cmd.getProjectIds();
+
+ final Account caller = CallContext.current().getCallingAccount();
+ NetworkVO network = validateNetworkPermissionParameters(caller, id);
+
+ accountIds = populateAccounts(caller, accountIds, network.getDomainId(), accountNames, projectIds);
+
+ _networkPermissionDao.removePermissions(id, accountIds);
+
+ return true;
+ }
+
+ @Override
+ public boolean resetNetworkPermissions(ResetNetworkPermissionsCmd cmd) {
+
+ final Long id = cmd.getNetworkId();
+
+ final Account caller = CallContext.current().getCallingAccount();
+ NetworkVO network = validateNetworkPermissionParameters(caller, id);
+
+ _networkPermissionDao.removeAllPermissions(id);
+
+ return true;
+ }
+
+ private NetworkVO validateNetworkPermissionParameters(Account caller, Long id) {
+
+ final NetworkVO network = _networksDao.findById(id);
+
+ if (network == null) {
+ throw new InvalidParameterValueException("unable to find network with id " + id);
+ }
+
+ if (network.getAclType() == ACLType.Domain) {
+ throw new InvalidParameterValueException("network is already shared in domain");
+ }
+
+ if (network.getVpcId() != null) {
+ throw new InvalidParameterValueException("VPC tiers cannot be shared");
+ }
+
+ _accountMgr.checkAccess(caller, AccessType.OperateEntry, true, network);
+
+ final Account owner = _accountMgr.getAccount(network.getAccountId());
+ if (owner.getType() == Account.ACCOUNT_TYPE_PROJECT) {
+ // Currently project owned networks cannot be shared outside project but is available to all users within project by default.
+ throw new InvalidParameterValueException("Update network permissions is an invalid operation on network " + network.getName()
+ + ". Project owned networks cannot be shared outside network.");
+ }
+
+ //Only admin or owner of the network should be able to change its permissions
+ if (caller.getId() != owner.getId() && !_accountMgr.isAdmin(caller.getId())) {
+ throw new InvalidParameterValueException("Unable to grant permission to account " + caller.getAccountName() + " as it is neither admin nor owner or the network");
+ }
+
+ return network;
+ }
+
+ private List<Long> populateAccounts(Account caller, List<Long> accountIds, Long domainId, List<String> accountNames, List<Long> projectIds) {
+ if (accountIds == null) {
+ accountIds = new ArrayList<Long>();
+ }
+ // convert projectIds to accountIds
+ if (projectIds != null) {
+ for (Long projectId : projectIds) {
+ Project project = _projectMgr.getProject(projectId);
+ if (project == null) {
+ throw new InvalidParameterValueException("Unable to find project by id " + projectId);
+ }
+
+ if (!_projectMgr.canAccessProjectAccount(caller, project.getProjectAccountId())) {
+ throw new InvalidParameterValueException("Account " + caller + " can't access project id=" + projectId);
+ }
+ accountIds.add(project.getProjectAccountId());
+ }
+ }
Review comment:
`convertProjectIdsToAccountIds()`
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/network/ListNetworkPermissionsCmdByAdmin.java
##########
@@ -0,0 +1,28 @@
+// 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.api.command.admin.network;
+
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ResponseObject.ResponseView;
+import org.apache.cloudstack.api.command.admin.AdminCmd;
+import org.apache.cloudstack.api.command.user.network.ListNetworkPermissionsCmd;
+import org.apache.cloudstack.api.response.NetworkPermissionsResponse;
+
+@APICommand(name = "listNetworkPermissions", description = "List network visibility and all accounts that have permissions to view this network.", responseObject = NetworkPermissionsResponse.class, responseView = ResponseView.Full,
+ requestHasSensitiveInfo = false,
+ responseHasSensitiveInfo = false)
+public class ListNetworkPermissionsCmdByAdmin extends ListNetworkPermissionsCmd implements AdminCmd {}
Review comment:
I suppose this is needed to see what we are allowed to do at service level. As it is not clear here, can you add a short javadoc describing the (purpose of) the class?
i.e.
```suggestion
/**
* We need to nothing special but the service needs to no the root admin called the API.
*/
public class ListNetworkPermissionsCmdByAdmin extends ListNetworkPermissionsCmd implements AdminCmd {}
```
##########
File path: ui/src/config/section/network.js
##########
@@ -44,8 +44,8 @@ export default {
}
return fields
},
- filters: ['all', 'isolated', 'shared', 'l2'],
- searchFilters: ['keyword', 'zoneid', 'domainid', 'account', 'tags'],
+ filters: ['all', 'account', 'domain', 'shared'],
Review comment:
account and domain are both isolated right? or are they both both isolated and l2?
##########
File path: server/src/main/java/com/cloud/network/NetworkServiceImpl.java
##########
@@ -4908,6 +4988,162 @@ public boolean releasePodIp(ReleasePodIpCmdByAdmin ip) throws CloudRuntimeExcept
return new Pair<List<? extends GuestVlan>, Integer>(vlans.first(), vlans.second());
}
+ @Override
+ public List<? extends NetworkPermission> listNetworkPermissions(ListNetworkPermissionsCmd cmd) {
+ final Long networkId = cmd.getNetworkId();
+ NetworkVO network = _networksDao.findById(networkId);
+ if (network == null) {
+ throw new InvalidParameterValueException("unable to find network with id " + networkId);
+ }
+ final Account caller = CallContext.current().getCallingAccount();
+ _accountMgr.checkAccess(caller, AccessType.OperateEntry, true, network);
+
+ List<String> accountNames = new ArrayList<String>();
+ List<NetworkPermissionVO> permissions = _networkPermissionDao.findByNetwork(networkId);
+ return permissions;
+ }
+
+ @Override
+ public boolean createNetworkPermissions(CreateNetworkPermissionsCmd cmd) {
+ final Long id = cmd.getNetworkId();
+ List<String> accountNames = cmd.getAccountNames();
+ List<Long> accountIds = cmd.getAccountIds();
+ List<Long> projectIds = cmd.getProjectIds();
+
+ final Account caller = CallContext.current().getCallingAccount();
+ NetworkVO network = validateNetworkPermissionParameters(caller, id);
+
+ accountIds = populateAccounts(caller, accountIds, network.getDomainId(), accountNames, projectIds);
+
+ final List<Long> accountIdsFinal = accountIds;
+ final Account owner = _accountMgr.getAccount(network.getAccountId());
+ Transaction.execute(new TransactionCallbackNoReturn() {
+ @Override
+ public void doInTransactionWithoutResult(TransactionStatus status) {
+ for (Long accountId : accountIdsFinal) {
+ Account permittedAccount = _accountDao.findActiveAccountById(accountId, network.getDomainId());
+ if (permittedAccount != null) {
+ if (permittedAccount.getId() == owner.getId()) {
+ continue; // don't grant permission to the network owner, they implicitly have permission
+ }
+ NetworkPermissionVO existingPermission = _networkPermissionDao.findByNetworkAndAccount(id, permittedAccount.getId());
+ if (existingPermission == null) {
+ NetworkPermissionVO networkPermission = new NetworkPermissionVO(id, permittedAccount.getId());
+ _networkPermissionDao.persist(networkPermission);
+ }
+ } else {
+ throw new InvalidParameterValueException("Unable to find account " + accountId + " in the domain of network " + network + ". No permissions is added");
+ }
+ }
+ }
+ });
+
+ return true;
+ }
+
+ @Override
+ public boolean removeNetworkPermissions(RemoveNetworkPermissionsCmd cmd) {
+ final Long id = cmd.getNetworkId();
+ List<String> accountNames = cmd.getAccountNames();
+ List<Long> accountIds = cmd.getAccountIds();
+ List<Long> projectIds = cmd.getProjectIds();
+
+ final Account caller = CallContext.current().getCallingAccount();
+ NetworkVO network = validateNetworkPermissionParameters(caller, id);
+
+ accountIds = populateAccounts(caller, accountIds, network.getDomainId(), accountNames, projectIds);
+
+ _networkPermissionDao.removePermissions(id, accountIds);
+
+ return true;
+ }
+
+ @Override
+ public boolean resetNetworkPermissions(ResetNetworkPermissionsCmd cmd) {
+
+ final Long id = cmd.getNetworkId();
+
+ final Account caller = CallContext.current().getCallingAccount();
+ NetworkVO network = validateNetworkPermissionParameters(caller, id);
+
+ _networkPermissionDao.removeAllPermissions(id);
+
+ return true;
+ }
+
+ private NetworkVO validateNetworkPermissionParameters(Account caller, Long id) {
+
+ final NetworkVO network = _networksDao.findById(id);
+
+ if (network == null) {
+ throw new InvalidParameterValueException("unable to find network with id " + id);
+ }
+
+ if (network.getAclType() == ACLType.Domain) {
+ throw new InvalidParameterValueException("network is already shared in domain");
+ }
+
+ if (network.getVpcId() != null) {
+ throw new InvalidParameterValueException("VPC tiers cannot be shared");
+ }
+
+ _accountMgr.checkAccess(caller, AccessType.OperateEntry, true, network);
+
+ final Account owner = _accountMgr.getAccount(network.getAccountId());
+ if (owner.getType() == Account.ACCOUNT_TYPE_PROJECT) {
+ // Currently project owned networks cannot be shared outside project but is available to all users within project by default.
+ throw new InvalidParameterValueException("Update network permissions is an invalid operation on network " + network.getName()
+ + ". Project owned networks cannot be shared outside network.");
+ }
+
+ //Only admin or owner of the network should be able to change its permissions
+ if (caller.getId() != owner.getId() && !_accountMgr.isAdmin(caller.getId())) {
+ throw new InvalidParameterValueException("Unable to grant permission to account " + caller.getAccountName() + " as it is neither admin nor owner or the network");
+ }
+
+ return network;
+ }
+
+ private List<Long> populateAccounts(Account caller, List<Long> accountIds, Long domainId, List<String> accountNames, List<Long> projectIds) {
+ if (accountIds == null) {
+ accountIds = new ArrayList<Long>();
+ }
+ // convert projectIds to accountIds
+ if (projectIds != null) {
+ for (Long projectId : projectIds) {
+ Project project = _projectMgr.getProject(projectId);
+ if (project == null) {
+ throw new InvalidParameterValueException("Unable to find project by id " + projectId);
+ }
+
+ if (!_projectMgr.canAccessProjectAccount(caller, project.getProjectAccountId())) {
+ throw new InvalidParameterValueException("Account " + caller + " can't access project id=" + projectId);
+ }
+ accountIds.add(project.getProjectAccountId());
+ }
+ }
+ // convert accountNames to accountIds
+ final Domain domain = _domainDao.findById(domainId);
+ if (accountNames != null) {
+ for (String accountName : accountNames) {
+ Account permittedAccount = _accountDao.findActiveAccount(accountName, domain.getId());
+ if (permittedAccount != null) {
+ if (permittedAccount.getId() == caller.getId()) {
+ continue;
+ }
+ accountIds.add(permittedAccount.getId());
+ }
+ }
+ }
Review comment:
`convertAccountNamesToAccountIds()` and this utility method I'm sure would be used elsewhere in the system as well.
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org