Need to pass the user's group in to automate when the provision starts. #61

lfu · 2017-07-31T21:23:35Z

User's current group might have changed before the provision finishes.
So the user's current group from DB may be different from the user's group when the request is sent into automate.

Part of ManageIQ/manageiq#15696.
~~Blocks ManageIQ/manageiq-content#156.~~
Includes ManageIQ/manageiq#15760.

https://bugzilla.redhat.com/show_bug.cgi?id=1467364

@miq-bot add_label bug, euwe/yes, fine/yes

gmcculloug · 2017-08-02T18:28:01Z

@lfu Please review failing tests.

lfu · 2017-08-02T22:41:49Z

lib/miq_automation_engine/engine/miq_ae_engine/miq_ae_object.rb

@@ -250,7 +250,7 @@ def process_assertions(message)

    def user_info_attributes(user)
      {'user' => user, 'tenant' => user.current_tenant, 'miq_group' => user.current_group}.each do |k, v|
-        value = MiqAeObject.convert_value_based_on_datatype(v.id, v.class.name)
+        value = MiqAeMethodService::MiqAeServiceModelBase.wrap_results(v)


This solves the issue that the user object is got from DB with wrong current group in automate.

MiqAeObject.convert_value_based_on_datatype calls find at service model layer which fetches the object from DB then calls MiqAeMethodService::MiqAeServiceModelBase.wrap_results on the object.

Since the user object is already available with the correct current group, we can call MiqAeMethodService::MiqAeServiceModelBase.wrap_results on it directly.

With this change, then ManageIQ/manageiq-content#156 is not needed as the in-memory user object has the current group point to the group that the user is in when the request is made.

@lfu I am not sure about this call, I am thinking the find should get the service model. Can we check if the find is doing the right thing.

@mkanoor The find gets the user object from DB which is not what we want here since the user's current group from DB is not the correct requester group.

We should use the in-memory user object that we already have at hand and wrap it into service model object. This way we don't need to change the expose of current_group in MiqAeServiceUser because the user object has got correct current group in memory. expose itself does not query the DB.

mkanoor · 2017-08-03T14:13:48Z

lib/miq_automation_engine/engine/miq_ae_engine/miq_ae_object.rb

@@ -250,7 +250,7 @@ def process_assertions(message)

    def user_info_attributes(user)
      {'user' => user, 'tenant' => user.current_tenant, 'miq_group' => user.current_group}.each do |k, v|
-        value = MiqAeObject.convert_value_based_on_datatype(v.id, v.class.name)
+        value = MiqAeMethodService::MiqAeServiceModelBase.wrap_results(v)


@lfu I am not sure about this call, I am thinking the find should get the service model. Can we check if the find is doing the right thing.

mkanoor · 2017-08-03T14:15:01Z

lib/miq_automation_engine/engine/miq_ae_engine.rb

@@ -307,7 +307,8 @@ def self.ae_user_object(options = {})
    raise "user_id not specified in Automation request" if options[:user_id].blank?
    # raise "group_id not specified in Automation request" if options[:miq_group_id].blank?


@lfu Can we raise this exception because we are expecting that all calls into Automate are going to pass in the miq_group_id.

lfu · 2017-08-07T14:42:23Z

@miq-bot remove_label wip

gmcculloug · 2017-08-07T14:54:16Z

@mkanoor Please take another look.

mkanoor

We might have to change the documentation to indicate that instead of using $evm.root['user'].current_group we should be using $evm.root['miq_group'] to get the requesters current group

gmcculloug · 2017-08-07T17:21:56Z

lib/miq_automation_engine/engine/miq_ae_engine.rb

