Skip to content

refactor: cleanup introspect logic - #8331

Open
paula-stacho wants to merge 3 commits into
mainfrom
worktree-introspect
Open

refactor: cleanup introspect logic#8331
paula-stacho wants to merge 3 commits into
mainfrom
worktree-introspect

Conversation

@paula-stacho

@paula-stacho paula-stacho commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Description

Compass won't have access to the Auth's server introspect endpoint as it's a protected resource and Compass as a desktop client can't securely authenticate itself.
Instead, we'll optimistically assume the token is valid, until we get a 401 from the server.

Side fix: When the app reopens, we restore the userInfo by refreshing the token. This will also ensure that when the app opens, we will verify if the token is still valid.

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added
  • If this change updates the UI, screenshots/videos are added and a design review is requested
  • If this change could impact the load on the MongoDB cluster, please describe the expected and worst case impact
  • I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

Copilot AI lite review requested due to automatic review settings August 7, 2026 14:10

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

This PR refactors Atlas auth “introspection” behavior by removing the previous introspect endpoint call and shifting authentication state to be derived from locally tracked user/token state.

Changes:

  • Removed the introspect API surface (type, IPC exposure, implementation, and tests).
  • Made isAuthenticated() rely on currentUser and added startup restoration of currentUser from stored token state.
  • Updated test coverage to reflect the new “restore + local state” authentication model.

Code quality / correctness notes:

  • There is a correctness risk from isAuthenticated() not waiting for service initialization (can yield a false negative during startup). This needs to be addressed before approval (see PR comment).

Reviewed changes

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

File Description
packages/atlas-service/src/util.ts Removes the no-longer-used IntrospectInfo type.
packages/atlas-service/src/main.ts Drops introspection logic and restores auth state from stored tokens; isAuthenticated now derives from currentUser.
packages/atlas-service/src/main.spec.ts Updates tests to validate isAuthenticated via local state and adds restoreCurrentUser coverage.

Comment thread packages/atlas-service/src/main.ts
@paula-stacho
paula-stacho marked this pull request as ready for review August 7, 2026 14:51
@paula-stacho
paula-stacho requested a review from a team as a code owner August 7, 2026 14:51
@paula-stacho
paula-stacho requested a review from esvm August 7, 2026 14:51
Comment thread packages/atlas-service/src/main.ts Outdated
Comment on lines +135 to +136
// This is not a sign in flow, most likely a token refresh - so we don't want to allow user interaction
return [];

@gribnoysup gribnoysup Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

IIRC this throws because just returning an empty list wasn't causing oidc-plugin to fail as we expect it to at this point, so this is (was?) the only way to force a fail if we're accidentally triggering an auth process when we shouldn't. Did we change that on the oidc-plugin side to be able to remove this?

@paula-stacho paula-stacho Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I might be misunderstanding this bit. Under what circumstances do we need this to fail, when do we "accidentally trigger an auth process"?
I am trying to ensure that we can do a "soft" token refresh when the app starts, purely to show the user if they're still signed in, without triggering a user interaction if it fails.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oh. If you look at the history of this change, this is exactly what we are not supposed to do manually based on some previous discussions about how OIDC should work and be used in general, the logic in Compass should just always request token without trying to do the refresh manually or making any assumptions about whether or not it needs a refresh

@gribnoysup gribnoysup Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And in this case requesting the token instead of refreshing it might cause the auth flow to be triggered, and so this particular method makes sure that if we're ending up in the "request token" flow when we don't expect it, we fail (because just an empty array passed to oidc-plugin doesn't cause this to happen)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

reverting to throwing

@paula-stacho paula-stacho added feature flagged PRs labeled with this label will not be included in the release notes of the next release no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion) labels Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature flagged PRs labeled with this label will not be included in the release notes of the next release no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants