Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Oauth2 consumer #679

Merged
merged 47 commits into from
Feb 22, 2017
Merged

Oauth2 consumer #679

merged 47 commits into from
Feb 22, 2017

Conversation

willemvd
Copy link
Contributor

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
@lunny lunny added this to the 1.1.0 milestone Jan 16, 2017
@lunny 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
Copy link
Member

@bkcsoft bkcsoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ‼️ 💯 🎉

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to verify this...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@@ -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:]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this done by gofmt? otherwise revert this line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gofmt did this


// 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above...

}
}

sources := make([]*LoginSource, 0, 3)
sources := make([]*LoginSource, 0, 5)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

len(...) - 1 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will use that!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

request := ctx.Req.Request
response := ctx.Resp

/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will use proper tabs :)

<!-- 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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will use proper tabs

@willemvd
Copy link
Contributor Author

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

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

@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
Copy link
Contributor Author

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...

@willemvd
Copy link
Contributor Author

@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
Copy link
Member

bkcsoft commented Jan 20, 2017

@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
Copy link
Contributor Author

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)

Copy link
Member

@bkcsoft bkcsoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that should fix it


// 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with this


// 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this

@willemvd willemvd changed the title [WIP] Oauth2 consumer Oauth2 consumer Jan 23, 2017
@willemvd
Copy link
Contributor Author

Changed it from WIP to final , please review 😄

)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please prefix the directory with gitea- :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lunny lunny removed the pr/wip This PR is not ready for review label Jan 23, 2017
@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Feb 7, 2017
@strk
Copy link
Member

strk commented Feb 7, 2017

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)

@willemvd
Copy link
Contributor Author

willemvd commented Feb 8, 2017

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
Copy link
Contributor Author

willemvd commented Feb 8, 2017

Also fixed a merge conflict

Copy link
Member

@bkcsoft bkcsoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has to go at the end of the list

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was the copyright-header removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #992 to fix this missing header in master

@willemvd
Copy link
Contributor Author

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

@bkcsoft
Copy link
Member

bkcsoft commented Feb 20, 2017

When the changed are done you have my LGTM 😄

@tboerger 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
Copy link
Contributor Author

@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
Copy link
Member

bkcsoft commented Feb 21, 2017

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

@strk
Copy link
Member

strk commented Feb 21, 2017

hint: rename v17 to v18 before rebasing again to master

@bkcsoft
Copy link
Member

bkcsoft commented Feb 22, 2017

LGTM :)

@bkcsoft
Copy link
Member

bkcsoft commented Feb 22, 2017

make L-G-T-M work!

@bkcsoft bkcsoft merged commit 01d9576 into go-gitea:master Feb 22, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants