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 #10127 - remove dynflow stacktrace when unregistering, BZ 1208100 #5175

Merged
merged 1 commit into from
Apr 14, 2015
Merged

fixes #10127 - remove dynflow stacktrace when unregistering, BZ 1208100 #5175

merged 1 commit into from
Apr 14, 2015

Conversation

komidore64
Copy link
Contributor

Fixing a DynFlow stacktrace that is thrown when a content-host attempts
to unregister. DynFlow would throw an ActiveRecord Validation error
stating 'Resource cannot be blank'. The resource is it referring to is a
user/owner of the DynFlow task that is being planned. Since the
content-host was registered with an activation key it is userless, and
dynflow doesn't like that.

@ehelms
Copy link
Member

ehelms commented Apr 13, 2015

Shouldn't we be seeing this with any unregister? What makes AK as the original registration special?

@ehelms
Copy link
Member

ehelms commented Apr 14, 2015

A little more background on the question, we appear to be doing an authorize_client call before the consumer_destroy action which calls into this code and thus should be setting User.current based upon the consumer itself -- https://github.com/Katello/katello/blob/master/app/services/katello/authentication/client_authentication.rb#L28

@komidore64
Copy link
Contributor Author

@ehelms the same validation error is being thrown when unregistering from a non-activation-key registered content-host.

@ehelms
Copy link
Member

ehelms commented Apr 14, 2015

Ahh, looks like Foreman Tasks is expecting the owner of the task to have an 'id' and since we set User.current to a CpConsumerUser when a content host is preforming the action itself the 'id' field is nil (since we represent content hosts via uuid).

So you are proposing to override the current user to the anonymous admin when performing this action such that the owner of the action is our anonymous admin instead of the content host itself if I understand it correctly?

@komidore64
Copy link
Contributor Author

yes.

do you have something better in mind?

@ehelms
Copy link
Member

ehelms commented Apr 14, 2015

I could see it being nice to know that the content host initiated the action when looking at a task list; however, thinking and looking at the task list in the UI, I am guessing the user has to be a real object of class User with a field of 'login'. So, given that I suppose I am alright with this way of handling it.

@ehelms
Copy link
Member

ehelms commented Apr 14, 2015

Forgot the ACK

@komidore64
Copy link
Contributor Author

would it be too far fetched to see about modifying foreman_tasks to look for an owner or a content-host?

is that prone to causing too many other problems?

@ehelms
Copy link
Member

ehelms commented Apr 14, 2015

From what I saw in the code, Foreman tasks is looking specifically for 'Users' and properties of them. If you look at (https://github.com/theforeman/foreman-tasks/blob/781b7c58636e2fc96e1c7a43ec6c22271900f6e4/app/models/foreman_tasks/task.rb#L54) or search within the file you will see references to the User table. Further, I am not clear whether for search and some other what happens if a User no longer exists.

@komidore64
Copy link
Contributor Author

you've successfully deterred me

@ehelms
Copy link
Member

ehelms commented Apr 14, 2015

ACK

Fixing a DynFlow stacktrace that is thrown when a content-host attempts
to unregister. DynFlow would throw an ActiveRecord Validation error
stating 'Resource cannot be blank'. The resource is it referring to is a
user/owner of the DynFlow task that is being planned. Since the
content-host was registered with an activation key it is userless, and
dynflow doesn't like that.
@komidore64
Copy link
Contributor Author

updating commit message

komidore64 added a commit that referenced this pull request Apr 14, 2015
fixes #10127 - remove dynflow stacktrace when unregistering, BZ 1208100
@komidore64 komidore64 merged commit 4b3d9bb into Katello:master Apr 14, 2015
@komidore64 komidore64 deleted the bz1208100 branch April 14, 2015 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants