Posted by
flyerhzm
on
August 23, 2010
I don't remember how many times I need to fetch current user in models, such as audit log. Here is a flexible way to set the current user in and fetch the current user from User model.
I don't remember how many times I need to fetch the current user in models, for example, I want to log who creates, updates or destroys a post in the Audit model. There is no default way to fetch the current user in models, current_user object is always assigned in controllers (thanks to authentication plugins, restful_authentication, authlogic and devise), we can pass the current user object from controllers to models, but it is too ugly. I think the better way is to add the current user to User model by Thread.current hash.
class User < ActiveRecord::Base
def self.current
Thread.current[:user]
end
def self.current=(user)
Thread.current[:user] = user
end
end
As you seen, we add two class methods to User model, User.current to fetch the current user and User.current= to assign the current user. So what you need to do is to assign the current user in User model every request you need a current user. Such as
class ApplicationController < ActionController::Base
def set_current_user
User.current = current_user
end
end
class PostsController < ApplicationController
before_filter :require_user # require_user will set the current_user in controllers
before_filter :set_current_user
end
Now you can easily fetch the current_user in models by User.current, enjoy it!
Comments
Far better to say: Permission.unlock!(current_user) than just Permission.unlock! though. It makes it clear in the controller what you are actually doing.
As for the other guys... you should probably not store things in class variables that are mutable per request. Much better to use the Thread.current[] technique if you do need to keep some things available for the whole request.
You're essentially just creating a global variable and using it to store data that should be sensitive and persisted only for the current user. It's an awful no-no.
Models aren't supposed to have direct access to session state; it's outside their jurisdiction and a violation of separation of concerns.
Since you'd have to call User.current= from the controller anyway, you should just let the controller handle it. If your model needs to associate with the current user, pass the current user to the model when the controller creates it.
In my opinion, patterns are very important, I always think what are the benefits some design pattern brings us, and what are the disadvantages. MVC pattern decouples the views and models, and increases the flexibility and maintainability of codes. In general, we should always follow this pattern, but if it makes my codes too complex to follow it, I will violate it.
For example, we have a collaborative system, and we want to log who creates, updates or deletes an item (maybe a question, a post or other things), we use Observer to track all of the operations when item is created, updated and destroyed, it makes things much easier if we can access the User.current in Observer.
You may say we can do the track logic in the controllers, but that means you have to write your track logic all over the controllers, because the track logic is depend on models.
I just give you a solution to fetch the current user from models, you can decide to use it or not, I think it depends on whether it makes your codes simple and easy.
@eric - you s'd realy refactor your application, using just cattr_accessor is not thread safe!
User.current provides a defined interface. The models know that User.current stores the current user (if there is a current user). They don't care how that value get's set (that is a concern for the specific environment that the model is being run in). In a desktop app we might set it when the app launches. On a web-app we reset it every time a request comes in to simulate a stateful environment on a stateless protocol. At the console we might set it prior to carrying out ad-hoc operations. Regardless of the environment the models have a defined API for knowing the current user that they can use in their Observers, callbacks, etc. This is great for things like logging actions (like @flyerhzm mentions) or for things like model-level security.
I'm not saying that in all cases you should have this API. For simpler models you might just want to do something like
current_user.posts.create params[:post]to create a Post that has the current user attached. But for more complex models and observers it might be necessary for them to know the current user to carry out their operations. Having User.current as a defined API for determining the current user (and then in the case of a web-app set that variable on each request) seems like a reasonable approach.handling the other thing just *for* response to the request.
Also, to the folks saying this breaks MVC...flyerhzm is using this method as a form of aspect oriented programming. He's able to access the current user from the Observer so he doesn't have to duplicate that code all over controllers. That's a very useful pattern. I'd like to see how you accomplish the same thing without "breaking MVC" and duplicating code.
@smitjel, MVC isn't broken here. What's broken is the chain of responsibility. Current user is part of the request / session context. User model sits underneath and "knows" nothing about where it's used. I can open console and start using User model without even making a request to the app through the controller stack.
Moreover, current_user makes no sense on the model level if the app is designed correctly. The practice itself gives a sign that the design is somewhat smelly. I assume that there are models that need some user (a post needs an author, a friend needs... well... a friend etc), but to them "user" instance is just another object that doesn't have that additional burden of being mystically "current".
To illustrate my point, imagine a scenario when you are logged in as an admin user and impersonating someone. This feature is quite common, but who will be your current_user in this case? Will you add another field to the User model and "teach" whole bunch of models in your domain to respect these two? Is that going to be DRY?
My solution would be to leave it to the controller to decide who's current and what user instance to give to models. Traditionally, current_user is the method of ApplicationController and a helper method at the same time to be available in the views. If you Surgeon model needs a user, send her through the door; don't ask a User model who's current today.
For example, when I submit this comment, the Comment model will presumably have a user_id field to fill, right? Assuming that this forum requires authentication before allowing comments, then user_id will only and always be assigned the current_user's id, and that, to me, is business logic.
In practice, from the controller one can of course pass current_user as an additional parameter to the call to the model, as some suggest. Others prefer setting User.current in advance, so that it can be repeatedly referred to within the model layer, and I personally like this approach.
What I am not sure I understand, stylistic considerations aside, is the safety of the solution proposed by flyerhzm.
idahoev says it's a big no-no, that the possibility exists for sensitive data to persist and leak to other sessions. flyerhzm, in his reply, does not seem to specifically address the concern.
Can anyone please help discriminate between the beauty of the pattern (or lack thereof) and its safety?
Thanks!
The one example the API doc gives is simple--logging some strings. When you start applying Observer to more complex uses, you realize that the class is incomplete. It should provide a binding or be relocated to another module where it has access to more information.
I find the Thread technique to be the least offensive but I have a problem with it because it splits observation between controller and model. Why not just do everything in the controller?
This kind of problem points out the fact that you can't do everything strictly adhering to MVC. But I don't think having part of the code in one class and another part in a different class is the solution. This Observer class needs to be refactored to a location where it works for any use.
This is thought-provoking. Thanks for sharing. For a moment, I liked the idea. I am new to rails paradigm, so its possible that I might be over thinking this. But, what about thread safety? I have read that rails is single-threaded, but I am a noob and don't know whether any context-switching every happens between threads. Is User.current not effectively a singleton? If so if there is a possibility of thread context switches, this method can a) lead to data integrity issues or b) if rails is implicitly guaranteeing thread safety then race conditions etc. are possible...
As I said, I just got started with rails, and I would like to hear your and others' thoughts on this topic from this angle.
Thanks for raising the topic.
-TS