Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove token instrospection #5422

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

isaacroldan
Copy link
Contributor

@isaacroldan isaacroldan commented Feb 18, 2025

WHY are these changes introduced?

Token instrospection adds an extra query on every command run, but with the new device auth is not really useful.

WHAT is this pull request doing?

Removes the identity token introspection endpoint validation and its associated cache storage. Session validation will now rely solely on token expiration checks rather than making additional HTTP requests to validate tokens.

How to test your changes?

  1. Log in to the CLI
  2. Verify that authentication still works as expected
  3. Confirm that session validation continues to function without making introspection requests after a few hours.

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@isaacroldan isaacroldan force-pushed the 02-18-remove_token_instrospection branch 4 times, most recently from 91a6c28 to a7b81b6 Compare February 19, 2025 15:08
@isaacroldan isaacroldan marked this pull request as ready for review February 19, 2025 15:12
@isaacroldan isaacroldan requested a review from a team as a code owner February 19, 2025 15:12
Copy link
Contributor

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

@isaacroldan isaacroldan force-pushed the 02-18-remove_token_instrospection branch from a7b81b6 to 2dc254b Compare February 19, 2025 15:23
Copy link
Contributor

github-actions bot commented Feb 19, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.92% (+0.2% 🔼)
9068/11944
🟡 Branches
71.18% (+0.19% 🔼)
4424/6215
🟡 Functions
75.46% (+0.1% 🔼)
2371/3142
🟡 Lines
76.44% (+0.22% 🔼)
8564/11203
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
95.18% (-1.2% 🔻)
86.49% (-2.7% 🔻)
95.45% 100%
🟢
... / validate.ts
89.47% (-1% 🔻)
85.71% (-0.95% 🔻)
100%
90.63% (-0.8% 🔻)

Test suite run success

2055 tests passing in 920 suites.

Report generated by 🧪jest coverage report action from d4bd953

@isaacroldan isaacroldan force-pushed the 02-18-remove_token_instrospection branch 2 times, most recently from 9372323 to 33612c2 Compare February 19, 2025 16:45
@isaacroldan isaacroldan force-pushed the 02-18-remove_token_instrospection branch from 33612c2 to d4bd953 Compare February 20, 2025 11:15
Copy link
Contributor

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/private/node/conf-store.d.ts
@@ -3,16 +3,14 @@ interface CacheValue<T> {
     value: T;
     timestamp: number;
 }
-export type IntrospectionUrlKey = ;
 export type PackageVersionKey = ;
 export type NotificationsKey = ;
 export type NotificationKey = ;
 export type GraphQLRequestKey = ;
 type MostRecentOccurrenceKey = ;
 type RateLimitKey = ;
-type ExportedKey = IntrospectionUrlKey | PackageVersionKey | NotificationsKey | NotificationKey | GraphQLRequestKey;
+type ExportedKey = PackageVersionKey | NotificationsKey | NotificationKey | GraphQLRequestKey;
 interface Cache {
-    [introspectionUrlKey: IntrospectionUrlKey]: CacheValue<string>;
     [packageVersionKey: PackageVersionKey]: CacheValue<string>;
     [notifications: NotificationsKey]: CacheValue<string>;
     [notification: NotificationKey]: CacheValue<string>;

Copy link
Contributor

@gonzaloriestra gonzaloriestra left a comment

Choose a reason for hiding this comment

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

Great! 👏

I think it makes sense to remove the check and be reactive instead. If the token expires or fails, just try to refresh, instead of wasting time checking if it's valid beforehand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants