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

Refs #5217: Merging roles branch to master. #4073

Merged
merged 57 commits into from May 15, 2014
Merged

Refs #5217: Merging roles branch to master. #4073

merged 57 commits into from May 15, 2014

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented May 13, 2014

This merges the following converted entities to master:

Lifecycle Environments
Subscriptions
Activation Keys
Products and Repositories
Sync Functionality
Sync Plans
Content Views (except for promote)
Content Hosts
GPG Keys

For each of the above entities, Foreman based permissions can be used to create filters that are then added to roles. This management is done through the normal Foreman roles/filters interface.

Testing of these entities can be done by creating a new user and assigning role(s) with various Katello entity permissions and attempting to perform functions through the API or UI.

Note the following:

  • Not all UI actions are hidden/shown based upon current permissions but the server will deny
  • Not all Katello entities are converted (host collections, content and content search and environment locking are being finish up and targeted for master)
  • Super admin users will still have all functionality available to them

ehelms and others added 30 commits April 16, 2014 19:45
Fixes #5230: Adds CRUD permissions for GPG Keys.
Fixes #5260 - adding CRUD permissions for Sync Plans.
Merge remote-tracking branch 'upstream/master' into roles
Merge remote-tracking branch 'upstream/master' into roles
Removing unused controller methods from the environments controller.
Also wiping out unused tests.
Fixes #5416 - adding CRUD permissions for activation keys.
Fixes # 5437 - Env controller cleanup
Conflicts:
	app/controllers/katello/environments_controller.rb
	app/controllers/katello/gpg_keys_controller.rb
	app/controllers/katello/sync_plans_controller.rb
	config/routes.rb
	engines/bastion/app/controllers/bastion/bastion_controller.rb
Merge remote-tracking branch 'upstream/master' into roles
Fixes #5261: Adds CRUD permissions for Products and Repositories.
Fixes #5536: Wraps Organization APIs in authorization protections.
Fixes 5521: Adding permissions for managing subscriptions and manifests.
Fixes #5531: Wrapping sync controllers in with new permissions.
Fixes #5434, adding permissions for Content Views.
Also includes changes for the UI and api to deal with the new
set of perms
1) New Env shown only if you have create
2) Can update env only if you have update
3) Can delete env only if you have delete

Controllers and tests modified appropriately.
Refs #5217: Adjusts product organization_id migration to account for provider field.
@@ -34,6 +34,7 @@ class Engine < ::Rails::Engine
initializer "katello.load_app_instance_data" do |app|
app.config.paths['db/migrate'] += Katello::Engine.paths['db/migrate'].existent
app.config.autoload_paths += Dir["#{config.root}/app/lib"]
app.config.autoload_paths += Dir["#{config.root}/app/policies"]
Copy link
Member

Choose a reason for hiding this comment

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

We need this only on loadpath not as auto-loadpath. Files are required from policies not auto-loaded.

Copy link
Member

Choose a reason for hiding this comment

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

+1, Ideally, I would not add a new load path as all: the more load paths, the harder to find where the things are defined

@ehelms
Copy link
Member Author

ehelms commented May 14, 2014

Please note - there are stories that will NOT be deemed fixed until the entire UI and API has been scoured post full conversion. We don't mind fixing egregious errors, but if there are edge cases, instances that we are unsure of if they go one way or another, given the current timing I would prefer if we handled those potential issues via the aforementioned stories that we created to ensure that everything is locked down post full conversion.

@iNecas
Copy link
Member

iNecas commented May 14, 2014

Opened issue in redmine is a valid response to such comments

@@ -72,6 +85,16 @@ def no_permission_user
user.own_role.permissions.delete_all
user
end

def add_role(permission_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use Role#add_permissions to simplify this
see https://github.com/theforeman/foreman/blob/develop/app/models/role.rb#L138

@bbuckingham
Copy link
Member

@partha, In UI, I am not able to create a valid filter for "Lifecycle Environments". Selecting that resource type does not update the list of available permissions.

@parthaa
Copy link
Contributor

parthaa commented May 14, 2014

@bbuckingham theforeman/foreman#1431 needs to get merged before it will work normally.
I suggest trying this instead to get around this ..

  1. Click on "New Filter",
  2. Select "Lifecycle Environment"
    3 Click on the text box next to permission. Type * and hit enter. You should see all the permissions for env. From there you can assign and continue..

@bbuckingham
Copy link
Member

@parthaa, Attempting to create an env for a user w/ lifecycle environment CRUD permissions generates an error indicating:

An error occurred creating the environment: Validation failed: Locations You cannot remove locations that are used by hosts or inherited., Config templates You cannot remove config templates that are used by hosts or inherited.

EDIT: I saw this issue on master also, so will create a redmine issue for it. It is outside of the scope of this PR.

Refs #5217: Adding check that the consumer cert matches the passed in consumer identity when present.
@bbuckingham
Copy link
Member

Attempting to delete an activation key (e.g. as admin or user with key permissions) generates an error. E.g.:

An error occurred removing the Activation Key: Katello::Resources::Candlepin::ActivationKey: 404 Resource Not Found {"displayMessage":"ActivationKey with id 8a8d019245faf29e0145fbdedc030015 could not be found.","requestUuid":"92865350-96c1-4bee-9ee7-b00818eca1ab"} (GET /candlepin/activation_keys/8a8d019245faf29e0145fbdedc030015)

EDIT: Confirmed fixed with #4088

@bbuckingham
Copy link
Member

A user with only 'view_content_hosts' is not exposed to the Hosts -> Content Hosts navigation.

EDIT: Confirmed fixed with #4092

This was introduced with merges from master and would cause errors
on deletion since the show rabl file sends attributes stored in Candlepin.
And since the activation key has been deleted at that point from Candlepin,
the object is no longer available.
Refs #5217: Fix activation key destroy.
@ehelms
Copy link
Member Author

ehelms commented May 15, 2014

@iNecas @pitr-ch mind giving this another once over with the updates ?

@iNecas
Copy link
Member

iNecas commented May 15, 2014

Could you rebase first?

@ehelms
Copy link
Member Author

ehelms commented May 15, 2014

I was hoping to avoid merging master until the end of review but can of you
feel you need it.
On May 15, 2014 7:20 AM, "Ivan Necas" notifications@github.com wrote:

Could you rebase first?


Reply to this email directly or view it on GitHubhttps://github.com//pull/4073#issuecomment-43197915
.

…er-to-roles

Conflicts:
	app/controllers/katello/api/v1/content_uploads_controller.rb
	app/controllers/katello/api/v1/environments_controller.rb
	app/controllers/katello/api/v1/puppet_modules_controller.rb
	app/controllers/katello/api/v1/systems_controller.rb
	app/controllers/katello/api/v2/activation_keys_controller.rb
	app/controllers/katello/api/v2/content_view_filters_controller.rb
	app/controllers/katello/api/v2/content_view_puppet_modules_controller.rb
	app/controllers/katello/api/v2/content_view_versions_controller.rb
	app/controllers/katello/api/v2/content_views_controller.rb
	app/controllers/katello/api/v2/environments_controller.rb
	app/controllers/katello/api/v2/organizations_controller.rb
	app/controllers/katello/api/v2/products_controller.rb
	app/controllers/katello/api/v2/repositories_controller.rb
	app/controllers/katello/api/v2/subscriptions_controller.rb
	app/controllers/katello/api/v2/sync_controller.rb
	app/controllers/katello/api/v2/sync_plans_controller.rb
	app/controllers/katello/api/v2/systems_controller.rb
Refs #5217: Fix content host menu item and add organization scoping.
@bbuckingham
Copy link
Member

ACK wrt my comments

@ehelms
Copy link
Member Author

ehelms commented May 15, 2014

@iNecas merged master into PR, should be good now

@iNecas
Copy link
Member

iNecas commented May 15, 2014

All my comments addressed or commented. ACK

ehelms added a commit that referenced this pull request May 15, 2014
Refs #5217: Merging roles branch to master.
@ehelms ehelms merged commit 374ea70 into master May 15, 2014
@stbenjam stbenjam deleted the roles branch December 12, 2014 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants