Skip to content

Rename Authentication, User to Controller, Model #268

Closed
wants to merge 4 commits into from

5 participants

@croaky
thoughtbot, inc. member
croaky commented Feb 24, 2013

No description provided.

@gylaz gylaz commented on an outdated diff Feb 25, 2013
lib/clearance/model.rb
@@ -10,8 +10,10 @@ module User
include Validations
include Callbacks
- include (Clearance.configuration.password_strategy ||
- Clearance::PasswordStrategies::BCrypt)
+ include (
@gylaz
thoughtbot, inc. member
gylaz added a note Feb 25, 2013

Space before (

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gylaz gylaz commented on an outdated diff Feb 25, 2013
lib/generators/clearance/install/install_generator.rb
)
end
def create_or_inject_clearance_into_user_model
if File.exists? 'app/models/user.rb'
- inject_into User, 'app/models/user.rb', 'include Clearance::User'
+ inject_into User, 'app/models/user.rb', 'include Clearance::Model'
else
copy_file 'user.rb', 'app/models/user.rb'
@gylaz
thoughtbot, inc. member
gylaz added a note Feb 25, 2013

Does this need to change?

copy_file 'model.rb', 'app/models/user.rb'
@gylaz
thoughtbot, inc. member
gylaz added a note Feb 25, 2013

Nevermind, i think this is copying the user.rb template file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gylaz gylaz commented on an outdated diff Feb 25, 2013
spec/models/user_spec.rb
@@ -33,33 +33,28 @@
end
it 'is authenticated with correct email and password' do
- (Clearance.configuration.user_model.authenticate(@user.email, @password)).
- should be
+ User.authenticate(@user.email, @password).should be
@gylaz
thoughtbot, inc. member
gylaz added a note Feb 25, 2013

Could we be more explicit here and use should be true or should == true?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gylaz gylaz commented on an outdated diff Feb 25, 2013
spec/models/user_spec.rb
@user.should be_authenticated(@password)
end
it 'is authenticated with correct uppercased email and correct password' do
- (Clearance.configuration.user_model.authenticate(@user.email.upcase, @password)).
- should be
+ User.authenticate(@user.email.upcase, @password).should be
@gylaz
thoughtbot, inc. member
gylaz added a note Feb 25, 2013

Same as above, can help with understanding the api.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gylaz gylaz commented on an outdated diff Feb 25, 2013
spec/models/user_spec.rb
@user.should be_authenticated(@password)
end
it 'is authenticated with incorrect credentials' do
- (Clearance.configuration.user_model.authenticate(@user.email, 'bad_password')).
- should_not be
+ User.authenticate(@user.email, 'bad_password').should_not be
@gylaz
thoughtbot, inc. member
gylaz added a note Feb 25, 2013

should eq false?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@croaky
thoughtbot, inc. member
croaky commented Feb 25, 2013

Thanks, @gylaz. Applied your feedback in 9b1cba9.

@jferris
thoughtbot, inc. member
jferris commented Feb 25, 2013

I like the information the old names provided. I think the main issue was that Clearance::Authentication maybe misrepresented what it provided to controllers, but I think the new names go too far in the other direction.

Clearance itself has multiple controllers, and they don't descend from Clearance::Controller, which I find a little suprising. The Controller name tells you what it's meant to be mixed into, but not what it provides. Maybe AuthenticatedController or something like that? The module provides helpers related to authentication (with just a sprinkle of authorization) and is meant to be mixed into ApplicationController.

I think Clearance::User was fine as a name. Was there a specific issue that renaming was designed to fix? The new name doesn't tell you what kind of role the model plays. "Model" basically means "Thing" and has some vague connotations when it's in a Rails application, whereas "User" means something pretty specific in a Rails application.

If we do rename classes from the public API, do we want to include deprecated fallbacks under the old names? I know clearance is technically not 1.0 yet, but there are a lot of applications out there that will break if we rename these.

@croaky
thoughtbot, inc. member
croaky commented Feb 25, 2013

I think the main issue was that Clearance::Authentication maybe misrepresented what it provided to controllers, but I think the new names go too far in the other direction.

Right, this was the issue I was trying to address. Also, it's not the entire contents of the Clearance::Authentication that are potentially misrepresented, just authorize in the public API (the private API of store_location, return_to, and clear_return_to might be a worthwhile different conversation).

I don't feel strongly about this change. I wanted to make the change to see what it looked and felt like and am seeking feedback like yours about whether this causes more trouble than it's worth. I also wanted to do it pre-1.0, which we're only one issue or two away from reaching: #243

@jferris
thoughtbot, inc. member
jferris commented Feb 26, 2013

My general feelings about the authorize vs authenticate debate:

  • The authorize method performs authorization - it denies access to unauthenticated users. The sessions controller performs authentication, as does Clearance::Session.
  • I've generally assumed that controllers would override authorize for controllers that require specific authentication.
  • It's sort of strange that Clearance::Authentication contains a bunch of authorization logic.

How about this?

  • Revert Clearance::Model back to Clearance::User.
  • Split Clearance::Controller into Clearance::Authentication and Clearance::Authorization, both of which get mixed into Clearance::Controller.
  • Mix Clearance::Controller into ApplicationController in the install generator.

In order to clarify the authorize vs authenticate debate, we can define the method as authorize_authenticated_users and then just call that from authorize. Not sure if that actually helps, though.

@croaky
thoughtbot, inc. member
croaky commented Mar 2, 2013

@jferris Done. Ready for re-review.

@croaky croaky referenced this pull request Mar 2, 2013
Closed

Authenticate vs Authorize #239

@jonathanhefner

To add a use case to the authorize vs authenticate method debate:

rescue_from CanCan::AccessDenied do |exception|
  if signed_in?
    raise
  else
    # get the user to sign in, since they haven't already done so
    authorize
  end
end

To me, the call to authorize makes a lot more sense as authenticate because all you're asking the user to do is verify they are who they say they are by providing an email/password. It isn't authorizing them for anything--even if they complete the authorize successfully (i.e. successfully log in), they can still be denied access.

But in the end, it's just one line of code in one base controller, and a comment right next to it suffices to resolve any confusion down the road.

@croaky
thoughtbot, inc. member
croaky commented Mar 3, 2013

@jonathanhefner In that use case, would redirect_to sign_in work better in your else branch?

authorize calls unless signed_in?, which you already cover. It then calls deny_access, which stores the current location then calls redirect_to url_after_denied_access_when_signed_out.

Alternatively, could you use deny_access or redirect_to url_after_denied_access_when_signed_out in the constraint?

@calebthompson calebthompson commented on the diff Mar 4, 2013
lib/clearance/authentication.rb
@@ -4,8 +4,14 @@ module Authentication
included do
helper_method :current_user, :signed_in?, :signed_out?
- hide_action :authorize, :current_user, :current_user=, :deny_access,
- :sign_in, :sign_out, :signed_in?, :signed_out?
+ hide_action(
@calebthompson
thoughtbot, inc. member
calebthompson added a note Mar 4, 2013

If we just make these methods private, they don't need to be hidden.

@croaky
thoughtbot, inc. member
croaky added a note Mar 4, 2013

@calebthompson If we make them private methods, we get errors like the following:

private method `signed_in?' called for #<ForgeriesController:0x007f8338c2dc48>
private method `current_user=' called for #<Clearance::SessionsController:0x007f833c8bd848>
@calebthompson
thoughtbot, inc. member
@croaky
thoughtbot, inc. member
croaky added a note Mar 4, 2013

I'm not sure I know what you mean by a helper module. Are you referring to the helper method?

http://api.rubyonrails.org/classes/AbstractController/Helpers/ClassMethods.html#method-i-helper

If the goal is to make current_user, signed_in?, and signed_out? available to view templates, that's handled by line 6. The other methods are intended to be used by controllers.

@calebthompson
thoughtbot, inc. member
@croaky
thoughtbot, inc. member
croaky added a note Mar 4, 2013

Okay, cool. No worries. Thanks for the comments. Got me wondering whether this stuff was still necessary on current versions of Rails and refreshed my memory on how hide_action and module visibility worked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonathanhefner

@croaky Looking at the implementation of authorize and deny_access, I could simply call deny_access directly (because I don't need the extra signed_in? check, as you pointed out). The effect would be the same though: I'd still add a comment to that method call to note that I'm only getting to the user to sign in, not denying (or authorizing) them for access. (Semantics, I know. :)

@croaky
thoughtbot, inc. member
croaky commented Mar 11, 2013

@jferris Could I get you to re-review this one when you have a moment?

@jferris
thoughtbot, inc. member
jferris commented Mar 11, 2013

This looks good to me.

@croaky croaky closed this Mar 11, 2013
@croaky
thoughtbot, inc. member
croaky commented Mar 11, 2013

Thanks, @jferris. Merged as 91a984c.

@geoffharcourt geoffharcourt pushed a commit to geoffharcourt/clearance that referenced this pull request Jul 8, 2014
@croaky croaky Split Clearance::{Authentication,Authorization}
There has been confusion about the `authorize` method residing in the
`Authentication` module:

* The `authorize` method performs authorization - it denies access to
  unauthenticated users.
* It is assumed that controllers would override `authorize` for
  controllers that require specific authentication.
* It's sort of strange that `Clearance::Authentication` contains a bunch
  of authorization logic.

So, we:

* Split `Clearance::Controller` into `Clearance::Authentication` and
  `Clearance::Authorization`, both of which get mixed into
  `Clearance::Controller`.
* Mix `Clearance::Controller` into `ApplicationController` in the install
  generator.

Read more:

thoughtbot#268
thoughtbot#257
91a984c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.