chore: move logout functionality into auth crate #7443

Zertsov · 2024-02-21T00:31:32Z

vercel · 2024-02-21T00:31:36Z

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

Name	Status	Preview	Comments	Updated (UTC)
examples-designsystem-docs	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Mar 1, 2024 5:56pm
examples-nonmonorepo	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 1, 2024 5:56pm
examples-vite-web	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Mar 1, 2024 5:56pm
rust-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 1, 2024 5:56pm
turbo-site	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 1, 2024 5:56pm

6 Ignored Deployments

Name	Status	Preview	Updated (UTC)
examples-basic-web	⬜️ Ignored (Inspect)	Visit Preview	Mar 1, 2024 5:56pm
examples-gatsby-web	⬜️ Ignored (Inspect)	Visit Preview	Mar 1, 2024 5:56pm
examples-kitchensink-blog	⬜️ Ignored (Inspect)	Visit Preview	Mar 1, 2024 5:56pm
examples-native-web	⬜️ Ignored (Inspect)	Visit Preview	Mar 1, 2024 5:56pm
examples-svelte-web	⬜️ Ignored (Inspect)	Visit Preview	Mar 1, 2024 5:56pm
examples-tailwind-web	⬜️ Ignored (Inspect)	Visit Preview	Mar 1, 2024 5:56pm

Zertsov · 2024-02-21T00:31:44Z

feature: Add invalidate flag to logout #7444
chore: move logout functionality into auth crate #7443 👈
fix: More robust token validation #7407
main

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-21T00:33:30Z

🟢 Turbopack Benchmark CI successful 🟢

Thanks

github-actions · 2024-02-21T01:10:47Z

🟢 CI successful 🟢

Thanks

chris-olszewski

I think there was some unintentional behavior change in this PR.

chris-olszewski · 2024-02-21T18:06:59Z

crates/turborepo-auth/src/auth/mod.rs

+    pub api_client: &'a T,
+
+    /// The path where we should look for the token to logout.
+    pub path: &'a AbsoluteSystemPathBuf,


For borrowed/owned type pairs like &[T]/Vec<T>/&str/String/&Path/PathBuf you almost always want to use the borrowed variant instead of &String. The gist is that for all of these types the owned variant has a pointer to some heap allocated memory. So &Vec<T> is a pointer to Vec which is on the stack which then contains a pointer to the heap memory. If convert &Vec<T> to &[u8] you skip needing to resolve that first pointer and following it will take you directly to the heap memory.

Suggested change

pub path: &'a AbsoluteSystemPathBuf,

pub path: &'a AbsoluteSystemPath,

I always get confused which ASP is the borrowed variant vs the owned one. I've thought about if we could give these more straightforward names but haven't come up with anything - might be worth discussing as a team since I know Mehul also mentioned he got confused by these names

They're based off of &Path/PathBuf from the stdlib.

I still don't like it 😤

chris-olszewski · 2024-02-21T18:10:28Z

crates/turborepo-auth/src/auth/logout.rs

+    let mut data: serde_json::Value = serde_json::from_str(&content)?;
+    data["token"] = serde_json::Value::Null;
+
+    let new_content = serde_json::to_string_pretty(&data)?;
+    fs::write(global_config_path, new_content)?;


If we want to preserve previous behavior we should keep using unset_path. serde will reformat/reorder the contents of the JSON.

Suggested change

let mut data: serde_json::Value = serde_json::from_str(&content)?;

data["token"] = serde_json::Value::Null;

let new_content = serde_json::to_string_pretty(&data)?;

fs::write(global_config_path, new_content)?;

if let Some(after) = unset_path(&before, &["token"], true)? {

global_config_path.create_with_contents(after).map_err(|e| {

Error::Config(config::Error::FailedToSetConfig {

config_path: global_config_path.clone(),

error: e,

})

})

}

the problem with unset_path is that it lives in the crate that will call this one. I can move this same logic or similar logic into this crate or create a new one for file interactions, but otherwise this causes a cyclic import which makes Rust and myself sad. I could be importing incorrectly and maybe I can just grab this module, but it still feels a little odd.

As for the contents, AFAIK we don't store anything but the token in the config.json, so ordering / formatting won't be an issue at the moment. If we add more to the JSON then this would need to change, so I'm open to doing some of the previous suggestions.

crates/turborepo-auth/src/auth/logout.rs

