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

Added user_id, group_id, tenant_id #16089

Merged
merged 4 commits into from
Oct 3, 2017
Merged

Conversation

mkanoor
Copy link
Contributor

@mkanoor mkanoor commented Oct 2, 2017

If the User and group ids are passed in we run the method
using User.with_user which sets the User.current_user

When we execute object#methods via the Queue we loose the user context under which the method should run. When we execute methods for Generic Objects we need the user context, this PR would allow the user_id/group_id/tenant_id to be recorded in the Queue and set using User.with_user

Depends on Schema PR
ManageIQ/manageiq-schema#83

This PR is needed by these other PRs
#16077

If the User and group ids are passed in we run the method
using User.with_user which sets the User.current_user
@mkanoor
Copy link
Contributor Author

mkanoor commented Oct 3, 2017

The specs are failing because the schema changes have not been merged

@kbrock
Copy link
Member

kbrock commented Oct 3, 2017

@mkanoor I'm not sure if we need more around setting the current group

I do see https://github.com/kbrock/manageiq/blob/master/lib/rbac/filterer.rb#L499

@jntullo @abellotti I remember thinking through a bunch of implications when we set the user.current_group in the api controller. But I couldn't find it. To be honest, I didn't look THAT hard. Could you point us to that logic, so we make sure that we use this logic when we pull items off of the queue?

oh and, ugh Timeout.timeout (We need it, but it also needed to be said)

@jntullo
Copy link

jntullo commented Oct 3, 2017

@kbrock not entirely sure I understand what you're looking for, because for the most part we aren't setting a user/group, which is what created the need for #16077 , where we can then set ae_user_identity on a generic object before calling the method

We are setting the userid here when creating the task, if this is what you mean

@Fryguy
Copy link
Member

Fryguy commented Oct 3, 2017

Merged the schema, so bouncing

@Fryguy Fryguy closed this Oct 3, 2017
@Fryguy Fryguy reopened this Oct 3, 2017
@mkanoor
Copy link
Contributor Author

mkanoor commented Oct 3, 2017

@jntullo
We set the User.current_user based on the logged in user here https://github.com/ManageIQ/manageiq-api/blob/master/app/controllers/api/base_controller/authentication.rb#L22
This line sets the current group for the in memory user object.

So when the API makes the call into create generic task we can use

https://github.com/ManageIQ/manageiq-api/blob/eb172af56203e8f1747d98a17217d1d91ce715a9/app/controllers/api/base_controller/action.rb#L27

:user_id => User.current_user.id,
:group_id =>User.current_user.current_group.id,
:tenant_id => User.current_user.current_tenant.id

After this PR is merged

@kbrock
Copy link
Member

kbrock commented Oct 3, 2017

@mkanoor thanks - you found it - It uses current_group_by_description= -- which ensures that the group (and therefore tenant) is valid for this user, with a special provision for super_user?.

Probably too complex for our needs here.

@miq-bot
Copy link
Member

miq-bot commented Oct 3, 2017

Checked commits mkanoor/manageiq@06cea72~...21404dc with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks fine. ⭐

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.

5 participants