CLOUDSTACK-10327: Do not invalidate the session when API command not found

[olivierlemasle]

Description

CloudStack SSO (using security.singlesignon.key) does not work anymore with CloudStack 4.11, since commit 9988c26, which introduced a regression due to a refactoring: every API request that is not "validated" generates the same error (401 - Unauthorized) and invalidates the session.

However, CloudStack UI executes a call to listConfigurations in method bypassLoginCheck. A non-admin user does not have the permissions to execute this request, which causes an error 401:

{"listconfigurationsresponse":{"uuidList":[],"errorcode":401,"errortext":"unable to verify user credentials and/or request signature"}}

The session (already created by SSO) is then invalidated and the user cannot access to CloudStack UI (error "Session Expired").

Before 9988c26 (up to CloudStack 4.10), an error 432 was returned (and ignored):

{"errorresponse":{"uuidList":[],"errorcode":432,"cserrorcode":9999,"errortext":"The user is not allowed to request the API command or the API command does not exist"}}

Even if the call to listConfigurations was removed, another call to listIdps also lead to an error 401 for user accounts if the SAML plugin is not enabled.

This pull request aims to fix the SSO issue, by restoring errors 432 (instead of 401 + invalidate session) for commands not available. However, if an API command is explicitly denied using ACLs or if the session key is incorrect, it still generates an error 401 and invalidates the session.

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)

How Has This Been Tested?

Compiled and ran CloudStack.

• API commands not explicitly allowed for a role (e.g. listConfigurations for a user) generate 432.
• API commands not enabled (e.g. listIdps with SAML disabled) generate 432.
• non-existing API commands (e.g. fooBar) generate 432

However, API calls with session key removed generate 401 and invalidate session.
SSO is ok.

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.
• I have added tests to cover my changes.
• All new and existing tests passed.

[yadvr]

@olivierlemasle can you edit the PR and rebase against 4.11 branch? I tried to do that, but Github requires rebasing against 4.11.

[olivierlemasle]

@rhtyd Ok, I've rebased the PR against 4.11 and updated the base branch accordingly.

[yadvr]

Why not throw ServerApiException with suitable http error code so as to not cause log out in the UI. Simply handle permission denied exception here? (or the line below)

[olivierlemasle]

@rhtyd I'm not sure I understand your suggestion. The Http error code is specified here, in line 964). The ServerApiException is caught on ApiServlet which generates the HTTP response.

[yadvr]

Thanks @olivierlemasle. I've one comment, since the last change around this was made by Wido, let's ask @wido for review.

[wido]

I don't see anything odd in this PR, looks good to me. I didn't catch this situation.

[yadvr]

@blueorangutan package

[blueorangutan]

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

[blueorangutan]

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-1818

[borisstoyanov]

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

Trillian test result (tid-2449)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 7042 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2498-t2449-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_accounts.py
Smoke tests completed. 0 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

[borisstoyanov]

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

Trillian test result (tid-2456)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 95355 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2498-t2456-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_loadbalance.py
Intermitten failure detected: /marvin/tests/smoke/test_routers.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_host_maintenance.py
Smoke tests completed. 65 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_04_restart_network_wo_cleanup	Failure	4.11	test_routers.py
test_04_rvpc_network_garbage_collector_nics	Failure	538.30	test_vpc_redundant.py

[borisstoyanov]

LGTM

[rafaelweingartner]

@borisstoyanov here we also have that same test_04_restart_network_wo_cleanup failure.

[GabrielBrascher]

Code and Tests LGTM.

[yadvr]

Merged based on test results and code review.