Additional OAuth2 providers #1010

willemvd · 2017-02-22T12:41:42Z

This is a followup on #679 (this PR is based on the merged #679 master) which only included GitHub as an OAuth2 provider.

For now the following providers are implemented (incl signin image and tip text in admin/auths/new):
Google+
GitLab
Bitbucket
Twitter
Facebook
Dropbox
OpenID Connect

For GitLab and Github it is now possible to change the default urls to custom urls, this means you can connect them to your own private/local/on-premise/enterprise installation

I will add and test new providers on request in this PR (or on gitter?), list of possible providers can be found at : https://github.com/markbates/goth/#supported-providers

lunny · 2017-02-22T13:18:52Z

build failed.

willemvd · 2017-02-22T14:20:05Z

fixed

appleboy · 2017-02-22T14:24:54Z

models/login_source.go

@@ -23,6 +23,7 @@ import (
 	"code.gitea.io/gitea/modules/auth/pam"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/auth/oauth2"
+	"sort"


Move local package to top

done, changed to goimports style

appleboy · 2017-02-22T14:26:12Z

modules/auth/oauth2/oauth2.go

+	"github.com/markbates/goth/providers/facebook"
+	"github.com/markbates/goth/providers/dropbox"
+	"github.com/markbates/goth/providers/openidConnect"
+	"math"


See https://github.com/go-gitea/gitea/blob/master/CONTRIBUTING.md#styleguide

done, changed to goimports style

appleboy · 2017-02-22T14:41:27Z

@willemvd build fails

willemvd · 2017-02-22T14:42:32Z

Build failure over a file I didn't change and cannot be found at that location? 😕
modules/helper.go -> golint failure but file does not exist

appleboy · 2017-02-23T01:14:05Z

@willemvd Please rebase the latest version of master. See #1017

willemvd · 2017-02-23T07:43:13Z

Not sure what happend , but rebase seems to have messed up my PR
See duplicate commits in my history 😕
Not sure what will happen when this gets merged... maybe create a new PR ?

willemvd · 2017-02-23T07:54:15Z

@lunny @appleboy any idea ? (see my previous comment) & when I do a merge locally on master with this PR , history will be messed up with duplicate entries

lunny · 2017-02-23T08:22:12Z

git fetch origin
git rebase master
... resolve the conflict
git add <conflict file>
git rebase --continue
git push --force <your repo and branch>

willemvd · 2017-02-23T08:28:50Z

Will that work even in this broken state?

… same

…New Source"

…hile building cross platform (and disk overflow)

willemvd · 2017-02-23T08:46:23Z

@lunny created a new branch of upstream/master and cherry-picked the commits on top of it, after git push --force it seems to be ok now 😄

strk · 2017-02-23T19:32:49Z

models/error.go

+//  /    |    \/    |    \    |  /  |    | \    Y    /
+//  \_______  /\____|__  /______/   |____|  \___|_  /
+//          \/         \/                         \/
+


Maybe you could take the chance to start with splitting error.go into error_oauth.go ...

done , also moved some stuff from login_source.go to oauth2.go

…ly these could have a different host)

pgaskin · 2017-03-17T14:04:35Z

models/login_source.go

 	_, err := x.Id(source.ID).AllCols().Update(source)
-	if err == nil && source.IsOAuth2() {
+	if err == nil && source.IsOAuth2() && source.IsActived {


Spelling: source.IsActive , this needs to be renamed.

This is reuse of existing code, so I think out of scope for this PR

Not only that, it's also in a DB model so changing that field would require a migration :/

pgaskin · 2017-03-17T14:05:25Z

models/oauth2.go

+	},
+	"gplus":         {Name: "gplus", DisplayName: "Google+", Image: "/img/auth/google_plus.png"},
+	"openidConnect": {Name: "openidConnect", DisplayName: "OpenID Connect", Image: "/img/auth/openid_connect.png"},
+	"twitter":       {Name: "twitter", DisplayName: "Twitter", Image: "/img/auth/twitter.png"},


Maybe add stack exchange as well?

Think this is not yet available in https://github.com/markbates/goth/#supported-providers or is it named differently?

for the record; stackexchange (also?) supports OpenID-2.0, and you can use openid.stackexchange.com to login (will be converted to actual URL upon logging in)

pgaskin · 2017-03-17T14:06:51Z

templates/admin/auth/new.tmpl

+					{{.i18n.Tr "admin.auths.tips"}}
+				</h4>
+				<div class="ui attached segment">
+					<h5>GMail Settings:</h5>


Gmail, not GMail. Maybe also add this to translations?

This is reuse of existing code, so I think out of scope for this PR

pgaskin · 2017-03-17T14:07:22Z

templates/admin/auth/new.tmpl

+						<span>{{.i18n.Tr "admin.auths.tip.twitter"}}</span>
+					</div>
+				</div>
+			</div>


Is there a way to make this automated (maybe a loop)?

Will have a look if this can be a loop over the existing OAuth2 providers

strk · 2017-03-20T11:32:19Z

It would also be nice to get #1300 merged before this. @geek1011 can you resolve conflicts there ?

# Conflicts: # routers/user/auth.go # templates/user/auth/signin_inner.tmpl

lunny · 2017-04-06T01:55:41Z

@willemvd is this ready to review?

willemvd · 2017-04-06T09:09:41Z

@lunny yes it is, unfortunately the last push didn't triggered a drone build so maybe you can manually trigger this (or I could do a merge/rebase with master)

lunny · 2017-04-06T12:56:57Z

@willemvd please send a rebase, I cannot find the link to the drone build.

willemvd · 2017-04-06T14:10:44Z

@lunny drone is almost done without failures, so it is ready! 😄

lunny · 2017-04-07T15:10:31Z

I have tested google+ and twitter, both are OK. but /user/link_account UI is broken. Otherwise LGTM

bkcsoft · 2017-04-19T11:33:44Z

Are we allowed to use these logos? Why not use the ones in FontAwesome?

(Edit: I know for a fact that GitLab is missing from FA...)

bkcsoft

formatting

bkcsoft · 2017-04-19T11:36:59Z

routers/admin/auths.go

 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
+	"github.com/Unknwon/com"


Newline before external packages

bkcsoft · 2017-04-19T11:37:39Z

templates/admin/auth/edit.tmpl

-								{{range $key, $value := .OAuth2Providers}}
-									<div class="item" data-value="{{$key}}">{{$value.DisplayName}}</div>
-								{{end}}
+						<!-- OAuth2 -->


Weird indentation

sapk · 2017-04-28T14:42:52Z

Build seems to failed because the vendor github.com/mrjones/oauth doesn't correspond to remote. If you make change in the lib please revert them. A simple command to revert to original version is govendor fetch github.com/mrjones/oauth. This will also update the lib but, in this case, since it is not allready in repo there is no problems.

willemvd · 2017-05-01T06:10:18Z

Didnt change anything to that lib so not sure why this happened, but I have updated the lib

appleboy · 2017-05-01T13:24:09Z

LGTM

lunny added this to the 1.2.0 milestone Feb 22, 2017

lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Feb 22, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 22, 2017

appleboy reviewed Feb 22, 2017

View reviewed changes

willemvd added 11 commits February 23, 2017 09:42

add google+

b98777f

sort signin oauth2 providers based on the name so order is always the…

a24e238

… same

update auth tip for google+

02638f0

add gitlab provider

6c48e16

add bitbucket provider (and some go fmt)

6a5b0c3

add twitter provider

213e67b

add facebook provider

92aec11

add dropbox provider

9e797b2

add openid connect provider incl. new format of tips section in "Add …

107eeea

…New Source"

lower the amount of disk storage for each session to prevent issues w…

94c8a48

…hile building cross platform (and disk overflow)

imports according to goimport and code style

8b8b5f4

willemvd force-pushed the additional-oauth2-providers branch from acc2097 to 8b8b5f4 Compare February 23, 2017 08:44

strk reviewed Feb 23, 2017

View reviewed changes

morrildl mentioned this pull request Feb 23, 2017

Determine desired password-handling behavior for ExternalLoginUsers #1036

Open

make it possible to set custom urls to gitlab and github provider (on…

e2544ae

…ly these could have a different host)

pgaskin reviewed Mar 17, 2017

View reviewed changes

willemvd added 2 commits March 28, 2017 10:51

Merge branch 'upstream-master' into additional-oauth2-providers

33ad313

# Conflicts: # routers/user/auth.go # templates/user/auth/signin_inner.tmpl

fix indention

92e802d

Merge branch 'upstream-master' into additional-oauth2-providers

7c63b03

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 7, 2017

lunny mentioned this pull request Apr 8, 2017

Better oauth2 button and make horizontal navbar scroll on overflow #1300

Closed

bkcsoft suggested changes Apr 19, 2017

View reviewed changes

willemvd added 4 commits April 28, 2017 11:33

fix indentation

5f0067e

fix new line before external import

922c5d4

fix layout of signin part

e18dfe5

Merge branch 'upstream-master' into additional-oauth2-providers

2342225

bkcsoft approved these changes Apr 28, 2017

View reviewed changes

update "broken" dependency

fe450e2

appleboy approved these changes May 1, 2017

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 1, 2017

lunny merged commit 950f2e2 into go-gitea:master May 1, 2017

strk mentioned this pull request Jan 3, 2018

Wrong method to fetch token for openID connect #2421

Closed

7 tasks

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Additional OAuth2 providers #1010

Additional OAuth2 providers #1010

Conversation

willemvd commented Feb 22, 2017 • edited Loading

lunny commented Feb 22, 2017

willemvd commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appleboy commented Feb 22, 2017

willemvd commented Feb 22, 2017

appleboy commented Feb 23, 2017

willemvd commented Feb 23, 2017

willemvd commented Feb 23, 2017

lunny commented Feb 23, 2017 • edited Loading

willemvd commented Feb 23, 2017

willemvd commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strk commented Mar 20, 2017

lunny commented Apr 6, 2017

willemvd commented Apr 6, 2017

lunny commented Apr 6, 2017

willemvd commented Apr 6, 2017

lunny commented Apr 7, 2017

bkcsoft commented Apr 19, 2017 • edited Loading

bkcsoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapk commented Apr 28, 2017

willemvd commented May 1, 2017

appleboy commented May 1, 2017

willemvd commented Feb 22, 2017 •

edited

Loading

lunny commented Feb 23, 2017 •

edited

Loading

bkcsoft commented Apr 19, 2017 •

edited

Loading