bwsw commented on issue #905: BUG-ID: CLOUDSTACK-8922: Unable to delete IP tag URL: https://github.com/apache/cloudstack/pull/905#issuecomment-346590497 It looks overcomplicated, really, search over resourceId is a wrong logic. It should search only against uuds. We discussed the problem in a mail list with @rafaelweingartner. Right now I'm working on fix for it too. The code is here: https://github.com/bwsw/cloudstack/blob/4.10.1-NP/server/src/com/cloud/tags/TaggedResourceManagerImpl.java All marvin component and smoke are passing, and tag specific too: ``` nosetests-2.7 --with-marvin --marvin-config=setup/dev/advanced.cfg -w test/integration/component --with-xunit --xunit-file=/tmp/bvt_selfservice_cases.xml --zone=Sandbox-simulator --hypervisor=simulator -a tags=advanced,required_hardware=false -a speed=0 `pwd`/test/integration/component/test_tags.py ``` I believe, leaving internal id search is a wrong way to deal with it, because they can not and shouldn't appear in API. Also, the code I'm working on includes security fix for listing resource tags because right now the code from the PR doesn't even checks privileges for listing: ``` @Override public List<? extends ResourceTag> listByResourceTypeAndId(ResourceObjectType type, long resourceId) { return _resourceTagDao.listBy(resourceId, type); } ``` Mine: ``` @Override public List<? extends ResourceTag> listByResourceTypeAndId(ResourceObjectType resourceType, long resourceId) { Account caller = CallContext.current().getCallingAccount(); Pair<Long, Long> accountDomainPair = getAccountDomain(resourceId, resourceType); Long domainId = accountDomainPair.second(); Long accountId = accountDomainPair.first(); checkResourceAccessible(accountId, domainId, "Account '" + caller + "' doesn't have permissions to list tags" + " for resource '" + resourceId + "' of type '" + resourceType + "'."); return _resourceTagDao.listBy(resourceId, resourceType); } ``` So, basically the PR: 1. it doesn't fix the semantical problem allowing to search with internal ids 2. doesn't fix security fault when listing resource tags. I need couple of days to finish the code because I want to add account tags feature and have to implement set of marvin tests for it.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
