> On Jan. 24, 2014, 6:52 a.m., Prachi Damle wrote: > > I see many files unrelated to this fix have been updated and present in the > > patch. > > Please can you update the patch with changes needed for the fix only? > > > > Following are some such files. > > api/src/com/cloud/user/AccountService.java > > engine/schema/src/com/cloud/user/UserVO.java > > plugins/dedicated-resources/test/org/apache/cloudstack/dedicated/manager/DedicatedApiUnitTest.java: > > 8 changes [ 1 2 3 4 5 6 7 8 ] > > plugins/deployment-planners/implicit-dedication/test/org/apache/cloudstack/implicitplanner/ImplicitPlannerTest.java: > > 16 changes [ 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 ] > > plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java > > plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java > > > > Also the below file has all Mockito references replaced by Matchers - what > > is the reason for this change? > > plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/VmwareDatacenterApiUnitTest.java > > Rajani Karuturi wrote: > The major changes for this patch are in the following files: > api/src/com/cloud/user/AccountService.java: 1 change [ 1 ] - > added a > new overloaded method to take the source of the account for the create call > api/src/com/cloud/user/User.java: 2 changes [ 1 2 ] -> added the new > field's getter and its enum > engine/schema/src/com/cloud/user/UserVO.java: 4 changes [ 1 2 3 4 ] -> > mapped the new database cloumn and added getter/setter, also changed the > constructor to take the new field > > plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java: > 2 changes [ 1 2 ] -> this is the one which is called when creating an ldap > user and this calls create account with source as LDAP > > plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java: > 2 changes [ 1 2 ] -> this is ldap's import multiple users command. change is > same as above > server/src/com/cloud/user/AccountManagerImpl.java: 6 changes [ 1 2 3 4 5 > 6 ] -> implementation of the new method > setup/db/db/schema-421to430.sql: 2 changes [ 1 2 ] -> SQL to add the new > column > > The rest changes in tests/mock classes are due to change in constructor > of UserVo which cant be avoided > VmwareDatacenterApiUnitTest also has the same change as above. I dont see > the Mockito/Matchers change you mentioned. > https://reviews.apache.org/r/17120/diff/#5 > > Also note that patch1/diff1 is for 4.3 and and patch2/diff2 is for > master. As we are getting conflicts due to spaces changes I created two > patches. The changes are same in both the patches. > > > > Prachi Damle wrote: > You can avoid changing all the tests/mock classes. Don't change the > existing UserVo constuctor - in the existing constructor default to source > NATIVE so that no other callers are changed. > Add a new constructor to take in source. > > What about existing ldap users on some existing setup - is there a way to > identify those? We should add upgrade steps to update the 'source' for those > user records > > > > Rajani Karuturi wrote: > I followed this approach for AccountService. The changes to the UserVo > constructor mainly changed only the test files. the tests passed before and > after the changes. I dont see a reason why we should avoid changing > tests/mocks. > > There is no way to identify the existing ldap users in the current > scenario. This change helps us do that from this release. > > Prachi Damle wrote: > Yes same approach as AccountService. I thought you said you cant avoid > changing other classes due to UserVO change. > In general the reason is to avoid breaking any integration elsewhere and > since you already have a known default value to set, this is a way to avoid > changes in places other than your usecase. Ofcourse, not necessary to do it > here considering its test code. > > > Provided this does not need to be supported backwards, then no upgrade is > fine too. > > > > > > > > > > > > > > > Rajani Karuturi wrote: > Hi Prachi, > If you are fine with changes, can you push this commit? > > Thanks, > Rajani > > Rohit Yadav wrote: > Hi, any update on this? > > In case of SAML, I simply use a different namespace in UUID that starts > with saml://, that helps to identify that the user is authenticated/create by > a SAML authentication. I used this to avoid changing the schema at all.
Hi Rohit, these changes got merged with another security issue I was working and is under review. I added the extra column. I will discard this review request. - Rajani ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17120/#review32702 ----------------------------------------------------------- On Aug. 13, 2014, 10:31 a.m., Rajani Karuturi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17120/ > ----------------------------------------------------------- > > (Updated Aug. 13, 2014, 10:31 a.m.) > > > Review request for cloudstack, Abhinandan Prateek, Ian Duffy, Min Chen, and > Prachi Damle. > > > Bugs: CLOUDSTACK-5910 > https://issues.apache.org/jira/browse/CLOUDSTACK-5910 > > > Repository: cloudstack-git > > > Description > ------- > > added an extra column to mark the imported ldap users as from ldap > > > Diffs > ----- > > api/src/com/cloud/user/AccountService.java a9be292 > api/src/com/cloud/user/User.java 36e9028 > engine/schema/src/com/cloud/user/UserVO.java 68879f6 > > plugins/dedicated-resources/test/org/apache/cloudstack/dedicated/manager/DedicatedApiUnitTest.java > 213174b > > plugins/deployment-planners/implicit-dedication/test/org/apache/cloudstack/implicitplanner/ImplicitPlannerTest.java > 4182193 > > plugins/hypervisors/vmware/test/com/cloud/hypervisor/vmware/VmwareDatacenterApiUnitTest.java > 4a00489 > > plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java > 2f81688 > > plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java > 100ffe6 > > plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java > 89cec65 > server/src/com/cloud/user/AccountManagerImpl.java 5204589 > server/test/com/cloud/configuration/ConfigurationManagerTest.java f1bb30a > server/test/com/cloud/network/DedicateGuestVlanRangesTest.java 1615b84 > server/test/com/cloud/user/MockAccountManagerImpl.java 62e7fc8 > server/test/com/cloud/vm/UserVmManagerTest.java 83f7520 > server/test/com/cloud/vpc/NetworkACLManagerTest.java 629afa3 > server/test/com/cloud/vpc/NetworkACLServiceTest.java 786789f > server/test/org/apache/cloudstack/affinity/AffinityApiUnitTest.java 061fd42 > server/test/org/apache/cloudstack/network/lb/CertServiceTest.java a67a9ab > > server/test/org/apache/cloudstack/region/gslb/GlobalLoadBalancingRulesServiceImplTest.java > d152c66 > setup/db/db/schema-421to430.sql ccff7c1 > > Diff: https://reviews.apache.org/r/17120/diff/ > > > Testing > ------- > > manually tested > > > Thanks, > > Rajani Karuturi > >
