comply with api key constraint by DaanHoogland · Pull Request #2710 · apache/cloudstack

DaanHoogland · Jun 13, 2018

Description

moveUser only works for users without API keys which beats the (one of the two) main purpose(s) of the moveUser API.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

GitHub Issue/PRs

Fixes: #2707

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration testsuite with all test that can run on my environment has passed.

rafaelweingartner · Jun 13, 2018

server/src/com/cloud/user/AccountManagerImpl.java

@@ -1734,6 +1734,8 @@ public Boolean doInTransaction(TransactionStatus status) {
                UserVO newUser = new UserVO(user);


Why creating a new user entry? I mean, isn't this a matter of simply updating the accountId of the user? Then, you would not change the UUID of the "old" user entry, and you would also not need to update the keys and then mark the user as removed.

Then you would loose the audit trail in the old account

hmm, what do you mean by audit trail? Is it possible for the old account to list the "moved" users?

that would be a nice extra feature for the future ;)
No, actions in the old account must be tracable to a user in that account. As the ownership of assets is with accounts and not users, and the physical user is going to loose access to their old assets anyway, it is more convenient to copy the login data of the user to a new user object.

Ok. Then, there is one thing I still do not understand. If we want to keep the history, why are we changing the old user UUID? Shouldn't it be the opposite?

We are not changing db id which is used for foreign key relations. and we are copying the uuid to the external identity for future reference. through the new user we can then also find any 'real external' ids.

Got it. Thanks for the details.

DaanHoogland · Jun 13, 2018

@blueorangutan package

blueorangutan · Jun 13, 2018

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · Jun 13, 2018

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2122

DaanHoogland · Jun 14, 2018

@blueorangutan test

blueorangutan · Jun 14, 2018

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

DaanHoogland · Jun 14, 2018

not coded any tests but here is the procedure:
create accounts:
6e349308-babf-4e2a-9dde-b956bb142b9a and
c7413b70-7ef0-4dc3-9470-57719d3a354e in this example
create a user 'mover' in c7413b70-7ef0-4dc3-9470-57719d3a354e
give 'mover' keys:

(local) SBCM5> list users username=mover domainid=efdbedc6-6f38-11e8-b011-0673d0010712 listall=true
{
  "count": 1,
  "user": [
    {
      "account": "hoogland",
      "accountid": "c7413b70-7ef0-4dc3-9470-57719d3a354e",
      "accounttype": 1,
      "apikey": "PASEKfhttrjez87e9I2qsdiL95lYpvVTnVlibaMHDiJYN4KWKa_IYeFngkTP3XCp18triFmMA7jdmdmZJUTihw",
      "created": "2018-06-14T13:10:52+0000",
      "domain": "ROOT",
      "domainid": "efdbedc6-6f38-11e8-b011-0673d0010712",
      "email": "m@ov.er",
      "firstname": "Mo",
      "id": "97005683-b42a-4912-b3f7-d0bb68ee9c9e",
      "iscallerchilddomain": false,
      "isdefault": false,
      "lastname": "Ver",
      "roleid": "058540e9-6f39-11e8-b011-0673d0010712",
      "rolename": "Root Admin",
      "roletype": "Admin",
      "state": "enabled",
      "timezone": "UTC",
      "username": "mover",
      "usersource": "native"
    }
  ]
}

move user accountid=6e349308-babf-4e2a-9dde-b956bb142b9a  id=97005683-b42a-4912-b3f7-d0bb68ee9c9e 
{
  "success": "true"
}

mdesaive

Looks good to me! Thanks a lot for taking care!

yadvr · Jun 18, 2018

@blueorangutan package

blueorangutan · Jun 18, 2018

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · Jun 18, 2018

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2125

yadvr · Jun 18, 2018

@blueorangutan test

blueorangutan · Jun 18, 2018

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · Jun 18, 2018

Trillian test result (tid-2760)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31327 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2710-t2760-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
Intermitten failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 61 look OK, 6 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestDeployVirtioSCSIVM>:setup	`Error`	0.00	test_deploy_virtio_scsi_vm.py
test_03_vpc_privategw_restart_vpc_cleanup	`Error`	187.84	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	318.82	test_privategw_acl.py
test_03_secured_to_nonsecured_vm_migration	`Error`	199.69	test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration	`Error`	60.49	test_vm_life_cycle.py
test_02_VPC_default_routes	`Failure`	122.36	test_vpc_router_nics.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	2.42	test_host_maintenance.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	3.96	test_hostha_kvm.py

DaanHoogland · Jun 18, 2018

The errors are mostly know to be related to other issues. @PaulAngus can we merge?

PaulAngus · Jun 19, 2018

LGTM - tested moving a user who had API keys - no issues.

..Merging

comply with api key constraint

d126cd2

rafaelweingartner reviewed Jun 13, 2018

View reviewed changes

DaanHoogland added this to the 4.11.1.0 milestone Jun 14, 2018

rafaelweingartner approved these changes Jun 15, 2018

View reviewed changes

mdesaive mentioned this pull request Jun 15, 2018

Moving User with API key fails through MySQLIntegrityConstraintViolationException #2707

Closed

mdesaive approved these changes Jun 15, 2018

View reviewed changes

yadvr approved these changes Jun 18, 2018

View reviewed changes

PaulAngus added component:management-server component:api status:ready-for-merge labels Jun 19, 2018

PaulAngus merged commit 2891662 into apache:4.11 Jun 19, 2018

DaanHoogland removed the status:ready-for-merge label Dec 4, 2024

		@@ -1734,6 +1734,8 @@ public Boolean doInTransaction(TransactionStatus status) {
		UserVO newUser = new UserVO(user);

Search code, repositories, users, issues, pull requests...

Conversation

DaanHoogland commented Jun 13, 2018

Description

Types of changes

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

Uh oh!

rafaelweingartner Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner Jun 13, 2018

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jun 13, 2018

Uh oh!

blueorangutan commented Jun 13, 2018

Uh oh!

blueorangutan commented Jun 13, 2018

Uh oh!

DaanHoogland commented Jun 14, 2018

Uh oh!

blueorangutan commented Jun 14, 2018

Uh oh!

DaanHoogland commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdesaive left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Jun 18, 2018

Uh oh!

blueorangutan commented Jun 18, 2018

Uh oh!

blueorangutan commented Jun 18, 2018

Uh oh!

yadvr commented Jun 18, 2018

Uh oh!

blueorangutan commented Jun 18, 2018

Uh oh!

blueorangutan commented Jun 18, 2018

Uh oh!

DaanHoogland commented Jun 18, 2018

Uh oh!

PaulAngus commented Jun 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DaanHoogland Jun 14, 2018 •

edited

Loading

DaanHoogland commented Jun 14, 2018 •

edited

Loading