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

Jump to bottom

Fixes #24268 - Add permissions to canned admin #7532

Merged
merged 1 commit into from Nov 8, 2018
Merged

Fixes #24268 - Add permissions to canned admin #7532

merged 1 commit into from Nov 8, 2018

Conversation

xprazak2
Copy link
Member

@theforeman-bot
Copy link

Issues: #24268

@chris1984
Copy link
Member

[test katello]

@xprazak2
Copy link
Member Author

Tests will be failing until the foreman part is merged.

@san7ket
Copy link

san7ket commented Sep 20, 2018

I am getting a error, on rake:compile while I try to compile it with foreman's PR.
Looks like we need a rebase here on katello 3.9.0

rake aborted!
Foreman::Exception: ERF42-2432 [Foreman::Exception]: Invalid role name, only ''Viewer', 'Manager', 'Organization admin', 'System admin'' is allowed to be extended from a plugin
/home/vagrant/foreman/app/registries/foreman/plugin/rbac_support.rb:42:in `check_role_name_before_extending'
/home/vagrant/foreman/app/registries/foreman/plugin/rbac_support.rb:33:in `block in add_permissions_to_default_roles'
/home/vagrant/foreman/app/registries/foreman/plugin/rbac_support.rb:32:in `each'
/home/vagrant/foreman/app/registries/foreman/plugin/rbac_support.rb:32:in `add_permissions_to_default_roles'
/home/vagrant/foreman/app/registries/foreman/plugin.rb:310:in `block (2 levels) in add_permissions_to_default_roles'

san7ket
san7ket reviewed Sep 24, 2018
View reviewed changes
lib/katello/plugin.rb Outdated
@@ -300,6 +300,8 @@
Katello::PermissionCreator.new(self).define
add_all_permissions_to_default_roles

add_permissions_to_default_roles 'Canned admin' => [:create_lifecycle_environments, :create_content_views]
Copy link

Choose a reason for hiding this comment

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

Needs a name change here to System admin

@xprazak2 xprazak2 force-pushed the canned-org branch 2 times, most recently from cc404a9 to 0e1f545 Compare September 24, 2018 12:45
@xprazak2
Copy link
Member Author

Rebased and updated.

@johnsonm325
Copy link
Member

[test katello]

@johnsonm325
Copy link
Member

@xprazak2 Looks like we have another "rake aborted!" error in the tests.

rake aborted!
ActiveRecord::NoDatabaseError: FATAL:  database "katello-pr-test-3452-development" does not exist

@xprazak2
Copy link
Member Author

[test katello]

@xprazak2
Copy link
Member Author

@johnsonm325, what do I have to do to make the tests pass? The db still fails to get configured properly...

@johnsonm325
Copy link
Member

[test katello]

evgeni
evgeni reviewed Oct 25, 2018
View reviewed changes
lib/katello/plugin.rb
@@ -315,6 +315,8 @@
Katello::PermissionCreator.new(self).define
add_all_permissions_to_default_roles

add_permissions_to_default_roles 'System admin' => [:create_lifecycle_environments, :create_content_views]
Copy link
Member

Choose a reason for hiding this comment

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

I think add_permissions_to_default_roles needs to go inside Foreman::Plugin.register :katello do to work properly

Copy link
Member

Choose a reason for hiding this comment

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

nvm, I can't count.

Copy link
Member Author

Choose a reason for hiding this comment

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

The whole file is one big block, I think it is inside.

@xprazak2 xprazak2 mentioned this pull request Oct 25, 2018
Closed
@evgeni
Copy link
Member

evgeni commented Oct 25, 2018

So I had another look at this.

Please have a look at
https://github.com/theforeman/foreman/blob/0c08b71336684e60669fe952ca65e36bd4f7c133/app/registries/foreman/plugin.rb#L317-L324
add_all_permissions_to_default_roles has a return if Foreman.in_setup_db_rake? || !permission_table_exists?, so that function is basically a noop while we run rake db:create. But add_permissions_to_default_roles does not have such a guard, and so it executes, before the table was created and boom.

@evgeni
Copy link
Member

evgeni commented Oct 25, 2018

Opened https://projects.theforeman.org/issues/25312

@tbrisker
Copy link
Member

core side has been merged

@evgeni evgeni mentioned this pull request Oct 28, 2018
Merged
@evgeni
Copy link
Member

evgeni commented Oct 28, 2018

[test katello]

@evgeni
Copy link
Member

evgeni commented Oct 28, 2018

Tests pass now. Rubocop is unhappy, but that's unrelated to this PR.

@jturel jturel self-assigned this Oct 31, 2018
@jturel
Copy link
Member

jturel commented Oct 31, 2018

[test katello]

chris1984
chris1984 reviewed Oct 31, 2018
View reviewed changes
Copy link
Member

@chris1984 chris1984 left a comment

Choose a reason for hiding this comment

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

@jturel Tests are passing now

@beav
Copy link
Contributor

beav commented Nov 5, 2018

is this no longer "blocked by another pr"?

@evgeni
Copy link
Member

evgeni commented Nov 5, 2018

Correct, both the core feature and the test fix were merged

@san7ket
Copy link

san7ket commented Nov 6, 2018

Yes, the core feature is merged, I have one question though, if this is merged now, will we still get this in katello 3.9 RC (if there's another) or it goes in 3.10 ?

@jturel
Copy link
Member

jturel commented Nov 7, 2018

Looks good. I pinged @zjhuntin on getting it into 3.9 since he is the release owner.

One question - how was the determination made as to which permissions should be applied to 'System admin'

@xprazak2
Copy link
Member Author

xprazak2 commented Nov 8, 2018

'System admin' is supposed to create new organizations and because the organization default content view and library lifecycle env are created during that process, permissions for these two are needed for organization to be successfully created.

jturel
jturel approved these changes Nov 8, 2018
View reviewed changes
Copy link
Member

@jturel jturel left a comment

Choose a reason for hiding this comment

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

Works as advertised 🥇

@jturel
Copy link
Member

jturel commented Nov 8, 2018

@san7ket this will be part of 3.9!

@jturel jturel merged commit 5eb08cf into Katello:master Nov 8, 2018
@xprazak2
Copy link
Member Author

xprazak2 commented Nov 9, 2018

I opened #7826 with a cp into 3.9 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chris1984 chris1984 chris1984 left review comments

@san7ket san7ket san7ket left review comments

@evgeni evgeni evgeni approved these changes

@jturel jturel jturel approved these changes

Assignees

@jturel jturel

Labels
Needs testing Not yet reviewed
Projects
None yet
Milestone
No milestone
10 participants
@xprazak2 @theforeman-bot @chris1984 @san7ket @johnsonm325 @evgeni @tbrisker @jturel @beav @jlsherrill