chris-olszewski · 2024-02-21T18:23:57Z

crates/turborepo-auth/src/auth/logout.rs

+        })?;
+
+    let mut data: serde_json::Value = serde_json::from_str(&content)?;
+    data["token"] = serde_json::Value::Null;


This will panic if the JSON file isn't an object or null. We don't ever write this, but we don't want to panic if for some reason the auth file is corrupted.

chris-olszewski

Only blocker is what we set the token to null and not the empty string.

I will note that this is a behavior change from previous behavior as the previous behavior would remove the token entirely e.g. {"token": "foobar"} -> {}. I think we treat a missing key and a key present with a value of null as the same, but it is probably worth double checking.

chris-olszewski · 2024-02-21T21:49:10Z

crates/turborepo-auth/src/auth/logout.rs

+    let mut data: serde_json::Value = serde_json::from_str(&content)?;
+    if let Some(token) = data.get_mut("token") {
+        // Set token field to empty string
+        *token = "".into();


I'm pretty sure we want null and not an empty string

Suggested change

*token = "".into();

*token = Value::Null;

Wasn't 100% sure how all this interacts with null but in theory it's fine

Wasn't 100% sure how all this interacts with null but in theory it's fine

That sounds like we need more testing.

Zertsov · 2024-03-01T18:25:14Z

Merge activity

Mar 1, 1:25 PM EST: @Zertsov merged this pull request with Graphite.

Zertsov requested a review from a team as a code owner February 21, 2024 00:31

Zertsov requested review from gsoltis and NicholasLYang and removed request for a team February 21, 2024 00:31

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

This was referenced Feb 21, 2024

fix: More robust token validation #7407

Merged

feature: Add invalidate flag to logout #7444

Merged

vercel bot deployed to Preview – rust-docs February 21, 2024 00:39 View deployment

chris-olszewski requested changes Feb 21, 2024

View reviewed changes

Base automatically changed from voz/more-validation to main February 21, 2024 19:05

Zertsov force-pushed the voz/logout-to-crate branch from 86f6320 to df29695 Compare February 21, 2024 20:15

vercel bot deployed to Preview – rust-docs February 21, 2024 20:24 View deployment

Zertsov force-pushed the voz/logout-to-crate branch from df29695 to c5762ae Compare February 21, 2024 20:28

vercel bot deployed to Preview – rust-docs February 21, 2024 20:37 View deployment

Zertsov requested a review from chris-olszewski February 21, 2024 20:47

Zertsov force-pushed the voz/logout-to-crate branch from c5762ae to 4d29369 Compare February 21, 2024 20:48

vercel bot deployed to Preview – rust-docs February 21, 2024 20:58 View deployment

Zertsov force-pushed the voz/logout-to-crate branch from 4d29369 to 94d0db3 Compare February 21, 2024 21:42

vercel bot deployed to Preview – rust-docs February 21, 2024 21:52 View deployment

chris-olszewski approved these changes Feb 21, 2024

View reviewed changes

Zertsov force-pushed the voz/logout-to-crate branch 2 times, most recently from 2266713 to 3b33ee1 Compare February 21, 2024 22:34

vercel bot deployed to Preview – rust-docs February 21, 2024 22:52 View deployment

Zertsov force-pushed the voz/logout-to-crate branch from 3b33ee1 to b71e269 Compare February 21, 2024 23:22

vercel bot deployed to Preview – rust-docs February 21, 2024 23:32 View deployment

vercel bot deployed to Preview – rust-docs February 21, 2024 23:48 View deployment

Zertsov force-pushed the voz/logout-to-crate branch from 3335715 to 96fadd9 Compare February 22, 2024 17:24

vercel bot deployed to Preview – rust-docs February 22, 2024 17:33 View deployment

Zertsov force-pushed the voz/logout-to-crate branch from 96fadd9 to 30031c0 Compare February 28, 2024 16:09

vercel bot deployed to Preview – examples-nonmonorepo February 28, 2024 16:10 View deployment

vercel bot deployed to Preview – rust-docs February 28, 2024 16:19 View deployment

Zertsov force-pushed the voz/logout-to-crate branch from 30031c0 to 626fff4 Compare February 28, 2024 17:17

vercel bot deployed to Preview – examples-nonmonorepo February 28, 2024 17:17 View deployment

vercel bot deployed to Preview – turbo-site February 28, 2024 17:18 View deployment

vercel bot deployed to Preview – rust-docs February 28, 2024 17:26 View deployment

Zertsov force-pushed the voz/logout-to-crate branch from 626fff4 to a5794a3 Compare February 28, 2024 20:37

vercel bot deployed to Preview – examples-nonmonorepo February 28, 2024 20:38 View deployment

vercel bot deployed to Preview – rust-docs February 28, 2024 20:47 View deployment

Zertsov force-pushed the voz/logout-to-crate branch from a5794a3 to c4d0cf6 Compare February 29, 2024 16:43

vercel bot deployed to Preview – examples-nonmonorepo February 29, 2024 16:44 View deployment

vercel bot deployed to Preview – rust-docs February 29, 2024 16:53 View deployment

Zertsov force-pushed the voz/logout-to-crate branch from c4d0cf6 to 1f91819 Compare February 29, 2024 18:00

vercel bot deployed to Preview – examples-nonmonorepo February 29, 2024 18:01 View deployment

vercel bot deployed to Preview – rust-docs February 29, 2024 18:10 View deployment

Zertsov force-pushed the voz/logout-to-crate branch from 1f91819 to 95711cd Compare February 29, 2024 18:26

vercel bot deployed to Preview – examples-nonmonorepo February 29, 2024 18:27 View deployment

vercel bot deployed to Preview – rust-docs February 29, 2024 18:36 View deployment

chore: move logout functionality into auth crate

1f293c8

Zertsov force-pushed the voz/logout-to-crate branch from 95711cd to 1f293c8 Compare March 1, 2024 17:46

vercel bot deployed to Preview – examples-nonmonorepo March 1, 2024 17:47 View deployment

vercel bot deployed to Preview – rust-docs March 1, 2024 17:56 View deployment

Zertsov merged commit bef4f25 into main Mar 1, 2024
60 checks passed

Zertsov deleted the voz/logout-to-crate branch March 1, 2024 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: move logout functionality into auth crate #7443

chore: move logout functionality into auth crate #7443

Zertsov commented Feb 21, 2024 •

edited by padmaia

vercel bot commented Feb 21, 2024 •

edited

Zertsov commented Feb 21, 2024 •

edited

github-actions bot commented Feb 21, 2024 •

edited

github-actions bot commented Feb 21, 2024 •

edited

chris-olszewski left a comment

chris-olszewski Feb 21, 2024

Zertsov Feb 21, 2024

chris-olszewski Feb 21, 2024

Zertsov Feb 21, 2024

chris-olszewski Feb 21, 2024

Zertsov Feb 21, 2024

chris-olszewski Feb 21, 2024

chris-olszewski left a comment

chris-olszewski Feb 21, 2024

Zertsov Feb 21, 2024

chris-olszewski Feb 21, 2024

Zertsov commented Mar 1, 2024

	pub path: &'a AbsoluteSystemPathBuf,
	pub path: &'a AbsoluteSystemPath,

-    let mut data: serde_json::Value = serde_json::from_str(&content)?;
-    data["token"] = serde_json::Value::Null;
-    let new_content = serde_json::to_string_pretty(&data)?;
-    fs::write(global_config_path, new_content)?;
+    if let Some(after) = unset_path(&before, &["token"], true)? {
+        global_config_path.create_with_contents(after).map_err(|e| {
+            Error::Config(config::Error::FailedToSetConfig {
+                config_path: global_config_path.clone(),
+                error: e,
+            })
+        })
+    }

chore: move logout functionality into auth crate #7443

chore: move logout functionality into auth crate #7443

Conversation

Zertsov commented Feb 21, 2024 • edited by padmaia

vercel bot commented Feb 21, 2024 • edited

Zertsov commented Feb 21, 2024 • edited

github-actions bot commented Feb 21, 2024 • edited

🟢 Turbopack Benchmark CI successful 🟢

github-actions bot commented Feb 21, 2024 • edited

🟢 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

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

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

Zertsov commented Mar 1, 2024

Merge activity

Zertsov commented Feb 21, 2024 •

edited by padmaia

vercel bot commented Feb 21, 2024 •

edited

Zertsov commented Feb 21, 2024 •

edited

github-actions bot commented Feb 21, 2024 •

edited

github-actions bot commented Feb 21, 2024 •

edited