diff --git a/server/cmd/api/api/chromium_configure.go b/server/cmd/api/api/chromium_configure.go index f141ceab..98927e86 100644 --- a/server/cmd/api/api/chromium_configure.go +++ b/server/cmd/api/api/chromium_configure.go @@ -50,7 +50,7 @@ func (st *chromiumConfigureState) cleanup() { } // ChromiumConfigure batched Chromium/session configuration plus optional navigation. -func (s *ApiService) ChromiumConfigure(ctx context.Context, request oapi.ChromiumConfigureRequestObject) (resp oapi.ChromiumConfigureResponseObject, err error) { +func (s *ApiService) ChromiumConfigure(ctx context.Context, request oapi.ChromiumConfigureRequestObject) (oapi.ChromiumConfigureResponseObject, error) { start := time.Now() if request.Body == nil { @@ -80,7 +80,68 @@ func (s *ApiService) ChromiumConfigure(ctx context.Context, request oapi.Chromiu s.chromiumConfigMu.Lock() defer s.chromiumConfigMu.Unlock() - needsStop := chromiumNeedsStopCycle(st) + var configureResp oapi.ChromiumConfigureResponseObject + switch chromiumConfigureModeFor(st) { + case chromiumConfigureModeLive: + configureResp = s.chromiumConfigureLive(ctx, st, spec) + case chromiumConfigureModeRestart: + configureResp = s.chromiumConfigureRestart(ctx, st, spec) + default: + return cfg500Configure("unhandled configure mode"), nil + } + if configureResp != nil { + return configureResp, nil + } + + logger.FromContext(ctx).Info("chromium configure finished", "elapsed", time.Since(start).String()) + return oapi.ChromiumConfigure200JSONResponse{Ok: true}, nil +} + +type chromiumConfigureMode uint8 + +const ( + chromiumConfigureModeLive chromiumConfigureMode = iota + chromiumConfigureModeRestart +) + +func chromiumConfigureModeFor(st *chromiumConfigureState) chromiumConfigureMode { + if st.hasProfile || + len(st.extItems) > 0 || + policiesContentNonEmpty(st.chromePoliciesJSON) || + flagsContentNonEmpty(st.chromiumFlagsJSON) { + return chromiumConfigureModeRestart + } + return chromiumConfigureModeLive +} + +func (s *ApiService) chromiumConfigureLive(ctx context.Context, st *chromiumConfigureState, spec startURLParsed) oapi.ChromiumConfigureResponseObject { + if st.displayJSON != nil && strings.TrimSpace(*st.displayJSON) != "" { + displayPlan, displayResp := chromiumPrepareDisplay(ctx, s, st.displayJSON) + if displayResp != nil { + return displayResp + } + if displayPlan != nil { + if resp := chromiumRunPatchDisplay(ctx, s, displayPlan.body); resp != nil { + return resp + } + } + } + + chromiumConfigureNavigate(ctx, s, spec) + return nil +} + +func chromiumConfigureNavigate(ctx context.Context, s *ApiService, spec startURLParsed) { + if !spec.needsNav { + return + } + if err := chromiumDoNavigate(ctx, s, spec); err != nil { + logger.FromContext(ctx).Warn("start_url dispatch failed", "error", err) + } +} + +func (s *ApiService) chromiumConfigureRestart(ctx context.Context, st *chromiumConfigureState, spec startURLParsed) (resp oapi.ChromiumConfigureResponseObject) { + var stoppedRecordings []stoppedRecordingInfo chromiumStopped := false restartAfterStop := func() error { if !chromiumStopped { @@ -93,109 +154,93 @@ func (s *ApiService) ChromiumConfigure(ctx context.Context, request oapi.Chromiu return nil } defer func() { + // Error paths restart recordings before Chromium recovery. chromiumStopped + // also covers a panic before the explicit restart below. + if (resp != nil || chromiumStopped) && len(stoppedRecordings) > 0 { + go s.startNewRecordingSegments(context.WithoutCancel(ctx), stoppedRecordings) + stoppedRecordings = nil + } if restartErr := restartAfterStop(); restartErr != nil { if resp != nil { logger.FromContext(ctx).Error("failed to restart chromium after configure error", "error", restartErr) return } resp = cfg500ConfigureStep(chromiumConfigureStepStart, restartErr.Error()) - err = nil } }() - if needsStop { - logger.FromContext(ctx).Info("chromium configure (stop/start path)") - if err := s.stopChromium(ctx); err != nil { - return cfg500ConfigureStep(chromiumConfigureStepStop, err.Error()), nil - } - chromiumStopped = true + logger.FromContext(ctx).Info("chromium configure (stop/start path)") + if err := s.stopChromium(ctx); err != nil { + return cfg500ConfigureStep(chromiumConfigureStepStop, err.Error()) + } + chromiumStopped = true - policyOverrides, err := chromiumValidatePolicies(st.chromePoliciesJSON) - if err != nil { - return cfgResponseFromStepError(chromiumConfigureStepPolicies, err), nil - } - if err := chromiumApplyPolicies(ctx, s, policyOverrides); err != nil { - return cfgResponseFromStepError(chromiumConfigureStepPolicies, err), nil - } + policyOverrides, err := chromiumValidatePolicies(st.chromePoliciesJSON) + if err != nil { + return cfgResponseFromStepError(chromiumConfigureStepPolicies, err) + } + if err := chromiumApplyPolicies(ctx, s, policyOverrides); err != nil { + return cfgResponseFromStepError(chromiumConfigureStepPolicies, err) + } - if reqMsgs, ierr := chromiumApplyExtensions(ctx, s, st.extItems); reqMsgs != "" { - return cfg400(fmt.Sprintf("%s: %s", chromiumConfigureStepExtensions, reqMsgs)), nil - } else if ierr != nil { - return cfg500ConfigureStep(chromiumConfigureStepExtensions, ierr.Error()), nil - } + if reqMsgs, ierr := chromiumApplyExtensions(ctx, s, st.extItems); reqMsgs != "" { + return cfg400(fmt.Sprintf("%s: %s", chromiumConfigureStepExtensions, reqMsgs)) + } else if ierr != nil { + return cfg500ConfigureStep(chromiumConfigureStepExtensions, ierr.Error()) + } - if st.displayJSON != nil && strings.TrimSpace(*st.displayJSON) != "" { - displayPlan, displayResp := chromiumPrepareDisplay(ctx, s, st.displayJSON) - if displayResp != nil { - return displayResp, nil + if st.displayJSON != nil && strings.TrimSpace(*st.displayJSON) != "" { + displayPlan, displayResp := chromiumPrepareDisplay(ctx, s, st.displayJSON) + if displayResp != nil { + return displayResp + } + if displayPlan != nil { + stopped, stopErr := s.stopActiveRecordings(ctx) + if stopErr != nil { + return cfg500ConfigureStep(chromiumConfigureStepDisplay, fmt.Sprintf("failed to stop recordings: %v", stopErr)) } - if displayPlan != nil { - stopped, stopErr := s.stopActiveRecordings(ctx) - if stopErr != nil { - return cfg500ConfigureStep(chromiumConfigureStepDisplay, fmt.Sprintf("failed to stop recordings: %v", stopErr)), nil - } - if len(stopped) > 0 { - defer func() { - go s.startNewRecordingSegments(context.WithoutCancel(ctx), stopped) - }() - } - if rr := chromiumDisplayApplyWhileStopped(ctx, s, displayPlan); rr != nil { - return rr, nil - } + stoppedRecordings = stopped + if rr := chromiumDisplayApplyWhileStopped(ctx, s, displayPlan); rr != nil { + return rr } } + } - flagsPlan, err := chromiumValidateFlags(st.chromiumFlagsJSON) - if err != nil { - return cfgResponseFromStepError(chromiumConfigureStepFlags, err), nil - } - if err := chromiumMergeFlags(ctx, s, flagsPlan); err != nil { - return cfgResponseFromStepError(chromiumConfigureStepFlags, err), nil - } + flagsPlan, err := chromiumValidateFlags(st.chromiumFlagsJSON) + if err != nil { + return cfgResponseFromStepError(chromiumConfigureStepFlags, err) + } + if err := chromiumMergeFlags(ctx, s, flagsPlan); err != nil { + return cfgResponseFromStepError(chromiumConfigureStepFlags, err) + } - if st.hasProfile { - preparedProfile, cleanupProfile, err := chromiumPrepareProfileArchive(st.profileTemp, st.stripComponents) - if cleanupProfile != nil { - defer cleanupProfile() - } - if err != nil { - return cfg500ConfigureStep(chromiumConfigureStepProfile, err.Error()), nil - } - if spec.needsNav { - if err := stripProfileSessionRestore(preparedProfile); err != nil { - return cfg500ConfigureStep(chromiumConfigureStepProfile, err.Error()), nil - } - } - if err := chromiumInstallPreparedProfile(preparedProfile); err != nil { - return cfg500ConfigureStep(chromiumConfigureStepProfile, err.Error()), nil - } + if st.hasProfile { + preparedProfile, cleanupProfile, err := chromiumPrepareProfileArchive(st.profileTemp, st.stripComponents) + if cleanupProfile != nil { + defer cleanupProfile() } - - if err := restartAfterStop(); err != nil { - return cfg500ConfigureStep(chromiumConfigureStepStart, err.Error()), nil + if err != nil { + return cfg500ConfigureStep(chromiumConfigureStepProfile, err.Error()) } - } else { - if st.displayJSON != nil && strings.TrimSpace(*st.displayJSON) != "" { - displayPlan, displayResp := chromiumPrepareDisplay(ctx, s, st.displayJSON) - if displayResp != nil { - return displayResp, nil - } - if displayPlan != nil { - if rr := chromiumRunPatchDisplay(ctx, s, displayPlan.body); rr != nil { - return rr, nil - } + if spec.needsNav { + if err := stripProfileSessionRestore(preparedProfile); err != nil { + return cfg500ConfigureStep(chromiumConfigureStepProfile, err.Error()) } } - } - - if spec.needsNav { - if err := chromiumDoNavigate(ctx, s, spec); err != nil { - logger.FromContext(ctx).Warn("start_url dispatch failed", "error", err) + if err := chromiumInstallPreparedProfile(preparedProfile); err != nil { + return cfg500ConfigureStep(chromiumConfigureStepProfile, err.Error()) } } - logger.FromContext(ctx).Info("chromium configure finished", "elapsed", time.Since(start).String()) - return oapi.ChromiumConfigure200JSONResponse{Ok: true}, nil + if err := restartAfterStop(); err != nil { + return cfg500ConfigureStep(chromiumConfigureStepStart, err.Error()) + } + chromiumConfigureNavigate(ctx, s, spec) + if len(stoppedRecordings) > 0 { + go s.startNewRecordingSegments(context.WithoutCancel(ctx), stoppedRecordings) + stoppedRecordings = nil + } + return nil } type startURLParsed struct { @@ -300,13 +345,6 @@ type chromiumDisplayPlan struct { refreshRate int } -func chromiumNeedsStopCycle(st *chromiumConfigureState) bool { - return st.hasProfile || - len(st.extItems) > 0 || - policiesContentNonEmpty(st.chromePoliciesJSON) || - flagsContentNonEmpty(st.chromiumFlagsJSON) -} - func policiesContentNonEmpty(s *string) bool { if !policiesNonEmpty(s) { return false diff --git a/server/cmd/api/api/chromium_configure_test.go b/server/cmd/api/api/chromium_configure_test.go index eac0e5f1..f1b46c50 100644 --- a/server/cmd/api/api/chromium_configure_test.go +++ b/server/cmd/api/api/chromium_configure_test.go @@ -29,30 +29,57 @@ func TestPoliciesContentNonEmpty(t *testing.T) { require.True(t, policiesContentNonEmpty(&real)) } -func TestChromiumConfigureActionableFlags(t *testing.T) { - emptyFlags := `{"flags":[]}` - realFlags := `{"flags":["--kiosk"]}` +func TestChromiumConfigureModeFor(t *testing.T) { + stringPtr := func(value string) *string { return &value } - st := &chromiumConfigureState{chromiumFlagsJSON: &emptyFlags} - require.Equal(t, 0, cfgActionables(st)) - require.False(t, chromiumNeedsStopCycle(st)) + tests := []struct { + name string + state chromiumConfigureState + want chromiumConfigureMode + }{ + {name: "no restart fields", want: chromiumConfigureModeLive}, + {name: "display only", state: chromiumConfigureState{displayJSON: stringPtr(`{"width":1280}`)}, want: chromiumConfigureModeLive}, + {name: "start URL only", state: chromiumConfigureState{startURLRaw: stringPtr("https://example.com")}, want: chromiumConfigureModeLive}, + {name: "empty policies", state: chromiumConfigureState{chromePoliciesJSON: stringPtr(`{}`)}, want: chromiumConfigureModeLive}, + {name: "nonempty policies", state: chromiumConfigureState{chromePoliciesJSON: stringPtr(`{"QuicAllowed":false}`)}, want: chromiumConfigureModeRestart}, + {name: "invalid policies", state: chromiumConfigureState{chromePoliciesJSON: stringPtr(`{bad-json`)}, want: chromiumConfigureModeRestart}, + {name: "empty flags", state: chromiumConfigureState{chromiumFlagsJSON: stringPtr(`{"flags":[]}`)}, want: chromiumConfigureModeLive}, + {name: "nonempty flags", state: chromiumConfigureState{chromiumFlagsJSON: stringPtr(`{"flags":["--kiosk"]}`)}, want: chromiumConfigureModeRestart}, + {name: "invalid flags", state: chromiumConfigureState{chromiumFlagsJSON: stringPtr(`{bad-json`)}, want: chromiumConfigureModeRestart}, + {name: "profile", state: chromiumConfigureState{hasProfile: true}, want: chromiumConfigureModeRestart}, + {name: "extensions", state: chromiumConfigureState{extItems: []extensionZipItem{{name: "test"}}}, want: chromiumConfigureModeRestart}, + {name: "display and extension", state: chromiumConfigureState{displayJSON: stringPtr(`{"width":1280}`), extItems: []extensionZipItem{{name: "test"}}}, want: chromiumConfigureModeRestart}, + } - st = &chromiumConfigureState{chromiumFlagsJSON: &realFlags} - require.Equal(t, 1, cfgActionables(st)) - require.True(t, chromiumNeedsStopCycle(st)) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.want, chromiumConfigureModeFor(&tt.state)) + }) + } } -func TestChromiumConfigureActionablePolicies(t *testing.T) { +func TestChromiumConfigureActionables(t *testing.T) { + emptyFlags := `{"flags":[]}` + realFlags := `{"flags":["--kiosk"]}` emptyPolicies := `{}` realPolicies := `{"QuicAllowed":false}` - st := &chromiumConfigureState{chromePoliciesJSON: &emptyPolicies} - require.Equal(t, 0, cfgActionables(st)) - require.False(t, chromiumNeedsStopCycle(st)) + tests := []struct { + name string + state chromiumConfigureState + want int + }{ + {name: "empty flags", state: chromiumConfigureState{chromiumFlagsJSON: &emptyFlags}}, + {name: "nonempty flags", state: chromiumConfigureState{chromiumFlagsJSON: &realFlags}, want: 1}, + {name: "empty policies", state: chromiumConfigureState{chromePoliciesJSON: &emptyPolicies}}, + {name: "nonempty policies", state: chromiumConfigureState{chromePoliciesJSON: &realPolicies}, want: 1}, + } - st = &chromiumConfigureState{chromePoliciesJSON: &realPolicies} - require.Equal(t, 1, cfgActionables(st)) - require.True(t, chromiumNeedsStopCycle(st)) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.want, cfgActionables(&tt.state)) + }) + } } func TestChromiumStartURLSpec(t *testing.T) { diff --git a/server/e2e/e2e_chromium_configure_powerset_test.go b/server/e2e/e2e_chromium_configure_powerset_test.go index fcba4c6a..0342da87 100644 --- a/server/e2e/e2e_chromium_configure_powerset_test.go +++ b/server/e2e/e2e_chromium_configure_powerset_test.go @@ -44,6 +44,7 @@ func TestChromiumConfigureMultipartPowerset(t *testing.T) { matrix := []int{ matDisplay, + matStartURL, matPolicy | matKioskFlags, matExtension, matDisplay | matPolicy | matKioskFlags | matExtension | matStartURL, @@ -73,6 +74,9 @@ func TestChromiumConfigureMultipartPowerset(t *testing.T) { defer func() { _ = c.Stop(context.WithoutCancel(ctx)) }() require.NoError(t, c.WaitReady(ctx)) + require.NoError(t, c.WaitDevTools(ctx)) + browserWebSocketBefore, err := fetchBrowserWebSocketURL(ctx, c) + require.NoError(t, err) var body bytes.Buffer w := multipart.NewWriter(&body) @@ -89,10 +93,23 @@ func TestChromiumConfigureMultipartPowerset(t *testing.T) { "bits=%02x unexpected status=%s body=%s", bits, rsp.Status(), string(rsp.Body)) require.NotNil(t, rsp.JSON200, "want ok JSON") require.True(t, rsp.JSON200.Ok) + + browserWebSocketAfter, err := fetchBrowserWebSocketURL(ctx, c) + require.NoError(t, err) + if chromiumConfigurePowersetRestarts(bits) { + require.NotEqual(t, browserWebSocketBefore, browserWebSocketAfter, "restart path must replace the browser WebSocket identity") + } else { + require.Equal(t, browserWebSocketBefore, browserWebSocketAfter, "live path must preserve the browser WebSocket identity") + } }) } } +func chromiumConfigurePowersetRestarts(bits int) bool { + // This matrix runs against headless Xvfb, where display-only configure stays live. + return bits&(matPolicy|matKioskFlags|matExtension) != 0 +} + func chromiumConfigurePowersetLabel(bits int) string { var p []string if bits&matDisplay != 0 {