Save post request properly into database with a model, as a record of where to

redirect back to after login.
Token generation for these saved logins.

12 files changed, 129 insertions, 38 deletions

diff --git a/app/controllers/application.rb b/app/controllers/application.rb
index 824f6f77b..fb56497f4 100644
--- a/app/controllers/application.rb
+++ b/app/controllers/application.rb
@@ -6,7 +6,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: application.rb,v 1.21 2007-11-01 05:35:43 francis Exp $
+# $Id: application.rb,v 1.22 2007-11-01 14:45:56 francis Exp $
 
 
 class ApplicationController < ActionController::Base
@@ -21,9 +21,9 @@ class ApplicationController < ActionController::Base
     # Check the user is logged in
     def authenticated?
         unless session[:user]
-            session[:intended_uri] = request.request_uri
-            session[:intended_params] = params
-            redirect_to signin_url
+            post_redirect = PostRedirect.new(:uri => request.request_uri, :post_params => params)
+            post_redirect.save!
+            redirect_to signin_url(:token => post_redirect.token)
             return false
         end
         return true
@@ -34,12 +34,13 @@ class ApplicationController < ActionController::Base
         return User.find(session[:user])
     end
 
-    # Do a POST redirect. This is a nasty hack - we store the posted values to
-    # the controller, and when the GET redirect with "?post_redirect=1"
-    # happens, load them in.
-    def post_redirect(uri, params)
+    # Do a POST redirect. This is a nasty hack - we store the posted values in
+    # the session, and when the GET redirect with "?post_redirect=1" happens,
+    # load them in.
+    def do_post_redirect(uri, params)
         session[:post_redirect_params] = params
-        # XXX what is built in Ruby URI munging function?
+        # XXX what is the built in Ruby URI munging function that can do this
+        # choice of & vs. ? more elegantly than this dumb if statement?
         if uri.include?("?")
             uri += "&post_redirect=1"
         else
diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb
index 084bbbc81..2f12c8319 100644
--- a/app/controllers/user_controller.rb
+++ b/app/controllers/user_controller.rb
@@ -4,7 +4,7 @@
 # Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
 # Email: francis@mysociety.org; WWW: http://www.mysociety.org/
 #
-# $Id: user_controller.rb,v 1.7 2007-10-31 17:25:29 francis Exp $
+# $Id: user_controller.rb,v 1.8 2007-11-01 14:45:56 francis Exp $
 
 class UserController < ApplicationController
     # XXX See controllers/application.rb simplify_url_part for reverse of expression in SQL below
@@ -16,31 +16,35 @@ class UserController < ApplicationController
     def signin
         # The explict signin link uses this to store where it is to go back to
         if params[:r]
-            session[:intended_uri] = params[:r]
-            session[:intended_params] = nil
+            post_redirect = PostRedirect.new(:uri => params[:r], :post_params => {})
+            post_redirect.save!
+            params[:token] = post_redirect.token
         end
 
         if not params[:user] 
             # First time page is shown
-            render :template => 'user_accounts/signin' and return
+            render :template => 'user_accounts/signin' 
+            return
         else
             @user = User.authenticate(params[:user][:email], params[:user][:password])
             if @user
                 # Successful login
                 session[:user] = @user.id
-                post_redirect session[:intended_uri], session[:intended_params] and return
+                post_redirect = PostRedirect.find_by_token(params[:token])
+                do_post_redirect post_redirect.uri, post_redirect.post_params
+                return
             else
                 if User.find(:first, :conditions => [ "email = ?", params[:user][:email] ])
                     # Failed to authenticate
                     flash[:error] = "Password not correct, please try again"
                     @user = User.new(params[:user])
-                    render :template => 'user_accounts/signin' and return
+                    render :template => 'user_accounts/signin' 
+                    return
                 else 
-                    # "I am new to FOIFA"
-                    session[:email] = params[:user][:email]
-                    session[:password] = params[:user][:password]
-                    session[:first_time] = true
-                    redirect_to :action => 'signup' and return
+                    # Create a new account
+                    params[:first_time] = true
+                    self.signup
+                    return
                 end
             end
         end
@@ -48,25 +52,20 @@ class UserController < ApplicationController
 
     # Create new account form
     def signup
-        # Default to value saved from signin form
-        params[:user] ||= { :email => session[:email] }
-        params[:user] ||= { :password => session[:password] }
-
         # Make the user and try to save it
         @user = User.new(params[:user])
         if not @user.save
             # First time get to form (e.g. from signin) , don't show errors
-            if session[:first_time]
-                @first_time = true
-                @user.errors.clear
-                session[:first_time] = false
-            end
+            @first_time = params[:first_time]
+            @user.errors.clear if @first_time
             # Show the form
             render :template => 'user_accounts/signup'
         else
             # New user made, redirect back to where we were
             session[:user] = @user.id
-            post_redirect session[:intended_uri], session[:intended_params] and return
+            post_redirect = PostRedirect.find_by_token(params[:token])
+            do_post_redirect post_redirect.uri, post_redirect.post_params
+            return
         end
     end
 
diff --git a/app/models/post_redirect.rb b/app/models/post_redirect.rb
new file mode 100644
index 000000000..ab1a365f9
--- /dev/null
+++ b/app/models/post_redirect.rb
@@ -0,0 +1,36 @@
+# models/postredirect.rb:
+# Saves an HTTP POST request, so it can be redirected to later.
+# For example, after registering / logging in.
+#
+# Copyright (c) 2007 UK Citizens Online Democracy. All rights reserved.
+# Email: francis@mysociety.org; WWW: http://www.mysociety.org/
+#
+# $Id: post_redirect.rb,v 1.1 2007-11-01 14:45:56 francis Exp $
+
+require 'openssl' # for random bytes function
+
+class PostRedirect < ActiveRecord::Base
+    # We store YAML version of POST parameters in the database
+    def post_params=(params)
+        self.post_params_yaml = params.to_yaml
+    end
+    def post_params
+        YAML.load(self.post_params_yaml)
+    end
+
+    # Make the token 
+    def after_initialize
+        if not self.token
+            bytes = OpenSSL::Random.random_bytes(12)
+            # XXX Ruby has some base function that can do base 62 or 32 more easily?
+            base64 = [bytes].pack("m9999").strip
+            base64.gsub("+", "a")
+            base64.gsub("/", "b")
+            base64.gsub("=", "c")
+            self.token = base64
+        end
+    end
+
+end
+
+
diff --git a/app/views/layouts/default.rhtml b/app/views/layouts/default.rhtml
index 45e062f78..1cd853628 100644
--- a/app/views/layouts/default.rhtml
+++ b/app/views/layouts/default.rhtml
@@ -11,7 +11,7 @@
             <a href="/">GovernmentSpy</a> 
             <span id="beta">Beta</span>
         </h1>
-        <div id="tagline">Freeing your information from them</div>
+        <div id="tagline">It's your information. Free it from them.</div>
     </div>
     <ul id="navigation">
         <li><%= link_to "All Requests", request_list_url %></li>
diff --git a/app/views/user_accounts/signin.rhtml b/app/views/user_accounts/signin.rhtml
index d390c7174..137de91b3 100644
--- a/app/views/user_accounts/signin.rhtml
+++ b/app/views/user_accounts/signin.rhtml
@@ -18,6 +18,7 @@
 
     <p>
     <label for="submit">&nbsp;</label>
+    <%= hidden_field_tag 'token', params[:token] %>
     <%= submit_tag "Sign in" %>
     </p>
 
diff --git a/app/views/user_accounts/signup.rhtml b/app/views/user_accounts/signup.rhtml
index 7d6abc907..dd779a0f3 100644
--- a/app/views/user_accounts/signup.rhtml
+++ b/app/views/user_accounts/signup.rhtml
@@ -1,8 +1,12 @@
 <%= foi_error_messages_for :user %>
 
 <% form_tag({:action => "signup"}, {:id => "accountForm"}) do %>
-    <h1>Create a new account</h1>
-
+    <% if @first_time %>
+    <div class="form_note">
+    <h1>Register new account</h1>
+    </div>
+    <% end %>
+        
     <p>
     <label for="user_email" id="signin_email"><strong>E-mail:</strong></label>
     <%= text_field 'user', 'email', :size => 20 %>
@@ -37,6 +41,7 @@
 
     <p>
     <label for="submit">&nbsp;</label>
+    <%= hidden_field_tag 'token', params[:token] %>
     <%= submit_tag "Sign in" %>
     </p>
 
diff --git a/db/migrate/014_create_post_redirects.rb b/db/migrate/014_create_post_redirects.rb
new file mode 100644
index 000000000..26c561dac
--- /dev/null
+++ b/db/migrate/014_create_post_redirects.rb
@@ -0,0 +1,15 @@
+class CreatePostRedirects < ActiveRecord::Migration
+  def self.up
+    create_table :post_redirects do |t|
+      t.column :token, :text
+      t.column :uri, :text
+      t.column :post_params_yaml, :text
+      t.column :created_at, :datetime
+      t.column :updated_at, :datetime
+    end
+  end
+
+  def self.down
+    drop_table :post_redirects
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 8350f3943..1f0181d25 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -2,7 +2,7 @@
 # migrations feature of ActiveRecord to incrementally modify your database, and
 # then regenerate this schema definition.
 
-ActiveRecord::Schema.define(:version => 13) do
+ActiveRecord::Schema.define(:version => 14) do
 
   create_table "incoming_messages", :force => true do |t|
     t.column "info_request_id", :integer
@@ -29,6 +29,14 @@ ActiveRecord::Schema.define(:version => 13) do
     t.column "sent_at",         :datetime
   end
 
+  create_table "post_redirects", :force => true do |t|
+    t.column "token",            :text
+    t.column "uri",              :text
+    t.column "post_params_yaml", :text
+    t.column "created_at",       :datetime
+    t.column "updated_at",       :datetime
+  end
+
   create_table "public_bodies", :force => true do |t|
     t.column "name",              :text
     t.column "short_name",        :text
diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb
index 41ff2b6b6..7ba066ad2 100644
--- a/spec/controllers/request_controller_spec.rb
+++ b/spec/controllers/request_controller_spec.rb
@@ -74,7 +74,13 @@ describe RequestController, "when creating a new request" do
         post :create, :info_request => { :public_body_id => public_bodies(:geraldine_public_body).id, 
             :title => "Why is your quango called Geraldine?"},
             :outgoing_message => { :body => "This is a silly letter. It is too short to be interesting." }  
-        response.should redirect_to(:controller => 'user', :action => 'signin')
+        # XXX yeuch - no other easy way of getting the token so we can check
+        # the redirect URL, as it is by definition opaque to the controller
+        # apart from in the place that it redirects to.
+        post_redirects = PostRedirect.find_by_sql("select * from post_redirects order by id desc limit 1")
+        post_redirects.size.should == 1
+        post_redirect = post_redirects[0]
+        response.should redirect_to(:controller => 'user', :action => 'signin', :token => post_redirect.token)
     end
 
     it "should create the request and outgoing message and redirec to request page when input is good and somebody is logged in" do
diff --git a/test/fixtures/post_redirects.yml b/test/fixtures/post_redirects.yml
new file mode 100644
index 000000000..b49c4eb4e
--- /dev/null
+++ b/test/fixtures/post_redirects.yml
@@ -0,0 +1,5 @@
+# Read about fixtures at http://ar.rubyonrails.org/classes/Fixtures.html
+one:
+  id: 1
+two:
+  id: 2
diff --git a/test/unit/post_redirect_test.rb b/test/unit/post_redirect_test.rb
new file mode 100644
index 000000000..785f53170
--- /dev/null
+++ b/test/unit/post_redirect_test.rb
@@ -0,0 +1,10 @@
+require File.dirname(__FILE__) + '/../test_helper'
+
+class PostRedirectTest < Test::Unit::TestCase
+  fixtures :post_redirects
+
+  # Replace this with your real tests.
+  def test_truth
+    assert true
+  end
+end
diff --git a/todo.txt b/todo.txt
index 3743a84fc..d3aab458d 100644
--- a/todo.txt
+++ b/todo.txt
@@ -1,8 +1,8 @@
-Offline
-=======
+Online
+======
 
 Work out how to do controller/view integrated specs and add some
-Change to will_paginate
+Move things from views/user_accounts into views/user
 
 Next
 ====
@@ -24,6 +24,8 @@ Write some tests (using rspec)
 
 Migrate to will_paginate from classic_pagination
 
+Replace all find :conditions with find_by_
+
 Tidying
 =======
 
@@ -43,6 +45,7 @@ Add SQL foreign keys to database schema
         http://www.redhillconsulting.com.au/rails_plugins.html#foreign_key_migrations
         http://rubyforge.org/projects/mig-constraints/
 Call "delete from sessions where now() - updated_at > 3600" (one hour) or whatever
+     - take care about this if you're still keeping POST requests in sessions during login
 
 Do pretty error messages, e.g. on invalid public body name page etc.
 
@@ -86,6 +89,8 @@ http://www.ordnancesurvey.co.uk/oswebsite/aboutus/foi/coiindex.html
 
 Maybe prepend letter for them with "this is FOI request blah" boilerplate?
 
+Forgotten password link
+
 Sources of public bodies
 ========================