fix: clear credentials before auth (#406)

Author: ShawkyZ

pkg/local_workflows/auth_workflow.go

@@ -60,6 +60,8 @@ func authEntryPoint(invocationCtx workflow.InvocationContext, _ []workflow.Data)
 	logger := invocationCtx.GetEnhancedLogger()
 	engine := invocationCtx.GetEngine()
 
+	config.ClearCache()
+
 	httpClient := invocationCtx.GetNetworkAccess().GetUnauthorizedHttpClient()
 	authenticator := auth.NewOAuth2AuthenticatorWithOpts(
 		config,
@@ -107,10 +109,13 @@ func entryPointDI(invocationCtx workflow.InvocationContext, logger *zerolog.Logg
 	logger.Printf("Authentication Type: %s", authType)
 	analytics.AddExtensionStringValue(authTypeParameter, authType)
 
-	if strings.EqualFold(authType, auth.AUTH_TYPE_OAUTH) { // OAUTH flow
-		logger.Printf("Unset legacy token key %q from config", configuration.AUTHENTICATION_TOKEN)
-		config.Unset(configuration.AUTHENTICATION_TOKEN)
+	existingSnykToken := config.GetString(configuration.AUTHENTICATION_TOKEN)
+	// always attempt to clear existing tokens before triggering auth
+	logger.Print("Unset existing auth keys")
+	config.Unset(configuration.AUTHENTICATION_TOKEN)
+	config.Unset(auth.CONFIG_KEY_OAUTH_TOKEN)
 
+	if strings.EqualFold(authType, auth.AUTH_TYPE_OAUTH) { // OAUTH flow
 		headless := config.GetBool(headlessFlag)
 		logger.Printf("Headless: %v", headless)
 
@@ -125,25 +130,19 @@ func entryPointDI(invocationCtx workflow.InvocationContext, logger *zerolog.Logg
 		}
 	} else if strings.EqualFold(authType, auth.AUTH_TYPE_PAT) { // PAT flow
 		engine.GetConfiguration().PersistInStorage(auth.CONFIG_KEY_TOKEN)
-
-		oldToken := config.GetString(configuration.AUTHENTICATION_TOKEN)
 		pat := config.GetString(ConfigurationNewAuthenticationToken)
 
-		logger.Print("Unset existing auth keys from config")
-		config.Unset(auth.CONFIG_KEY_OAUTH_TOKEN)
-		config.Unset(configuration.AUTHENTICATION_TOKEN)
-
 		logger.Print("Validating pat")
 		whoamiConfig := config.Clone()
-		// we don't want to use the cache here, so this is a workaround
 		whoamiConfig.ClearCache()
+		// we don't want to use the cache here, so this is a workaround
 		whoamiConfig.Set(configuration.FLAG_EXPERIMENTAL, true)
 		whoamiConfig.Set(configuration.AUTHENTICATION_TOKEN, pat)
 		_, whoamiErr := engine.InvokeWithConfig(workflow.NewWorkflowIdentifier("whoami"), whoamiConfig)
 		if whoamiErr != nil {
 			// reset config file
-			if len(oldToken) > 0 {
-				config.Set(auth.CONFIG_KEY_TOKEN, oldToken)
+			if len(existingSnykToken) > 0 {
+				config.Set(auth.CONFIG_KEY_TOKEN, existingSnykToken)
 			}
 			return whoamiErr
 		}
@@ -158,9 +157,6 @@ func entryPointDI(invocationCtx workflow.InvocationContext, logger *zerolog.Logg
 			logger.Debug().Err(err).Msg("Failed to output authenticated message")
 		}
 	} else { // LEGACY flow
-		logger.Printf("Unset oauth key %q from config", auth.CONFIG_KEY_OAUTH_TOKEN)
-		config.Unset(auth.CONFIG_KEY_OAUTH_TOKEN)
-
 		config.Set(configuration.RAW_CMD_ARGS, os.Args[1:])
 		config.Set(configuration.WORKFLOW_USE_STDIO, true)
 		config.Set(configuration.AUTHENTICATION_TOKEN, "") // clear token to avoid using it during authentication

pkg/local_workflows/auth_workflow_test.go

@@ -163,6 +163,74 @@ func Test_pat(t *testing.T) {
 	})
 }
 
+func Test_clearAllCredentialsBeforeAuth(t *testing.T) {
+	mockCtl := gomock.NewController(t)
+	defer mockCtl.Finish()
+
+	logContent := &bytes.Buffer{}
+	logger := zerolog.New(logContent)
+	analytics := analytics.New()
+	engine := mocks.NewMockEngine(mockCtl)
+	authenticator := mocks.NewMockAuthenticator(mockCtl)
+
+	testCases := []struct {
+		name       string
+		authType   string
+		setupMocks func()
+	}{
+		{
+			name:     "OAuth flow clears all credentials",
+			authType: auth.AUTH_TYPE_OAUTH,
+			setupMocks: func() {
+				authenticator.EXPECT().Authenticate().Return(nil)
+			},
+		},
+		{
+			name:     "PAT flow clears all credentials",
+			authType: auth.AUTH_TYPE_PAT,
+			setupMocks: func() {
+				engine.EXPECT().GetConfiguration().Return(configuration.NewWithOpts()).AnyTimes()
+				engine.EXPECT().InvokeWithConfig(gomock.Any(), gomock.Any()).Return(nil, nil)
+			},
+		},
+		{
+			name:     "Token flow clears all credentials",
+			authType: auth.AUTH_TYPE_TOKEN,
+			setupMocks: func() {
+				engine.EXPECT().InvokeWithConfig(gomock.Any(), gomock.Any()).Return(nil, nil)
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			config := configuration.NewWithOpts()
+			config.Set(authTypeParameter, tc.authType)
+			if tc.authType == auth.AUTH_TYPE_PAT {
+				config.Set(ConfigurationNewAuthenticationToken, "snyk_uat.12345678.abcdefg-hijklmnop.qrstuvwxyz-123456")
+			}
+
+			// Set existing tokens that should be cleared
+			config.Set(auth.CONFIG_KEY_OAUTH_TOKEN, "existing-oauth-token")
+			config.Set(configuration.AUTHENTICATION_TOKEN, "existing-auth-token")
+
+			mockInvocationContext := mocks.NewMockInvocationContext(mockCtl)
+			mockInvocationContext.EXPECT().GetConfiguration().Return(config).AnyTimes()
+			mockInvocationContext.EXPECT().GetEnhancedLogger().Return(&logger).AnyTimes()
+			mockInvocationContext.EXPECT().GetAnalytics().Return(analytics).AnyTimes()
+
+			tc.setupMocks()
+
+			err := entryPointDI(mockInvocationContext, &logger, engine, authenticator)
+			assert.NoError(t, err)
+
+			// Verify both tokens are cleared regardless of auth type
+			assert.Empty(t, config.GetString(auth.CONFIG_KEY_OAUTH_TOKEN), "OAuth token should be cleared for %s flow", tc.authType)
+			assert.Empty(t, config.GetString(configuration.AUTHENTICATION_TOKEN), "Authentication token should be cleared for %s flow", tc.authType)
+		})
+	}
+}
+
 func Test_autodetectAuth(t *testing.T) {
 	t.Run("in stable versions, token by default", func(t *testing.T) {
 		expected := auth.AUTH_TYPE_OAUTH