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

Fixes #9895 - user in Discovery Reader role should see discovery rules #173

Closed
wants to merge 1 commit into from

Conversation

orrabin
Copy link
Member

@orrabin orrabin commented Mar 25, 2015

after adding this I needed to re-add the filters with the correct resource and then the role started working for me.

@lzap
Copy link
Member

lzap commented Mar 25, 2015

Can you add couple of tests for this? We can inspire from Foreman Core where we have some permission tests already.

@lzap
Copy link
Member

lzap commented Mar 25, 2015

I think we need to do a db migration here or whatever is needed to get this working since we already released discovery 2.0 with this bug. Also, as part of this migration can you please delete the old role called Discovery? Do this only in case there are no users associated with it, otherwise skip. I have renamed this role to Discovery Manager for 2.0 and forgot to do this.

@lzap
Copy link
Member

lzap commented Mar 31, 2015

Ok disregard part of the old role called Discovery from previous comment as I've already included this fix in #176 but we still need to delete extra permissions which has not been deleted. This will cause issues in the future. Before this patch:

[15] pry(main)> Permission.where("name like '%_discovery_rules'").select("name, resource_type")
2015-03-31 15:05:07 [D]   Permission Load (0.2ms)  SELECT name, resource_type FROM "permissions" WHERE (name like '%_discovery_rules')
=> [#<Permission name: "view_discovery_rules", resource_type: nil>, #<Permission name: "new_discovery_rules", resource_type: nil>, #<Permission name: "edit_discovery_rules", resource_type: nil>, #<Permission name: "execute_discovery_rules", resource_type: nil>, #<Permission name: "delete_discovery_rules", resource_type: nil>]

After this patch:

[16] pry(main)> Permission.where("name like '%_discovery_rules'").select("name, resource_type")
2015-03-31 15:06:01 [D]   Permission Load (0.4ms)  SELECT name, resource_type FROM "permissions" WHERE (name like '%_discovery_rules')
=> [#<Permission name: "view_discovery_rules", resource_type: nil>, #<Permission name: "new_discovery_rules", resource_type: nil>, #<Permission name: "edit_discovery_rules", resource_type: nil>, #<Permission name: "execute_discovery_rules", resource_type: nil>, #<Permission name: "delete_discovery_rules", resource_type: nil>, #<Permission name: "view_discovery_rules", resource_type: "DiscoveryRule">, #<Permission name: "new_discovery_rules", resource_type: "DiscoveryRule">, #<Permission name: "edit_discovery_rules", resource_type: "DiscoveryRule">, #<Permission name: "execute_discovery_rules", resource_type: "DiscoveryRule">, #<Permission name: "delete_discovery_rules", resource_type: "DiscoveryRule">]

We need to delete those. To trigger the error add a new testing role with a permission you have changed in the engine.rb:

role "Discovery Executor Test", [:execute_discovery_rules]

Restart and voilla:

/home/lzap/work/foreman/app/models/role.rb:131:in `add_permissions': some permissions were not found (ArgumentError)
        from /home/lzap/work/foreman/app/models/role.rb:146:in `add_permissions!'
        from /home/lzap/work/foreman/app/services/foreman/plugin.rb:198:in `block in role'

@lzap lzap added this to the 2.0.3 milestone Mar 31, 2015
@ohadlevy
Copy link
Member

ping?

@orrabin
Copy link
Member Author

orrabin commented May 13, 2015

@lzap I added a migration, can you please retest?

@lzap
Copy link
Member

lzap commented May 18, 2015

Merged as 0d9cee2, thank you!

@lzap lzap closed this May 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants