Skip to content

0018: app: Register OAuth callback providers - #7281

Open
illume wants to merge 6 commits into
kubernetes-sigs:mainfrom
illume:upstream-oauth-provider-registry-0018
Open

0018: app: Register OAuth callback providers#7281
illume wants to merge 6 commits into
kubernetes-sigs:mainfrom
illume:upstream-oauth-provider-registry-0018

Conversation

@illume

@illume illume commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a validated registry for desktop OAuth callback providers
  • route startup, second-instance, and macOS protocol callbacks to registered providers
  • keep unmatched product protocol URLs on the existing deep-link path
  • cover registration, ownership, dispatch, and failure behavior with focused unit and Electron process tests

Stack

This PR is stacked on prerequisite patch 0009:

The first three commits belong to that prerequisite; the final three commits are patch 0018. This PR will be rebased onto main after #7252 merges.

Provenance

The implementation commit preserves Thomas Gamble as author and the original author date, with Rene Dudfield as co-author.

Improvements over the existing changes

  • Put focused unit and Electron process tests before the implementation commit so reviewers can see the intended behavior and regression boundaries independently of the production change.
  • Raise focused registry branch coverage from the retained tests to 100%, with an enforced 80% threshold, so validation, cleanup, dispatch, and error paths cannot regress without failing the focused check.
  • Document exported interfaces, fields, callbacks, parameters, and return values with TSDoc so downstream provider implementations have an explicit contract instead of relying on implementation details.
  • Reject callback paths containing traversal segments so registrations cannot claim ambiguous or path-normalized routes that differ from the exact callback URL being dispatched.
  • Verify case-normalized callback ownership, stale cleanup safety, and synchronous and asynchronous provider failures so route ownership remains deterministic and one failing provider cannot disrupt protocol handling.
  • Exercise provider registration and callback dispatch in a real Electron main process without requiring Kubernetes so the process boundary is tested quickly and reliably in CI.
  • Buffer macOS launch callbacks until providers and the application window are ready because Electron can emit open-url before ready; without buffering, a cold-start OAuth callback can be lost.
  • Keep protocol listeners, startup arguments, window focus, validation, dialog errors, and renderer fallback out of main.ts so the Electron entry point remains wiring-focused and the protocol lifecycle can be tested as one encapsulated unit.

Testing

  • make app-i18n-check
  • npm --prefix app test -- electron/protocol.test.ts electron/oauthProvider.test.ts electron/protocolHandler.test.ts (55 tests)
  • focused Istanbul coverage for app/electron/oauthProvider.ts and app/electron/protocolHandler.ts: 100% branches (80% threshold enforced)
  • npm --prefix app/e2e-tests run test-app -- tests/protocolScheme.spec.ts tests/oauthProvider.spec.ts --workers=1 (2 tests)
  • npm --prefix app run tsc
  • npm --prefix app run compile-electron -- --dev
  • git diff --check 8464d801..HEAD

Screenshots

Not applicable. This change adds main-process callback infrastructure with no visual difference.

Assisted by copilot.

illume and others added 6 commits August 15, 2026 10:38
Cover validation, ownership cleanup, dispatch matching, and provider failures
before introducing the desktop OAuth callback registry.
Exercise provider registration and protocol callback dispatch in a real
Electron main process without requiring a Kubernetes cluster.
Route validated product protocol callbacks to their owning OAuth provider so
desktop integrations can complete authentication safely.

Co-authored-by: René Dudfield <renedudfield@microsoft.com>
Exercise the default protocol through Electron so configurable schemes
preserve the existing renderer route.
Pin the Istanbul provider to the app Vitest version so protocol branch
coverage can be measured reproducibly.
Use each product's packaged protocol for deep links so branded desktop
apps do not handle one another's callbacks.

