Great work Rodrigo!
Marius and I have reviewed your work so far, and here is our feedback:
The password field changed length from 40 to 128. Is the idea here to make
encryption strategy configurable?
This comment:
# I'm not sure if the tests are correct. TODO: verify if we really
need
# this skip filter. Also notice that this is Devise's internal and can
# change in future versions. It doesn't make much sense to access new
or
# create while logged in. By default, once the user is already logged
in,
# Trying to access them will redirect to '/' before the action is
called.
I'm not entirely sure what you mean here. What's the underlying problem?
As for the session/caching problems; I will add a test for this on master.
It appears that one of our foulest hacks (logged_in = :false) is going away,
is that right? :) If so, yuppie!
The Varnish cache/cookie stuff can be removed entirely. I suspect this was
added before properly understanding how Varnish works - Basically, as long
as the app sends a session cookie, Varnish will not touch the response.
Is the AuthenticatedSystem module going to stay around? Seems most of it has
moved inside of Devise. The only remaining parts are the remember path
related pieces, am I right?
This diff:
def login_as(user)
- @request.session[:user_id] = user ? user_instance(user).id : nil
- @request.session_options[:expire_after] = 1.hour
+ if user
+ sign_in :user, user_instance(user)
+ else
+ sign_out :user
+ end
This is much better than setting the :user_id manually in tests. However, I
think it might be easier to read the tests if we used `logout` rather than
`login_as nil`. What do you think?
This assertion is repeated (and thus changed) very many times in the tests:
assert_redirected_to(new_user_session_path)
I suggest we replace it with a custom assertion, something along the lines
of assert_redirected_to_login. This way we can change the login semantics
without having to touch all the tests.
Same thing goes for this snippet: assert warden.authenticated?. I'd prefer
assert authenticated?, where `authenticated?` can be defined in our
authentication test helper.
+ post :create, :user => {:email => "[email protected]",
:password => "test"}
+ assert warden.authenticated? # <- over specification
+ get :destroy
+ assert !warden.authenticated?
This may be a little nitpicking, but this test is over specified. The first
assertion is not necessary, because there is another test that ensures that
the `post :create` logs the user in. I suggest you just remove it.
def controller_names_plural
- return @controller_names_plural unless
@controller_names_plural.blank?
+ # don't cache while Rails didn't finish initialization
+ return @controller_names_plural if
@controller_names_plural.present? && Rails.initialized?
@controller_names_plural =
ActionController::Routing.possible_controllers.map{|s| s.split("/").first }
end
In what cases is this snippet run before Rails is properly initialized?
This is mostly nitpicking. All over great work, Rodrigo!
Christian
On Mon, Jun 6, 2011 at 02:43, Rodrigo Rosenfeld Rosas <[email protected]>wrote:
> Please review this branch:
>
>
> https://gitorious.org/~rosenfeld/gitorious/rosenfeld-gitorious/commits/rails-3.1-devise<
> https://gitorious.org/%7Erosenfeld/gitorious/rosenfeld-gitorious/commits/rails-3.1-devise
> >
>
> It is rebased with mainline/master. All commits can be merged to master
> except the last one, which is not completed yet.
>
> Currently, here are the missing bits:
>
> - Cookies are not marked as "secure" yet;
> - OpenID authentication is the only failing test and is not implemented
> yet. I still need to investigate how OpenID works in Gitorious. But this
> needs a rework for sure in the user interface with buttons pointing out to
> Google, Yahoo, etc besides the OpenID URL edit for easing its usage. Maybe
> we could do that after migrating Gitorious to Rails 3;
> - The session expire check was disabled for the SSL tests to pass. It would
> be great to add some test for verifying this cache behavior. I didn't verify
> yet if this check is necessary for Warden/Devise;
> - There is no test regarding the Varnish cache server and I don't know what
> to expect from it;
>
> Please review the current status and guide me with suggestions for
> achieving the missing parts and detect bugs not covered by tests as well as
> improvement suggestions to the patch.
>
> If someone wants to help integrate OpenID to Gitorious with Devise, one can
> start from here:
>
>
> https://gitorious.org/~rosenfeld/gitorious/rosenfeld-gitorious/commits/rails-3.1-devise-openid-temp<
> https://gitorious.org/%7Erosenfeld/gitorious/rosenfeld-gitorious/commits/rails-3.1-devise-openid-temp
> >
>
> There is nothing yet, except from new dependencies set.
>
> Here is an example application using Devise and the
> devise_openid_authenticable gem:
>
> https://github.com/rosenfeld/devise_openid_example
>
> It is a fork of the repository below with updated config/environment.rb to
> reflect the Gitorious situation:
>
> https://github.com/nbudin/devise_openid_example
>
> Best regards,
>
> Rodrigo.
>
> --
> To post to this group, send email to [email protected]
> To unsubscribe from this group, send email to
> [email protected]
>
--
MVH
Christian
--
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