@@ -305,9 +305,11 @@ def self.resolve_automation_object(uri, user_obj, attr = nil, options = {}, read

  def self.ae_user_object(options = {})
    raise "user_id not specified in Automation request" if options[:user_id].blank?
-    # raise "group_id not specified in Automation request" if options[:miq_group_id].blank?
+    raise "miq_group_id not specified in Automation request" if options[:miq_group_id].blank?


@lfu Do you know why this was commented out before? Are there any code paths you are aware of that might run into this new requirement?

@gmcculloug miq_group_id was not required because a user's current_group is used as default.
Seems AutomationTask is not passing miq_group_id when calling into automate: here. Will fix it in another PR.

Ok, please like the related PR back here when it is ready, I would like to have it available before merging this one since we know it will break Automation requests.

Add ManageIQ/manageiq#15760.

A user may belong to multiple groups. miq_group_id became required via ManageIQ/manageiq-automation_engine#61. https://bugzilla.redhat.com/show_bug.cgi?id=1467364

User's current group might have changed before the provision finishes. So the user's current group from DB may be different from the user's group when the request is sent into automate. https://bugzilla.redhat.com/show_bug.cgi?id=1467364

miq-bot · 2017-08-09T20:49:07Z

Checked commit lfu@660b111 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 1 offense detected

lib/miq_automation_engine/engine/miq_ae_engine.rb

❗ - Line 312, Col 7 - Style/GlobalVars - Do not introduce global variables.

gmcculloug · 2017-08-09T20:51:03Z

lib/miq_automation_engine/engine/miq_ae_engine.rb

@@ -305,9 +305,11 @@ def self.resolve_automation_object(uri, user_obj, attr = nil, options = {}, read

  def self.ae_user_object(options = {})
    raise "user_id not specified in Automation request" if options[:user_id].blank?
-    # raise "group_id not specified in Automation request" if options[:miq_group_id].blank?
+    # raise "miq_group_id not specified in Automation request" if options[:miq_group_id].blank?


The raise will be re-introduced upstream after this PR is merged. This will allow the PR to be back-ported and to older branch and give more burn-in time upstream.

simaishi · 2017-08-10T00:47:58Z

Fine backport (to manageiq repo) details:

$ git log -1
commit a3bb99ccd92d428c2a7c8218eb09b6e92c731500
Author: Greg McCullough <[email protected]>
Date:   Wed Aug 9 17:01:12 2017 -0400

    Merge pull request #61 from lfu/service_user_in_groups_1467364
    
    Need to pass the user's group in to automate when the provision starts.
    (cherry picked from commit 8de13ba4c7413710e4481cd7484b3675a12bd517)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1480007

simaishi · 2017-08-15T22:09:58Z

Euwe backport (to manageiq repo) details:

$ git log -1
commit eea7b864c36f4cb1dadc87043bc2c8844c48c4db
Author: Greg McCullough <[email protected]>
Date:   Wed Aug 9 17:01:12 2017 -0400

    Merge pull request #61 from lfu/service_user_in_groups_1467364
    
    Need to pass the user's group in to automate when the provision starts.
    (cherry picked from commit 8de13ba4c7413710e4481cd7484b3675a12bd517)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1481859

miq-bot added bug euwe/yes labels Jul 31, 2017

This was referenced Jul 31, 2017

Set user's group to the requester group. ManageIQ/manageiq#15696

Merged

[WIP] Get the correct group when a user belongs to multiple groups. ManageIQ/manageiq-content#156

Closed

gmcculloug self-assigned this Jul 31, 2017

gmcculloug requested a review from mkanoor July 31, 2017 21:30

gmcculloug changed the title ~~Need to pass the user's group in to automate when the provision starts.~~ [WIP] Need to pass the user's group in to automate when the provision starts. Aug 1, 2017

gmcculloug added the wip label Aug 1, 2017

lfu force-pushed the service_user_in_groups_1467364 branch from 415396b to 77267db Compare August 2, 2017 17:53

lfu force-pushed the service_user_in_groups_1467364 branch from 77267db to 6db2dfb Compare August 2, 2017 18:40

lfu commented Aug 2, 2017

View reviewed changes

mkanoor reviewed Aug 3, 2017

View reviewed changes

lfu force-pushed the service_user_in_groups_1467364 branch 2 times, most recently from c200e29 to d686357 Compare August 7, 2017 14:04

lfu changed the title ~~[WIP] Need to pass the user's group in to automate when the provision starts.~~ Need to pass the user's group in to automate when the provision starts. Aug 7, 2017

miq-bot removed the wip label Aug 7, 2017

mkanoor approved these changes Aug 7, 2017

View reviewed changes

gmcculloug reviewed Aug 7, 2017

View reviewed changes

lfu added a commit to lfu/manageiq that referenced this pull request Aug 8, 2017

miq_group_id is required by automate.

46c7340

A user may belong to multiple groups. miq_group_id became required via ManageIQ/manageiq-automation_engine#61. https://bugzilla.redhat.com/show_bug.cgi?id=1467364

lfu mentioned this pull request Aug 8, 2017

miq_group_id is required by automate. ManageIQ/manageiq#15760

Merged

lfu force-pushed the service_user_in_groups_1467364 branch from d686357 to 660b111 Compare August 9, 2017 20:45

gmcculloug reviewed Aug 9, 2017

View reviewed changes

gmcculloug approved these changes Aug 9, 2017

View reviewed changes

gmcculloug merged commit 8de13ba into ManageIQ:master Aug 9, 2017

gmcculloug added this to the Sprint 67 Ending Aug 21, 2017 milestone Aug 9, 2017

simaishi added fine/backported and removed fine/yes labels Aug 10, 2017

simaishi added euwe/backported and removed euwe/yes labels Aug 15, 2017

lfu deleted the service_user_in_groups_1467364 branch September 29, 2018 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Need to pass the user's group in to automate when the provision starts. #61

Need to pass the user's group in to automate when the provision starts. #61

lfu commented Jul 31, 2017 •

edited

Loading

gmcculloug commented Aug 2, 2017

lfu Aug 2, 2017

mkanoor Aug 3, 2017

lfu Aug 4, 2017

mkanoor Aug 3, 2017

mkanoor Aug 3, 2017

lfu commented Aug 7, 2017

gmcculloug commented Aug 7, 2017

mkanoor left a comment

gmcculloug Aug 7, 2017

lfu Aug 7, 2017

gmcculloug Aug 7, 2017

lfu Aug 8, 2017

miq-bot commented Aug 9, 2017

gmcculloug Aug 9, 2017

simaishi commented Aug 10, 2017

simaishi commented Aug 15, 2017

		@@ -307,7 +307,8 @@ def self.ae_user_object(options = {})
		raise "user_id not specified in Automation request" if options[:user_id].blank?
		# raise "group_id not specified in Automation request" if options[:miq_group_id].blank?

Need to pass the user's group in to automate when the provision starts. #61

Need to pass the user's group in to automate when the provision starts. #61

Conversation

lfu commented Jul 31, 2017 • edited Loading

gmcculloug commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lfu commented Aug 7, 2017

gmcculloug commented Aug 7, 2017

mkanoor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Aug 9, 2017

Choose a reason for hiding this comment

simaishi commented Aug 10, 2017

simaishi commented Aug 15, 2017

lfu commented Jul 31, 2017 •

edited

Loading