Hi there! I know I've disappeared for a while, but don't worry: I'm
alive! :) It is just that I was upgraded to 3.0 on monday so I'm not as
fast as I was in my twenties ;)
I've just rebased by rails-3.1-devise branch to master and forced pushed
of the changes after resolving some conflicts...
Yesterday I've sent a pull request in Github to Devise's v1.0 branch:
https://github.com/plataformatec/devise/pull/1142
I'll try to use the version from my repository as soon as I get some
time for getting rid of the secure cookie limitation. Then, the only
missing part will be OpenID support if I remember correctly. I'm still
waiting for the tests on session and caching.
Those were hurry days where, besides my birthday, several personal stuff
happened and are still happening alongside with some unusual activities
on my Gitorious cookbook for Debian/Ubuntu that had 2 pull requests only
this week and some comments in my site's article. An atypical week for
sure! :)
Hope to finish the Devise migration as soon as possible for starting the
more interesting part: convert Gitorious to Rails 3 :) [the
rails-3.1-devise branch at this moment should be read as devise only ;) ]
I would like if you could help me merging all commits before the
conversion to Devise into master. All of them are not Devise related but
may result in unnecessary work when rebasing my branch... :(
How about you? How are you doing? (yes, being 30 years old also mean I
get more social, I guess - I don't know, I'm still getting used to it ;) )
I've seen new commits every day, so I guess you're doing well :)
Best regards!
Rodrigo.
Em 07-06-2011 11:22, Rodrigo Rosenfeld Rosas escreveu:
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]