Co-authored-by: René Dudfield <renedudfield@microsoft.com>
@kubernetes-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: illume

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubernetes-prow kubernetes-prow Bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 16, 2026
@illume
illume requested a balanced review from Copilot August 16, 2026 07:21
@illume
illume marked this pull request as draft August 16, 2026 07:21
@kubernetes-prow kubernetes-prow Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 16, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds desktop OAuth callback provider registration and routes product protocol URLs through registered providers.

Changes:

  • Adds validated OAuth provider registration and dispatch.
  • Integrates startup, second-instance, and macOS callback handling.
  • Adds unit, coverage, and Electron process tests.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
app/package.json Adds Istanbul coverage support.
app/package-lock.json Locks coverage dependencies.
app/electron/protocol.ts Reads and validates protocol schemes.
app/electron/protocol.test.ts Tests protocol utilities.
app/electron/oauth-provider.ts Implements the provider registry.
app/electron/oauth-provider.test.ts Tests registry behavior.
app/electron/main.ts Routes protocol callbacks.
app/e2e-tests/tests/protocolScheme.spec.ts Tests custom deep-link routing.
app/e2e-tests/tests/oauthProvider.spec.ts Tests Electron callback dispatch.
app/e2e-tests/fixtures/oauth-provider-main.ts Provides the Electron test fixture.
Files not reviewed (1)
  • app/package-lock.json: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread app/electron/main.ts Outdated
Comment thread app/electron/oauth-provider.ts
Comment thread app/electron/oauth-provider.test.ts
@illume
illume force-pushed the upstream-oauth-provider-registry-0018 branch from e6b98c1 to 7976769 Compare August 16, 2026 07:35
@illume
illume force-pushed the upstream-oauth-provider-registry-0018 branch from 7976769 to f3862a6 Compare August 16, 2026 08:12
@illume
illume marked this pull request as ready for review August 16, 2026 08:45
@kubernetes-prow kubernetes-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 16, 2026
@illume
illume requested a balanced review from Copilot August 16, 2026 08:45
@kubernetes-prow
kubernetes-prow Bot requested a review from sniok August 16, 2026 08:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • app/package-lock.json: Generated file

Comment thread app/package-lock.json Outdated
@illume
illume force-pushed the upstream-oauth-provider-registry-0018 branch from f3862a6 to 64ae9a5 Compare August 16, 2026 09:14
@illume
illume marked this pull request as draft August 16, 2026 09:32
@kubernetes-prow kubernetes-prow Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 16, 2026
@illume
illume force-pushed the upstream-oauth-provider-registry-0018 branch from 64ae9a5 to 267bc30 Compare August 16, 2026 10:35
@illume
illume requested a balanced review from Copilot August 16, 2026 11:36
@illume illume added this to the v0.45.0 milestone Aug 16, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • app/package-lock.json: Generated file
Suppressed comments (2)

app/electron/protocolHandler.ts:143

  • This drains callbacks queued by open-url/second-instance before processing the original startup callback, reversing the ordering promised by setReady and the callbacks' arrival order. For example, callback B from a fast second launch is delivered before startup callback A. Process the non-macOS argv callback first, then drain pendingUrls.
      for (const value of pendingUrls.splice(0)) {
        processUrl(value);
      }
      if (platform !== 'darwin') {
        handleFirstProtocolUrl(argv);

app/electron/oauthProvider.ts:115

  • A stale cleanup still removes a replacement when the caller re-registers the same registration object: after firstUnregister(); registerOAuthProvider(registration); firstUnregister(), this identity check succeeds again and deletes the new registration. That contradicts the documented replacement-safety guarantee. Associate each registration call with a unique stored entry/token and compare that token during cleanup rather than the caller-owned object.
  return () => {
    if (providersByCallback.get(key) === registration) {
      providersByCallback.delete(key);
    }

@illume
illume marked this pull request as ready for review August 16, 2026 21:54
@kubernetes-prow kubernetes-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 16, 2026
@kubernetes-prow
kubernetes-prow Bot requested a review from kahirokunn August 16, 2026 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants