diff options
| -rwxr-xr-x | db/rerun_dbic_loader.pl | 2 | ||||
| -rw-r--r-- | db/schema_0001.sql | 9 | ||||
| -rw-r--r-- | perllib/FixMyStreet.pm | 33 | ||||
| -rw-r--r-- | perllib/FixMyStreet/App/Controller/Auth.pm | 178 | ||||
| -rw-r--r-- | perllib/FixMyStreet/DB/Result/User.pm | 33 | ||||
| -rw-r--r-- | perllib/FixMyStreet/DB/ResultSet/User.pm | 45 | ||||
| -rw-r--r-- | t/app/controller/auth.t | 127 | ||||
| -rw-r--r-- | templates/email/default/auth_new_account_welcome | 13 | ||||
| -rw-r--r-- | templates/email/default/test | 6 | ||||
| -rw-r--r-- | templates/web/default/auth/confirm.html | 23 | ||||
| -rw-r--r-- | templates/web/default/auth/general.html | 48 | ||||
| -rw-r--r-- | templates/web/default/auth/logout.html | 8 | ||||
| -rw-r--r-- | templates/web/default/auth/welcome.html | 11 | ||||
| -rw-r--r-- | web/css/core.css | 4 |
14 files changed, 531 insertions, 9 deletions
diff --git a/db/rerun_dbic_loader.pl b/db/rerun_dbic_loader.pl index fa93165de..ed2fd6375 100755 --- a/db/rerun_dbic_loader.pl +++ b/db/rerun_dbic_loader.pl @@ -16,7 +16,7 @@ my @tables_to_ignore = ( 'abuse', 'admin_log', 'alert', 'alert_sent', 'alert_type', 'comment', 'contacts', 'contacts_history', 'debugdate', 'flickr_imported', 'partial_user', 'problem', - 'questionnaire', 'secret', 'textmystreet', + 'questionnaire', 'secret', 'textmystreet', ); my $exclude = '^(?:' . join( '|', @tables_to_ignore ) . ')$'; diff --git a/db/schema_0001.sql b/db/schema_0001.sql index 1c2bf4ffc..918fdcbc5 100644 --- a/db/schema_0001.sql +++ b/db/schema_0001.sql @@ -11,10 +11,11 @@ CREATE TABLE sessions ( -- users table create table users ( - id serial not null primary key, - email text not null unique, - name text, - password text, + id serial not null primary key, + email text not null unique, + name text, + password text, + is_confirmed bool not null default false ); -- add PK to contacts table diff --git a/perllib/FixMyStreet.pm b/perllib/FixMyStreet.pm index f581631cf..d63f708d2 100644 --- a/perllib/FixMyStreet.pm +++ b/perllib/FixMyStreet.pm @@ -9,6 +9,7 @@ my $ROOT_DIR = file(__FILE__)->parent->parent->absolute->resolve; use Readonly; use mySociety::Config; +use mySociety::DBHandle; # load the config file and store the contents in a readonly hash mySociety::Config::set_file( __PACKAGE__->path_to("conf/general") ); @@ -99,6 +100,9 @@ Most of the values are read from the config file and others are hordcoded here. # # we use the one that is most similar to DBI's connect. +# FIXME - should we just use mySociety::DBHandle? will that lead to AutoCommit +# woes (we want it on, it sets it to off)? + sub dbic_connect_info { my $class = shift; my $config = $class->config; @@ -122,4 +126,33 @@ sub dbic_connect_info { return [ $dsn, $user, $password, $dbi_args, $dbic_args ]; } +=head2 configure_mysociety_dbhandle + + FixMyStreet->configure_mysociety_dbhandle(); + +Calls configure in mySociety::DBHandle with args from the config. We need to do +this so that old code that uses mySociety::DBHandle finds it properly set up. We +can't (might not) be able to share the handle as DBIx::Class wants it with +AutoCommit on (so that its transaction code can be used in preference to calling +begin and commit manually) and mySociety::* code does not. + +This should be fixed/standardized to avoid having two database handles floating +around. + +=cut + +sub configure_mysociety_dbhandle { + my $class = shift; + my $config = $class->config; + + mySociety::DBHandle::configure( + Name => $config->{BCI_DB_NAME}, + User => $config->{BCI_DB_USER}, + Password => $config->{BCI_DB_PASS}, + Host => $config->{BCI_DB_HOST} || undef, + Port => $config->{BCI_DB_PORT} || undef, + ); + +} + 1; diff --git a/perllib/FixMyStreet/App/Controller/Auth.pm b/perllib/FixMyStreet/App/Controller/Auth.pm new file mode 100644 index 000000000..b21981417 --- /dev/null +++ b/perllib/FixMyStreet/App/Controller/Auth.pm @@ -0,0 +1,178 @@ +package FixMyStreet::App::Controller::Auth; +use Moose; +use namespace::autoclean; + +BEGIN { extends 'Catalyst::Controller'; } + +use Email::Valid; +use Net::Domain::TLD; +use mySociety::AuthToken; +use Digest::SHA1 qw(sha1_hex); + +=head1 NAME + +FixMyStreet::App::Controller::Auth - Catalyst Controller + +=head1 DESCRIPTION + +Controller for all the authentication related pages - create account, login, +logout. + +=head1 METHODS + +=head2 index + +Present the user with a login / create account page. + +=cut + +sub general : Path : Args(0) { + my ( $self, $c ) = @_; + my $req = $c->req; + + # all done unless we have a form posted to us + return unless $req->method eq 'POST'; + + # check that the email is valid - otherwise flag an error + my $raw_email = $req->param('email') || ''; + my $email_checker = Email::Valid->new( + -mxcheck => 1, + -tldcheck => 1, + -fqdn => 1, + ); + + if ( my $good_email = $email_checker->address($raw_email) ) { + $c->stash->{email} = $good_email; + } + else { + $c->stash->{email} = $raw_email; + $c->stash->{email_error} = + $raw_email ? $email_checker->details : 'missing'; + return; + } + + # decide which action to take + $c->detach('create_account') if $req->param('create_account'); + + # hmm - should not get this far. 404 so that user knows there is a problem + # rather than it silently not working. + $c->detach('/page_not_found'); + +} + +=head2 create_account + +Create an account for the user, send them an email with confirm link and log +them in straight away. If the email address already has an account send them an +email with a password reset link (slightly leaks privacy information but +required to allow instant logins). + +=cut + +sub create_account : Private { + my ( $self, $c ) = @_; + my $email = $c->stash->{email}; + + # get account from the database + my $account = $c->model('DB::User')->find_or_new( { email => $email } ); + + # Deal with existing accounts by treating it like a password reset link + if ( $account->in_storage ) { + $c->stash->{tried_to_create_account} = 1; + $c->detach('email_reset'); + } + + # we have a new account + my $password = mySociety::AuthToken::random_token(); + $account->password( sha1_hex($password) ); + $account->insert; # save to database + + # log the user in, send them an email and redirect to the welcome page + $c->authenticate( { email => $email, password => $password } ); + $c->send_email( 'auth_new_account_welcome', { to => $email } ); + $c->res->redirect( $c->uri_for('welcome') ); +} + +=head2 welcome + +Page that new users are redirected to after they have created an account. + +=cut + +sub welcome : Local { + my ( $self, $c ) = @_; + + # FIXME - check that user is logged in! + # pass thru +} + +=head2 confirm + +Confirm that a user can receive email - url is .../confirm/$token + +We don't assume that the user is logged in, but if they are they are logged out +and then logged in as the user they are confirming. The token is destroyed at +the end of the request so it cannot be reused. + +=cut + +sub confirm : Local { + my ( $self, $c, $url_token ) = @_; + + # Use the token to confirm the user and return them. + my $user = $c->model('DB::User')->confirm_user_from_token($url_token); + + # If we did not get a user back then the token was not valid + return if !$user; + + # got a user back which is now confirmed - auth as them + $c->logout(); + $c->authenticate( { email => $user->email }, 'no_password' ); + $c->stash->{user_now_confirmed} = 1; + + # TODO - should we redirect somewhere - perhaps to pending problems? + return; +} + +=head2 logout + +Log the user out. Tell them we've done so. + +=cut + +sub logout : Local { + my ( $self, $c ) = @_; + $c->logout(); +} + +=head2 check_auth + +Utility page - returns a simple message 'OK' and a 200 response if the user is +authenticated and a 'Unauthorized' / 401 reponse if they are not. + +Mainly intended for testing but might also be useful for ajax calls. + +=cut + +sub check_auth : Local { + my ( $self, $c ) = @_; + + # choose the response + my ( $body, $code ) # + = $c->user + ? ( 'OK', 200 ) + : ( 'Unauthorized', 401 ); + + # set the response + $c->res->body($body); + $c->res->code($code); + + # NOTE - really a 401 response should also contain a 'WWW-Authenticate' + # header but we ignore that here. The spec is not keeping up with usage. + + return; +} + +__PACKAGE__->meta->make_immutable; + +1; diff --git a/perllib/FixMyStreet/DB/Result/User.pm b/perllib/FixMyStreet/DB/Result/User.pm index 1ba2f094a..099ec2349 100644 --- a/perllib/FixMyStreet/DB/Result/User.pm +++ b/perllib/FixMyStreet/DB/Result/User.pm @@ -23,12 +23,39 @@ __PACKAGE__->add_columns( { data_type => "text", is_nullable => 1 }, "password", { data_type => "text", is_nullable => 1 }, + "is_confirmed", + { data_type => "boolean", default_value => \"false", is_nullable => 0 }, ); __PACKAGE__->set_primary_key("id"); __PACKAGE__->add_unique_constraint( "users_email_key", ["email"] ); -# Created by DBIx::Class::Schema::Loader v0.07009 @ 2011-03-02 12:20:05 -# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:M8SDJhpzhBZJmhar+MGQhQ +# Created by DBIx::Class::Schema::Loader v0.07009 @ 2011-03-03 10:05:03 +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:4dpBN1I88nB1BYtHT/AfKA + +=head2 create_confirm_token + + $token = $user->create_confirm_token(); + +Create a token that can be emailed to the user. When it is returned it can be +used to confirm that the email address works. + +See also the ::ResultSet::User method 'confirm_user_from_token'. + +=cut + +sub create_confirm_token { + my $self = shift; + + my $token_rs = $self->result_source->schema->resultset('Token'); + + my $token_obj = $token_rs->create( + { + scope => 'user_confirm', # + data => { email => $self->email } + } + ); + + return $token_obj->token; +} -# You can replace this text with custom code or comments, and it will be preserved on regeneration 1; diff --git a/perllib/FixMyStreet/DB/ResultSet/User.pm b/perllib/FixMyStreet/DB/ResultSet/User.pm new file mode 100644 index 000000000..a84286a78 --- /dev/null +++ b/perllib/FixMyStreet/DB/ResultSet/User.pm @@ -0,0 +1,45 @@ +package FixMyStreet::DB::ResultSet::User; +use base 'DBIx::Class::ResultSet'; + +use strict; +use warnings; + +=head2 confirm_user_from_token + + $user = $rs->confirm_user_from_token( $token ); + +Given a token retrieve it from the database, find the user it relates to and +confirm them. Return the user an the end. If anything goes wrong return undef. + +Delete the token afterwards. + +See also the ::Result::User method 'create_confirm_token' + +=cut + +sub confirm_user_from_token { + my $self = shift; + my $token_string = shift || return; + + # retrieve the token or return + my $token_rs = $self->result_source->schema->resultset('Token'); + my $token_obj = + $token_rs->find( { scope => 'user_confirm', token => $token_string, } ) + || return; + + # find the user related to the token + my $user = $self->find( { email => $token_obj->data->{email} } ); + + # If we found a user confirm them and delete the token - in transaction + $self->result_source->schema->txn_do( + sub { + $user->update( { is_confirmed => 1 } ) if $user; + $token_obj->delete; + } + ); + + # return the user (possibly undef if none found) + return $user; +} + +1; diff --git a/t/app/controller/auth.t b/t/app/controller/auth.t new file mode 100644 index 000000000..0a0280494 --- /dev/null +++ b/t/app/controller/auth.t @@ -0,0 +1,127 @@ +use strict; +use warnings; + +BEGIN { + use FixMyStreet; + FixMyStreet->test_mode(1); +} + +use Test::More tests => 44; +use Email::Send::Test; + +use FixMyStreet::App; + +use Test::WWW::Mechanize::Catalyst 'FixMyStreet::App'; +my $mech = Test::WWW::Mechanize::Catalyst->new; + +my $test_email = 'test@example.com'; + +END { + ok( + FixMyStreet::App->model('DB::User')->find( { email => $test_email } ) + ->delete, + "delete test user" + ); +} + +$mech->get_ok('/auth'); + +# check that we can't reach a page that is only available to authenticated users +is $mech->get('/auth/check_auth')->code, 401, "got 401 at check_auth"; + +# check that submitting form with no / bad email creates an error. +$mech->get_ok('/auth'); + +for my $test ( + [ '' => 'enter an email address' ], + [ 'not an email' => 'check your email address is correct' ], + [ 'bob@foo' => 'check your email address is correct' ], + [ 'bob@foonaoedudnueu.co.uk' => 'check your email address is correct' ], + ) +{ + my ( $email, $error_message ) = @$test; + pass "--- testing bad email '$email' gives error '$error_message'"; + $mech->get_ok('/auth'); + $mech->content_lacks($error_message); + $mech->submit_form_ok( + { + form_name => 'general_auth', + fields => { email => $email, }, + button => 'create_account', + }, + "try to create an account with email '$email'" + ); + is $mech->uri->path, '/auth', "still on auth page"; + $mech->content_contains($error_message); +} + +# create a new account +Email::Send::Test->clear; +$mech->get_ok('/auth'); +$mech->submit_form_ok( + { + form_name => 'general_auth', + fields => { email => $test_email, }, + button => 'create_account', + }, + "create an account for '$test_email'" +); +is $mech->uri->path, '/auth/welcome', "redirected to welcome page"; + +# check that we are now logged in +$mech->get_ok("/auth/check_auth"); + +# check that we got one email +{ + my @emails = Email::Send::Test->emails; + Email::Send::Test->clear; + + is scalar(@emails), 1, "got one email"; + is $emails[0]->header('Subject'), "Your new FixMyStreet.com account", + "subject is correct"; + is $emails[0]->header('To'), $test_email, "to is correct"; + + # extract the link + my ($link) = $emails[0]->body =~ m{(http://\S+)}; + ok $link, "Found a link in email '$link'"; + + # check that the user is currently not confirmed + my $user = + FixMyStreet::App->model('DB::User')->find( { email => $test_email } ); + ok $user, "got a user"; + ok !$user->is_confirmed, "user has not been confirmed"; + + # visit the confirm link (with bad token) and check user no confirmed + $mech->get_ok( $link . 'XXX' ); + $user->discard_changes; + ok !$user->is_confirmed, "user has not been confirmed"; + + # visit the confirm link and check user is confirmed + $mech->get_ok($link); + $user->discard_changes; + ok $user->is_confirmed, "user has been confirmed"; +} + +# logout +$mech->get_ok("/auth/logout"); +is $mech->get('/auth/check_auth')->code, 401, "got 401 at check_auth"; + +# login using valid details + +# logout + +# try to login with bad details + +# try to create an account with bad details + +# get a password reset email (for bad email address) + +# get a password reminder (for good email address) + +# try using bad reset token + +# use the good reset token and change the password + +# try to use the good token again + +# delete the test user diff --git a/templates/email/default/auth_new_account_welcome b/templates/email/default/auth_new_account_welcome new file mode 100644 index 000000000..1270c7d96 --- /dev/null +++ b/templates/email/default/auth_new_account_welcome @@ -0,0 +1,13 @@ +Subject: [% loc('Your new FixMyStreet.com account') %] + +Hello, + +We've just created an account on FIXME for you. + +Please use the following link to confirm that your email works - we need to do +this before sending reports to the council on your behalf. + + [% c.uri_for( '/auth/confirm', c.user.create_confirm_token ) %] + +Yours, + FIXME team.
\ No newline at end of file diff --git a/templates/email/default/test b/templates/email/default/test index 2d52e7f31..1200b8726 100644 --- a/templates/email/default/test +++ b/templates/email/default/test @@ -1,4 +1,4 @@ -Subject: test email +Subject: test email ☺ From: bad-sender@duff.com Hello, @@ -7,5 +7,9 @@ This is a test email where foo: [% foo %]. utf8: 我们应该能够无缝处理UTF8编码 + indented_text + +long line: Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. + Yours, FixMyStreet. diff --git a/templates/web/default/auth/confirm.html b/templates/web/default/auth/confirm.html new file mode 100644 index 000000000..fc3abeeba --- /dev/null +++ b/templates/web/default/auth/confirm.html @@ -0,0 +1,23 @@ +[% INCLUDE 'header.html', title => loc('Confirm account') %] + +[% IF user_now_confirmed %] + +<h1>[% loc('Account now confirmed') %]</h1> + +<p>Your account has been confirmed - thank you!</p> + +[% ELSE %] + +<h1>[% loc('Error') %]</h1> + +<p>We have not been able to confirm your account - sorry. This may be because:</p> + +<ul> + <li>Link too old or already used</li> + <li>URL not copied correctly</li> + <li>FIXME - reasons here</li> +</ul> + +[% END %] + +[% INCLUDE 'footer.html' %]
\ No newline at end of file diff --git a/templates/web/default/auth/general.html b/templates/web/default/auth/general.html new file mode 100644 index 000000000..f68843f41 --- /dev/null +++ b/templates/web/default/auth/general.html @@ -0,0 +1,48 @@ +[% INCLUDE 'header.html', title => loc('About Us') %] + +<h1>[% loc('Login or create an account') %]</h1> + + +<form action="[% c.uri_for() %]" method="post" name="general_auth"> + + [% IF email_error; + + # other keys include fqdn, mxcheck if you'd like to write a custom error message + + errors = { + missing => loc('Please enter an email address'), + other => loc('Please check your email address is correct') + }; + + loc_email_error = errors.$email_error || errors.other; + END %] + + + <div> + <span class="error">[% loc_email_error %]</span><br> + <label for="email">[% loc('Your email:') %]</label> + <input type="text" name="email" value="[% email || '' | html %]"> + </div> + + + <div> + <h3>I already have an account</h3> + <label for="password">[% loc('Your password:') %]</label> + <input type="password" name="password" value=""> + <input type="submit" name="login" value="[% loc('Log me in') %]"> + </div> + + <div> + <h3>I don't have an account...</h3> + <input type="submit" name="create_account" value="[% loc('...create a new account now') %]"> + </div> + + <div> + <h3>I've forgotten my password...</h3> + <input type="submit" name="email_reset" value="[% loc('...email me a reset link') %]"> + </div> + +</form> + + +[% INCLUDE 'footer.html' %] diff --git a/templates/web/default/auth/logout.html b/templates/web/default/auth/logout.html new file mode 100644 index 000000000..9f3390f0a --- /dev/null +++ b/templates/web/default/auth/logout.html @@ -0,0 +1,8 @@ +[% INCLUDE 'header.html', title => loc('Logout') %] + +<h1>[% loc('You have been logged out') %]</h1> + +<p>Please feel free to <a href="[% c.uri_for('/auth/') %]">login again</a>.</p> + + +[% INCLUDE 'footer.html' %]
\ No newline at end of file diff --git a/templates/web/default/auth/welcome.html b/templates/web/default/auth/welcome.html new file mode 100644 index 000000000..5727f82ad --- /dev/null +++ b/templates/web/default/auth/welcome.html @@ -0,0 +1,11 @@ +[% INCLUDE 'header.html', title => loc('Welcome') %] + +<h1>[% loc('Welcome') %]</h1> + +<p>Your new account on FIXME has been created!</p> + +FIXME - add blurb about being sent an email with a confirmation link to confirm +account. + + +[% INCLUDE 'footer.html' %]
\ No newline at end of file diff --git a/web/css/core.css b/web/css/core.css index 5c8dad666..0ff8fb1a2 100644 --- a/web/css/core.css +++ b/web/css/core.css @@ -46,6 +46,10 @@ p.error { font-size: larger; } +span.error { + color: #cc0000; +} + ul { padding: 0 0 0 1.5em; margin: 0; |