Em 07-06-2011 10:43, Christian Johansen escreveu:
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?
I didn't think thoroughly about this. I just followed the migration
guide here:
https://github.com/plataformatec/devise/wiki/How-To:-Migrate-from-restful_authentication-to-Devise-
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?
This is the problem. The configured macros will authenticate first
before trying access some URL and see if there was a redirect between
HTTP and HTTPS.
The problem is that Devise/Warden will insert a before_filter
(:require_no_authentication) before all others. This mean that this
filter takes precedence over the ssl_requirement related filter. Once
the user is authenticated, the user will be redirected to "/" when
accessing /sessions/users/sign_in or sign_out. So, the test will fail
because these URLs won't be redirected to the https equivalent if the
users are already authenticated by default in Devise. That is why I
skipped this filter for those actions if the tests are intended to
really pass. I just don't think there is any sense to directly access
the login URL before logging out.
As for the session/caching problems; I will add a test for this on master.
Great!
It appears that one of our foulest hacks (logged_in = :false) is going
away, is that right? :) If so, yuppie!
I can't remember this! :D I wasn't very interested in understanding how
it used to work as long as tests go green ;)
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.
Great!
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?
Not sure, but I can get rid of it if you agree. Instead of using
"login_as" in the tests I could replace them directly by sign_in :user,
@user and sign_out.
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?
I completely agree with you and I forgot to talk about this with you. I
would actually suggest changing this to the approach you are suggesting.
Actually, I would rather prefer to directly use sign_in and sign_out
instead of the methods in this module. Also, sign_in can be called like
"sign_in @user" instead of "sign_in :user, @user".
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.
I completely agree. Not just because of the reason of avoiding touching
the tests but it also makes the test code intention much clearer. That
is what I would code like in RSpec:
@user.should_not be_allowed_to_access(url)
I'll change them to assert_redirected_to_login.
Same thing goes for this snippet: assert warden.authenticated?. I'd
prefer assert authenticated?, where `authenticated?` can be defined in
our authentication test helper.
Agreed.
+ post :create, :user => {:email => "[email protected]
<mailto:[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.
Ok, I just maintained what was going on before the change :) But I agree
with you.
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?
I'm not sure but, while debugging, I noticed that introducing Devise was
calling this code before the end of Rails initialization and
@controller_names_plural was only partially filled and the routes
weren't working because of that.
This is mostly nitpicking. All over great work, Rodrigo!
Christian
Thanks! :)
On Mon, Jun 6, 2011 at 02:43, Rodrigo Rosenfeld Rosas
<[email protected] <mailto:[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>
<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>
<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]
<mailto:[email protected]>
To unsubscribe from this group, send email to
[email protected]
<mailto:gitorious%[email protected]>
--
MVH
Christian
--
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
--
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
