finish up some test fixing

coder · sreya · Aug 17, 2022 · Aug 9, 2022 · Aug 9, 2022 · Aug 9, 2022
commit d940daedb37e4b26fc08c176cfe421329a07d031
diff --git a/coderd/audit/table.go b/coderd/audit/table.go
@@ -94,8 +94,6 @@ var AuditableResources = auditMap(map[any]map[string]Action{
 		"updated_at":      ActionIgnore, // Changes, but is implicit and not helpful in a diff.
 		"status":          ActionTrack,
 		"rbac_roles":      ActionTrack,
-		"login_type":      ActionTrack,
-		"linked_id":       ActionTrack,
 	},
 	&database.Workspace{}: {
 		"id":                 ActionTrack,

diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go
@@ -2278,17 +2278,18 @@ func (q *fakeQuerier) GetUserLinkByUserIDLoginType(_ context.Context, params dat
 	return database.UserLink{}, sql.ErrNoRows
 }
 
-func (q *fakeQuerier) InsertUserLink(_ context.Context, params database.InsertUserLinkParams) (database.UserLink, error) {
+func (q *fakeQuerier) InsertUserLink(_ context.Context, args database.InsertUserLinkParams) (database.UserLink, error) {
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()
 
+	//nolint:gosimple
 	link := database.UserLink{
-		UserID:            params.UserID,
-		LoginType:         params.LoginType,
-		LinkedID:          params.LinkedID,
-		OAuthAccessToken:  params.OAuthAccessToken,
-		OAuthRefreshToken: params.OAuthRefreshToken,
-		OAuthExpiry:       params.OAuthExpiry,
+		UserID:            args.UserID,
+		LoginType:         args.LoginType,
+		LinkedID:          args.LinkedID,
+		OAuthAccessToken:  args.OAuthAccessToken,
+		OAuthRefreshToken: args.OAuthRefreshToken,
+		OAuthExpiry:       args.OAuthExpiry,
 	}
 
 	q.userLinks = append(q.userLinks, link)

diff --git a/coderd/database/migrations/000034_linked_user_id.up.sql b/coderd/database/migrations/000034_linked_user_id.up.sql
@@ -10,6 +10,13 @@ CREATE TABLE IF NOT EXISTS user_links (
 	UNIQUE(user_id, login_type)
 );
 
+-- This migrates columns on api_keys to the new user_links table.
+-- It does this by finding all the API keys for each user, choosing
+-- the most recently updated for each one and then assigning its relevant
+-- values to the user_links table.
+-- A user should at most have a row for an OIDC account and a Github account.
+-- 'password' login types are ignored.
+
 INSERT INTO user_links 
 	( 
 		user_id,
@@ -34,6 +41,9 @@ FROM
 	) as keys
  WHERE x=1 AND keys.login_type != 'password';
 
+-- Drop columns that have been migrated to user_links.
+-- It appears the 'oauth_id_token' was unused and so it has
+-- been dropped here as well to avoid future confusion.
 ALTER TABLE api_keys 
 	DROP COLUMN oauth_access_token,
 	DROP COLUMN oauth_refresh_token,

diff --git a/coderd/database/postgres/postgres.go b/coderd/database/postgres/postgres.go
@@ -122,30 +122,27 @@ func Open() (string, func(), error) {
 		return "", nil, xerrors.Errorf("expire resource: %w", err)
 	}
 
-	pool.MaxWait = 15 * time.Second
-	var retryErr error
+	pool.MaxWait = 120 * time.Second
 	err = pool.Retry(func() error {
-		var db *sql.DB
-		db, retryErr := sql.Open("postgres", dbURL)
-		if retryErr != nil {
-			return xerrors.Errorf("open postgres: %w", retryErr)
+		db, err := sql.Open("postgres", dbURL)
+		if err != nil {
+			return xerrors.Errorf("open postgres: %w", err)
 		}
 		defer db.Close()
 
-		retryErr = db.Ping()
-		if retryErr != nil {
-			return xerrors.Errorf("ping postgres: %w", retryErr)
+		err = db.Ping()
+		if err != nil {
+			return xerrors.Errorf("ping postgres: %w", err)
 		}
-		retryErr = database.MigrateUp(db)
-		if retryErr != nil {
-			fmt.Printf("err: %v\n", retryErr)
-			return xerrors.Errorf("migrate db: %w", retryErr)
+		err = database.MigrateUp(db)
+		if err != nil {
+			return xerrors.Errorf("migrate db: %w", err)
 		}
 
 		return nil
 	})
 	if err != nil {
-		return "", nil, retryErr
+		return "", nil, err
 	}
 	return dbURL, func() {
 		_ = pool.Purge(resource)

diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go
@@ -159,7 +159,7 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs, redirectToLogin bool
 				if err != nil {
 					write(http.StatusInternalServerError, codersdk.Response{
 						Message: "A database error occurred",
-						Detail:  err.Error(),
+						Detail:  fmt.Sprintf("get user link by user ID and login type: %s", err.Error()),
 					})
 					return
 				}
@@ -250,6 +250,7 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs, redirectToLogin bool
 				if link.UserID != uuid.Nil {
 					link, err = db.UpdateUserLink(r.Context(), database.UpdateUserLinkParams{
 						UserID:            link.UserID,
+						LoginType:         link.LoginType,
 						OAuthAccessToken:  link.OAuthAccessToken,
 						OAuthRefreshToken: link.OAuthRefreshToken,
 						OAuthExpiry:       link.OAuthExpiry,

diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -140,7 +140,8 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
 	user, link, err := findLinkedUser(ctx, api.Database, githubLinkedID(ghUser), verifiedEmails...)
 	if err != nil {
 		httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{
-			Message: "Failed to find user.",
+			Message: "An internal error occurred.",
+			Detail:  err.Error(),
 		})
 		return
 	}
@@ -195,7 +196,6 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
 			})
 			return
 		}
-
 	}
 
 	// This can happen if a user is a built-in user but is signing in
@@ -220,7 +220,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
 
 	// LEGACY: Remove 10/2022.
 	// We started tracking linked IDs later so it's possible for a user to be a
-	// pre-existing Github user and not have a linked ID.
+	// pre-existing Github user and not have a linked ID. The migration
+	// to user_links did not populate this field as it requires calling out
+	// to Github to query the user's ID.
 	if link.LinkedID == "" {
 		link, err = api.Database.UpdateUserLinkedID(ctx, database.UpdateUserLinkedIDParams{
 			UserID:    user.ID,
@@ -387,7 +389,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 	if link.UserID == uuid.Nil {
 		link, err = api.Database.InsertUserLink(ctx, database.InsertUserLinkParams{
 			UserID:            user.ID,
-			LoginType:         database.LoginTypeGithub,
+			LoginType:         database.LoginTypeOIDC,
 			LinkedID:          oidcLinkedID(idToken),
 			OAuthAccessToken:  state.Token.AccessToken,
 			OAuthRefreshToken: state.Token.RefreshToken,
@@ -400,12 +402,13 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 			})
 			return
 		}
-
 	}
 
 	// LEGACY: Remove 10/2022.
 	// We started tracking linked IDs later so it's possible for a user to be a
 	// pre-existing OIDC user and not have a linked ID.
+	// The migration that added the user_links table could not populate
+	// the 'linked_id' field since it requires fields off the access token.
 	if link.LinkedID == "" {
 		link, err = api.Database.UpdateUserLinkedID(ctx, database.UpdateUserLinkedIDParams{
 			UserID:    user.ID,
@@ -440,7 +443,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 		if err != nil {
 			httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{
 				Message: "Failed to update user profile.",
-				Detail:  err.Error(),
+				Detail:  fmt.Sprintf("udpate user profile: %s", err.Error()),
 			})
 			return
 		}
@@ -486,6 +489,9 @@ func findLinkedUser(ctx context.Context, db database.Store, linkedID string, ema
 
 	if err == nil {
 		user, err = db.GetUserByID(ctx, link.UserID)
+		if err != nil {
+			return database.User{}, database.UserLink{}, xerrors.Errorf("get user by id: %w", err)
+		}
 		return user, link, nil
 	}
 

diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go
@@ -175,6 +175,34 @@ func TestUserOAuth2Github(t *testing.T) {
 		resp := oauth2Callback(t, client)
 		require.Equal(t, http.StatusForbidden, resp.StatusCode)
 	})
+	t.Run("Login", func(t *testing.T) {
+		t.Parallel()
+		client := coderdtest.New(t, &coderdtest.Options{
+			GithubOAuth2Config: &coderd.GithubOAuth2Config{
+				OAuth2Config:       &oauth2Config{},
+				AllowOrganizations: []string{"coder"},
+				ListOrganizationMemberships: func(ctx context.Context, client *http.Client) ([]*github.Membership, error) {
+					return []*github.Membership{{
+						Organization: &github.Organization{
+							Login: github.String("coder"),
+						},
+					}}, nil
+				},
+				AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
+					return &github.User{}, nil
+				},
+				ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) {
+					return []*github.UserEmail{{
+						Email:    github.String("testuser@coder.com"),
+						Verified: github.Bool(true),
+					}}, nil
+				},
+			},
+		})
+		_ = coderdtest.CreateFirstUser(t, client)
+		resp := oauth2Callback(t, client)
+		require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
+	})
 	t.Run("Signup", func(t *testing.T) {
 		t.Parallel()
 		client := coderdtest.New(t, &coderdtest.Options{
@@ -210,7 +238,6 @@ func TestUserOAuth2Github(t *testing.T) {
 		client.SessionToken = resp.Cookies()[0].Value
 		user, err := client.User(context.Background(), "me")
 		require.NoError(t, err)
-		require.Equal(t, "1234", user.LinkedID)
 		require.Equal(t, "kyle@coder.com", user.Email)
 		require.Equal(t, "kyle", user.Username)
 	})
@@ -341,7 +368,6 @@ func TestUserOIDC(t *testing.T) {
 				user, err := client.User(ctx, "me")
 				require.NoError(t, err)
 				require.Equal(t, tc.Username, user.Username)
-				require.Equal(t, "https://coder.com||hello", user.LinkedID)
 			}
 		})
 	}
@@ -385,35 +411,6 @@ func TestUserOIDC(t *testing.T) {
 		resp := oidcCallback(t, client)
 		require.Equal(t, http.StatusBadRequest, resp.StatusCode)
 	})
-
-	// Test that we do not allow collisions with pre-existing accounts
-	// of differing login types.
-	t.Run("InvalidLoginType", func(t *testing.T) {
-		t.Parallel()
-		config := createOIDCConfig(t, jwt.MapClaims{
-			"email":              "kyle@kwc.io",
-			"email_verified":     true,
-			"preferred_username": "kyle",
-		})
-
-		client := coderdtest.New(t, &coderdtest.Options{
-			OIDCConfig: config,
-		})
-
-		_, err := client.CreateFirstUser(context.Background(), codersdk.CreateFirstUserRequest{
-			Email:            "kyle@kwc.io",
-			Username:         "kyle",
-			Password:         "yeah",
-			OrganizationName: "default",
-		})
-		require.NoError(t, err)
-
-		config.AllowSignups = true
-		config.EmailDomain = "kwc.io"
-
-		resp := oidcCallback(t, client)
-		assert.Equal(t, http.StatusConflict, resp.StatusCode)
-	})
 }
 
 // createOIDCConfig generates a new OIDCConfig that returns a static token

diff --git a/codersdk/users.go b/codersdk/users.go
@@ -50,8 +50,6 @@ type User struct {
 	Status          UserStatus  `json:"status"`
 	OrganizationIDs []uuid.UUID `json:"organization_ids"`
 	Roles           []Role      `json:"roles"`
-	LoginType       LoginType   `json:"login_type"`
-	LinkedID        string      `json:"linked_id"`
 }
 
 type APIKey struct {

diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts
@@ -352,8 +352,6 @@ export interface User {
   readonly status: UserStatus
   readonly organization_ids: string[]
   readonly roles: Role[]
-  readonly login_type: LoginType
-  readonly linked_id: string
 }
 
 // From codersdk/users.go