Merge pull request #45 from luizfonseca/fix/team-check

fix: check for the correct variable holding GitHub team information

Author: luizfonseca

.gitignore

@@ -13,4 +13,5 @@
 
 # Dependency directories (remove the comment below to include it)
 # vendor/
-dist/*
+dist/*
+.envrc

README.md

@@ -50,7 +50,6 @@ providing a more secure way for users to access protected routes.
      --label 'traefik.http.middlewares.whoami-github-oauth.plugin.github-oauth.apiBaseUrl=http://traefik-github-oauth-server' \
      --label 'traefik.http.middlewares.whoami-github-oauth.plugin.github-oauth.whitelist.logins[0]=luizfonseca' \
      --label 'traefik.http.middlewares.whoami-github-oauth.plugin.github-oauth.whitelist.teams[0]=827726' \
-     --label 'traefik.http.middlewares.whoami-github-oauth.plugin.github-oauth.whitelist.twoFactorAuthRequired=true' \
      --label 'traefik.http.routers.whoami.rule=Host(`whoami.example.com`)' \
      --label 'traefik.http.routers.whoami.middlewares=whoami-github-oauth' \
     traefik/whoami
@@ -89,11 +88,6 @@ logLevel: info
 
 # whitelist
 whitelist:
-  # When set to `true`, the middleware will check if the given user has 2FA
-  # configured, otherwise they will be denied access
-  # Default is `false`
-  twoFactorAuthRequired: 'true'
-
   # The list of GitHub user ids that are whitelisted to access the resources
   ids:
     - 996

internal/app/traefik-github-oauth-server/app.go

@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"os"
 	"os/signal"
+	"strings"
 	"time"
 
 	"github.com/go-chi/chi/v5"
@@ -35,22 +36,6 @@ func NewApp(
 	authRequestManager *AuthRequestManager,
 	logger *zerolog.Logger,
 ) *App {
-	if config.DebugMode {
-		config.LogLevel = "DEBUG"
-	}
-
-	switch config.LogLevel {
-	case "DEBUG", "debug":
-		logger.Level(zerolog.DebugLevel)
-	case "INFO", "info":
-		logger.Level(zerolog.InfoLevel)
-	case "WARNING", "warning", "WARN", "warn":
-		logger.Level(zerolog.WarnLevel)
-	case "ERROR", "error":
-		logger.Level(zerolog.ErrorLevel)
-	default:
-		logger.Level(zerolog.InfoLevel)
-	}
 
 	server.Addr = config.ServerAddress
 	server.Handler = router
@@ -72,8 +57,31 @@ func NewApp(
 	return app
 }
 
+func stringToLogLevel(config *Config) zerolog.Level {
+	if config.DebugMode {
+		config.LogLevel = "DEBUG"
+	}
+
+	switch strings.ToLower(config.LogLevel) {
+	case "debug":
+		return zerolog.DebugLevel
+	case "info":
+		return zerolog.InfoLevel
+	case "warn":
+		return zerolog.WarnLevel
+	case "error":
+		return zerolog.ErrorLevel
+	case "fatal":
+		return zerolog.FatalLevel
+	default:
+		return zerolog.InfoLevel
+	}
+}
+
 func NewDefaultApp() *App {
-	logger := zerolog.New(os.Stdout).With().Str("service", "traefik-github-oauth").Timestamp().Logger()
+	config := NewConfigFromEnv()
+
+	logger := zerolog.New(os.Stdout).Level(stringToLogLevel(config)).With().Str("service", "traefik-github-oauth").Timestamp().Logger()
 
 	router := chi.NewRouter()
 
@@ -100,7 +108,6 @@ func NewDefaultApp() *App {
 	// Recoverer middleware recovers from panics and writes a 500 if there was one.
 	router.Use(middleware.Recoverer)
 
-	config := NewConfigFromEnv()
 	return NewApp(
 		config,
 		&http.Server{

internal/app/traefik-github-oauth-server/router/oauth.go

@@ -111,7 +111,7 @@ func OauthRedirectHandler(app *server.App) http.HandlerFunc {
 		authRequest.GitHubUserLogin = githubData.User.GetLogin()
 		authRequest.GithubUserTwoFactorAuth = githubData.User.GetTwoFactorAuthentication()
 
-		if authRequest.GithubTeamIDs != nil {
+		if githubData.Teams != nil {
 			var teamIDs []string
 			for _, team := range githubData.Teams {
 				teamIDs = append(teamIDs, cast.ToString(team.GetID()))
@@ -120,6 +120,8 @@ func OauthRedirectHandler(app *server.App) http.HandlerFunc {
 			authRequest.GithubTeamIDs = teamIDs
 		}
 
+		app.Logger.Debug().Interface("currentAuthRequest", authRequest).Msg("user auth request")
+
 		authURL, err := url.Parse(authRequest.AuthURL)
 		if err != nil {
 			app.Logger.Warn().
@@ -189,6 +191,7 @@ func oAuthCodeToUser(ctx context.Context, oAuthConfig *oauth2.Config, code strin
 	ctxExchange, cancelExchange := context.WithCancel(ctx)
 	defer cancelExchange()
 	token, err := oAuthConfig.Exchange(ctxExchange, code)
+
 	if err != nil {
 		return nil, err
 	}
@@ -201,7 +204,9 @@ func oAuthCodeToUser(ctx context.Context, oAuthConfig *oauth2.Config, code strin
 	// Get user information, login and ID
 	ctxGetUser, cancelGetUser := context.WithCancel(ctx)
 	defer cancelGetUser()
+
 	user, _, err := gitHubApiClient.Users.Get(ctxGetUser, "")
+
 	if err != nil {
 		return nil, err
 	}
@@ -210,6 +215,7 @@ func oAuthCodeToUser(ctx context.Context, oAuthConfig *oauth2.Config, code strin
 	// This won't cancel the main request
 	ctxTeams, cancelListTeams := context.WithCancel(ctx)
 	teams, _, err := gitHubApiClient.Teams.ListUserTeams(ctxTeams, &github.ListOptions{PerPage: 100})
+
 	defer cancelListTeams()
 	if err != nil {
 		// If the user is not a member of any teams, the API will return a 404

internal/pkg/jwt/jwt.go

@@ -7,18 +7,16 @@ import (
 )
 
 type PayloadUser struct {
-	Id               string   `json:"id"`
-	Login            string   `json:"login"`
-	Teams            []string `json:"teams"`
-	TwoFactorEnabled bool     `json:"two_factor_enabled"`
+	Id    string   `json:"id"`
+	Login string   `json:"login"`
+	Teams []string `json:"teams"`
 }
 
-func GenerateJwtTokenString(id string, login string, teamIds []string, key string, two_factor_enabled bool) (string, error) {
+func GenerateJwtTokenString(id string, login string, teamIds []string, key string) (string, error) {
 	token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.MapClaims{
-		"id":                 id,
-		"login":              login,
-		"teams":              teamIds,
-		"two_factor_enabled": two_factor_enabled,
+		"id":    id,
+		"login": login,
+		"teams": teamIds,
 	})
 	return token.SignedString([]byte(key))
 }
@@ -51,18 +49,10 @@ func ParseTokenString(tokenString, key string) (*PayloadUser, error) {
 			}
 		}
 
-		twoFactorEnabled := false
-		if claims["two_factor_enabled"] != nil {
-			if factorEnabled, ok := claims["two_factor_enabled"].(bool); ok {
-				twoFactorEnabled = factorEnabled
-			}
-		}
-
 		return &PayloadUser{
-			Id:               claims["id"].(string),
-			Login:            claims["login"].(string),
-			Teams:            teams,
-			TwoFactorEnabled: twoFactorEnabled,
+			Id:    claims["id"].(string),
+			Login: claims["login"].(string),
+			Teams: teams,
 		}, nil
 	} else {
 		return nil, fmt.Errorf("invalid token")

internal/pkg/jwt/jwt_test.go

@@ -16,7 +16,7 @@ const (
 
 func TestGenerateJwtTokenString(t *testing.T) {
 	// execution
-	tokenString, err := GenerateJwtTokenString(id, login, testTeams, key, false)
+	tokenString, err := GenerateJwtTokenString(id, login, testTeams, key)
 
 	// assertion
 	assert.NoError(t, err)
@@ -25,7 +25,7 @@ func TestGenerateJwtTokenString(t *testing.T) {
 
 func TestParseTokenString(t *testing.T) {
 	// setup
-	tokenString, _ := GenerateJwtTokenString(id, login, testTeams, key, false)
+	tokenString, _ := GenerateJwtTokenString(id, login, testTeams, key)
 
 	// execution
 	payload, err := ParseTokenString(tokenString, key)
@@ -35,12 +35,11 @@ func TestParseTokenString(t *testing.T) {
 	assert.Equal(t, id, payload.Id)
 	assert.Equal(t, login, payload.Login)
 	assert.Equal(t, testTeams, payload.Teams)
-	assert.False(t, payload.TwoFactorEnabled)
 }
 
 func TestParseTokenString_EmptyTeams(t *testing.T) {
 	// setup
-	tokenString, _ := GenerateJwtTokenString(id, login, []string{}, key, false)
+	tokenString, _ := GenerateJwtTokenString(id, login, []string{}, key)
 
 	// execution
 	payload, err := ParseTokenString(tokenString, key)
@@ -50,12 +49,11 @@ func TestParseTokenString_EmptyTeams(t *testing.T) {
 	assert.Equal(t, id, payload.Id)
 	assert.Equal(t, login, payload.Login)
 	assert.Equal(t, payload.Teams, []string{})
-	assert.False(t, payload.TwoFactorEnabled)
 }
 
 func TestParseTokenString_NoTeams(t *testing.T) {
 	// setup
-	tokenString, _ := GenerateJwtTokenString(id, login, nil, key, false)
+	tokenString, _ := GenerateJwtTokenString(id, login, nil, key)
 
 	// execution
 	payload, err := ParseTokenString(tokenString, key)
@@ -65,12 +63,11 @@ func TestParseTokenString_NoTeams(t *testing.T) {
 	assert.Equal(t, id, payload.Id)
 	assert.Equal(t, login, payload.Login)
 	assert.Equal(t, payload.Teams, []string{})
-	assert.False(t, payload.TwoFactorEnabled)
 }
 
 func TestParseTokenString_With2FAEnabled(t *testing.T) {
 	// setup
-	tokenString, _ := GenerateJwtTokenString(id, login, nil, key, true)
+	tokenString, _ := GenerateJwtTokenString(id, login, nil, key)
 
 	// execution
 	payload, err := ParseTokenString(tokenString, key)
@@ -80,7 +77,6 @@ func TestParseTokenString_With2FAEnabled(t *testing.T) {
 	assert.Equal(t, id, payload.Id)
 	assert.Equal(t, login, payload.Login)
 	assert.Equal(t, payload.Teams, []string{})
-	assert.True(t, payload.TwoFactorEnabled)
 }
 
 func TestParseTokenString_InvalidToken(t *testing.T) {
@@ -97,7 +93,7 @@ func TestParseTokenString_InvalidToken(t *testing.T) {
 
 func TestParseTokenString_InvalidKey(t *testing.T) {
 	// setup
-	tokenString, _ := GenerateJwtTokenString(id, login, testTeams, key, false)
+	tokenString, _ := GenerateJwtTokenString(id, login, testTeams, key)
 	invalidKey := "invalidkey"
 
 	// execution

middleware_plugin.go

@@ -139,11 +139,11 @@ func (middleware *TraefikGithubOauthMiddleware) handleRequest(rw http.ResponseWr
 	}
 
 	// Early check for 2FA -- if user is not whitelisted and 2FA is required, return 401
-	if middleware.whitelistRequires2FA && !user.TwoFactorEnabled {
-		setNoCacheHeaders(rw)
-		http.Error(rw, "", http.StatusUnauthorized)
-		return
-	}
+	// if middleware.whitelistRequires2FA && !user.TwoFactorEnabled {
+	// 	setNoCacheHeaders(rw)
+	// 	http.Error(rw, "", http.StatusUnauthorized)
+	// 	return
+	// }
 
 	// If cookie is present, check if user is whitelisted
 	// If nothing can be found, returns 404 as we don't want to leak information
@@ -152,7 +152,7 @@ func (middleware *TraefikGithubOauthMiddleware) handleRequest(rw http.ResponseWr
 	if !middleware.whitelistIdSet.Has(user.Id) &&
 		!middleware.whitelistLoginSet.Has(user.Login) && !middleware.whitelistTeamSet.HasAny(user.Teams...) {
 		setNoCacheHeaders(rw)
-		http.Error(rw, "", http.StatusNotFound)
+		http.Error(rw, "", http.StatusUnauthorized)
 		return
 	}
 
@@ -177,7 +177,6 @@ func (p TraefikGithubOauthMiddleware) handleAuthRequest(rw http.ResponseWriter,
 		result.GitHubUserLogin,
 		result.GithubTeamIDs,
 		p.jwtSecretKey,
-		p.whitelistRequires2FA,
 	)
 	if err != nil {
 		p.logger.Printf("Failed to generate JWT: %s", err.Error())