auth: coding style and glitches fixes for GitHub login source (#5340)

conf/auth.d/github.conf.example

@@ -1,8 +1,8 @@
 # This is an example of GitHub authentication
 #
-id           = 106
+id           = 105
 type         = github
-name         = GitHub OAuth 2.0
+name         = GitHub
 is_activated = true
 
 [config]

gogs.go

@@ -16,7 +16,7 @@ import (
 	"github.com/gogs/gogs/pkg/setting"
 )
 
-const APP_VER = "0.11.82.1218"
+const APP_VER = "0.11.83.1218"
 
 func init() {
 	setting.AppVer = APP_VER

models/login_source.go

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a MIT-style
 // license that can be found in the LICENSE file.
 
+// FIXME: Put this file into its own package and separate into different files based on login sources.
 package models
 
 import (
@@ -62,7 +63,7 @@ var (
 	_ core.Conversion = &LDAPConfig{}
 	_ core.Conversion = &SMTPConfig{}
 	_ core.Conversion = &PAMConfig{}
-	_ core.Conversion = &GITHUBConfig{}
+	_ core.Conversion = &GitHubConfig{}
 )
 
 type LDAPConfig struct {
@@ -110,15 +111,15 @@ func (cfg *PAMConfig) ToDB() ([]byte, error) {
 	return jsoniter.Marshal(cfg)
 }
 
-type GITHUBConfig struct {
-	ApiEndpoint string // Github service (e.g. https://github.com/api/v1/)
+type GitHubConfig struct {
+	APIEndpoint string // GitHub service (e.g. https://api.github.com/)
 }
 
-func (cfg *GITHUBConfig) FromDB(bs []byte) error {
+func (cfg *GitHubConfig) FromDB(bs []byte) error {
 	return jsoniter.Unmarshal(bs, &cfg)
 }
 
-func (cfg *GITHUBConfig) ToDB() ([]byte, error) {
+func (cfg *GitHubConfig) ToDB() ([]byte, error) {
 	return jsoniter.Marshal(cfg)
 }
 
@@ -191,7 +192,7 @@ func (s *LoginSource) BeforeSet(colName string, val xorm.Cell) {
 		case LOGIN_PAM:
 			s.Cfg = new(PAMConfig)
 		case LOGIN_GITHUB:
-			s.Cfg = new(GITHUBConfig)
+			s.Cfg = new(GitHubConfig)
 		default:
 			panic("unrecognized login source type: " + com.ToStr(*val))
 		}
@@ -227,7 +228,7 @@ func (s *LoginSource) IsPAM() bool {
 	return s.Type == LOGIN_PAM
 }
 
-func (s *LoginSource) IsGITHUB() bool {
+func (s *LoginSource) IsGitHub() bool {
 	return s.Type == LOGIN_GITHUB
 }
 
@@ -271,8 +272,8 @@ func (s *LoginSource) PAM() *PAMConfig {
 	return s.Cfg.(*PAMConfig)
 }
 
-func (s *LoginSource) GITHUB() *GITHUBConfig {
-	return s.Cfg.(*GITHUBConfig)
+func (s *LoginSource) GitHub() *GitHubConfig {
+	return s.Cfg.(*GitHubConfig)
 }
 
 func CreateLoginSource(source *LoginSource) error {
@@ -516,7 +517,7 @@ func LoadAuthSources() {
 			loginSource.Cfg = &PAMConfig{}
 		case "github":
 			loginSource.Type = LOGIN_GITHUB
-			loginSource.Cfg = &GITHUBConfig{}
+			loginSource.Cfg = &GitHubConfig{}
 		default:
 			log.Fatal(2, "Failed to load authentication source: unknown type '%s'", authType)
 		}
@@ -755,10 +756,18 @@ func LoginViaPAM(user *User, login, password string, sourceID int64, cfg *PAMCon
 	}
 	return user, CreateUser(user)
 }
-func LoginViaGITHUB(user *User, login, password string, sourceID int64, cfg *GITHUBConfig, autoRegister bool) (*User, error) {
-	login_id, fullname, email, url, location, err := github.GITHUBAuth(cfg.ApiEndpoint, login, password)
+
+//________.__  __     ___ ___      ___.
+///  _____/|__|/  |_  /   |   \ __ _\_ |__
+///   \  ___|  \   __\/    ~    \  |  \ __ \
+//\    \_\  \  ||  |  \    Y    /  |  / \_\ \
+//\______  /__||__|   \___|_  /|____/|___  /
+//\/                 \/           \/
+
+func LoginViaGitHub(user *User, login, password string, sourceID int64, cfg *GitHubConfig, autoRegister bool) (*User, error) {
+	fullname, email, url, location, err := github.Authenticate(cfg.APIEndpoint, login, password)
 	if err != nil {
-		if strings.Contains(err.Error(), "Authentication failure") {
+		if strings.Contains(err.Error(), "401") {
 			return nil, errors.UserNotExist{0, login}
 		}
 		return nil, err
@@ -769,7 +778,7 @@ func LoginViaGITHUB(user *User, login, password string, sourceID int64, cfg *GIT
 	}
 	user = &User{
 		LowerName:   strings.ToLower(login),
-		Name:        login_id,
+		Name:        login,
 		FullName:    fullname,
 		Email:       email,
 		Website:     url,
@@ -782,6 +791,7 @@ func LoginViaGITHUB(user *User, login, password string, sourceID int64, cfg *GIT
 	}
 	return user, CreateUser(user)
 }
+
 func remoteUserLogin(user *User, login, password string, source *LoginSource, autoRegister bool) (*User, error) {
 	if !source.IsActived {
 		return nil, errors.LoginSourceNotActivated{source.ID}
@@ -795,7 +805,7 @@ func remoteUserLogin(user *User, login, password string, source *LoginSource, au
 	case LOGIN_PAM:
 		return LoginViaPAM(user, login, password, source.ID, source.Cfg.(*PAMConfig), autoRegister)
 	case LOGIN_GITHUB:
-		return LoginViaGITHUB(user, login, password, source.ID, source.Cfg.(*GITHUBConfig), autoRegister)
+		return LoginViaGitHub(user, login, password, source.ID, source.Cfg.(*GitHubConfig), autoRegister)
 	}
 
 	return nil, errors.InvalidLoginSourceType{source.Type}

models/models.go

@@ -347,7 +347,7 @@ func ImportDatabase(dirPath string, verbose bool) (err error) {
 				case LOGIN_PAM:
 					bean.Cfg = new(PAMConfig)
 				case LOGIN_GITHUB:
-					bean.Cfg = new(GITHUBConfig)
+					bean.Cfg = new(GitHubConfig)
 				default:
 					return fmt.Errorf("unrecognized login source type:: %v", tp)
 				}

pkg/auth/github/github.go

@@ -1,9 +1,12 @@
+// Copyright 2018 The Gogs Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
 package github
 
 import (
 	"context"
 	"crypto/tls"
-	"errors"
 	"fmt"
 	"net/http"
 	"strings"
@@ -11,35 +14,37 @@ import (
 	"github.com/google/go-github/github"
 )
 
-func GITHUBAuth(apiEndpoint, userName, passwd string) (string, string, string, string, string, error) {
-	tr := &http.Transport{
-		TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
-	}
+func Authenticate(apiEndpoint, login, passwd string) (name string, email string, website string, location string, _ error) {
 	tp := github.BasicAuthTransport{
-		Username:  strings.TrimSpace(userName),
-		Password:  strings.TrimSpace(passwd),
-		Transport: tr,
+		Username: strings.TrimSpace(login),
+		Password: strings.TrimSpace(passwd),
+		Transport: &http.Transport{
+			TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
+		},
 	}
 	client, err := github.NewEnterpriseClient(apiEndpoint, apiEndpoint, tp.Client())
 	if err != nil {
-		return "", "", "", "", "", errors.New("Authentication failure: GitHub Api Endpoint can not be reached")
+		return "", "", "", "", fmt.Errorf("create new client: %v", err)
 	}
-	ctx := context.Background()
-	user, _, err := client.Users.Get(ctx, "")
-	if err != nil || user == nil {
-		fmt.Println(err)
-		msg := fmt.Sprintf("Authentication failure! Github Api Endpoint authticated failed! User %s", userName)
-		return "", "", "", "", "", errors.New(msg)
+	user, _, err := client.Users.Get(context.Background(), "")
+	if err != nil {
+		return "", "", "", "", fmt.Errorf("get user info: %v", err)
 	}
 
-	var website = ""
+	if user.Name != nil {
+		name = *user.Name
+	}
+	if user.Email != nil {
+		email = *user.Email
+	} else {
+		email = login + "+github@local"
+	}
 	if user.HTMLURL != nil {
 		website = strings.ToLower(*user.HTMLURL)
 	}
-	var location = ""
 	if user.Location != nil {
 		location = strings.ToUpper(*user.Location)
 	}
 
-	return *user.Login, *user.Name, *user.Email, website, location, nil
+	return name, email, website, location, nil
 }

pkg/form/auth.go

@@ -41,7 +41,7 @@ type Authentication struct {
 	TLS               bool
 	SkipVerify        bool
 	PAMServiceName    string
-	GithubApiEndpoint string `binding:Required;Url`
+	GitHubAPIEndpoint string `form:"github_api_endpoint" binding:"Url"`
 }
 
 func (f *Authentication) Validate(ctx *macaron.Context, errs binding.Errors) binding.Errors {

routes/admin/auths.go

@@ -6,17 +6,18 @@ package admin
 
 import (
 	"fmt"
-
+	"net/http"
 	"strings"
 
 	"github.com/Unknwon/com"
 	"github.com/go-xorm/core"
+	log "gopkg.in/clog.v1"
+
 	"github.com/gogs/gogs/models"
 	"github.com/gogs/gogs/pkg/auth/ldap"
 	"github.com/gogs/gogs/pkg/context"
 	"github.com/gogs/gogs/pkg/form"
 	"github.com/gogs/gogs/pkg/setting"
-	log "gopkg.in/clog.v1"
 )
 
 const (
@@ -141,11 +142,11 @@ func NewAuthSourcePost(c *context.Context, f form.Authentication) {
 			ServiceName: f.PAMServiceName,
 		}
 	case models.LOGIN_GITHUB:
-		config = &models.GITHUBConfig{
-			ApiEndpoint: f.GithubApiEndpoint,
+		config = &models.GitHubConfig{
+			APIEndpoint: strings.TrimSuffix(f.GitHubAPIEndpoint, "/") + "/",
 		}
 	default:
-		c.Error(400)
+		c.Error(http.StatusBadRequest)
 		return
 	}
 	c.Data["HasTLS"] = hasTLS
@@ -163,7 +164,7 @@ func NewAuthSourcePost(c *context.Context, f form.Authentication) {
 		Cfg:       config,
 	}); err != nil {
 		if models.IsErrLoginSourceAlreadyExist(err) {
-			c.Data["Err_Name"] = true
+			c.FormErr("Name")
 			c.RenderWithErr(c.Tr("admin.auths.login_source_exist", err.(models.ErrLoginSourceAlreadyExist).Name), AUTH_NEW, f)
 		} else {
 			c.ServerError("CreateSource", err)
@@ -227,15 +228,11 @@ func EditAuthSourcePost(c *context.Context, f form.Authentication) {
 			ServiceName: f.PAMServiceName,
 		}
 	case models.LOGIN_GITHUB:
-		var apiEndpoint = f.GithubApiEndpoint
-		if !strings.HasSuffix(apiEndpoint, "/") {
-			apiEndpoint += "/"
-		}
-		config = &models.GITHUBConfig{
-			ApiEndpoint: apiEndpoint,
+		config = &models.GitHubConfig{
+			APIEndpoint: strings.TrimSuffix(f.GitHubAPIEndpoint, "/") + "/",
 		}
 	default:
-		c.Error(400)
+		c.Error(http.StatusBadRequest)
 		return
 	}
 

templates/.VERSION

@@ -1 +1 @@
-0.11.82.1218
+0.11.83.1218

templates/admin/auth/edit.tmpl

@@ -168,14 +168,15 @@
 							</div>
 						{{end}}
 
-						<!-- GitHub OAuth 2.0 -->
-						{{if .Source.IsGITHUB}}
-							{{ $cfg:=.Source.GITHUB }}
+						<!-- GitHub -->
+						{{if .Source.IsGitHub}}
+							{{ $cfg:=.Source.GitHub }}
 							<div class="required field">
 								<label for="github_api_endpoint">{{.i18n.Tr "admin.auths.github_api_endpoint"}}</label>
-								<input id="github_api_endpoint" name="github_api_endpoint" value="{{$cfg.ApiEndpoint}}" placeholder="e.g. https://api.github.com" required>
+								<input id="github_api_endpoint" name="github_api_endpoint" value="{{$cfg.APIEndpoint}}" placeholder="e.g. https://api.github.com/" required>
 							</div>
 						{{end}}
+						
 						<div class="inline field {{if not .Source.IsSMTP}}hide{{end}}">
 							<div class="ui checkbox">
 								<label><strong>{{.i18n.Tr "admin.auths.enable_tls"}}</strong></label>

templates/admin/auth/list.tmpl

@@ -45,7 +45,7 @@
 						<tfoot class="full-width">
 							<tr>
 								<th></th>
-								<th colspan="6">
+								<th colspan="7">
 									<div class="ui right">
 										<a class="ui blue small button" href="{{AppSubURL}}/admin/auths/new">{{.i18n.Tr "admin.auths.new"}}</a>
 									</div>

templates/admin/auth/new.tmpl

@@ -157,11 +157,13 @@
 							<label for="pam_service_name">{{.i18n.Tr "admin.auths.pam_service_name"}}</label>
 							<input id="pam_service_name" name="pam_service_name" value="{{.pam_service_name}}" />
 						</div>
-						<!-- GitHub Oauth 2.0 -->
+
+						<!-- GitHub -->
 						<div class="github required field {{if not (eq .type 6)}}hide{{end}}">
 							<label for="github_api_endpoint">{{.i18n.Tr "admin.auths.github_api_endpoint"}}</label>
-							<input id="github_api_endpoint" name="github_api_endpoint" value="{{.github_api_endpoint}}" placeholder="e.g. https://api.github.com" />
+							<input id="github_api_endpoint" name="github_api_endpoint" value="{{.github_api_endpoint}}" placeholder="e.g. https://api.github.com/" />
 						</div>
+
 						<div class="ldap field">
 							<div class="ui checkbox">
 								<label><strong>{{.i18n.Tr "admin.auths.attributes_in_bind"}}</strong></label>

templates/user/auth/login.tmpl

@@ -19,7 +19,7 @@
 					</div>
 					{{if .LoginSources}}
 						<div class="required inline field {{if .Err_LoginSource}}error{{end}}">
-							<label for="password">{{.i18n.Tr "auth.auth_source"}}</label>
+							<label>{{.i18n.Tr "auth.auth_source"}}</label>
 							<div class="ui selection dropdown">
 								<input type="hidden" id="login_source" name="login_source" value="{{.login_source}}" required>
 								<span class="text">