fix: do better validation for existing tokens #7358

Zertsov · 2024-02-12T21:54:39Z

Description

Found a path in which a token can be invalid but we claim it isn't. The user would have to run turbo logout and then login to get a new token.

Testing Instructions

Login
Go to Vercel account or whatever API issues tokens and delete the token (invalidate it)
run turbo login again
Ensure that login process proceeds as normal

Closes TURBO-2334

vercel · 2024-02-12T21:54:41Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
examples-basic-web	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Feb 14, 2024 9:13pm
examples-designsystem-docs	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Feb 14, 2024 9:13pm
examples-nonmonorepo	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Feb 14, 2024 9:13pm
examples-vite-web	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Feb 14, 2024 9:13pm
rust-docs	❌ Failed (Inspect)			Feb 14, 2024 9:13pm

6 Ignored Deployments

Name	Status	Preview	Updated (UTC)
examples-gatsby-web	⬜️ Ignored (Inspect)	Visit Preview	Feb 14, 2024 9:13pm
examples-kitchensink-blog	⬜️ Ignored (Inspect)	Visit Preview	Feb 14, 2024 9:13pm
examples-native-web	⬜️ Ignored (Inspect)	Visit Preview	Feb 14, 2024 9:13pm
examples-svelte-web	⬜️ Ignored (Inspect)	Visit Preview	Feb 14, 2024 9:13pm
examples-tailwind-web	⬜️ Ignored (Inspect)	Visit Preview	Feb 14, 2024 9:13pm
turbo-site	⬜️ Ignored (Inspect)	Visit Preview	Feb 14, 2024 9:13pm

Zertsov · 2024-02-12T21:54:55Z

main
- fix: do better validation for existing tokens #7358 👈

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

Join @Zertsov and the rest of your teammates on Graphite

github-actions · 2024-02-12T21:55:26Z

🟢 Turbopack Benchmark CI successful 🟢

Thanks

github-actions · 2024-02-12T22:30:03Z

🟢 CI successful 🟢

Thanks

chris-olszewski

Trying to login with a valid existing token gives a pretty cryptic error:

crates/turborepo-auth/src/lib.rs

chris-olszewski · 2024-02-13T00:48:47Z

crates/turborepo-api-client/src/lib.rs

+            .error_for_status()?;
+
+        Ok(response.json().await?)


Between these two conversions, I think it might be beneficial to handle errors from Vercel API. Like we do with 403 errors

crates/turborepo-auth/src/lib.rs

Zertsov · 2024-02-13T21:13:56Z

crates/turborepo-auth/src/lib.rs

+    #[test]
+    fn test_is_token_active() {
+        let current_time = current_unix_time();
+        let quick_scope = |expiry| Scope {


MW2 had it right all along: quick scopes on Rust are the way 😤

chris-olszewski

Just some minor suggestions

chris-olszewski · 2024-02-14T19:33:15Z

crates/turborepo-auth/src/lib.rs

+    let all_scopes_active = metadata.scopes.iter().all(|scope| {
+        scope
+            .expires_at
+            .map_or(true, |expires_at| expires_at > current_time)
+    });


Complete bikeshed. My brain understands this a lil faster, something about an all that contains map_or confuses me.

Feel free to ignore if you disagree

Suggested change

let all_scopes_active = metadata.scopes.iter().all(|scope| {

scope

.expires_at

.map_or(true, |expires_at| expires_at > current_time)

});

let earliest_expiration = metadata

.scopes

.iter()

.filter_map(|scope| scope.expires_at)

.max();

let all_scopes_active =

earliest_expiration.map_or(true, |expiration| current_time < expiration);

no problem here, I'll take your suggestion since I have no strong preference either way.

chris-olszewski · 2024-02-14T19:38:00Z

crates/turborepo-auth/src/lib.rs

+            ),
+        ];
+
+        for (active_at, scopes, expected) in cases {


I do think using test_case instead of manual table tests has some benefits, primarily it'll run all of the test cases on failure and not bail after the first failure.

Not blocking, just sharing my 2 cents

yeah I've been experimenting with different ways to write tests, and originally this was a tokio::test which I wasn't sure of how it'd interact with test_case, so I went back to my old Go route. I'll probably make this use test_case in a future PR

chris-olszewski · 2024-02-14T19:42:29Z

crates/turborepo-auth/src/lib.rs

+            ),
+            // Case: Token inactive (future `active_at`)
+            (
+                current_time + 10,


I realize that this is unlikely, but I think we should proactively bump this threshold since there's a chance this could fail if it takes 10ms or more to perform the previous 4 test cases.

Also, I don't think this affects us, but the big "SystemTime is not monotonic" warning on the docs makes a little concerned about some possible drift that causes these tests fail.

I'll just do like, 1000 since this is a test that's supposed to be inactive.

The monotonic issue is valid, and I'm purely going off of vibes and instinct in saying that I don't think it'll be an issue. If it does become an issue I can hard code something, but that feels a little more flakey / less exhaustive as a test

chris-olszewski · 2024-02-14T19:44:26Z

crates/turborepo-auth/src/lib.rs

+fn is_token_active(metadata: &ResponseTokenMetadata) -> bool {
+    let current_time = current_unix_time();


Passing in the current time makes this a pure function which might make testing a little more reliable.

Suggested change

fn is_token_active(metadata: &ResponseTokenMetadata) -> bool {

let current_time = current_unix_time();

fn is_token_active(metadata: &ResponseTokenMetadata, current_time: u128) -> bool {

Zertsov · 2024-02-14T21:52:00Z

Merge activity

Feb 14, 4:51 PM EST: @Zertsov merged this pull request with Graphite.

Zertsov requested a review from a team as a code owner February 12, 2024 21:54

Zertsov requested review from tknickman and chris-olszewski February 12, 2024 21:54

turbo-orchestrator bot added created-by: turborepo owned-by: turborepo labels Feb 12, 2024

vercel bot had a problem deploying to Preview – rust-docs February 12, 2024 21:55 Failure

chris-olszewski requested changes Feb 13, 2024

View reviewed changes

Zertsov force-pushed the voz/fix-existing-tokens branch from a96f143 to 8f3b544 Compare February 13, 2024 18:18

vercel bot had a problem deploying to Preview – rust-docs February 13, 2024 18:19 Failure

Zertsov force-pushed the voz/fix-existing-tokens branch from 8f3b544 to 827a123 Compare February 13, 2024 21:03

vercel bot had a problem deploying to Preview – rust-docs February 13, 2024 21:04 Failure

Zertsov commented Feb 13, 2024

View reviewed changes

chris-olszewski approved these changes Feb 14, 2024

View reviewed changes

Zertsov force-pushed the voz/fix-existing-tokens branch from 827a123 to 5afd5c2 Compare February 14, 2024 20:23

vercel bot had a problem deploying to Preview – rust-docs February 14, 2024 20:24 Failure

fix: do better validation for existing tokens

d3a7cdb

Zertsov force-pushed the voz/fix-existing-tokens branch from 5afd5c2 to d3a7cdb Compare February 14, 2024 21:12

vercel bot had a problem deploying to Preview – rust-docs February 14, 2024 21:13 Failure

Zertsov merged commit e4aeda1 into main Feb 14, 2024
57 of 58 checks passed

Zertsov deleted the voz/fix-existing-tokens branch February 14, 2024 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: do better validation for existing tokens #7358

fix: do better validation for existing tokens #7358

Zertsov commented Feb 12, 2024 •

edited by padmaia

Loading

vercel bot commented Feb 12, 2024 •

edited

Loading

Zertsov commented Feb 12, 2024

github-actions bot commented Feb 12, 2024 •

edited

Loading

github-actions bot commented Feb 12, 2024 •

edited

Loading

chris-olszewski left a comment

chris-olszewski Feb 13, 2024

Zertsov Feb 13, 2024

chris-olszewski left a comment

chris-olszewski Feb 14, 2024

Zertsov Feb 14, 2024

chris-olszewski Feb 14, 2024

Zertsov Feb 14, 2024

chris-olszewski Feb 14, 2024

Zertsov Feb 14, 2024

chris-olszewski Feb 14, 2024

Zertsov commented Feb 14, 2024

		fn is_token_active(metadata: &ResponseTokenMetadata) -> bool {
		let current_time = current_unix_time();

fix: do better validation for existing tokens #7358

fix: do better validation for existing tokens #7358

Conversation

Zertsov commented Feb 12, 2024 • edited by padmaia Loading

Description

Testing Instructions

vercel bot commented Feb 12, 2024 • edited Loading

Zertsov commented Feb 12, 2024

github-actions bot commented Feb 12, 2024 • edited Loading

🟢 Turbopack Benchmark CI successful 🟢

github-actions bot commented Feb 12, 2024 • edited Loading

🟢 CI successful 🟢

chris-olszewski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-olszewski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zertsov commented Feb 14, 2024

Merge activity

Zertsov commented Feb 12, 2024 •

edited by padmaia

Loading

vercel bot commented Feb 12, 2024 •

edited

Loading

github-actions bot commented Feb 12, 2024 •

edited

Loading

github-actions bot commented Feb 12, 2024 •

edited

Loading