Add unit test

coder · Emyrk · Jun 30, 2023 · Jun 13, 2023 · Jun 14, 2023 · Jun 14, 2023
commit a4961b9dba17ff62da03f8465f2e29b00007f4e6
diff --git a/coderd/audit/diff.go b/coderd/audit/diff.go
@@ -17,7 +17,8 @@ type Auditable interface {
 		database.WorkspaceBuild |
 		database.AuditableGroup |
 		database.License |
-		database.WorkspaceProxy
+		database.WorkspaceProxy |
+		database.OauthMergeState
 }
 
 // Map is a map of changed fields in an audited resource. It maps field names to

diff --git a/coderd/audit/request.go b/coderd/audit/request.go
@@ -84,6 +84,8 @@ func ResourceTarget[T Auditable](tgt T) string {
 		return strconv.Itoa(int(typed.ID))
 	case database.WorkspaceProxy:
 		return typed.Name
+	case database.OauthMergeState:
+		return typed.StateString
 	default:
 		panic(fmt.Sprintf("unknown resource %T", tgt))
 	}
@@ -111,6 +113,9 @@ func ResourceID[T Auditable](tgt T) uuid.UUID {
 		return typed.UUID
 	case database.WorkspaceProxy:
 		return typed.ID
+	case database.OauthMergeState:
+		// The merge state is for the given user
+		return typed.UserID
 	default:
 		panic(fmt.Sprintf("unknown resource %T", tgt))
 	}
@@ -138,6 +143,8 @@ func ResourceType[T Auditable](tgt T) database.ResourceType {
 		return database.ResourceTypeLicense
 	case database.WorkspaceProxy:
 		return database.ResourceTypeWorkspaceProxy
+	case database.OauthMergeState:
+		return database.ResourceTypeConvertLogin
 	default:
 		panic(fmt.Sprintf("unknown resource %T", typed))
 	}

diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go
@@ -1175,9 +1175,9 @@ func (q *fakeQuerier) DeleteReplicasUpdatedBefore(_ context.Context, before time
 	return nil
 }
 
-func (q *fakeQuerier) DeleteUserOauthMergeStates(ctx context.Context, userID uuid.UUID) error {
-	q.mutex.RLock()
-	defer q.mutex.RUnlock()
+func (q *fakeQuerier) DeleteUserOauthMergeStates(_ context.Context, userID uuid.UUID) error {
+	q.mutex.Lock()
+	defer q.mutex.Unlock()
 
 	i := 0
 	for {
@@ -3961,8 +3961,8 @@ func (q *fakeQuerier) InsertUserLink(_ context.Context, args database.InsertUser
 }
 
 func (q *fakeQuerier) InsertUserOauthMergeState(_ context.Context, arg database.InsertUserOauthMergeStateParams) (database.OauthMergeState, error) {
-	q.mutex.RLock()
-	defer q.mutex.RUnlock()
+	q.mutex.Lock()
+	defer q.mutex.Unlock()
 
 	if err := validateDatabaseType(arg); err != nil {
 		return database.OauthMergeState{}, err

diff --git a/coderd/database/models.go b/coderd/database/models.go
diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -38,16 +38,14 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) {
 	var (
 		ctx               = r.Context()
 		auditor           = api.Auditor.Load()
-		aReq, commitAudit = audit.InitRequest[database.APIKey](rw, &audit.RequestParams{
+		aReq, commitAudit = audit.InitRequest[database.OauthMergeState](rw, &audit.RequestParams{
 			Audit:   *auditor,
 			Log:     api.Logger,
 			Request: r,
 			Action:  database.AuditActionCreate,
 		})
 	)
-	// TODO: @emyrk This does make a new api key. Make a new auditable resource
-	// for oidc state.
-	aReq.Old = database.APIKey{}
+	aReq.Old = database.OauthMergeState{}
 	defer commitAudit()
 
 	var req codersdk.ConvertLoginRequest
@@ -102,12 +100,12 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) {
 		// We should only ever have 1 oauth merge state per user. So delete
 		// any existing if they exist.
 		//nolint:gocritic // Keeping the table clean
-		err := api.Database.DeleteUserOauthMergeStates(dbauthz.AsSystemRestricted(ctx), user.ID)
+		err := store.DeleteUserOauthMergeStates(dbauthz.AsSystemRestricted(ctx), user.ID)
 		if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
 			return err
 		}
 
-		mergeState, err = api.Database.InsertUserOauthMergeState(ctx, database.InsertUserOauthMergeStateParams{
+		mergeState, err = store.InsertUserOauthMergeState(ctx, database.InsertUserOauthMergeStateParams{
 			UserID:      user.ID,
 			StateString: stateString,
 			ToLoginType: database.LoginType(req.ToLoginType),
@@ -134,6 +132,7 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) {
 		ToLoginType: codersdk.LoginType(mergeState.ToLoginType),
 		UserID:      mergeState.UserID,
 	})
+	aReq.New = mergeState
 }
 
 // Authenticates the user with an email and password.
@@ -532,6 +531,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
 		Username:               ghUser.GetLogin(),
 		AvatarURL:              ghUser.GetAvatarURL(),
 		OauthConversionEnabled: api.DeploymentValues.EnableOauthAccountConversion.Value(),
+		InitAuditRequest: func(params *audit.RequestParams) (*audit.Request[database.OauthMergeState], func()) {
+			return audit.InitRequest[database.OauthMergeState](rw, params)
+		},
 	})
 	var httpErr httpError
 	if xerrors.As(err, &httpErr) {
@@ -864,6 +866,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 		UsingGroups:            usingGroups,
 		Groups:                 groups,
 		OauthConversionEnabled: api.DeploymentValues.EnableOauthAccountConversion.Value(),
+		InitAuditRequest: func(params *audit.RequestParams) (*audit.Request[database.OauthMergeState], func()) {
+			return audit.InitRequest[database.OauthMergeState](rw, params)
+		},
 	})
 	var httpErr httpError
 	if xerrors.As(err, &httpErr) {
@@ -946,6 +951,8 @@ type oauthLoginParams struct {
 	UsingGroups            bool
 	Groups                 []string
 	OauthConversionEnabled bool
+
+	InitAuditRequest func(params *audit.RequestParams) (*audit.Request[database.OauthMergeState], func())
 }
 
 type httpError struct {
@@ -997,13 +1004,27 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook
 			if !params.OauthConversionEnabled {
 				return wrongLoginTypeErr
 			}
-			mergeState, err := api.Database.GetUserOauthMergeState(dbauthz.AsSystemRestricted(ctx), database.GetUserOauthMergeStateParams{
+			var (
+				auditor                                    = *api.Auditor.Load()
+				oauthConvertAudit, commitOauthConvertAudit = params.InitAuditRequest(&audit.RequestParams{
+					Audit:   auditor,
+					Log:     api.Logger,
+					Request: r,
+					Action:  database.AuditActionLogin,
+				})
+			)
+			defer commitOauthConvertAudit()
+
+			// nolint:gocritic // Required to auth the oidc convert
+			mergeState, err := tx.GetUserOauthMergeState(dbauthz.AsSystemRestricted(ctx), database.GetUserOauthMergeStateParams{
 				UserID:      user.ID,
 				StateString: params.State.StateString,
 			})
 			if xerrors.Is(err, sql.ErrNoRows) {
 				return wrongLoginTypeErr
 			}
+			oauthConvertAudit.Old = mergeState
+
 			failedMsg := fmt.Sprintf("Request to convert login type from %s to %s failed", user.LoginType, params.LoginType)
 			if err != nil {
 				return httpError{

diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go
@@ -782,6 +782,46 @@ func TestUserOIDC(t *testing.T) {
 		})
 	}
 
+	t.Run("OIDCConvert", func(t *testing.T) {
+		t.Parallel()
+		auditor := audit.NewMock()
+		conf := coderdtest.NewOIDCConfig(t, "")
+
+		config := conf.OIDCConfig(t, nil)
+		config.AllowSignups = true
+
+		cfg := coderdtest.DeploymentValues(t)
+		cfg.EnableOauthAccountConversion = true
+		client := coderdtest.New(t, &coderdtest.Options{
+			Auditor:          auditor,
+			OIDCConfig:       config,
+			DeploymentValues: cfg,
+		})
+		owner := coderdtest.CreateFirstUser(t, client)
+
+		user, userData := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID)
+
+		numLogs := len(auditor.AuditLogs())
+
+		code := conf.EncodeClaims(t, jwt.MapClaims{
+			"email": userData.Email,
+		})
+
+		numLogs++ // add an audit log for login
+		ctx := testutil.Context(t, testutil.WaitShort)
+		convertResponse, err := user.ConvertToOAuthLogin(ctx, codersdk.ConvertLoginRequest{
+			ToLoginType: codersdk.LoginTypeOIDC,
+			LoginWithPasswordRequest: codersdk.LoginWithPasswordRequest{
+				Email:    userData.Email,
+				Password: "SomeSecurePassword!",
+			},
+		})
+		require.NoError(t, err)
+
+		resp := oidcCallbackWithState(t, client, code, convertResponse.StateString)
+		require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
+	})
+
 	t.Run("AlternateUsername", func(t *testing.T) {
 		t.Parallel()
 		auditor := audit.NewMock()
@@ -1004,17 +1044,21 @@ func oauth2Callback(t *testing.T, client *codersdk.Client) *http.Response {
 }
 
 func oidcCallback(t *testing.T, client *codersdk.Client, code string) *http.Response {
+	return oidcCallbackWithState(t, client, code, "somestate")
+}
+
+func oidcCallbackWithState(t *testing.T, client *codersdk.Client, code, state string) *http.Response {
 	t.Helper()
 	client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
 		return http.ErrUseLastResponse
 	}
-	oauthURL, err := client.URL.Parse(fmt.Sprintf("/api/v2/users/oidc/callback?code=%s&state=somestate", code))
+	oauthURL, err := client.URL.Parse(fmt.Sprintf("/api/v2/users/oidc/callback?code=%s&state=%s", code, state))
 	require.NoError(t, err)
 	req, err := http.NewRequestWithContext(context.Background(), "GET", oauthURL.String(), nil)
 	require.NoError(t, err)
 	req.AddCookie(&http.Cookie{
 		Name:  codersdk.OAuth2StateCookie,
-		Value: "somestate",
+		Value: state,
 	})
 	res, err := client.HTTPClient.Do(req)
 	require.NoError(t, err)

diff --git a/codersdk/deployment.go b/codersdk/deployment.go
@@ -1435,6 +1435,8 @@ when required by your organization's security policy.`,
 			Value:       &c.EnableOauthAccountConversion,
 			Group:       &deploymentGroupNetworkingHTTP,
 			YAML:        "enableOauthAuthConversion",
+			// Do not show this until the feature is fully ready.
+			Hidden: true,
 		},
 		{
 			Name:          "Config Path",

diff --git a/codersdk/users.go b/codersdk/users.go
@@ -312,6 +312,26 @@ func (c *Client) LoginWithPassword(ctx context.Context, req LoginWithPasswordReq
 	return resp, nil
 }
 
+// ConvertToOAuthLogin will send a request to convert the user from password
+// based authentication to oauth based. The response has the oauth state code
+// to use in the oauth flow.
+func (c *Client) ConvertToOAuthLogin(ctx context.Context, req ConvertLoginRequest) (OauthConversionResponse, error) {
+	res, err := c.Request(ctx, http.MethodPost, "/api/v2/users/convert-login", req)
+	if err != nil {
+		return OauthConversionResponse{}, err
+	}
+	defer res.Body.Close()
+	if res.StatusCode != http.StatusCreated {
+		return OauthConversionResponse{}, ReadBodyAsError(res)
+	}
+	var resp OauthConversionResponse
+	err = json.NewDecoder(res.Body).Decode(&resp)
+	if err != nil {
+		return OauthConversionResponse{}, err
+	}
+	return resp, nil
+}
+
 // Logout calls the /logout API
 // Call `ClearSessionToken()` to clear the session token of the client.
 func (c *Client) Logout(ctx context.Context) error {

diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go
@@ -155,6 +155,13 @@ var auditableResourcesTypes = map[any]map[string]Action{
 		"scope":            ActionIgnore,
 		"token_name":       ActionIgnore,
 	},
+	&database.OauthMergeState{}: {
+		"state_string":  ActionSecret,
+		"created_at":    ActionTrack,
+		"expires_at":    ActionTrack,
+		"to_login_type": ActionTrack,
+		"user_id":       ActionTrack,
+	},
 	// TODO: track an ID here when the below ticket is completed:
 	// https://github.com/coder/coder/pull/6012
 	&database.License{}: {