fix(cef): avoid overwriting resumed oauth parent state
- commit
- 31b1bbf5470337deeef1c7d072359156cbe294a2
- author
- brice · · 2026-04-21 15:02 UTC
- parent
- bc78f4e
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
5 files · +52 -47
M internal/ui/coordinator/content/coordinator.go +6 -5
| @@ -125,11 +125,12 @@ type pendingThemeUpdate struct { | |||
| 125 | 125 | } | |
| 126 | 126 | ||
| 127 | 127 | type popupOAuthState struct { | |
| 128 | - | ParentPaneID entity.PaneID | |
| 129 | - | CallbackURI string | |
| 130 | - | Success bool | |
| 131 | - | Error bool | |
| 132 | - | Seen bool | |
| 128 | + | ParentPaneID entity.PaneID | |
| 129 | + | ParentURIAtOpen string | |
| 130 | + | CallbackURI string | |
| 131 | + | Success bool | |
| 132 | + | Error bool | |
| 133 | + | Seen bool | |
| 133 | 134 | } | |
| 134 | 135 | ||
| 135 | 136 | // PermissionActivityState represents the visible state for media permission activity. | |
| @@ -125,11 +125,12 @@ type pendingThemeUpdate struct { | |||||
| 125 | } | 125 | } | ||
| 126 | 126 | ||||
| 127 | type popupOAuthState struct { | 127 | type popupOAuthState struct { | ||
| 128 | - | ParentPaneID entity.PaneID | 128 | + | ParentPaneID entity.PaneID |
| 129 | - | CallbackURI string | 129 | + | ParentURIAtOpen string |
| 130 | - | Success bool | 130 | + | CallbackURI string |
| 131 | - | Error bool | 131 | + | Success bool |
| 132 | - | Seen bool | 132 | + | Error bool |
| 133 | + | Seen bool | |||
| 133 | } | 134 | } | ||
| 134 | 135 | ||||
| 135 | // PermissionActivityState represents the visible state for media permission activity. | 136 | // PermissionActivityState represents the visible state for media permission activity. | ||
M internal/ui/coordinator/content/oauth.go +13 -31
| @@ -9,7 +9,6 @@ import ( | |||
| 9 | 9 | ||
| 10 | 10 | "github.com/bnema/dumber/internal/application/port" | |
| 11 | 11 | "github.com/bnema/dumber/internal/domain/entity" | |
| 12 | - | urlutil "github.com/bnema/dumber/internal/domain/url" | |
| 13 | 12 | "github.com/bnema/dumber/internal/logging" | |
| 14 | 13 | "github.com/bnema/puregotk/v4/glib" | |
| 15 | 14 | ) | |
| @@ -324,14 +323,15 @@ func (c *Coordinator) setupOAuthAutoClose( | |||
| 324 | 323 | }) | |
| 325 | 324 | } | |
| 326 | 325 | ||
| 327 | - | func (c *Coordinator) trackOAuthPopup(popupID port.WebViewID, parentPaneID entity.PaneID) { | |
| 326 | + | func (c *Coordinator) trackOAuthPopup(popupID port.WebViewID, parentPaneID entity.PaneID, parentURIAtOpen string) { | |
| 328 | 327 | c.popupMu.Lock() | |
| 329 | 328 | defer c.popupMu.Unlock() | |
| 330 | 329 | if c.popupOAuth == nil { | |
| 331 | 330 | c.popupOAuth = make(map[port.WebViewID]*popupOAuthState) | |
| 332 | 331 | } | |
| 333 | 332 | c.popupOAuth[popupID] = &popupOAuthState{ | |
| 334 | - | ParentPaneID: parentPaneID, | |
| 333 | + | ParentPaneID: parentPaneID, | |
| 334 | + | ParentURIAtOpen: strings.TrimSpace(parentURIAtOpen), | |
| 335 | 335 | } | |
| 336 | 336 | } | |
| 337 | 337 | ||
| @@ -350,15 +350,6 @@ func (c *Coordinator) capturePopupOAuthState(popupID port.WebViewID, uri string) | |||
| 350 | 350 | state.Error = IsOAuthError(uri) | |
| 351 | 351 | } | |
| 352 | 352 | ||
| 353 | - | func shouldLoadOAuthCallbackInParent(parentURI, callbackURI string) bool { | |
| 354 | - | if callbackURI == "" || !IsOAuthCallback(callbackURI) { | |
| 355 | - | return false | |
| 356 | - | } | |
| 357 | - | parentDomain := urlutil.ExtractDomain(parentURI) | |
| 358 | - | callbackDomain := urlutil.ExtractDomain(callbackURI) | |
| 359 | - | return parentDomain != "" && parentDomain == callbackDomain | |
| 360 | - | } | |
| 361 | - | ||
| 362 | 353 | func (c *Coordinator) handlePopupOAuthClose(ctx context.Context, popupID port.WebViewID) { | |
| 363 | 354 | log := logging.FromContext(ctx) | |
| 364 | 355 | ||
| @@ -384,21 +375,18 @@ func (c *Coordinator) handlePopupOAuthClose(ctx context.Context, popupID port.We | |||
| 384 | 375 | return | |
| 385 | 376 | } | |
| 386 | 377 | ||
| 387 | - | if strings.TrimSpace(state.CallbackURI) != "" { | |
| 388 | - | c.scheduleParentPaneOAuthResume(ctx, state.ParentPaneID, popupID, state.CallbackURI) | |
| 389 | - | return | |
| 390 | - | } | |
| 391 | 378 | if !state.Success { | |
| 392 | 379 | return | |
| 393 | 380 | } | |
| 394 | 381 | ||
| 395 | - | c.scheduleParentPaneOAuthResume(ctx, state.ParentPaneID, popupID, "") | |
| 382 | + | c.scheduleParentPaneOAuthResume(ctx, state.ParentPaneID, popupID, state.ParentURIAtOpen, state.CallbackURI) | |
| 396 | 383 | } | |
| 397 | 384 | ||
| 398 | 385 | func (c *Coordinator) scheduleParentPaneOAuthResume( | |
| 399 | 386 | ctx context.Context, | |
| 400 | 387 | parentPaneID entity.PaneID, | |
| 401 | 388 | popupID port.WebViewID, | |
| 389 | + | parentURIAtOpen string, | |
| 402 | 390 | callbackURI string, | |
| 403 | 391 | ) { | |
| 404 | 392 | c.popupMu.Lock() | |
| @@ -413,7 +401,7 @@ func (c *Coordinator) scheduleParentPaneOAuthResume( | |||
| 413 | 401 | delete(c.popupRefresh, parentPaneID) | |
| 414 | 402 | c.popupMu.Unlock() | |
| 415 | 403 | cb := glib.SourceFunc(func(_ uintptr) bool { | |
| 416 | - | c.resumeParentPaneAfterOAuth(ctx, parentPaneID, popupID, callbackURI) | |
| 404 | + | c.resumeParentPaneAfterOAuth(ctx, parentPaneID, popupID, parentURIAtOpen, callbackURI) | |
| 417 | 405 | return false | |
| 418 | 406 | }) | |
| 419 | 407 | glib.IdleAdd(&cb, 0) | |
| @@ -425,6 +413,7 @@ func (c *Coordinator) resumeParentPaneAfterOAuth( | |||
| 425 | 413 | ctx context.Context, | |
| 426 | 414 | parentPaneID entity.PaneID, | |
| 427 | 415 | popupID port.WebViewID, | |
| 416 | + | parentURIAtOpen string, | |
| 428 | 417 | callbackURI string, | |
| 429 | 418 | ) { | |
| 430 | 419 | log := logging.FromContext(ctx) | |
| @@ -437,23 +426,15 @@ func (c *Coordinator) resumeParentPaneAfterOAuth( | |||
| 437 | 426 | return | |
| 438 | 427 | } | |
| 439 | 428 | ||
| 440 | - | parentURI := wv.URI() | |
| 441 | - | if shouldLoadOAuthCallbackInParent(parentURI, callbackURI) { | |
| 442 | - | if err := wv.LoadURI(ctx, callbackURI); err != nil { | |
| 443 | - | log.Warn(). | |
| 444 | - | Err(err). | |
| 445 | - | Str("parent_pane_id", string(parentPaneID)). | |
| 446 | - | Uint64("popup_id", uint64(popupID)). | |
| 447 | - | Str("callback_uri", logging.TruncateURL(callbackURI, logURLMaxLen)). | |
| 448 | - | Msg("failed to load oauth callback URI in parent pane") | |
| 449 | - | return | |
| 450 | - | } | |
| 451 | - | ||
| 429 | + | parentURI := strings.TrimSpace(wv.URI()) | |
| 430 | + | if parentURIAtOpen != "" && parentURI != "" && parentURI != parentURIAtOpen { | |
| 452 | 431 | log.Info(). | |
| 453 | 432 | Str("parent_pane_id", string(parentPaneID)). | |
| 454 | 433 | Uint64("popup_id", uint64(popupID)). | |
| 434 | + | Str("parent_uri_at_open", logging.TruncateURL(parentURIAtOpen, logURLMaxLen)). | |
| 435 | + | Str("current_parent_uri", logging.TruncateURL(parentURI, logURLMaxLen)). | |
| 455 | 436 | Str("callback_uri", logging.TruncateURL(callbackURI, logURLMaxLen)). | |
| 456 | - | Msg("loaded oauth callback URI in parent pane") | |
| 437 | + | Msg("skipping parent pane oauth resume: parent already navigated") | |
| 457 | 438 | return | |
| 458 | 439 | } | |
| 459 | 440 | ||
| @@ -469,5 +450,6 @@ func (c *Coordinator) resumeParentPaneAfterOAuth( | |||
| 469 | 450 | log.Info(). | |
| 470 | 451 | Str("parent_pane_id", string(parentPaneID)). | |
| 471 | 452 | Uint64("popup_id", uint64(popupID)). | |
| 453 | + | Str("callback_uri", logging.TruncateURL(callbackURI, logURLMaxLen)). | |
| 472 | 454 | Msg("refreshed parent pane after oauth popup close") | |
| 473 | 455 | } | |
| @@ -9,7 +9,6 @@ import ( | |||||
| 9 | 9 | ||||
| 10 | "github.com/bnema/dumber/internal/application/port" | 10 | "github.com/bnema/dumber/internal/application/port" | ||
| 11 | "github.com/bnema/dumber/internal/domain/entity" | 11 | "github.com/bnema/dumber/internal/domain/entity" | ||
| 12 | - | urlutil "github.com/bnema/dumber/internal/domain/url" | |||
| 13 | "github.com/bnema/dumber/internal/logging" | 12 | "github.com/bnema/dumber/internal/logging" | ||
| 14 | "github.com/bnema/puregotk/v4/glib" | 13 | "github.com/bnema/puregotk/v4/glib" | ||
| 15 | ) | 14 | ) | ||
| @@ -324,14 +323,15 @@ func (c *Coordinator) setupOAuthAutoClose( | |||||
| 324 | }) | 323 | }) | ||
| 325 | } | 324 | } | ||
| 326 | 325 | ||||
| 327 | - | func (c *Coordinator) trackOAuthPopup(popupID port.WebViewID, parentPaneID entity.PaneID) { | 326 | + | func (c *Coordinator) trackOAuthPopup(popupID port.WebViewID, parentPaneID entity.PaneID, parentURIAtOpen string) { |
| 328 | c.popupMu.Lock() | 327 | c.popupMu.Lock() | ||
| 329 | defer c.popupMu.Unlock() | 328 | defer c.popupMu.Unlock() | ||
| 330 | if c.popupOAuth == nil { | 329 | if c.popupOAuth == nil { | ||
| 331 | c.popupOAuth = make(map[port.WebViewID]*popupOAuthState) | 330 | c.popupOAuth = make(map[port.WebViewID]*popupOAuthState) | ||
| 332 | } | 331 | } | ||
| 333 | c.popupOAuth[popupID] = &popupOAuthState{ | 332 | c.popupOAuth[popupID] = &popupOAuthState{ | ||
| 334 | - | ParentPaneID: parentPaneID, | 333 | + | ParentPaneID: parentPaneID, |
| 334 | + | ParentURIAtOpen: strings.TrimSpace(parentURIAtOpen), | |||
| 335 | } | 335 | } | ||
| 336 | } | 336 | } | ||
| 337 | 337 | ||||
| @@ -350,15 +350,6 @@ func (c *Coordinator) capturePopupOAuthState(popupID port.WebViewID, uri string) | |||||
| 350 | state.Error = IsOAuthError(uri) | 350 | state.Error = IsOAuthError(uri) | ||
| 351 | } | 351 | } | ||
| 352 | 352 | ||||
| 353 | - | func shouldLoadOAuthCallbackInParent(parentURI, callbackURI string) bool { | |||
| 354 | - | if callbackURI == "" || !IsOAuthCallback(callbackURI) { | |||
| 355 | - | return false | |||
| 356 | - | } | |||
| 357 | - | parentDomain := urlutil.ExtractDomain(parentURI) | |||
| 358 | - | callbackDomain := urlutil.ExtractDomain(callbackURI) | |||
| 359 | - | return parentDomain != "" && parentDomain == callbackDomain | |||
| 360 | - | } | |||
| 361 | - | ||||
| 362 | func (c *Coordinator) handlePopupOAuthClose(ctx context.Context, popupID port.WebViewID) { | 353 | func (c *Coordinator) handlePopupOAuthClose(ctx context.Context, popupID port.WebViewID) { | ||
| 363 | log := logging.FromContext(ctx) | 354 | log := logging.FromContext(ctx) | ||
| 364 | 355 | ||||
| @@ -384,21 +375,18 @@ func (c *Coordinator) handlePopupOAuthClose(ctx context.Context, popupID port.We | |||||
| 384 | return | 375 | return | ||
| 385 | } | 376 | } | ||
| 386 | 377 | ||||
| 387 | - | if strings.TrimSpace(state.CallbackURI) != "" { | |||
| 388 | - | c.scheduleParentPaneOAuthResume(ctx, state.ParentPaneID, popupID, state.CallbackURI) | |||
| 389 | - | return | |||
| 390 | - | } | |||
| 391 | if !state.Success { | 378 | if !state.Success { | ||
| 392 | return | 379 | return | ||
| 393 | } | 380 | } | ||
| 394 | 381 | ||||
| 395 | - | c.scheduleParentPaneOAuthResume(ctx, state.ParentPaneID, popupID, "") | 382 | + | c.scheduleParentPaneOAuthResume(ctx, state.ParentPaneID, popupID, state.ParentURIAtOpen, state.CallbackURI) |
| 396 | } | 383 | } | ||
| 397 | 384 | ||||
| 398 | func (c *Coordinator) scheduleParentPaneOAuthResume( | 385 | func (c *Coordinator) scheduleParentPaneOAuthResume( | ||
| 399 | ctx context.Context, | 386 | ctx context.Context, | ||
| 400 | parentPaneID entity.PaneID, | 387 | parentPaneID entity.PaneID, | ||
| 401 | popupID port.WebViewID, | 388 | popupID port.WebViewID, | ||
| 389 | + | parentURIAtOpen string, | |||
| 402 | callbackURI string, | 390 | callbackURI string, | ||
| 403 | ) { | 391 | ) { | ||
| 404 | c.popupMu.Lock() | 392 | c.popupMu.Lock() | ||
| @@ -413,7 +401,7 @@ func (c *Coordinator) scheduleParentPaneOAuthResume( | |||||
| 413 | delete(c.popupRefresh, parentPaneID) | 401 | delete(c.popupRefresh, parentPaneID) | ||
| 414 | c.popupMu.Unlock() | 402 | c.popupMu.Unlock() | ||
| 415 | cb := glib.SourceFunc(func(_ uintptr) bool { | 403 | cb := glib.SourceFunc(func(_ uintptr) bool { | ||
| 416 | - | c.resumeParentPaneAfterOAuth(ctx, parentPaneID, popupID, callbackURI) | 404 | + | c.resumeParentPaneAfterOAuth(ctx, parentPaneID, popupID, parentURIAtOpen, callbackURI) |
| 417 | return false | 405 | return false | ||
| 418 | }) | 406 | }) | ||
| 419 | glib.IdleAdd(&cb, 0) | 407 | glib.IdleAdd(&cb, 0) | ||
| @@ -425,6 +413,7 @@ func (c *Coordinator) resumeParentPaneAfterOAuth( | |||||
| 425 | ctx context.Context, | 413 | ctx context.Context, | ||
| 426 | parentPaneID entity.PaneID, | 414 | parentPaneID entity.PaneID, | ||
| 427 | popupID port.WebViewID, | 415 | popupID port.WebViewID, | ||
| 416 | + | parentURIAtOpen string, | |||
| 428 | callbackURI string, | 417 | callbackURI string, | ||
| 429 | ) { | 418 | ) { | ||
| 430 | log := logging.FromContext(ctx) | 419 | log := logging.FromContext(ctx) | ||
| @@ -437,23 +426,15 @@ func (c *Coordinator) resumeParentPaneAfterOAuth( | |||||
| 437 | return | 426 | return | ||
| 438 | } | 427 | } | ||
| 439 | 428 | ||||
| 440 | - | parentURI := wv.URI() | 429 | + | parentURI := strings.TrimSpace(wv.URI()) |
| 441 | - | if shouldLoadOAuthCallbackInParent(parentURI, callbackURI) { | 430 | + | if parentURIAtOpen != "" && parentURI != "" && parentURI != parentURIAtOpen { |
| 442 | - | if err := wv.LoadURI(ctx, callbackURI); err != nil { | |||
| 443 | - | log.Warn(). | |||
| 444 | - | Err(err). | |||
| 445 | - | Str("parent_pane_id", string(parentPaneID)). | |||
| 446 | - | Uint64("popup_id", uint64(popupID)). | |||
| 447 | - | Str("callback_uri", logging.TruncateURL(callbackURI, logURLMaxLen)). | |||
| 448 | - | Msg("failed to load oauth callback URI in parent pane") | |||
| 449 | - | return | |||
| 450 | - | } | |||
| 451 | - | ||||
| 452 | log.Info(). | 431 | log.Info(). | ||
| 453 | Str("parent_pane_id", string(parentPaneID)). | 432 | Str("parent_pane_id", string(parentPaneID)). | ||
| 454 | Uint64("popup_id", uint64(popupID)). | 433 | Uint64("popup_id", uint64(popupID)). | ||
| 434 | + | Str("parent_uri_at_open", logging.TruncateURL(parentURIAtOpen, logURLMaxLen)). | |||
| 435 | + | Str("current_parent_uri", logging.TruncateURL(parentURI, logURLMaxLen)). | |||
| 455 | Str("callback_uri", logging.TruncateURL(callbackURI, logURLMaxLen)). | 436 | Str("callback_uri", logging.TruncateURL(callbackURI, logURLMaxLen)). | ||
| 456 | - | Msg("loaded oauth callback URI in parent pane") | 437 | + | Msg("skipping parent pane oauth resume: parent already navigated") |
| 457 | return | 438 | return | ||
| 458 | } | 439 | } | ||
| 459 | 440 | ||||
| @@ -469,5 +450,6 @@ func (c *Coordinator) resumeParentPaneAfterOAuth( | |||||
| 469 | log.Info(). | 450 | log.Info(). | ||
| 470 | Str("parent_pane_id", string(parentPaneID)). | 451 | Str("parent_pane_id", string(parentPaneID)). | ||
| 471 | Uint64("popup_id", uint64(popupID)). | 452 | Uint64("popup_id", uint64(popupID)). | ||
| 453 | + | Str("callback_uri", logging.TruncateURL(callbackURI, logURLMaxLen)). | |||
| 472 | Msg("refreshed parent pane after oauth popup close") | 454 | Msg("refreshed parent pane after oauth popup close") | ||
| 473 | } | 455 | } | ||
M internal/ui/coordinator/content/oauth_test.go +22 -6
| @@ -59,7 +59,7 @@ func TestHandlePopupOAuthClose_SuccessSchedulesParentResume(t *testing.T) { | |||
| 59 | 59 | popupRefresh: make(map[entity.PaneID]*time.Timer), | |
| 60 | 60 | } | |
| 61 | 61 | ||
| 62 | - | c.trackOAuthPopup(popupID, parentPaneID) | |
| 62 | + | c.trackOAuthPopup(popupID, parentPaneID, "https://www.notion.so/login") | |
| 63 | 63 | c.capturePopupOAuthState(popupID, "https://www.notion.so/googlepopupcallback?code=123") | |
| 64 | 64 | c.handlePopupOAuthClose(context.Background(), popupID) | |
| 65 | 65 | ||
| @@ -78,13 +78,13 @@ func TestHandlePopupOAuthClose_SuccessSchedulesParentResume(t *testing.T) { | |||
| 78 | 78 | }) | |
| 79 | 79 | } | |
| 80 | 80 | ||
| 81 | - | func TestResumeParentPaneAfterOAuth_SameDomainLoadsCallbackURI(t *testing.T) { | |
| 81 | + | func TestResumeParentPaneAfterOAuth_UnchangedParentFallsBackToReload(t *testing.T) { | |
| 82 | 82 | parentPaneID := entity.PaneID("parent-pane") | |
| 83 | 83 | popupID := port.WebViewID(102) | |
| 84 | 84 | parentWV := portmocks.NewMockWebView(t) | |
| 85 | 85 | parentWV.EXPECT().IsDestroyed().Return(false).Once() | |
| 86 | 86 | parentWV.EXPECT().URI().Return("https://www.notion.so/login").Once() | |
| 87 | - | parentWV.EXPECT().LoadURI(mock.Anything, "https://www.notion.so/googlepopupcallback?code=123").Return(nil).Once() | |
| 87 | + | parentWV.EXPECT().Reload(mock.Anything).Return(nil).Once() | |
| 88 | 88 | ||
| 89 | 89 | c := &Coordinator{ | |
| 90 | 90 | webViews: map[entity.PaneID]port.WebView{ | |
| @@ -92,14 +92,30 @@ func TestResumeParentPaneAfterOAuth_SameDomainLoadsCallbackURI(t *testing.T) { | |||
| 92 | 92 | }, | |
| 93 | 93 | } | |
| 94 | 94 | ||
| 95 | - | c.resumeParentPaneAfterOAuth(context.Background(), parentPaneID, popupID, "https://www.notion.so/googlepopupcallback?code=123") | |
| 95 | + | c.resumeParentPaneAfterOAuth(context.Background(), parentPaneID, popupID, "https://www.notion.so/login", "https://www.notion.so/googlepopupcallback?code=123") | |
| 96 | 96 | } | |
| 97 | 97 | ||
| 98 | - | func TestResumeParentPaneAfterOAuth_DifferentDomainFallsBackToReload(t *testing.T) { | |
| 98 | + | func TestResumeParentPaneAfterOAuth_ChangedParentSkipsIntervention(t *testing.T) { | |
| 99 | 99 | parentPaneID := entity.PaneID("parent-pane") | |
| 100 | 100 | popupID := port.WebViewID(103) | |
| 101 | 101 | parentWV := portmocks.NewMockWebView(t) | |
| 102 | 102 | parentWV.EXPECT().IsDestroyed().Return(false).Once() | |
| 103 | + | parentWV.EXPECT().URI().Return("https://www.notion.so/oauth2callback?state=abc").Once() | |
| 104 | + | ||
| 105 | + | c := &Coordinator{ | |
| 106 | + | webViews: map[entity.PaneID]port.WebView{ | |
| 107 | + | parentPaneID: parentWV, | |
| 108 | + | }, | |
| 109 | + | } | |
| 110 | + | ||
| 111 | + | c.resumeParentPaneAfterOAuth(context.Background(), parentPaneID, popupID, "https://www.notion.so/login", "https://www.notion.so/googlepopupcallback?code=123") | |
| 112 | + | } | |
| 113 | + | ||
| 114 | + | func TestResumeParentPaneAfterOAuth_DifferentDomainFallsBackToReload(t *testing.T) { | |
| 115 | + | parentPaneID := entity.PaneID("parent-pane") | |
| 116 | + | popupID := port.WebViewID(104) | |
| 117 | + | parentWV := portmocks.NewMockWebView(t) | |
| 118 | + | parentWV.EXPECT().IsDestroyed().Return(false).Once() | |
| 103 | 119 | parentWV.EXPECT().URI().Return("https://www.notion.so/login").Once() | |
| 104 | 120 | parentWV.EXPECT().Reload(mock.Anything).Return(nil).Once() | |
| 105 | 121 | ||
| @@ -109,7 +125,7 @@ func TestResumeParentPaneAfterOAuth_DifferentDomainFallsBackToReload(t *testing. | |||
| 109 | 125 | }, | |
| 110 | 126 | } | |
| 111 | 127 | ||
| 112 | - | c.resumeParentPaneAfterOAuth(context.Background(), parentPaneID, popupID, "https://accounts.example.com/callback?code=123") | |
| 128 | + | c.resumeParentPaneAfterOAuth(context.Background(), parentPaneID, popupID, "https://www.notion.so/login", "https://accounts.example.com/callback?code=123") | |
| 113 | 129 | } | |
| 114 | 130 | ||
| 115 | 131 | func TestIsOAuthURL_DoesNotTreatGenericStateParamAsOAuth(t *testing.T) { | |
| @@ -59,7 +59,7 @@ func TestHandlePopupOAuthClose_SuccessSchedulesParentResume(t *testing.T) { | |||||
| 59 | popupRefresh: make(map[entity.PaneID]*time.Timer), | 59 | popupRefresh: make(map[entity.PaneID]*time.Timer), | ||
| 60 | } | 60 | } | ||
| 61 | 61 | ||||
| 62 | - | c.trackOAuthPopup(popupID, parentPaneID) | 62 | + | c.trackOAuthPopup(popupID, parentPaneID, "https://www.notion.so/login") |
| 63 | c.capturePopupOAuthState(popupID, "https://www.notion.so/googlepopupcallback?code=123") | 63 | c.capturePopupOAuthState(popupID, "https://www.notion.so/googlepopupcallback?code=123") | ||
| 64 | c.handlePopupOAuthClose(context.Background(), popupID) | 64 | c.handlePopupOAuthClose(context.Background(), popupID) | ||
| 65 | 65 | ||||
| @@ -78,13 +78,13 @@ func TestHandlePopupOAuthClose_SuccessSchedulesParentResume(t *testing.T) { | |||||
| 78 | }) | 78 | }) | ||
| 79 | } | 79 | } | ||
| 80 | 80 | ||||
| 81 | - | func TestResumeParentPaneAfterOAuth_SameDomainLoadsCallbackURI(t *testing.T) { | 81 | + | func TestResumeParentPaneAfterOAuth_UnchangedParentFallsBackToReload(t *testing.T) { |
| 82 | parentPaneID := entity.PaneID("parent-pane") | 82 | parentPaneID := entity.PaneID("parent-pane") | ||
| 83 | popupID := port.WebViewID(102) | 83 | popupID := port.WebViewID(102) | ||
| 84 | parentWV := portmocks.NewMockWebView(t) | 84 | parentWV := portmocks.NewMockWebView(t) | ||
| 85 | parentWV.EXPECT().IsDestroyed().Return(false).Once() | 85 | parentWV.EXPECT().IsDestroyed().Return(false).Once() | ||
| 86 | parentWV.EXPECT().URI().Return("https://www.notion.so/login").Once() | 86 | parentWV.EXPECT().URI().Return("https://www.notion.so/login").Once() | ||
| 87 | - | parentWV.EXPECT().LoadURI(mock.Anything, "https://www.notion.so/googlepopupcallback?code=123").Return(nil).Once() | 87 | + | parentWV.EXPECT().Reload(mock.Anything).Return(nil).Once() |
| 88 | 88 | ||||
| 89 | c := &Coordinator{ | 89 | c := &Coordinator{ | ||
| 90 | webViews: map[entity.PaneID]port.WebView{ | 90 | webViews: map[entity.PaneID]port.WebView{ | ||
| @@ -92,14 +92,30 @@ func TestResumeParentPaneAfterOAuth_SameDomainLoadsCallbackURI(t *testing.T) { | |||||
| 92 | }, | 92 | }, | ||
| 93 | } | 93 | } | ||
| 94 | 94 | ||||
| 95 | - | c.resumeParentPaneAfterOAuth(context.Background(), parentPaneID, popupID, "https://www.notion.so/googlepopupcallback?code=123") | 95 | + | c.resumeParentPaneAfterOAuth(context.Background(), parentPaneID, popupID, "https://www.notion.so/login", "https://www.notion.so/googlepopupcallback?code=123") |
| 96 | } | 96 | } | ||
| 97 | 97 | ||||
| 98 | - | func TestResumeParentPaneAfterOAuth_DifferentDomainFallsBackToReload(t *testing.T) { | 98 | + | func TestResumeParentPaneAfterOAuth_ChangedParentSkipsIntervention(t *testing.T) { |
| 99 | parentPaneID := entity.PaneID("parent-pane") | 99 | parentPaneID := entity.PaneID("parent-pane") | ||
| 100 | popupID := port.WebViewID(103) | 100 | popupID := port.WebViewID(103) | ||
| 101 | parentWV := portmocks.NewMockWebView(t) | 101 | parentWV := portmocks.NewMockWebView(t) | ||
| 102 | parentWV.EXPECT().IsDestroyed().Return(false).Once() | 102 | parentWV.EXPECT().IsDestroyed().Return(false).Once() | ||
| 103 | + | parentWV.EXPECT().URI().Return("https://www.notion.so/oauth2callback?state=abc").Once() | |||
| 104 | + | ||||
| 105 | + | c := &Coordinator{ | |||
| 106 | + | webViews: map[entity.PaneID]port.WebView{ | |||
| 107 | + | parentPaneID: parentWV, | |||
| 108 | + | }, | |||
| 109 | + | } | |||
| 110 | + | ||||
| 111 | + | c.resumeParentPaneAfterOAuth(context.Background(), parentPaneID, popupID, "https://www.notion.so/login", "https://www.notion.so/googlepopupcallback?code=123") | |||
| 112 | + | } | |||
| 113 | + | ||||
| 114 | + | func TestResumeParentPaneAfterOAuth_DifferentDomainFallsBackToReload(t *testing.T) { | |||
| 115 | + | parentPaneID := entity.PaneID("parent-pane") | |||
| 116 | + | popupID := port.WebViewID(104) | |||
| 117 | + | parentWV := portmocks.NewMockWebView(t) | |||
| 118 | + | parentWV.EXPECT().IsDestroyed().Return(false).Once() | |||
| 103 | parentWV.EXPECT().URI().Return("https://www.notion.so/login").Once() | 119 | parentWV.EXPECT().URI().Return("https://www.notion.so/login").Once() | ||
| 104 | parentWV.EXPECT().Reload(mock.Anything).Return(nil).Once() | 120 | parentWV.EXPECT().Reload(mock.Anything).Return(nil).Once() | ||
| 105 | 121 | ||||
| @@ -109,7 +125,7 @@ func TestResumeParentPaneAfterOAuth_DifferentDomainFallsBackToReload(t *testing. | |||||
| 109 | }, | 125 | }, | ||
| 110 | } | 126 | } | ||
| 111 | 127 | ||||
| 112 | - | c.resumeParentPaneAfterOAuth(context.Background(), parentPaneID, popupID, "https://accounts.example.com/callback?code=123") | 128 | + | c.resumeParentPaneAfterOAuth(context.Background(), parentPaneID, popupID, "https://www.notion.so/login", "https://accounts.example.com/callback?code=123") |
| 113 | } | 129 | } | ||
| 114 | 130 | ||||
| 115 | func TestIsOAuthURL_DoesNotTreatGenericStateParamAsOAuth(t *testing.T) { | 131 | func TestIsOAuthURL_DoesNotTreatGenericStateParamAsOAuth(t *testing.T) { | ||
M internal/ui/coordinator/content/popup.go +7 -2
| @@ -434,6 +434,10 @@ func (c *Coordinator) handlePopupCreate( | |||
| 434 | 434 | } | |
| 435 | 435 | ||
| 436 | 436 | parentID := parentWV.ID() | |
| 437 | + | parentURIAtOpen := "" | |
| 438 | + | if parent := c.getWebViewLocked(parentPaneID); parent != nil { | |
| 439 | + | parentURIAtOpen = parent.URI() | |
| 440 | + | } | |
| 437 | 441 | if !req.NoJavaScriptAccess { | |
| 438 | 442 | if reused, ok := c.reuseNamedPopup(ctx, parentPaneID, req.FrameName, req.TargetURI); ok { | |
| 439 | 443 | return reused | |
| @@ -467,7 +471,7 @@ func (c *Coordinator) handlePopupCreate( | |||
| 467 | 471 | // Create popup pane entity | |
| 468 | 472 | paneID, popupPane := c.createPopupPane(popupID, parentPaneID, req.TargetURI) | |
| 469 | 473 | ||
| 470 | - | return c.finishPopupCreate(ctx, parentPaneID, parentID, popupID, popupWV, popupPane, paneID, popupType, behavior, placement, req) | |
| 474 | + | return c.finishPopupCreate(ctx, parentPaneID, parentID, parentURIAtOpen, popupID, popupWV, popupPane, paneID, popupType, behavior, placement, req) | |
| 471 | 475 | } | |
| 472 | 476 | ||
| 473 | 477 | func (c *Coordinator) reuseNamedPopup( | |
| @@ -500,6 +504,7 @@ func (c *Coordinator) finishPopupCreate( | |||
| 500 | 504 | ctx context.Context, | |
| 501 | 505 | parentPaneID entity.PaneID, | |
| 502 | 506 | parentID port.WebViewID, | |
| 507 | + | parentURIAtOpen string, | |
| 503 | 508 | popupID port.WebViewID, | |
| 504 | 509 | popupWV port.WebView, | |
| 505 | 510 | popupPane *entity.Pane, | |
| @@ -564,7 +569,7 @@ func (c *Coordinator) finishPopupCreate( | |||
| 564 | 569 | // Setup OAuth auto-close if configured | |
| 565 | 570 | if hasConfig && oauthEnabled && isOAuth { | |
| 566 | 571 | popupPane.AutoClose = true | |
| 567 | - | c.trackOAuthPopup(popupID, parentPaneID) | |
| 572 | + | c.trackOAuthPopup(popupID, parentPaneID, parentURIAtOpen) | |
| 568 | 573 | c.setupOAuthAutoClose(ctx, paneID, popupID, popupWV) | |
| 569 | 574 | log.Debug().Str("pane_id", string(paneID)).Msg("OAuth auto-close enabled for popup") | |
| 570 | 575 | } | |
| @@ -434,6 +434,10 @@ func (c *Coordinator) handlePopupCreate( | |||||
| 434 | } | 434 | } | ||
| 435 | 435 | ||||
| 436 | parentID := parentWV.ID() | 436 | parentID := parentWV.ID() | ||
| 437 | + | parentURIAtOpen := "" | |||
| 438 | + | if parent := c.getWebViewLocked(parentPaneID); parent != nil { | |||
| 439 | + | parentURIAtOpen = parent.URI() | |||
| 440 | + | } | |||
| 437 | if !req.NoJavaScriptAccess { | 441 | if !req.NoJavaScriptAccess { | ||
| 438 | if reused, ok := c.reuseNamedPopup(ctx, parentPaneID, req.FrameName, req.TargetURI); ok { | 442 | if reused, ok := c.reuseNamedPopup(ctx, parentPaneID, req.FrameName, req.TargetURI); ok { | ||
| 439 | return reused | 443 | return reused | ||
| @@ -467,7 +471,7 @@ func (c *Coordinator) handlePopupCreate( | |||||
| 467 | // Create popup pane entity | 471 | // Create popup pane entity | ||
| 468 | paneID, popupPane := c.createPopupPane(popupID, parentPaneID, req.TargetURI) | 472 | paneID, popupPane := c.createPopupPane(popupID, parentPaneID, req.TargetURI) | ||
| 469 | 473 | ||||
| 470 | - | return c.finishPopupCreate(ctx, parentPaneID, parentID, popupID, popupWV, popupPane, paneID, popupType, behavior, placement, req) | 474 | + | return c.finishPopupCreate(ctx, parentPaneID, parentID, parentURIAtOpen, popupID, popupWV, popupPane, paneID, popupType, behavior, placement, req) |
| 471 | } | 475 | } | ||
| 472 | 476 | ||||
| 473 | func (c *Coordinator) reuseNamedPopup( | 477 | func (c *Coordinator) reuseNamedPopup( | ||
| @@ -500,6 +504,7 @@ func (c *Coordinator) finishPopupCreate( | |||||
| 500 | ctx context.Context, | 504 | ctx context.Context, | ||
| 501 | parentPaneID entity.PaneID, | 505 | parentPaneID entity.PaneID, | ||
| 502 | parentID port.WebViewID, | 506 | parentID port.WebViewID, | ||
| 507 | + | parentURIAtOpen string, | |||
| 503 | popupID port.WebViewID, | 508 | popupID port.WebViewID, | ||
| 504 | popupWV port.WebView, | 509 | popupWV port.WebView, | ||
| 505 | popupPane *entity.Pane, | 510 | popupPane *entity.Pane, | ||
| @@ -564,7 +569,7 @@ func (c *Coordinator) finishPopupCreate( | |||||
| 564 | // Setup OAuth auto-close if configured | 569 | // Setup OAuth auto-close if configured | ||
| 565 | if hasConfig && oauthEnabled && isOAuth { | 570 | if hasConfig && oauthEnabled && isOAuth { | ||
| 566 | popupPane.AutoClose = true | 571 | popupPane.AutoClose = true | ||
| 567 | - | c.trackOAuthPopup(popupID, parentPaneID) | 572 | + | c.trackOAuthPopup(popupID, parentPaneID, parentURIAtOpen) |
| 568 | c.setupOAuthAutoClose(ctx, paneID, popupID, popupWV) | 573 | c.setupOAuthAutoClose(ctx, paneID, popupID, popupWV) | ||
| 569 | log.Debug().Str("pane_id", string(paneID)).Msg("OAuth auto-close enabled for popup") | 574 | log.Debug().Str("pane_id", string(paneID)).Msg("OAuth auto-close enabled for popup") | ||
| 570 | } | 575 | } | ||
M internal/ui/coordinator/content/popup_test.go +4 -3
| @@ -388,7 +388,7 @@ func TestTrackOAuthPopup_StoresState(t *testing.T) { | |||
| 388 | 388 | popupID := port.WebViewID(1) | |
| 389 | 389 | parentPaneID := entity.PaneID("parent-pane") | |
| 390 | 390 | ||
| 391 | - | c.trackOAuthPopup(popupID, parentPaneID) | |
| 391 | + | c.trackOAuthPopup(popupID, parentPaneID, "https://parent.example.com/login") | |
| 392 | 392 | ||
| 393 | 393 | c.popupMu.RLock() | |
| 394 | 394 | state, ok := c.popupOAuth[popupID] | |
| @@ -397,6 +397,7 @@ func TestTrackOAuthPopup_StoresState(t *testing.T) { | |||
| 397 | 397 | require.True(t, ok, "oauth state should be tracked after trackOAuthPopup") | |
| 398 | 398 | require.NotNil(t, state) | |
| 399 | 399 | assert.Equal(t, parentPaneID, state.ParentPaneID) | |
| 400 | + | assert.Equal(t, "https://parent.example.com/login", state.ParentURIAtOpen) | |
| 400 | 401 | assert.False(t, state.Seen, "Seen should be false before capturing callback URI") | |
| 401 | 402 | } | |
| 402 | 403 | ||
| @@ -409,7 +410,7 @@ func TestCapturePopupOAuthState_SuccessCallback(t *testing.T) { | |||
| 409 | 410 | } | |
| 410 | 411 | ||
| 411 | 412 | popupID := port.WebViewID(2) | |
| 412 | - | c.trackOAuthPopup(popupID, entity.PaneID("parent")) | |
| 413 | + | c.trackOAuthPopup(popupID, entity.PaneID("parent"), "https://parent.example.com/login") | |
| 413 | 414 | c.capturePopupOAuthState(popupID, "https://app.example.com/callback?code=abc123") | |
| 414 | 415 | ||
| 415 | 416 | c.popupMu.RLock() | |
| @@ -432,7 +433,7 @@ func TestCapturePopupOAuthState_ErrorCallback(t *testing.T) { | |||
| 432 | 433 | } | |
| 433 | 434 | ||
| 434 | 435 | popupID := port.WebViewID(3) | |
| 435 | - | c.trackOAuthPopup(popupID, entity.PaneID("parent")) | |
| 436 | + | c.trackOAuthPopup(popupID, entity.PaneID("parent"), "https://parent.example.com/login") | |
| 436 | 437 | c.capturePopupOAuthState(popupID, "https://app.example.com/callback?error=access_denied") | |
| 437 | 438 | ||
| 438 | 439 | c.popupMu.RLock() | |
| @@ -388,7 +388,7 @@ func TestTrackOAuthPopup_StoresState(t *testing.T) { | |||||
| 388 | popupID := port.WebViewID(1) | 388 | popupID := port.WebViewID(1) | ||
| 389 | parentPaneID := entity.PaneID("parent-pane") | 389 | parentPaneID := entity.PaneID("parent-pane") | ||
| 390 | 390 | ||||
| 391 | - | c.trackOAuthPopup(popupID, parentPaneID) | 391 | + | c.trackOAuthPopup(popupID, parentPaneID, "https://parent.example.com/login") |
| 392 | 392 | ||||
| 393 | c.popupMu.RLock() | 393 | c.popupMu.RLock() | ||
| 394 | state, ok := c.popupOAuth[popupID] | 394 | state, ok := c.popupOAuth[popupID] | ||
| @@ -397,6 +397,7 @@ func TestTrackOAuthPopup_StoresState(t *testing.T) { | |||||
| 397 | require.True(t, ok, "oauth state should be tracked after trackOAuthPopup") | 397 | require.True(t, ok, "oauth state should be tracked after trackOAuthPopup") | ||
| 398 | require.NotNil(t, state) | 398 | require.NotNil(t, state) | ||
| 399 | assert.Equal(t, parentPaneID, state.ParentPaneID) | 399 | assert.Equal(t, parentPaneID, state.ParentPaneID) | ||
| 400 | + | assert.Equal(t, "https://parent.example.com/login", state.ParentURIAtOpen) | |||
| 400 | assert.False(t, state.Seen, "Seen should be false before capturing callback URI") | 401 | assert.False(t, state.Seen, "Seen should be false before capturing callback URI") | ||
| 401 | } | 402 | } | ||
| 402 | 403 | ||||
| @@ -409,7 +410,7 @@ func TestCapturePopupOAuthState_SuccessCallback(t *testing.T) { | |||||
| 409 | } | 410 | } | ||
| 410 | 411 | ||||
| 411 | popupID := port.WebViewID(2) | 412 | popupID := port.WebViewID(2) | ||
| 412 | - | c.trackOAuthPopup(popupID, entity.PaneID("parent")) | 413 | + | c.trackOAuthPopup(popupID, entity.PaneID("parent"), "https://parent.example.com/login") |
| 413 | c.capturePopupOAuthState(popupID, "https://app.example.com/callback?code=abc123") | 414 | c.capturePopupOAuthState(popupID, "https://app.example.com/callback?code=abc123") | ||
| 414 | 415 | ||||
| 415 | c.popupMu.RLock() | 416 | c.popupMu.RLock() | ||
| @@ -432,7 +433,7 @@ func TestCapturePopupOAuthState_ErrorCallback(t *testing.T) { | |||||
| 432 | } | 433 | } | ||
| 433 | 434 | ||||
| 434 | popupID := port.WebViewID(3) | 435 | popupID := port.WebViewID(3) | ||
| 435 | - | c.trackOAuthPopup(popupID, entity.PaneID("parent")) | 436 | + | c.trackOAuthPopup(popupID, entity.PaneID("parent"), "https://parent.example.com/login") |
| 436 | c.capturePopupOAuthState(popupID, "https://app.example.com/callback?error=access_denied") | 437 | c.capturePopupOAuthState(popupID, "https://app.example.com/callback?error=access_denied") | ||
| 437 | 438 | ||||
| 438 | c.popupMu.RLock() | 439 | c.popupMu.RLock() | ||