Em 26-05-2011 08:25, Christian Johansen escreveu:
First of all: great job once again Rodrigo :)
That is all fantastic when it is finished, but I'm a bit far of
finishing it right now. There is a lot of things to do yet.
I need to get rid of the SessionsController and think what to do
about its functional tests. I think we should move it to
integration tests. Can I add the capybara gem dependency to tests?
Moving this over to integration tests seems just fine to me. Any
particular reason you could not use the Rails integration tests for this?
Actually this is just a matter of taste, like with RSpec. I prefer to
read specs as well as Capybara write syntax (visit url, etc). The tests
get much more readable for my taste. But I'll do it using Rails
integration tests if you prefer.
Also, I guess the only reason for Gitorious to set the
"_logged_in" cookie is to easy the functional tests for they
knowing if the user was correctly authenticated. I don't like this
approach very much and I'm going to remove it if that is ok with
you. This could be set adding a Warden hook after successful
authentication, but I really don't think this is a great approach
for testing this. In the integration tests, I'll try to do
something like this:
in setup:
get '/logout' # don't remember exactly the URL...
in the test:
try_to_log_in_with_some_method
check_for_presence_of login_name
Sounds fine.
Or something like that... I've noticed that Gitorious tests used
to be specs using Rspec sometime in the past. Then, there was one
commit converting them all to Shoulda. Is there any reason why
RSpec was dropped? I really like to read specs because they are
clearer to me. For instance:
non_admin_user.should_not be_allowed_to_view('/admin')
And then, adding a custom RSpec matcher for BeAllowedToView.
Well, basically, neither Marius nor I care much for RSpec, so going
back is not an option at this point. Anyway, it's just as easy to
create helpers and custom assertions, so I see this mostly as a matter
of syntax preference.
Getting back to strategies, the rememberable strategy will store
the cookie for 2 weeks by default in Devise, which is the same
period Gitorious currently use. Should I make this time span
explicit in Devise config of is it ok for Gitorious to rely on
Devise's default? Also, I needed to change the field names to use
the "user" scope. For instance, <input name="email"> should be
changed to <input name="user[email]">.
It would be nice to be explicit on the configuration here so we
control our own defaults. If it's not too much work you might as well
add a session_timeout config option to gitorious.yml (you don't have,
just an idea).
That is ok, very easy to do.
Gitorious currently use the "auth_token" cookie for storing this
setting, but I'll change it to "authenticated_user_token" (or
something like that), which is used by the :rememberable strategy
implemented by Devise for avoiding creating another strategy just
for maintaining the same cookie name. Also, Devise 1.0 won't set
the cookie as secure I guess. But I'm not worried with it right
now since the newest Devise support passing options to generated
cookies in Devise config, so we can do that later after migrating
to Rails 3 and newest Devise. If Devise is going to be merged into
master before we convert Gitorious to Rails 3, we'll probably need
to replace the Rememberable strategy with a custom one anyway for
keeping the cookie secure... I'll take a closer look at this later...
This worries me a bit, because I suspect we will take some heat for
security regressions, even if only temporary. Is there a convenient
way to monkey patch Device to add the secure option? Just while we
wait for Rails 3.
Yes, that is what I've said. Since you are open to change to Devise
before we finish the migration to Rails 3, then we *should* use secure
cookies. In that case we can either write another strategy, or monkey
patch the current strategy or send a pull request to Devise 1.0 branch
(prefered alternative for me) to make it compatible with newest Devise.
This is another thing I would like to talk about: timing for
merging this. If we keep such a big change in a separate branch
for a long while, it will be very hard to rebase or merge it with
master as the time passes. It would be great if we could integrate
it as soon as possible to master once it is finished. Is it possible?
Of course it's possible! If it works, we'll use it :) What we usually
do with changes of this magnitude is to deploy changes to an internal
staging area for a week or so before launching it on gitorious.org
<http://gitorious.org>.
This remembers me another thing I would like to talk with you. Is it
possible for you to set some staging server that could be tested by the
public? I mean, backup data and repositories from the official Gitorious
and make them available in a staging server. That way, before we migrate
to Rails 3, people would be able to try it and report any problems... Of
course that changes involving security like cookie or password stealing,
etc, couldn't be available with real data in this staging server, but I
think that for the other changes this could be a great idea...
I'll keep you updated of this progress.
Awesome stuff Rodrigo :)
Thanks. Unfortunately, I wasn't able to work on Gitorious yesterday
night. I've spent all my little free time watching the awesome Aaron
Patterson (tenderlove) talk on RailsConf :)
Best regards,
Rodrigo.
--
To post to this group, send email to gitorious@googlegroups.com
To unsubscribe from this group, send email to
gitorious+unsubscr...@googlegroups.com
