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?
>
> 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).
> 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.
> 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.
>
> I'll keep you updated of this progress.
Awesome stuff Rodrigo :)
Christian
--
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
