Call controller's current_user method during audited model creation #344
Conversation
spec/audited/sweeper_spec.rb
Outdated
| @@ -1,6 +1,12 @@ | |||
| require "spec_helper" | |||
|
|
|||
| class AuditsController < ActionController::Base | |||
| if Rails::VERSION::MAJOR == 4 | |||
| before_filter :populate_user | |||
There was a problem hiding this comment.
this will cause deprecations on 4.2 ... and I raise on deprecations ... so kaboom :D
... also add the raise on deprecations option to find this in tests easier.
There was a problem hiding this comment.
The deprecation warning was added in 5.0 - in 4.2 the use of before_filter was just "discouraged" so this should be fine whether AS deprecations are raised or not.
There was a problem hiding this comment.
before_action already exists on rails 4.0
https://github.com/rails/rails/blob/f53132deea65ae673b29b811c606be103f1ae3ab/actionpack/lib/abstract_controller/callbacks.rb
I would suggest to remove this if here
There was a problem hiding this comment.
Ah, you're right - thanks for the info. I've updated the PR to remove the conditional entirely.
| @@ -18,7 +18,7 @@ def around(controller) | |||
| end | |||
|
|
|||
| def current_user | |||
| controller.send(Audited.current_user_method) if controller.respond_to?(Audited.current_user_method, true) | |||
| lambda { controller.send(Audited.current_user_method) if controller.respond_to?(Audited.current_user_method, true) } | |||
There was a problem hiding this comment.
just store the controller here and don't create a new lambda ?
... then move the remaining code to the sweeper
There was a problem hiding this comment.
Yeah, I was initially planning to store the controller but I didn't think it was a good idea to have the Audit model calling methods directly on the controller. I preferred to keep the controller-specific calls and logic in this class.
... then move the remaining code to the sweeper
Which code's that? Do you mean to the model?
|
nvm, already answered it :)
either way is fine, creating an extra lambda seemed a bit weird to me, but
if you want to keep controller code the sweeper idk how else to do it
…On Fri, Apr 21, 2017 at 7:44 AM, Dominic Cleal ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/audited/sweeper.rb
<#344 (comment)>
:
> @@ -18,7 +18,7 @@ def around(controller)
end
def current_user
- controller.send(Audited.current_user_method) if controller.respond_to?(Audited.current_user_method, true)
+ lambda { controller.send(Audited.current_user_method) if controller.respond_to?(Audited.current_user_method, true) }
Yeah, I was initially planning to store the controller but I didn't think
it was a good idea to have the Audit model calling methods directly on the
controller. I preferred to keep the controller-specific calls and logic in
this class.
... then move the remaining code to the sweeper
Which code's that? Do you mean to the model?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#344 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAsZ832-dV4Aobhg9byZf7a92wJlgS6ks5ryMDUgaJpZM4NEBVX>
.
|
|
Thanks for the review @grosser, appreciated. |
Restores behaviour prior to 87d402a where the sweeper would call the controller's current_user method from the audited model callback. Since 87d402a, the current_user method was called from an around action callback registered on the base controller which was being called prior to other callbacks that were authenticating the user. This caused problems in apps where the user hadn't yet been set (so audit users were nil), or CSRF issues because current_user was called prior to session changes. Fixes collectiveidea#336
9014960
to
96aebbc
Compare
|
Can we have a release with this fix? |
|
@danielmorrison I opened #345 with updates for 4.5.0 if you'd kindly push the gem. |
|
@domcleal @danielmorrison hey, any news on releasing the new version? |
|
audited 4.5.0's released. |
|
Great, thanks :) |
Restores behaviour prior to 87d402a where the sweeper would call the
controller's current_user method from the audited model callback.
Since 87d402a, the current_user method was called from an around action
callback registered on the base controller which was being called prior
to other callbacks that were authenticating the user. This caused
problems in apps where the user hadn't yet been set (so audit users were
nil), or CSRF issues because current_user was called prior to session
changes.
Fixes #336