Oauth2 consumer #679

willemvd · 2017-01-15T22:09:17Z

This is a WIP PR for issue #26
The OAuth2 login is configured as a new LoginSource, but with an alternative flow since this also involves callbacks from the OAuth2 provider.
For now the only implemented OAuth2 provider is GitHub
Outstanding points are the TODO's like fixing the login button to be only displayed when a OAuth2 provider is specified.

import cycle not allowed package main imports code.gitea.io/gitea/cmd imports code.gitea.io/gitea/models imports code.gitea.io/gitea/modules/auth/oauth2 imports code.gitea.io/gitea/modules/context imports code.gitea.io/gitea/models

…ck for each provider Only GitHub is implemented for now

# Conflicts: # public/js/index.js

bkcsoft

A couple of things. macaron.Context does not belong in models/, if it's in there that's a bug that we should not encourage 🙂

Otherwise AWESOME ‼️ 💯 🎉

bkcsoft · 2017-01-16T05:38:02Z

models/login_source.go

@@ -217,6 +247,8 @@ func (source *LoginSource) UseTLS() bool {
 		return source.LDAP().SecurityProtocol != ldap.SecurityProtocolUnencrypted
 	case LoginSMTP:
 		return source.SMTP().TLS
+	case LoginOAuth2:
+		return true


You need to verify this...

Not sure how to use this property correct if we are doing a call to the OAuth2 provider, suggestions?

bkcsoft · 2017-01-16T05:39:16Z

models/login_source.go

@@ -266,7 +303,7 @@ func CreateLoginSource(source *LoginSource) error {

 // LoginSources returns a slice of all login sources found in DB.
 func LoginSources() ([]*LoginSource, error) {
-	auths := make([]*LoginSource, 0, 5)
+	auths := make([]*LoginSource, 0, 6)


Can we maybe use len(LoginNames) instead of a hard-coded value?

this can be any value, since it will look for all the configured login sources (and so depends on the environment how many this will be)

bkcsoft · 2017-01-16T05:39:43Z

models/login_source.go

@@ -444,7 +481,7 @@ func LoginViaSMTP(user *User, login, password string, sourceID int64, cfg *SMTPC
 		idx := strings.Index(login, "@")
 		if idx == -1 {
 			return nil, ErrUserNotExist{0, login, 0}
-		} else if !com.IsSliceContainsStr(strings.Split(cfg.AllowedDomains, ","), login[idx+1:]) {
+		} else if !com.IsSliceContainsStr(strings.Split(cfg.AllowedDomains, ","), login[idx + 1:]) {


Is this done by gofmt? otherwise revert this line

gofmt did this

bkcsoft · 2017-01-16T05:40:42Z

models/login_source.go

+
+// LoginViaOAuth2 queries if login/password is valid against the OAuth2.0 provider,
+// and create a local user if success when enabled.
+func LoginViaOAuth2(cfg *OAuth2Config, ctx *macaron.Context, sess session.Store) {


macaron.Context does not belong in models/...

bkcsoft · 2017-01-16T05:40:51Z

models/login_source.go

 // ExternalUserLogin attempts a login using external source types.
-func ExternalUserLogin(user *User, login, password string, source *LoginSource, autoRegister bool) (*User, error) {
+func ExternalUserLogin(user *User, login, password string, source *LoginSource, autoRegister bool, ctx *macaron.Context, sess session.Store) (*User, error) {


same as above...

bkcsoft · 2017-01-16T05:41:41Z

models/login_source.go

 		}
 	}

-	sources := make([]*LoginSource, 0, 3)
+	sources := make([]*LoginSource, 0, 5)


len(...) - 1 ?

This will lookup all activated loginSources, so this cannot be determined up front

You added a single LoginSource but you incremented this number by 2, either there's a bug in current code (short count) or in your patch (long count) ?

see comment above, this can be any value because it is fetched from the database and so unpredictable, so what ever you want 😄

bkcsoft · 2017-01-16T05:42:58Z

modules/auth/oauth2/oauth2.go

+)
+
+func init() {
+	gothic.Store = sessions.NewFilesystemStore(os.TempDir(), []byte(sessionUsersStoreKey))


Gitea has a defined session-dir and a temp-dir :)

will use that!

Think you mean : setting.SessionConfig.ProviderConfig = "data/sessions" ?
But this seems only valid when the session provider is file and not for instance memory

bkcsoft · 2017-01-16T05:43:28Z

modules/auth/oauth2/oauth2.go

+	request := ctx.Req.Request
+	response := ctx.Resp
+
+	/*


Commented out code?

yes part that I still need to have a look at. Need to check if it is required to do custom storing to determine valid requests in the callback

bkcsoft · 2017-01-16T05:46:09Z

templates/admin/auth/edit.tmpl

+						<!-- OAuth2 -->
+						{{if .Source.IsOAuth2}}
+							{{ $cfg:=.Source.OAuth2 }}
+                            <div class="inline required field">


Indentation

will use proper tabs :)

bkcsoft · 2017-01-16T05:46:31Z

templates/admin/auth/new.tmpl

+						<!-- OAuth2 -->
+						<div class="oauth2 field {{if not (eq .type 6)}}hide{{end}}">
+							<div class="inline required field">
+                                <label>{{.i18n.Tr "admin.auths.oauth2_provider"}}</label>


Indentation

will use proper tabs

willemvd · 2017-01-16T08:04:48Z

macaron.Context is indeed a parameter that I added at some early stage, looked like the request was not properly filled in our own context when handling the callback. I will check if that is still needed

willemvd · 2017-01-16T10:00:18Z

@bkcsoft found out why I needed the macaron.Context instead of our own context:
can't load package: import cycle not allowed
package code.gitea.io/gitea
imports code.gitea.io/gitea/cmd
imports code.gitea.io/gitea/models
imports code.gitea.io/gitea/modules/auth/oauth2
imports code.gitea.io/gitea/modules/context
imports code.gitea.io/gitea/models

Maybe I can fix it by making a new oauth2 context in modules/auth/oauth2 and have the request and session in that as well?
Only that seems not a very nice and clean way....

willemvd · 2017-01-16T10:57:10Z

Changed the code a bit, so now the request, response and session are sent into the oauth2 module. This will prevent the import cycle and no macaron.Context is send around
Still maybe not the best way, but don't know a better way actually...

…tivated) remove the use of sessions in our own code for validating request

# Conflicts: # cmd/web.go

…lementing SkipVerify (waiting for markbates/goth#87)

willemvd · 2017-01-17T08:49:08Z

@bkcsoft resolved issues and did some cleanup of the code, could you please have another look (also on my comments on you previous review)

bkcsoft · 2017-01-20T10:12:45Z

@willemvd I'm still not exactly happy about passing around request/response either (essentially the same as passing around context...) what are they used? can't you just get what you need from them (variables) and pass that?

willemvd · 2017-01-20T11:36:28Z

It seems not totally right, I agree.. Point is that the goth library requires the original request in it's method signatures and uses it for redirecting the client to the OAuth2 provider (http.Redirect : https://github.com/markbates/goth/blob/master/gothic/gothic.go#L55)
So seems no way to work around this, other then overriding the goth library or use some other implementation (or do it our self, if all other fails)

bkcsoft

that should fix it

bkcsoft · 2017-01-21T02:36:58Z

models/login_source.go

+
+// LoginViaOAuth2 queries if login/password is valid against the OAuth2.0 provider,
+// and create a local user if success when enabled.
+func LoginViaOAuth2(cfg *OAuth2Config, request *http.Request, response http.ResponseWriter) {


Move this to modules to fix one request/response-issue

bkcsoft · 2017-01-21T02:37:26Z

models/login_source.go

@@ -596,3 +650,90 @@ func UserSignIn(username, password string) (*User, error) {

 	return nil, ErrUserNotExist{user.ID, user.Name, 0}
 }
+
+// OAuth2UserLogin attempts a login using a OAuth2 source type
+func OAuth2UserLogin(provider string, request *http.Request, response http.ResponseWriter) (*User, error) {


Same with this

bkcsoft · 2017-01-21T02:37:48Z

models/login_source.go

+
+// OAuth2UserLoginCallback attempts to handle the callback from the OAuth2 provider and if successful
+// login the user
+func OAuth2UserLoginCallback(provider string, request *http.Request, response http.ResponseWriter) (*User, string, error) {


willemvd · 2017-01-23T10:34:32Z

Changed it from WIP to final , please review 😄

bkcsoft · 2017-01-23T10:39:16Z

modules/auth/oauth2/oauth2.go

+)
+
+func init() {
+	gothic.Store = sessions.NewFilesystemStore(os.TempDir(), []byte(sessionUsersStoreKey))


Please prefix the directory with gitea- :)

changed it to use a new data/sessions/oauth2 folder based on the setting.WorkDir method

…data

strk · 2017-02-07T19:44:17Z

I just noticed another nasty consequence of having User.LoginType set to OAuth2: the User.IsLocal method would return false in that case, which would not allow updating a user's password. When registering a user with OAuth2, the form in this PR is still asking for a password, so a local password does exist, only cannot be used to be reset via forgot_password.

I keep thinking that User.LoginType should still only refer to the LoginSource to which the username/password pairs are to be sent, if anywhere (probably "LoginPlain" for these external-login-associated accounts, or "LoginNone" once it'll be supported)

… AccessToken is still valid instead of re-authenticating (prevent flooding the OAuth2 provider)

# Conflicts: # templates/user/auth/signup.tmpl

willemvd · 2017-02-08T08:13:00Z

Added the missed password reset and added an enhancement from goth where you can get the user if the OAuth2 AccessToken is still valid instead of re-authenticating (prevent flooding the OAuth2 provider with grant access requests)

willemvd · 2017-02-08T08:13:24Z

Also fixed a merge conflict

bkcsoft

I think a rebase/merge was messed up. The new migration has to go at the end.

bkcsoft · 2017-02-20T15:02:15Z

models/migrations/migrations.go

@@ -80,7 +80,9 @@ var migrations = []Migration{
 	NewMigration("create user column diff view style", createUserColumnDiffViewStyle),
 	// v15 -> v16
 	NewMigration("create user column allow create organization", createAllowCreateOrganizationColumn),
-	// v16
+	// V16 -> v17
+	NewMigration("create repo unit table and add units for all repos", addUnitsToTables),


This has to go at the end of the list

there was a new one added in master, mine is after this one
https://github.com/go-gitea/gitea/pull/679/files#diff-7366023de6e87b891be868e3b8cf68e6R85-R86

bkcsoft · 2017-02-20T15:02:49Z

models/migrations/v16.go

-// Copyright 2016 Gitea. All rights reserved.
-// Use of this source code is governed by a MIT-style
-// license that can be found in the LICENSE file.
-
 package migrations


Why was the copyright-header removed?

it seems to be missing in master (https://github.com/go-gitea/gitea/blob/master/models/migrations/v16.go) and merged removed this, mine is v17.go

Created #992 to fix this missing header in master

willemvd · 2017-02-20T15:12:50Z

@bkcsoft please look at my comments, code from master is "broken" so that's why the merge looks messed up but isn't

bkcsoft · 2017-02-20T15:48:57Z

When the changed are done you have my LGTM 😄

willemvd · 2017-02-20T20:25:01Z

@bkcsoft what do you mean?
migration.go change is because of a merge, mine is at the bottom of that file

For the missing copyright header in v16.go, I will create a new PR (since this is already wrong in the master)

Agree?

bkcsoft · 2017-02-21T15:44:46Z

@willemvd That explains it 😄 Fix the conflicts and you have my LGTM

strk · 2017-02-21T15:54:17Z

hint: rename v17 to v18 before rebasing again to master

# Conflicts: # models/migrations/migrations.go

…h2-consumer

bkcsoft · 2017-02-22T07:09:33Z

LGTM :)

bkcsoft · 2017-02-22T07:09:50Z

make L-G-T-M work!

willemvd added 3 commits January 10, 2017 14:38

initial stuff for oauth2 login, fails on:

ded3513

import cycle not allowed package main imports code.gitea.io/gitea/cmd imports code.gitea.io/gitea/models imports code.gitea.io/gitea/modules/auth/oauth2 imports code.gitea.io/gitea/modules/context imports code.gitea.io/gitea/models

login button on the signIn page to start the OAuth2 flow and a callba…

1f94c85

…ck for each provider Only GitHub is implemented for now

Merge branch 'upstream-master' into oauth2-consumer

642f36e

# Conflicts: # public/js/index.js

lunny added this to the 1.1.0 milestone Jan 16, 2017

lunny added type/feature Completely new functionality. Can only be merged if feature freeze is not active. pr/wip This PR is not ready for review labels Jan 16, 2017

bkcsoft suggested changes Jan 16, 2017

View reviewed changes

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

fix indentation

f93aad8

prevent macaron.Context in models

280913b

willemvd added 6 commits January 16, 2017 14:03

show login button only when the OAuth2 consumer is configured (and ac…

b597a86

…tivated) remove the use of sessions in our own code for validating request

move overrides of goth functions to init

8c2be7a

Merge remote-tracking branch 'upstream/master' into oauth2-consumer

f392ce9

# Conflicts: # cmd/web.go

create macaron group for oauth2 urls

2ba7833

fix the broken url (dubble oauth2)

8e1ea96

no support at this moment to use this and we should only consider imp…

6c98fa7

…lementing SkipVerify (waiting for markbates/goth#87)

bkcsoft approved these changes Jan 20, 2017

View reviewed changes

bkcsoft reviewed Jan 21, 2017

View reviewed changes

prevent net/http in modules (other then oauth2)

caeb911

willemvd changed the title ~~[WIP] Oauth2 consumer~~ Oauth2 consumer Jan 23, 2017

bkcsoft suggested changes Jan 23, 2017

View reviewed changes

lunny removed the pr/wip This PR is not ready for review label Jan 23, 2017

use a new data sessions oauth2 folder for storing the oauth2 session …

f3f3866

…data

tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Feb 7, 2017

willemvd added 4 commits February 8, 2017 08:47

fix missed password reset

0fa2e40

use updated goth lib that now supports getting the OAuth2 user if the…

f54f07a

… AccessToken is still valid instead of re-authenticating (prevent flooding the OAuth2 provider)

Merge branch 'upstream-master' into oauth2-consumer

873e5b7

# Conflicts: # templates/user/auth/signup.tmpl

manual merge the changes in signup.tmpl to signup_inner.tmpl

779e84b

bkcsoft suggested changes Feb 20, 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 Feb 20, 2017

willemvd mentioned this pull request Feb 20, 2017

add missing copyright header #992

Merged

bkcsoft approved these changes Feb 21, 2017

View reviewed changes

willemvd added 3 commits February 21, 2017 23:28

prepare merge

61fe261

Merge branch 'upstream-master' into oauth2-consumer

66e28df

# Conflicts: # models/migrations/migrations.go

Merge branch 'oauth2-consumer' of github.com:willemvd/gitea into oaut…

2c11c44

…h2-consumer

bkcsoft merged commit 01d9576 into go-gitea:master Feb 22, 2017

lunny mentioned this pull request Feb 22, 2017

Integrate an OAuth2 consumer #26

Closed

willemvd mentioned this pull request Feb 22, 2017

Additional OAuth2 providers #1010

Merged

This was referenced Feb 22, 2017

500 error on github OAuth2 login #1012

Closed

Determine desired password-handling behavior for ExternalLoginUsers #1036

Open

sapk mentioned this pull request Mar 16, 2017

Fix missing less sources for oauth #1288

Merged

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

Oauth2 consumer #679

Oauth2 consumer #679

Conversation

willemvd commented Jan 15, 2017

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

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

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

willemvd commented Jan 16, 2017

willemvd commented Jan 16, 2017

willemvd commented Jan 16, 2017

willemvd commented Jan 17, 2017

bkcsoft commented Jan 20, 2017

willemvd commented Jan 20, 2017

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

willemvd commented Jan 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strk commented Feb 7, 2017

willemvd commented Feb 8, 2017

willemvd commented Feb 8, 2017

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

Choose a reason for hiding this comment

willemvd commented Feb 20, 2017

bkcsoft commented Feb 20, 2017

willemvd commented Feb 20, 2017

bkcsoft commented Feb 21, 2017

strk commented Feb 21, 2017

bkcsoft commented Feb 22, 2017

bkcsoft commented Feb 22, 2017