Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allows specification for optional multiple identifiers #13827

Merged
merged 2 commits into from
Feb 10, 2017

Conversation

abellotti
Copy link
Member

@abellotti abellotti commented Feb 8, 2017

  • Supports current single identifier, where user must be allowed that specific role for authorization:
      :post:
      - :name: delete
        :identifier: vm_delete
  • Supports multiple identifiers (comma separated), where user must be allowed at least one of the roles specified for authorization:
      :post:
      - :name: delete
        :identifier: vm_delete,instance_terminate

Pivitol Story: https://www.pivotaltracker.com/story/show/139477969

@abellotti
Copy link
Member Author

abellotti commented Feb 8, 2017

/cc @Fryguy @jntullo as discussed.

- Supports current single identifier, where user must be allowed
  that specific role for authorization:
      :post:
      - :name: delete
        :identifier: vm_delete

- Supports multiple identifiers, where user must be
  allowed at least one of the roles specified for authorization:
      :post:
      - :name: delete
        :identifier:
        - vm_delete
        - instance_terminate

- Adding role based authorization rspecs
@abellotti abellotti force-pushed the api_multiple_identifiers branch from 1debae5 to 4e9bff8 Compare February 9, 2017 00:30
@abellotti abellotti changed the title [WIP] Allows specification for optional multiple identifiers Allows specification for optional multiple identifiers Feb 9, 2017
@abellotti abellotti removed the wip label Feb 9, 2017
@abellotti
Copy link
Member Author

Updated Usage to leverage yaml array.

  • Supports multiple identifiers, where user must be allowed at least one of the roles specified for authorization:
      :post:
      - :name: delete
        :identifier:
        - vm_delete
        - instance_terminate
  • Adding role based authorization rspecs

saved_identifier = config.identifier
config.identifier = new_identifier
yield
config.identifier = saved_identifier
Copy link
Member

@Fryguy Fryguy Feb 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in an ensure

def vms_get_test
   # stuff
   yield
ensure
   config.identifier = saved_identifier
end

This way if a test raises an exception, you can be sure it puts the settings back.

Alternately, the test should not modify settings in this way. Instead, there should be probably be a stub_settings type method, like we do for other Settings. stubbing ensures the values return to normal after test completion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A stub method would read better as well. It's not clear (perhaps because of the vms_get_test method name), that what you are testing is different combinations of RBAC identifiers via the settings.

compare:

vms_get_test("vm_view_role1") do
  #stuff
end

vs

stub_api_settings(:collections, :vms, :collection, :actions, :get, "vm_view_role1")
#stuff

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Fryguy
Copy link
Member

Fryguy commented Feb 9, 2017

cc @gtanzillo

- Introduced stub_api_action_role
- Updated tests to use the new stub_api_action_role
- Fixed issue in api spec helper update_user_role not able to handle identifier arrays.
@miq-bot
Copy link
Member

miq-bot commented Feb 10, 2017

Checked commits abellotti/manageiq@4e9bff8~...598ced7 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🏆

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

@gtanzillo gtanzillo added this to the Sprint 54 Ending Feb 13, 2017 milestone Feb 10, 2017
@gtanzillo gtanzillo merged commit 538ddd6 into ManageIQ:master Feb 10, 2017
@abellotti abellotti deleted the api_multiple_identifiers branch February 15, 2017 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants