Skip to content

Commit bcb9598

Browse files
haarghaarg
committed
refactor login handling
Previously, logins were handled by embedding a lot into the templates, requiring special handling for OpenID and PAUSE. It also required the oauth urls and keys to be available on every page, which meant it had to have special handling in the core view module. Move to using separate end points for each login type, which then can do their own handling as appropriate. For OpenID and PAUSE, this means showing a form. For others, it's an immediate redirect. Now only the login controller needs to know anything about the oauth keys and URLs. The templates can be simpler because they don't need any per-account handling. And it adds a form for the PAUSE login, rather than the ugly javascript prompt.
1 parent 40f3833 commit bcb9598

File tree

10 files changed

+75
-55
lines changed

10 files changed

+75
-55
lines changed

‎lib/MetaCPAN/Web/Controller/Login.pm

+39-3
Original file line numberDiff line numberDiff line change
@@ -3,24 +3,35 @@ package MetaCPAN::Web::Controller::Login;
33
use Moose;
44
use namespace::autoclean;
55

6+
use URI ();
7+
68
BEGIN { extends 'MetaCPAN::Web::Controller' }
79

810
has consumer_key => ( is => 'ro' );
911
has consumer_secret => ( is => 'ro' );
12+
has openid_url => ( is => 'ro' );
13+
has oauth_url => ( is => 'ro' );
1014

1115
sub COMPONENT {
1216
my ( $class, $app, $args ) = @_;
1317
my $config = $class->merge_config_hashes(
1418
{
1519
consumer_key => $app->config->{consumer_key},
1620
consumer_secret => $app->config->{consumer_secret},
21+
openid_url =>
22+
( $app->config->{api_public} || $app->config->{api} )
23+
. '/login/openid',
24+
oauth_url => ( $app->config->{api_public} || $app->config->{api} )
25+
. '/oauth2/authorize',
1726
},
1827
$args,
1928
);
2029
return $class->SUPER::COMPONENT( $app, $config );
2130
}
2231

23-
sub index : Path : Args(0) {
32+
sub login_root : Chained('/') : PathPart('login') : CaptureArgs(0) { }
33+
34+
sub index : Chained('login_root') : PathPart('') : Args(0) {
2435
my ( $self, $c ) = @_;
2536

2637
# Never cache at CDN
@@ -52,18 +63,43 @@ sub index : Path : Args(0) {
5263
}
5364
}
5465

55-
sub openid : Local : Args(0) {
66+
sub openid : Chained('login_root') : PathPart : Args(0) {
5667
my ( $self, $c ) = @_;
5768

5869
# Never cache at CDN
5970
$c->cdn_never_cache(1);
6071

6172
$c->stash( {
6273
consumer_key => $self->consumer_key,
63-
template => 'account/openid-login.html',
74+
openid_url => $self->openid_url,
6475
} );
6576
}
6677

78+
sub pause : Chained('login_root') : PathPart : Args(0) {
79+
my ( $self, $c ) = @_;
80+
81+
# Never cache at CDN
82+
$c->cdn_never_cache(1);
83+
84+
$c->stash( {
85+
oauth_url => $self->oauth_url,
86+
consumer_key => $self->consumer_key,
87+
} );
88+
}
89+
90+
sub login : Chained('login_root') : PathPart('') : Args(1) {
91+
my ( $self, $c, $choice ) = @_;
92+
93+
my $url = URI->new( $self->oauth_url );
94+
$url->query_form( {
95+
client_id => $self->consumer_key,
96+
choice => $choice,
97+
} );
98+
99+
$c->res->redirect( $url->as_string );
100+
$c->detach;
101+
}
102+
67103
__PACKAGE__->meta->make_immutable;
68104

69105
1;

‎lib/MetaCPAN/Web/View/HTML.pm

+4-8
Original file line numberDiff line numberDiff line change
@@ -47,14 +47,10 @@ around render => sub {
4747

4848
my $req = $c->req;
4949

50-
$vars->{api_public} = $self->api_public;
51-
$vars->{source_host} = $self->source_host;
52-
$vars->{assets} = $req->env->{'psgix.assets'} || [];
53-
$vars->{req} = $req;
54-
$vars->{oauth_prefix}
55-
= $self->api_public
56-
. '/oauth2/authorize?client_id='
57-
. $c->config->{consumer_key};
50+
$vars->{api_public} = $self->api_public;
51+
$vars->{source_host} = $self->source_host;
52+
$vars->{assets} = $req->env->{'psgix.assets'} || [];
53+
$vars->{req} = $req;
5854
$vars->{site_alert_message} = $c->config->{site_alert_message};
5955
$vars->{page_url} = sub {
6056
@_ ? $req->uri_with(@_) : $req->uri->clone;

‎root/account/identities.html

+7-9
Original file line numberDiff line numberDiff line change
@@ -5,24 +5,22 @@ <h4><strong>Information</strong></h4>
55
Identities you are connected to allow you to log into MetaCPAN. MetaCPAN also drags in some information from them to help you with filling in your <a href="/account/profile">profile</a>. This information is not exposed to anyone unless you explicitly publish it.
66
</div>
77
<table width="300">
8-
<%- FOREACH identity IN ['GitHub', 'PAUSE', 'Twitter', 'Google', 'OpenID']; found = user.identity.grep(->(a){ a.name == identity.lower }) %>
8+
<%- FOREACH identity IN ['GitHub', 'PAUSE', 'Twitter', 'Google', 'OpenID'];
9+
connected = user.identity.grep(->(a){ a.name == identity.lower }).0;
10+
%>
911
<tr>
1012
<td><big><% identity %></big></td>
1113
<td>
12-
<%- IF found.size %>
13-
<form action="/account/identities" method="POST">
14+
<%- IF connected %>
15+
<form method="POST">
1416
<input type="hidden" name="delete" value="<% identity.lower %>" />
1517
<button type="submit" class="btn btn-block btn-danger"><span class="fa fa-times"></span> Disconnect</button>
1618
</form>
1719
<%- ELSE %>
18-
<%- IF identity == 'OpenID' %>
19-
<a class="btn btn-block btn-success" href="/login/openid"><span class="fa fa-external-link"></span> Connect</a>
20-
<%- ELSE %>
21-
<a class="btn btn-block btn-success" href="<% oauth_prefix %>&amp;choice=<% identity.lower %>" onclick="return logInPAUSE(this)"><span class="fa fa-external-link"></span> Connect</a>
22-
<% END %>
20+
<a class="btn btn-block btn-success" href="/login/<% identity.lower %>"><span class="fa fa-external-link"></span> Connect</a>
2321
<%- END %>
2422
</td>
2523
</tr>
26-
<%- END %>
24+
<%- END %>
2725
</table>
2826
</div>

‎root/account/profile.html

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
<% IF no_profile -%>
44
<div class="alert alert-danger">
55
<h4 class="alert-heading">Error</h4>
6-
In order to change your profile you have to <a href="<% oauth_prefix %>&amp;choice=pause" onclick="return logInPAUSE(this)">connect your account to PAUSE</a>.
6+
In order to change your profile you have to <a href="/login/pause">connect your account to PAUSE</a>.
77
</div>
88
<% ELSE -%>
99
<form method="POST" action="" class="form-horizontal" role="form" accept-charset="utf-8">

‎root/account/openid-login.html renamed to ‎root/login/openid.html

+1-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
<div class="container-fluid">
33
<div class="col-md-offset-3 col-md-6">
44
<p class="text-center">Log into MetaCPAN using OpenID</p>
5-
<form action="<% api_public %>/login/openid">
5+
<form action="<% openid_url %>">
66
<input type="hidden" name="client_id" value="<% consumer_key %>">
77
<div class="input-group">
88
<span class="input-group-addon">

‎root/login/pause.html

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
<div class="jumbotron">
2+
<div class="container-fluid">
3+
<div class="col-md-offset-3 col-md-6">
4+
<p class="text-center">Connect to MetaCPAN using PAUSE</p>
5+
<form action="<% oauth_url %>">
6+
<input type="hidden" name="client_id" value="<% consumer_key %>">
7+
<div class="input-group">
8+
<span class="input-group-addon">
9+
PAUSE ID
10+
</span>
11+
<input type="text" class="form-control" name="id" />
12+
<span class="input-group-btn">
13+
<button class="btn btn-primary" type="submit">Log in</button>
14+
</span>
15+
</div>
16+
</form>
17+
</div>
18+
</div>
19+
</div>

‎root/static/js/cpan.js

-9
Original file line numberDiff line numberDiff line change
@@ -524,15 +524,6 @@ function set_page_size(selector, storage_name) {
524524
});
525525
}
526526

527-
528-
function logInPAUSE(a) {
529-
if (!a.href.match(/pause/))
530-
return true;
531-
var id = prompt('Please enter your PAUSE ID:');
532-
if (id) document.location.href = a.href + '&id=' + id;
533-
return false;
534-
}
535-
536527
function processUserData() {
537528

538529
// Could do some fancy localStorage thing here

‎root/static/less/login.less

-13
This file was deleted.

‎root/static/less/style.less

-1
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,5 @@
5656
@import "lab.less";
5757
@import "home.less";
5858
@import "syntaxhighlighter.less";
59-
@import "login.less";
6059
@import "pod2html.less";
6160
@import "notification.less";

‎root/wrapper.html

+4-10
Original file line numberDiff line numberDiff line change
@@ -143,20 +143,14 @@
143143
<b class="caret"></b>
144144
</a>
145145
<ul class="dropdown-menu">
146-
<%- FOREACH identity IN ['GitHub', 'Twitter', 'Google'] %>
146+
<%- FOREACH identity IN ['GitHub', 'Twitter', 'Google', 'OpenID'] %>
147147
<li>
148-
<a href="<% oauth_prefix %>&amp;choice=<% identity.lower %>" onclick="return logInPAUSE(this)">
149-
<i class="fa fa-<% identity.lower %> fa-fw"></i>
150-
<% identity %>
148+
<a href="/login/<% identity.lower %>">
149+
<i class="fa fa-<% identity.lower %> fa-fw"></i>
150+
<% identity %>
151151
</a>
152152
</li>
153153
<%- END %>
154-
<li>
155-
<a href="/login/openid">
156-
<i class="fa fa-openid fa-fw"></i>
157-
OpenID
158-
</a>
159-
</li>
160154
</ul>
161155
</li>
162156
</ul>

0 commit comments

Comments
 (0)