Enhance validation decorator with admin expected error code

Registered by Felipe Monteiro

Similar to [0].

A recent patch [1] spurred discussion around extending the validation decorator with an expected error code for admin and non-admin roles, because an API may throw different error codes.

For example (from [1]):

    @testtools.skipUnless(CONF.compute_feature_enabled.live_migration,
                          'Live migration feature is not enabled.')
    @rbac_rule_validation.action(
        service="nova",
        rule="os_compute_api:os-migrate-server:migrate_live")
    @decorators.idempotent_id('33520834-72c8-4edb-a189-a2e0fc9eb0d3')
    def test_migration_live(self):
        # Create a fake host to migrate. If a user is allowed, BadRequest
        # will be thrown else Forbidden will be thrown.
        target_host = data_utils.rand_name('host')
        self.rbac_utils.switch_role(self, switchToRbacRole=True)
        try:
            self.client.live_migrate_server(self.server['id'],
                                            host=target_host,
                                            block_migration=False,
                                            disk_over_commit=False)
        except exceptions.BadRequest:
            pass

The reason the code above is written this way is because the test, in order to succeed, requires at minimum 2 compute hosts to work, because migration by definition requires migrating a VM from one host to another. The Nova API for live_migrate_server [2] first checks whether the user has permissions to perform the action, and then tries to migrate the server; if a fake host is passed in, then a BadRequest is thrown. In other words, for non-admin Forbidden is thrown and admin BadRequest is thrown.

There are some advantages to this approach:
1) The test can be executed on any infrastructure; a basic DevStack will suffice.
2) Try/except blocks of the kind above are avoided.

There are some disadvantages to this approach:
1) Eating an exception like 403 or 404 makes sense in the context of Patrole, as Patrole expects permissions failures (i.e. negative testing), but eating an exception like a 400 transcends the scope of Patrole and enters into more questionable territory.
2) The test will perhaps be harder to maintain and debug, as expecting different error codes for different roles introduces more complexity and requires carefully reading the API code before writing the test.

Because Patrole is really only concerned with testing authorization against APIs given a particular role, the above code could be regarded as fair game and this blueprint as legitimate. Or, we could avoid writing code like this altogether and disregard this blueprint.

[0] https://blueprints.launchpad.net/patrole/+spec/enhance-validation-decorator-with-error-code
[1] https://review.openstack.org/#/c/445435/7/patrole_tempest_plugin/tests/api/compute/test_server_rbac.py
[2] https://github.com/openstack/nova/blob/99b1a4a7b78ea50ff2a259980c7e4370729eeafc/nova/api/openstack/compute/migrate_server.py

Blueprint information

Status:
Complete
Approver:
DavidPurcell
Priority:
Not
Drafter:
Felipe Monteiro
Direction:
Needs approval
Assignee:
None
Definition:
Obsolete
Series goal:
None
Implementation:
Unknown
Milestone target:
None
Completed by
Felipe Monteiro

Related branches

Sprints

Whiteboard

(?)

Work Items

This blueprint contains Public information 
Everyone can see this information.

Subscribers

No subscribers.