From b5d38f59f78891625e46098ba47b393c66dbae50 Mon Sep 17 00:00:00 2001
From: STeve Huang <xin.huang@goteleport.com>
Date: Sun, 8 Dec 2024 20:59:02 -0500
Subject: [PATCH] review comments round1

---
 api/proto/teleport/legacy/types/types.proto |  3 +-
 api/types/github_test.go                    | 93 +++++++++++++++++++++
 api/types/types.pb.go                       |  3 +-
 lib/auth/auth.go                            |  1 +
 4 files changed, 98 insertions(+), 2 deletions(-)
 create mode 100644 api/types/github_test.go

diff --git a/api/proto/teleport/legacy/types/types.proto b/api/proto/teleport/legacy/types/types.proto
index 78f5f3bcd8308..642e59597b825 100644
--- a/api/proto/teleport/legacy/types/types.proto
+++ b/api/proto/teleport/legacy/types/types.proto
@@ -5455,7 +5455,8 @@ message GithubClaims {
 
   // UserID is a global unique integer that is assigned to each GitHub user. The
   // user ID is immutable (unlike the GitHub username) and can be found in APIs
-  // like  get user.
+  // like get user.
+  // https://docs.github.com/en/rest/users/users
   string UserID = 4 [(gogoproto.jsontag) = "user_id,omitempty"];
 }
 
diff --git a/api/types/github_test.go b/api/types/github_test.go
new file mode 100644
index 0000000000000..4aceeadc25c23
--- /dev/null
+++ b/api/types/github_test.go
@@ -0,0 +1,93 @@
+// Copyright 2024 Gravitational, Inc
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package types
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestGithubAuthRequestCheck(t *testing.T) {
+	tests := []struct {
+		request *GithubAuthRequest
+		check   require.ErrorAssertionFunc
+	}{
+		{
+			request: &GithubAuthRequest{
+				ConnectorID: "valid",
+				StateToken:  "state-token",
+			},
+			check: require.NoError,
+		},
+		{
+			request: &GithubAuthRequest{
+				ConnectorID:   "invalid-connector-spec-set-for-regular-flow",
+				StateToken:    "state-token",
+				ConnectorSpec: &GithubConnectorSpecV3{},
+			},
+			check: require.Error,
+		},
+		{
+			request: &GithubAuthRequest{
+				ConnectorID:   "sso-test",
+				StateToken:    "state-token",
+				SSOTestFlow:   true,
+				ConnectorSpec: &GithubConnectorSpecV3{},
+			},
+			check: require.NoError,
+		},
+		{
+			request: &GithubAuthRequest{
+				ConnectorID: "connector-spec-missing-for-sso-test",
+				StateToken:  "state-token",
+				SSOTestFlow: true,
+			},
+			check: require.Error,
+		},
+		{
+			request: &GithubAuthRequest{
+				ConnectorID:       "authenticated-user",
+				StateToken:        "state-token",
+				AuthenticatedUser: "alice",
+				ConnectorSpec:     &GithubConnectorSpecV3{},
+			},
+			check: require.NoError,
+		},
+		{
+			request: &GithubAuthRequest{
+				ConnectorID:       "connector-spec-missing-for-authenticated-user",
+				StateToken:        "state-token",
+				AuthenticatedUser: "alice",
+			},
+			check: require.Error,
+		},
+		{
+			request: &GithubAuthRequest{
+				ConnectorID:  "both-new-and-deprecated-keys-are-set",
+				StateToken:   "state-token",
+				PublicKey:    []byte("deprecated"),
+				SshPublicKey: []byte("ssh-key"),
+				TlsPublicKey: []byte("tls-key"),
+			},
+			check: require.Error,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.request.ConnectorID, func(t *testing.T) {
+			test.check(t, test.request.Check())
+		})
+	}
+}
diff --git a/api/types/types.pb.go b/api/types/types.pb.go
index f84337830e8d3..eb150413722ef 100644
--- a/api/types/types.pb.go
+++ b/api/types/types.pb.go
@@ -13745,7 +13745,8 @@ type GithubClaims struct {
 	Teams []string `protobuf:"bytes,3,rep,name=Teams,proto3" json:"teams"`
 	// UserID is a global unique integer that is assigned to each GitHub user. The
 	// user ID is immutable (unlike the GitHub username) and can be found in APIs
-	// like  get user.
+	// like get user.
+	// https://docs.github.com/en/rest/users/users
 	UserID               string   `protobuf:"bytes,4,opt,name=UserID,proto3" json:"user_id,omitempty"`
 	XXX_NoUnkeyedLiteral struct{} `json:"-"`
 	XXX_unrecognized     []byte   `json:"-"`
diff --git a/lib/auth/auth.go b/lib/auth/auth.go
index 420f1364a1b9d..8b2c9a01d1640 100644
--- a/lib/auth/auth.go
+++ b/lib/auth/auth.go
@@ -3204,6 +3204,7 @@ func generateCert(ctx context.Context, a *Server, req certRequest, caType types.
 		return nil, trace.Wrap(err)
 	}
 
+	// At most one GitHub identity expected.
 	var githubUserID, githubUsername string
 	if githubIdentities := req.user.GetGithubIdentities(); len(githubIdentities) > 0 {
 		githubUserID = githubIdentities[0].UserID