auth: coding style and glitches fixes for GitHub login source

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]

models/login_source.go

@@ -4,6 +4,7 @@
 // This source code is licensed under the MIT license found in the
 // LICENSE file in the root directory of this source tree.
 
+// FIXME: Put this file into its own package and separate into different files based on login sources.
 package models
 
 import (
@@ -64,7 +65,7 @@ var (
 	_ core.Conversion = &LDAPConfig{}
 	_ core.Conversion = &SMTPConfig{}
 	_ core.Conversion = &PAMConfig{}
-	_ core.Conversion = &GITHUBConfig{}
+	_ core.Conversion = &GitHubConfig{}
 )
 
 type LDAPConfig struct {
@@ -79,15 +80,15 @@ func (cfg *LDAPConfig) 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)
 }
 
@@ -193,7 +194,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))
 		}
@@ -229,7 +230,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
 }
 
@@ -273,8 +274,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 {
@@ -521,7 +522,7 @@ func LoadAuthSources() {
 			loginSource.Cfg = &PAMConfig{}
 		case "github":
 			loginSource.Type = LOGIN_GITHUB
-			loginSource.Cfg = &GITHUBConfig{}
+			loginSource.Cfg = &GitHubConfig{}
 		default:
 			raven.CaptureErrorAndWait(err, nil)
 			log.Fatal(2, "Failed to load authentication source: unknown type '%s'", authType)
@@ -742,10 +743,10 @@ 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)
+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
@@ -756,7 +757,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,
@@ -783,7 +784,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

@@ -344,7 +344,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

@@ -9,7 +9,6 @@ package github
 import (
 	"context"
 	"crypto/tls"
-	"errors"
 	"fmt"
 	"net/http"
 	"strings"
@@ -17,35 +16,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

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

routes/admin/auths.go

@@ -13,6 +13,7 @@ import (
 	"gitote/gitote/pkg/context"
 	"gitote/gitote/pkg/form"
 	"gitote/gitote/pkg/setting"
+	"net/http"
 	"strings"
 
 	"github.com/Unknwon/com"
@@ -150,11 +151,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
@@ -172,7 +173,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)
@@ -238,15 +239,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/admin/auth/edit.tmpl

@@ -167,12 +167,12 @@
 								<input id="pam_service_name" name="pam_service_name" value="{{$cfg.ServiceName}}" required>
 							</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}}">

templates/admin/auth/new.tmpl

@@ -158,10 +158,10 @@
 							<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">

templates/user/auth/login.tmpl

@@ -22,7 +22,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>
 								{{if .DefaultSource}}