Dylan Hardison [:dylan] (he/him) Assignee
	Description • 9 years ago

I will be writing an extension to allow authentication via github's OAuth API, in a similar way to the way BMO supports Persona.

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 1 • 9 years ago

When this is all said and done, there is setup required on the github side -- the "app" has to have an owner. Ideas on who that would be? it's pretty trivial to transfer it.

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 2 • 9 years ago

Is it acceptable that this extension provides a new .cgi endpoint?
I'm doing so like BzAPI does, with a htaccess rewrite rule. I may have been possible to use page.cgi but that felt really dirty. Thoughts?

	:glob ✱
	Comment 3 • 9 years ago

(In reply to Dylan William Hardison [:dylan] from comment #1)
> When this is all said and done, there is setup required on the github side
> -- the "app" has to have an owner. Ideas on who that would be? it's pretty
> trivial to transfer it.

set it to yourself.

(In reply to Dylan William Hardison [:dylan] from comment #2)
> Is it acceptable that this extension provides a new .cgi endpoint?
> I'm doing so like BzAPI does, with a htaccess rewrite rule. I may have been
> possible to use page.cgi but that felt really dirty. Thoughts?

adding a new .cgi is fine with a rewrite rule is fine.

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 4 • 9 years ago

"And, has thou slain the Jabberwock?
  Come to my arms, my beamish boy!
O frabjous day! Callooh! Callay!'
  He chortled in his joy."

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 5 • 9 years ago

The patch doesn't include the two image files, so I pushed my code up to a branch on a github fork of bmo: https://github.com/dylanwh/webtools-bmo-bugzilla/tree/github-auth

To set this you'll need to register an application with github: https://github.com/settings/applications

You'll need to get the client_id and client_secret, and configure them on the Advanced tab of the admin params page. The callback URI needs to be reachable from your browser, and should end with github.callback (it doesn't need to be on the internet, I think)

	:glob ✱
	Comment 6 • 9 years ago

Comment on attachment 8574670 [details] [diff] [review]
bug-1118365-v1.patch

Review of attachment 8574670 [details] [diff] [review]:
-----------------------------------------------------------------

a quick drive-by..

::: .htaccess
@@ +55,3 @@
>  RewriteRule ^form[\.:]reps[\.:]swag$ enter_bug.cgi?product=Mozilla+Reps&format=remo-swag
>  RewriteRule ^form[\.:]reps[\.:]it$ enter_bug.cgi?product=Mozilla+Reps&format=remo-it
>  RewriteRule ^form[\.:]reps[\.:]payment$ page.cgi?id=remo-form-payment.html

oops

::: extensions/Github/Extension.pm
@@ +4,5 @@
> +#
> +# This Source Code Form is "Incompatible With Secondary Licenses", as
> +# defined by the Mozilla Public License, v. 2.0.
> +
> +package Bugzilla::Extension::Github;

this should probably been called GithubAuth or similar, to differentiate it from other github integration stuff (eg. the PR attachment handling).

@@ +27,5 @@
> +# See the documentation of Bugzilla::Hook ("perldoc Bugzilla::Hook"
> +# in the bugzilla directory) for a list of all available hooks.
> +sub install_update_db {
> +    my ($self, $args) = @_;
> +}

remove this empty method and comment

@@ +55,5 @@
> +}
> +
> +sub template_before_create {
> +    my ($self, $args) = @_;
> +    my $config = $args->{config};

there's no need to do this if there's a user logged in

::: extensions/Github/template/en/default/hook/README
@@ +1,5 @@
> +Template hooks go in this directory. Template hooks are called in normal
> + templates like [% Hook.process('some-hook') %].
> +More information about them can be found in the documentation of
> +Bugzilla::Extension. (Do "perldoc Bugzilla::Extension" from the main
> + directory to see that documentation.)
\ No newline at end of file

you can delete this file

::: extensions/Github/template/en/default/hook/account/auth/login-small-additional_methods.html.tmpl
@@ +14,5 @@
> +  });
> +  YAHOO.util.Event.addListener('hide_mini_login[% qs_suffix FILTER js %]','click', function () {
> +      var login_link = YAHOO.util.Dom.get('persona_mini_login[% qs_suffix FILTER js %]');
> +      YAHOO.util.Dom.addClass(login_link, 'bz_default_hidden');
> +  });

use jquery please

::: extensions/Github/web/README
@@ +3,5 @@
> +if you have a file called "style.css" and your extension is called
> +"Foo", you would put it in "extensions/Foo/web/style.css", and then
> +you could link to it in HTML like:
> +
> +<link href="extensions/Foo/web/style.css" rel="stylesheet" type="text/css">
\ No newline at end of file

another file to delete

	David Lawrence [:dkl]
	Comment 7 • 9 years ago

t/012throwables.t .... 1/607 
#   Failed test '--WARNING extensions/Github/template/en/default/hook/global/user-error-errors.html.tmpl has 1 unused error tag(s):
# user error tag 'github_error' is defined at line(s) (9) but is not used anywhere'
#   at t/012throwables.t line 212.
# Looks like you failed 1 test of 607.
t/012throwables.t .... Dubious, test returned 1 (wstat 256, 0x100)

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 8 • 9 years ago

whaaat? that's not in what I commited.

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 9 • 9 years ago

Right, that's the warning message that glob's bz tools changes. I'm going to make that at least conditional on an env variable, because having to modify a test to get proper coverage reports is super broken.

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 10 • 9 years ago

With the unused error removed. Note that the proxy config is not the global one currently -- that's a minor change that can happen either at commit, or more likely during the second revision.

	David Lawrence [:dkl]
	Comment 11 • 9 years ago

Comment on attachment 8575261 [details] [diff] [review]
bug-1118365-v1.5.patch

Review of attachment 8575261 [details] [diff] [review]:
-----------------------------------------------------------------

1. 2 taint issues I solved with trick_taint() but may want to do some verification to make sure nothing bad could be passed in:

AH01215: Insecure dependency in parameter 4 of DBI::db=HASH(0x5c038d8)->do method call while running with -T switch at /loader/0x4b1f320/Bugzilla/Extension/Github/State.pm line 77. at /loader/0x4b1f320/Bugzilla/Extension/Github/State.pm line 77. Bugzilla::Extension::Github::State::_build_token('Bugzilla::Extension::Github::State=HASH(0x5e09d00)') called at /loader/0x4b1f320/Bugzilla/Extension/Github/State.pm line 23 Bugzilla::Extension::Github::State::new('Bugzilla::Extension::Github::State', 'eventdata', 'index.cgi') called at /loader/0x4b1f320/Bugzilla/Extension/Github/Client.pm line 48 Bugzilla::Extension::Github::Client::login_uri('Bugzilla::Extension::Github::Client', 'index.cgi') called at ./extensions/Github/Extension.pm line 145 Bugzilla::Extension::Github::_github_login('Bugzilla::Extension::Github=HASH(0x1ae7868)', 'HASH(0x43f6000)') called at ./extensions/Github/Extension.pm line 78 Bugzilla::Extension::Github::page_before_template('Bugzilla::Extension::Github=HASH(0x1ae7868)', 'HASH(0x1adbee0)') called at Bugzilla/Hook.pm line 33 Bugzilla::Hook::process('page_before_template', 'HASH(0x1adbee0)') called at /home/bugzilla/devel/htdocs/1118365/page.cgi line 89 at /loader/0x4b1f320/Bugzilla/Extension/Github/State.pm line 77. \tBugzilla::Extension::Github::State::_build_token(...) called at /loader/0x4b1f320/Bugzilla/Extension/Github/State.pm line 23 \tBugzilla::Extension::Github::State::new(...) called at /loader/0x4b1f320/Bugzilla/Extension/Github/Client.pm line 48 \tBugzilla::Extension::Github::Client::login_uri(...) called at ./extensions/Github/Extension.pm line 145 \tBugzilla::Extension::Github::_github_login(...) called at ./extensions/Github/Extension.pm line 78 \tBugzilla::Extension::Github::page_before_template(...) called at Bugzilla/Hook.pm line 33 \tBugzilla::Hook::process(...) called at /home/bugzilla/devel/htdocs/1118365/page.cgi line 89, referer: http://localhost/1118365/

AH01215: Insecure dependency in parameter 3 of DBI::db=HASH(0x60762e0)->selectrow_array method call while running with -T switch at /loader/0x4f91688/Bugzilla/Extension/Github/State.pm line 50. at /loader/0x4f91688/Bugzilla/Extension/Github/State.pm line 50. Bugzilla::Extension::Github::State::is_valid('Bugzilla::Extension::Github::State=HASH(0x63f63c8)') called at ./extensions/Github/Extension.pm line 162 Bugzilla::Extension::Github::_github_finish('Bugzilla::Extension::Github=HASH(0x1f59930)', 'HASH(0x4868328)') called at ./extensions/Github/Extension.pm line 81 Bugzilla::Extension::Github::page_before_template('Bugzilla::Extension::Github=HASH(0x1f59930)', 'HASH(0x1f4e078)') called at Bugzilla/Hook.pm line 33 Bugzilla::Hook::process('page_before_template', 'HASH(0x1f4e078)') called at /home/bugzilla/devel/htdocs/1118365/page.cgi line 89 at /loader/0x4f91688/Bugzilla/Extension/Github/State.pm line 50. \tBugzilla::Extension::Github::State::is_valid(...) called at ./extensions/Github/Extension.pm line 162 \tBugzilla::Extension::Github::_github_finish(...) called at ./extensions/Github/Extension.pm line 81 \tBugzilla::Extension::Github::page_before_template(...) called at Bugzilla/Hook.pm line 33 \tBugzilla::Hook::process(...) called at /home/bugzilla/devel/htdocs/1118365/page.cgi line 89, referer: http://localhost/1118365/

2. As glob mentioned, let's rename the extension to GithubAuth.

3. Let's put the github param settings in their own Panel similar to how we do Persona. Advanced is getting too crowded and making it difficult to find like settings.

4. When an error occurs from Github such as 'Bad credentials', the data returned from get_user_emails($access_token) is not an array but rather a hash so we need to check for it and throw a descriptive error instead of an ISE 500.

AH01215: Not an ARRAY reference at ./extensions/Github/Extension.pm line 106.
AH01215:  at ./extensions/Github/Extension.pm line 106.
AH01215: \tBugzilla::Extension::Github::_github_callback('Bugzilla::Extension::Github=HASH(0xb58f58)', 'HASH(0x34685d8)') called at ./extensions/Github/Extension.pm line 75
AH01215: \tBugzilla::Extension::Github::page_before_template('Bugzilla::Extension::Github=HASH(0xb58f58)', 'HASH(0xb4ed90)') called at Bugzilla/Hook.pm line 33
AH01215: \tBugzilla::Hook::process('page_before_template', 'HASH(0xb4ed90)') called at /home/bugzilla/devel/htdocs/1118365/page.cgi line 89
End of script output before headers: page.cgi

More to come as I figure out what is happening in the code ;)

::: extensions/Github/lib/State.pm
@@ +10,5 @@
> +use warnings;
> +
> +use Bugzilla::Token ();
> +
> +use constant TOKEN_TYPE => 'github-state';

nit: s/github-state/github_state/

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 12 • 9 years ago

Not sure why I did not encounter the taint errors. Will get a revised version up ASAP.

	David Lawrence [:dkl]
	Comment 13 • 9 years ago

Comment on attachment 8575261 [details] [diff] [review]
bug-1118365-v1.5.patch

Review of attachment 8575261 [details] [diff] [review]:
-----------------------------------------------------------------

Just some minor things so far. Not a complete review but please include in your next version.

::: extensions/Github/template/en/default/hook/account/auth/login-additional_methods.html.tmpl
@@ +6,5 @@
> +  # defined by the Mozilla Public License, v. 2.0.
> +  #%]
> +
> +[% IF Param('user_info_class').split(',').contains('Github') %]
> +<p>

indentation

::: extensions/Github/template/en/default/hook/account/auth/login-small-additional_methods.html.tmpl
@@ +6,5 @@
> +  # defined by the Mozilla Public License, v. 2.0.
> +  #%]
> +
> +[% IF Param('user_info_class').split(',').contains('Github') %]
> +<script type="text/javascript">

indentation

@@ +12,5 @@
> +      var login_link = YAHOO.util.Dom.get('github_mini_login[% qs_suffix FILTER js %]');
> +      YAHOO.util.Dom.removeClass(login_link, 'bz_default_hidden');
> +  });
> +  YAHOO.util.Event.addListener('hide_mini_login[% qs_suffix FILTER js %]','click', function () {
> +      var login_link = YAHOO.util.Dom.get('persona_mini_login[% qs_suffix FILTER js %]');

s/persona/github/

::: extensions/Github/template/en/default/hook/global/code-error-errors.html.tmpl
@@ +7,5 @@
> +  #%]
> +
> +[% IF error == "github_invalid_state" %]
> +  [% title = "Invalid State Parameter" %]
> +  An invalid <em>state</em> parameter was passed to the github oauth2 callback.

nit: Change to ...Github OAuth2...

@@ +11,5 @@
> +  An invalid <em>state</em> parameter was passed to the github oauth2 callback.
> +
> +[% ELSIF error == "github_missing_code" %]
> +  [% title = "Missing Github Auth Code" %]
> +  Expected a <em>code</em> parameter in the github oauth2 callback.

Same

::: extensions/Github/template/en/default/hook/global/user-error-errors.html.tmpl
@@ +5,5 @@
> +  # This Source Code Form is "Incompatible With Secondary Licenses", as
> +  # defined by the Mozilla Public License, v. 2.0.
> +  #%]
> +
> +[% IF error == "github_emails" %]

change to github_no_emails or something similar.

::: extensions/Github/template/en/default/pages/github_callback.html.tmpl
@@ +31,5 @@
> +[% ELSE %]
> +  [% IF github_emails.size == 1 %]
> +    <a href="[% github_emails.0.uri FILTER html %]">Create [% terms.Bugzilla %] account for [% github_emails.0.address FILTER html %]?</a>
> +  [% ELSE %]
> +  <p>You have multiple email addresses associated with your Github account.

indention

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 14 • 9 years ago

Turns out the taint errors didn't manifest for a few reasons, including PerlSwitches has to be called before anything else, once the PerlInterp is loaded it's set in stone.

Additionally, on friday we discussed in IRC:

1. Remove both the github.login and github.finish callbacks
2. Investigate removing github.callback if possible, putting more logic into the Login/Verify code.

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 15 • 9 years ago

Major changes: Everything, no redirects or rewrites in .htaccess, no use of the tokens table, and no implementation of sessions. 

Here is the list of items from the review that I'd addressed:

** DONE .htaccess oops
** DONE rename to GithubAuth
** DONE remove this empty method and comment
** DONE delete extensions/Github/template/en/default/hook/README
** TODO switch to jquery
** DONE extensions/Github/web/README
** DONE issues with taint mode
** DONE Let's put the github param settings in their own Panel similar to how we do Persona.
** DONE Error handling from github / json errors
** DONE fix indentation in various places
** DONE Remove github.login endpoint
** DONE Remove github.finish endpoint
** DONE change github_emails to github_no_emails
** DONE change github oauth to Github OAuth

Switching to jQuery is not advisable at the moment, it will require changing the Persona extension, else YUI and jQuery fight over the click events. Will file as separate bug.

	David Lawrence [:dkl]
	Comment 16 • 9 years ago

Comment on attachment 8582409 [details] [diff] [review]
bug-118365-v5.patch

Review of attachment 8582409 [details] [diff] [review]:
-----------------------------------------------------------------

Much nicer than before! Almost there.

1. To allow the special form redirects to work we need to add [R] to each one to allow the github params to be appended to the redirect_url. This works for me:

RewriteRule ^form[\.:]moz[\.\-:]project[\.\-:]review$ enter_bug.cgi?product=mozilla.org&format=moz-project-review [R] 

2. Warning in error_log:

AH01215: Use of uninitialized value in subroutine entry at Bugzilla/Util.pm line 659. at Bugzilla/Util.pm line 659. \tBugzilla::Util::bz_crypt(...) called at Bugzilla/Auth/Verify/DB.pm line 60 \tBugzilla::Auth::Verify::DB::check_credentials(...) called at Bugzilla/Auth/Verify/Stack.pm line 62 \tBugzilla::Auth::Verify::Stack::check_credentials(...) called at Bugzilla/Auth.pm line 73 \tBugzilla::Auth::login(...) called at Bugzilla.pm line 367 \tBugzilla::login(...) called at /home/bugzilla/devel/htdocs/1118365/enter_bug.cgi line 56, referer: http://localhost/1118365/enter_bug.cgi
[Tue Mar 24 19:52:04.774369 2015] [cgi:error] [pid 610] [client 172.17.42.1:40341] AH01215: $VAR1 = bless( do{\\(my $o = 'http://localhost/1118365/index.cgi?GoAheadAndLogIn=1&github_login=1')}, 'URI::http' );, referer: http://localhost/1118365/enter_bug.cgi?GoAheadAndLogIn=1&code=17c26f39f7ceffaf7cb1&github_login=1&state=05926111389516745e3d39659272c85f

3. Remove extensions/GithubAuth/template/en/default/hook/admin/params/editparams-current_panel.html.tmpl

4. Extension.pm should create the no-github-auth group if it does not exist.

5. Rename extensions/GithubAuth/template/en/default/account/auth/github.html.tmpl to extensions/GithubAuth/template/en/default/account/auth/github-verify-account.html.tmpl

::: extensions/GithubAuth/lib/Client.pm
@@ +32,5 @@
> +    return $self;
> +}
> +
> +sub client_id {
> +    my ($self) = @_;

nit: not needed

@@ +38,5 @@
> +    return Bugzilla->params->{github_client_id};
> +}
> +
> +sub client_secret {
> +    my ($self) = @_;

nit: not needed

@@ +57,5 @@
> +
> +    return $uri;
> +}
> +
> +sub get_state {

1. We need to make sure the query params are always sorted as I had issue with it throwing state errors due to md5 not matching sometimes.

sub get_state {          
    my ($self, $target) = @_;          
                         
    # Make sure query params are always sorted
    my @sorted_params;   
    foreach my $key (sort $target->query_param()) {
        push(@sorted_params, $key, $target->query_param($key));
    }                    
    $target->query_form(@sorted_params);
                         
    my $md5 = Digest::MD5->new;        
    $md5->add($target->as_string);     
    $md5->add("::");     
    $md5->add(Bugzilla->localconfig->{site_wide_secret});                                                                                                                 
    return $md5->hexdigest;
}

2. Also I think we should add in the remote_ip() in case many people have the same target URL which could make the md5 guessable.

3. Use SHA1 instead of md5 as discussed in the BMO meeting.

@@ +124,5 @@
> +
> +    return $self->{user_agent};
> +}
> +
> +sub _build_user_agent {

to simplify, you could just put this code inside of user_agent()

::: extensions/GithubAuth/lib/Client/Error.pm
@@ +47,5 @@
> +        die __PACKAGE__->_new('code', @_);
> +    }
> +    else {
> +        # removes this subroutine from the call stack.
> +        goto &Bugzilla::Error::ThrowCodeError;

You said something in IRC about not really needing the goto as this part is not reached?

::: extensions/GithubAuth/lib/Login.pm
@@ +13,5 @@
> +
> +use Scalar::Util qw(blessed);
> +
> +use Bugzilla::Constants qw(AUTH_NODATA AUTH_ERROR USAGE_MODE_BROWSER );
> +use Bugzilla::Util qw(trick_taint detaint_natural correct_urlbase);

detaint_natural not used?

@@ +65,5 @@
> +        # The following variable lets us catch and return (rather than throw) errors
> +        # from our github client code, as required by the Auth API.
> +        local $Bugzilla::Extension::GithubAuth::Client::Error::USE_EXCEPTION_OBJECTS = 1;
> +        $access_token = $client->get_access_token($code);
> +        $user_info    = $client->get_user($access_token);

$user_info does not seem to be used anywhere

@@ +82,5 @@
> +
> +    my $choose_email = sub {
> +        my ($email) = @_;
> +        my $uri  = $target->clone;
> +        my $hash = md5_hex($email, Bugzilla->localconfig->{site_wide_secret});

Maybe we should add remote_ip here too and in _get_login_info_from_email for a little more caution?

@@ +127,5 @@
> +                github_emails => \@github_emails,
> +                choose_email  => $choose_email,
> +            },
> +        };
> +        Return { failure => AUTH_NODATA };

s/Return/return/

@@ +136,5 @@
> +}
> +
> +sub _get_login_info_from_email {
> +    my ($self, $github_email, $github_email_key) = @_;
> +    my $cgi     = Bugzilla->cgi;

Nit: remove extra whitespace

@@ +169,5 @@
> +}
> +
> +sub requires_verification   { 1 }
> +sub is_automatic            { 1 }
> +sub user_can_create_account { 1 }

These would look better as normal constants at the top similar to Bugzilla/Auth/Login.pm

use constant requires_verification => 1;
use constant is_automatic => 1;
use constant user_can_create_account => 1;

::: extensions/GithubAuth/template/en/default/account/auth/github.html.tmpl
@@ +29,5 @@
> +    [% END %]
> +  </ul>
> +[% ELSE %]
> +  [% IF github_emails.size == 1 %]
> +    <a href="[% choose_email(github_emails.0) FILTER html %]">Create [% terms.Bugzilla %] account for [% github_emails.0 FILTER html %]?</a>

This looks bad when this error occurs as it is just a sentence with a link. I would do something like this instead:

<h1>Account Not In [% terms.Bugzilla %]</h1>

<p>
  The email '[% github_emails.0 FILTER html %]' was not found in [% terms.Bugzilla %] and will need to be created to login.
</p>

<a href="[% choose_email(github_emails.0) FILTER html %]">Create account?</a>

@@ +30,5 @@
> +  </ul>
> +[% ELSE %]
> +  [% IF github_emails.size == 1 %]
> +    <a href="[% choose_email(github_emails.0) FILTER html %]">Create [% terms.Bugzilla %] account for [% github_emails.0 FILTER html %]?</a>
> +  [% ELSE %]

<h1>Multiple Github Accounts</h1>

::: extensions/GithubAuth/template/en/default/hook/account/auth/login-additional_methods.html.tmpl
@@ +8,5 @@
> +
> +[% IF Param('user_info_class').split(',').contains('GithubAuth') %]
> +  <p>
> +    <a href="[% github_auth.login FILTER html %]">
> +      <img src="extensions/GithubAuth/web/images/github_sign_in.png" alt="Sign into GitHub" width="185" height="25">

nit: s/Sign into Github/Sign in with Github/. Also Add title="Sign in with Github".

::: extensions/GithubAuth/template/en/default/hook/account/auth/login-small-additional_methods.html.tmpl
@@ +19,5 @@
> +  </script>
> +  <span id="github_mini_login[% qs_suffix FILTER html %]" class="bz_default_hidden">
> +    <a href="[% github_auth.login FILTER html %]">
> +      <img src="extensions/GithubAuth/web/images/sign_in.png" height="22" width="75" align="absmiddle"
> +           title="Sign in with Github"></a> or

Add alt="Sign in with Github"

::: extensions/GithubAuth/template/en/default/hook/global/user-error-errors.html.tmpl
@@ +10,5 @@
> +  Your Github account cannot be used because [% terms.Bugzilla %] cannot see any email addresses. Either your Github account
> +  has no verified email addresses or [% terms.Bugzilla %] is not authorized to see them.
> +
> +[% ELSIF error == "github_invalid_email" %]
> +  Your Github account email [% email FILTER html %] is not valid.

nit: add single quotes, '[% email FILTER html %]'

	Mark Côté [:mcote]
	Comment 17 • 9 years ago

General nit: it's "GitHub", not "Github".

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 18 • 9 years ago

> * DONE To allow the special form redirects to work we need to add [R]
Are we sure that redirecting explicitely is the right thing here?

> * DONE Warning in error_log:
I am unable to reproduce this... What conditions cause it? I may have to
monkey-patch Bugzilla::Auth::Verify::Stack->check_credentials to silence this
warning.

> * DONE Remove extensions/GithubAuth/template/en/default/hook/admin/params/editparams-current_panel.html.tmpl
> * DONE Extension.pm should create the no-github-auth group if it does not exist.
> * DONE Rename extensions/GithubAuth/template/en/default/account/auth/github.html.tmpl
> * DONE nit: not needed
> * DONE get_state stuff
> ** We need to make sure the query params are always sorted as I had issue with it throwing state errors due to md5 not matching sometimes.
    I reworked this so it still works with duplicate query param keys.
> ** Also I think we should add in the remote_ip() in case many people have the same target URL which could make the md5 guessable.
    Done, for both states ane email keys.
> ** Use SHA1 instead of md5 as discussed in the BMO meeting.
    Done. Changing to SHA-256 or whatever is a one line change too.

> * DONE You said something in IRC about not really needing the goto as this part is not reached?
   Well, the code path isn't *called* currently, but I would keep it there so
   that the ::Client code can work in other situations. I removed the goto, just the same.

> * DONE detaint_natural not used?
> * DONE $user_info does not seem to be used anywhere
   Removed, we can add it back if we need it.

> * DONE Maybe we should add remote_ip here too and in _get_login_info_from_email for a little more caution?
> * DONE Nit: remove extra whitespace
> * DONE These would look better as normal constants at the top similar to Bugzilla/Auth/Login.pm
   Done. I strongly disagree with this style and think it should fixed everywhere. It is an abuse of the constant
   pragma. Prototypes aren't considered for methods, there is a compile-time penalty
   for "use constant" rather than a sub declaration, and it is more characters on
   average.

> * DONE This looks bad when this error occurs as it is just a sentence with a link. I would do something like this instead:
> * DONE nit: s/Sign into Github/Sign in with Github/. Also Add title="Sign in with Github".
> * DONE Add alt="Sign in with Github"
> * DONE nit: add single quotes, '[% email FILTER html %]'

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 19 • 9 years ago

(In reply to Mark Côté [:mcote] from comment #17)
> General nit: it's "GitHub", not "Github".

This is addressed in the most recent patch as well. :)

	Reed Loden [:reed]
	Comment 20 • 9 years ago

Has there been a security review of this (both the plan and implementation)?

	Reed Loden [:reed]
	Comment 21 • 9 years ago

(In reply to Dylan William Hardison [:dylan] from comment #19)
> (In reply to Mark Côté [:mcote] from comment #17)
> > General nit: it's "GitHub", not "Github".
> 
> This is addressed in the most recent patch as well. :)

Extension is still GithubAuth

+       github_client_id     => "Your Github Client ID",
+       github_client_secret => "Your Github Client Secret"

Also still see it in a few places as well...

	Mark Côté [:mcote]
	Comment 22 • 9 years ago

(In reply to Reed Loden [:reed] (use needinfo?) from comment #20)
> Has there been a security review of this (both the plan and implementation)?

There is an RRA scheduled for April 1st at 2 pm EDT.

	David Lawrence [:dkl]
	Comment 23 • 9 years ago

Okay. I think we should just do s/Github/GitHub everywhere for uber consistency. I know it sucks but this hasn't been pushed out to the world yet so now is the best time. I will continue to review everything else.

dkl

	David Lawrence [:dkl]
	Comment 24 • 9 years ago

(In reply to David Lawrence [:dkl] from comment #16)
> 2. Warning in error_log:
> 
> AH01215: Use of uninitialized value in subroutine entry at Bugzilla/Util.pm
> line 659. at Bugzilla/Util.pm line 659. \tBugzilla::Util::bz_crypt(...)
> called at Bugzilla/Auth/Verify/DB.pm line 60
> \tBugzilla::Auth::Verify::DB::check_credentials(...) called at
> Bugzilla/Auth/Verify/Stack.pm line 62
> \tBugzilla::Auth::Verify::Stack::check_credentials(...) called at
> Bugzilla/Auth.pm line 73 \tBugzilla::Auth::login(...) called at Bugzilla.pm
> line 367 \tBugzilla::login(...) called at
> /home/bugzilla/devel/htdocs/1118365/enter_bug.cgi line 56, referer:
> http://localhost/1118365/enter_bug.cgi
> [Tue Mar 24 19:52:04.774369 2015] [cgi:error] [pid 610] [client
> 172.17.42.1:40341] AH01215: $VAR1 = bless( do{\\(my $o =
> 'http://localhost/1118365/index.cgi?GoAheadAndLogIn=1&github_login=1')},
> 'URI::http' );, referer:
> http://localhost/1118365/enter_bug.
> cgi?GoAheadAndLogIn=1&code=17c26f39f7ceffaf7cb1&github_login=1&state=05926111
> 389516745e3d39659272c85f

I found out this happens if it tries to use DB to verify before GithubAuth. Example:

'user_verify_class' => 'DB,GithubAuth',

If you switch them around, the error goes away.

dkl

	David Lawrence [:dkl]
	Comment 25 • 9 years ago

Comment on attachment 8585244 [details] [diff] [review]
bug-118365-v6.patch

Review of attachment 8585244 [details] [diff] [review]:
-----------------------------------------------------------------

1. Does not apply:
patching file .htaccess
Hunk #1 FAILED at 50.
1 out of 1 hunk FAILED -- saving rejects to file .htaccess.rej

2. Figure out issue with using [L,QSA] instead of [R] and how to deal with dupe keys when SHA1 the query string.

3. Change Github to GitHub 

Should be a quick r+ after once I verify the forms are working.

dkl

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 26 • 9 years ago

The trick was using $cgi->redirect_uri (REDIRECT_URI) rather than a custom env rule set by htaccces [E] which was my original idea.

renamed everything, rebased to master and cleaned up state generation a lot too.

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 27 • 9 years ago

binary patch. :)

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 28 • 9 years ago

without TRAMP'led whitespace

	David Lawrence [:dkl]
	Comment 29 • 9 years ago

Comment on attachment 8586569 [details] [diff] [review]
bug-1118365-v7.patch

Review of attachment 8586569 [details] [diff] [review]:
-----------------------------------------------------------------

patching file .htaccess
Hunk #1 FAILED at 50.
1 out of 1 hunk FAILED -- saving rejects to file .htaccess.rej

::: extensions/GitHubAuth/Extension.pm
@@ +47,5 @@
> +    return if Bugzilla->user->id && !Bugzilla->cgi->param('logout');
> +
> +    $args->{config}{VARIABLES}{github_auth} = {
> +        login => sub {
> +            my $target  = URI->new(substr(correct_urlbase(), 0, -1) . Bugzilla->cgi->request_uri);

This does not work if the Bugzilla installation is not in the document root. For example I have my devel env set up for this as http://localhost/1118365 and that is also the 'url_base' in data/params.
So the resulting URI value ends up being http://localhost/1118365//1118365/enter_bug.cgi which expectedly fails with a state error.

::: extensions/GitHubAuth/lib/Client.pm
@@ +78,5 @@
> +    $sorted_target->query_param_delete('github_email_key');
> +    $sorted_target->query_param_delete('github_email');
> +    $sorted_target->query_param_delete('GoAheadAndLogIn');
> +    $sorted_target->query_param_delete('github_login');
> +    warn "TARGET: $sorted_target\n";

not needed for production

	David Lawrence [:dkl]
	Comment 30 • 9 years ago

Comment on attachment 8586827 [details] [diff] [review]
bug-1118365-v8.patch

Same issues as v7 from comment 29 (minus the htaccess and images issues which are resolved in v8)

	Julien Vehent
	Comment 31 • 9 years ago

We reviewed risks with Dylan and Mark today, RRA notes are at https://docs.google.com/a/mozilla.com/spreadsheets/d/1JUBgSxGF2Cu47-Thb7PsfoT2bkYPcWTjZFSD--ydIO8/edit#gid=0
From a sec point of view, this is low risk enough to go ship without pentesting, but we're happy to test specific scenario if needed.

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 32 • 9 years ago

Note that I think the site_wide_secret is confused with the client_secret. The site wide secret is never shared with anything (although it is hashed with the redirect uri and the emails for verification purposes).

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 33 • 9 years ago

With better logic for figuring out the redirect url.

	David Lawrence [:dkl]
	Comment 34 • 9 years ago

Comment on attachment 8587456 [details] [diff] [review]
bug-1118365-v9.patch

Review of attachment 8587456 [details] [diff] [review]:
-----------------------------------------------------------------

Should be done after this change.

::: extensions/GitHubAuth/Extension.pm
@@ +31,5 @@
> +        return undef if $method eq 'fail_nodata';
> +        return $stack->UNIVERSAL::can($method);
> +    };
> +
> +    *Bugzilla::target_uri = sub {

As discussed in IRC, since this is not used anywhere outside of extension code (and isn't used in templates), let's move it to a Util.pm package.
Make sure to add some comments that describe why this workaround is necessary.

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 35 • 9 years ago

With 50% less monkey patching

	David Lawrence [:dkl]
	Comment 36 • 9 years ago

Comment on attachment 8587604 [details] [diff] [review]
bug-1118365-v10.patch

Review of attachment 8587604 [details] [diff] [review]:
-----------------------------------------------------------------

Let's Party! r=dkl

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 37 • 9 years ago

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   1317be9..c0d00a5  master -> master

Live on dev, no fires yet. We'll need to populate the no-github-auth group before enabling this.