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

Tenant scoping on vms #4316

Merged
merged 5 commits into from
Sep 16, 2015
Merged

Tenant scoping on vms #4316

merged 5 commits into from
Sep 16, 2015

Conversation

jrafanie
Copy link
Member

  • Include tenancy mixin to provide the scope_by_tenant? for VmOrTemplate
  • Scope Rbac.search by tenant only if the class respond_to?(:scope_by_tenant?)
  • Add current_tenant to user/group through MiqGroup#tenant.

Returns vms without a tenant and ones in my tenant.
If there is no tenant, no filtering by tenant is done.

https://trello.com/c/1uf5urJz/

@gtanzillo @Fryguy @kbrock Please review, throw 🍅

@@ -248,7 +248,33 @@ def self.find_targets_with_user_group_rbac(klass, scope, rbac_filters, find_opti
end
end

def self.group(user_or_group)
Copy link
Member Author

Choose a reason for hiding this comment

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

HATE 😱

@kbrock
Copy link
Member

kbrock commented Sep 11, 2015

The thing I need to figure out is how to say "I want to scope on a tenant and all of it's parents"

determining the tenant_id and all the parent_ids is easy.
determining what filtering is necessary is not

@jrafanie
Copy link
Member Author

I think we need use cases or existing ones translated to specific examples. I have a hard time understanding one off examples without more context.

@kbrock
Copy link
Member

kbrock commented Sep 11, 2015

In automate, I want to fetch all namespaces for a tenant. The rule around tenants is: all namespaces that the tenant and parents can see.

tenant_id = group(user_or_group).try(:tenant_id)
return find_options unless tenant_id

tenant_id_clause = ["#{klass.table_name}.tenant_id = ? OR #{klass.table_name}.tenant_id IS NULL", tenant_id]
Copy link
Member

Choose a reason for hiding this comment

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

tenant_id_clause = {klass.table_name => {:tenant_id => tenant_id}}

If we need the nil then:

tenant_id_clause = {klass.table_name => {:tenant_id => [tenant_id, nil]}}

Copy link
Member Author

Choose a reason for hiding this comment

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

Great, yeah, see my diagram above, not sure about NULL, I added it to spark conversation.

Copy link
Member

Choose a reason for hiding this comment

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

no objects will have a nil tenant

Returns vms without a tenant and ones in my tenant.
If there is no tenant, no filtering by tenant is done.

It feels wrong to not use Relations via .where here but we might need
a named scope to filter the remaining conditions that don't use .where.

https://trello.com/c/1uf5urJz/
@jrafanie jrafanie changed the title [WIP] Tenant scoping on vms Tenant scoping on vms Sep 15, 2015
@other_user.save
results, = Rbac.search(:class => "Vm", :results_format => :objects)
expect(results).to eq [@owner_vm]
end
Copy link
Member Author

Choose a reason for hiding this comment

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

for with_user - we want to possibly change a user's group and make sure it picks the right one

@kbrock Thanks, I added test cases for a user leaving/joining a tenant via changing groups. Is this what you meant?

@miq-bot
Copy link
Member

miq-bot commented Sep 16, 2015

Checked commits jrafanie/manageiq@acccd96~...73b176c with ruby 1.9.3, rubocop 0.33.0, and haml-lint 0.13.0
6 files checked, 1 offense detected

app/models/rbac.rb

def self.find_targets_with_rbac(klass, scope, rbac_filters, find_options = {}, user_or_group = nil)
# TODO: check if these find_options should be duplicated/modified in place
Copy link
Member

Choose a reason for hiding this comment

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

@jrafanie I think it's fine to modify find_options in place. They are built up by the caller (Rbac.search) for the sole purpose of using them in find_targets_with_rbac.

Copy link
Member

Choose a reason for hiding this comment

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

in the past we have been avoiding this.
since we tend to call this with a merge, that allows us to avoid actually manipulating.

but it is probably no harm

@gtanzillo
Copy link
Member

👍

gtanzillo added a commit that referenced this pull request Sep 16, 2015
@gtanzillo gtanzillo merged commit f563d59 into ManageIQ:master Sep 16, 2015
@jrafanie jrafanie deleted the tenant_scoping branch September 16, 2015 18:32
@jrafanie jrafanie added this to the Sprint 30 Ending Oct 5, 2015 milestone Sep 16, 2015
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