feat(rome_fs): implement permission checking in the memory filesystem (…

…#3882)

crates/rome_cli/tests/snap_test.rs

@@ -241,7 +241,7 @@ impl From<SnapshotPayload<'_>> for CliSnapshot {
         } = payload;
         let mut cli_snapshot = CliSnapshot::from_result(result);
         let config_path = PathBuf::from("rome.json");
-        let configuration = fs.read(&config_path).ok();
+        let configuration = fs.open(&config_path).ok();
         if let Some(mut configuration) = configuration {
             let mut buffer = String::new();
             if configuration.read_to_string(&mut buffer).is_ok() {

crates/rome_fs/src/fs.rs

@@ -46,11 +46,10 @@ pub trait File {
 
 /// This struct is a "mirror" of [std::fs::FileOptions].
 /// Refer to their documentation for more details
-#[derive(Default)]
+#[derive(Default, Debug)]
 pub struct OpenOptions {
     read: bool,
     write: bool,
-    append: bool,
     truncate: bool,
     create: bool,
     create_new: bool,
@@ -65,10 +64,6 @@ impl OpenOptions {
         self.write = write;
         self
     }
-    pub fn append(mut self, append: bool) -> Self {
-        self.append = append;
-        self
-    }
     pub fn truncate(mut self, truncate: bool) -> Self {
         self.truncate = truncate;
         self
@@ -86,7 +81,6 @@ impl OpenOptions {
         options
             .read(self.read)
             .write(self.write)
-            .append(self.append)
             .truncate(self.truncate)
             .create(self.create)
             .create_new(self.create_new)
@@ -95,20 +89,42 @@ impl OpenOptions {
 
 /// Trait that contains additional methods to work with [FileSystem]
 pub trait FileSystemExt: FileSystem {
-    /// Open a file with `read` and `write` options
+    /// Open a file with the `read` option
+    ///
+    /// Equivalent to [std::fs::File::open]
     fn open(&self, path: &Path) -> io::Result<Box<dyn File>> {
-        self.open_with_options(path, OpenOptions::default().read(true).write(true))
+        self.open_with_options(path, OpenOptions::default().read(true))
     }
-    /// Open a file with `write` and `create_new` options
+
+    /// Open a file with the `write` and `create` options
+    ///
+    /// Equivalent to [std::fs::File::create]
     fn create(&self, path: &Path) -> io::Result<Box<dyn File>> {
-        self.open_with_options(path, OpenOptions::default().write(true).create_new(true))
+        self.open_with_options(
+            path,
+            OpenOptions::default()
+                .write(true)
+                .create(true)
+                .truncate(true),
+        )
     }
-    /// Opens a file with read options
-    fn read(&self, path: &Path) -> io::Result<Box<dyn File>> {
-        self.open_with_options(path, OpenOptions::default().read(true).create_new(true))
+
+    /// Opens a file with the `read`, `write` and `create_new` options
+    ///
+    /// Equivalent to [std::fs::File::create_new]
+    fn create_new(&self, path: &Path) -> io::Result<Box<dyn File>> {
+        self.open_with_options(
+            path,
+            OpenOptions::default()
+                .read(true)
+                .write(true)
+                .create_new(true),
+        )
     }
 }
 
+impl<T: ?Sized> FileSystemExt for T where T: FileSystem {}
+
 type BoxedTraversal<'fs, 'scope> = Box<dyn FnOnce(&dyn TraversalScope<'scope>) + Send + 'fs>;
 
 pub trait TraversalScope<'scope> {

crates/rome_fs/src/fs/memory.rs

@@ -1,3 +1,4 @@
+use std::collections::hash_map::Entry;
 use std::collections::{hash_map::IntoIter, HashMap};
 use std::io;
 use std::panic::AssertUnwindSafe;
@@ -8,7 +9,7 @@ use std::sync::Arc;
 use parking_lot::{lock_api::ArcMutexGuard, Mutex, RawMutex, RwLock};
 use rome_diagnostics::Error;
 
-use crate::fs::{FileSystemExt, OpenOptions};
+use crate::fs::OpenOptions;
 use crate::{FileSystem, RomePath, TraversalContext, TraversalScope};
 
 use super::{BoxedTraversal, ErrorKind, File, FileSystemDiagnostic};
@@ -71,68 +72,76 @@ impl MemoryFileSystem {
 
 impl FileSystem for MemoryFileSystem {
     fn open_with_options(&self, path: &Path, options: OpenOptions) -> io::Result<Box<dyn File>> {
-        if options.read && options.write {
-            self.open(path)
-        } else if options.create_new || options.write {
-            self.create(path)
-        } else if options.read {
-            self.read(path)
+        let mut inner = if options.create || options.create_new {
+            // Acquire write access to the files map if the file may need to be created
+            let mut files = self.files.0.write();
+            match files.entry(PathBuf::from(path)) {
+                Entry::Vacant(entry) => {
+                    // we create an empty file
+                    let file: FileEntry = Arc::new(Mutex::new(vec![]));
+                    let entry = entry.insert(file);
+                    entry.lock_arc()
+                }
+                Entry::Occupied(entry) => {
+                    if options.create {
+                        // If `create` is true, truncate the file
+                        let entry = entry.into_mut();
+                        *entry = Arc::new(Mutex::new(vec![]));
+                        entry.lock_arc()
+                    } else {
+                        // This branch can only be reached if `create_new` was true,
+                        // we should return an error if the file already exists
+                        return Err(io::Error::new(
+                            io::ErrorKind::AlreadyExists,
+                            format!("path {path:?} already exists in memory filesystem"),
+                        ));
+                    }
+                }
+            }
         } else {
-            unimplemented!("the set of open options provided don't match any case")
+            let files = self.files.0.read();
+            let entry = files.get(path).ok_or_else(|| {
+                io::Error::new(
+                    io::ErrorKind::NotFound,
+                    format!("path {path:?} does not exists in memory filesystem"),
+                )
+            })?;
+
+            entry.lock_arc()
+        };
+
+        if options.truncate {
+            // Clear the buffer if the file was open with `truncate`
+            inner.clear();
         }
+
+        Ok(Box::new(MemoryFile {
+            inner,
+            can_read: options.read,
+            can_write: options.write,
+        }))
     }
 
     fn traversal<'scope>(&'scope self, func: BoxedTraversal<'_, 'scope>) {
         func(&MemoryTraversalScope { fs: self })
     }
 }
 
-impl FileSystemExt for MemoryFileSystem {
-    fn create(&self, path: &Path) -> io::Result<Box<dyn File>> {
-        let files = &mut self.files.0.write();
-        // we create an empty file
-        let file: FileEntry = Arc::new(Mutex::new(vec![]));
-        let path = PathBuf::from(path);
-        files.insert(path, file.clone());
-        let inner = file.lock_arc();
-        Ok(Box::new(MemoryFile { inner }))
-    }
-
-    fn open(&self, path: &Path) -> io::Result<Box<dyn File>> {
-        let files = &self.files.0.read();
-        let entry = files.get(path).ok_or_else(|| {
-            io::Error::new(
-                io::ErrorKind::NotFound,
-                format!("path {path:?} does not exists in memory filesystem"),
-            )
-        })?;
-
-        let lock = entry.lock_arc();
-
-        Ok(Box::new(MemoryFile { inner: lock }))
-    }
-
-    fn read(&self, path: &Path) -> io::Result<Box<dyn File>> {
-        let files = &self.files.0.read();
-        let entry = files.get(path).ok_or_else(|| {
-            io::Error::new(
-                io::ErrorKind::NotFound,
-                format!("path {path:?} does not exists in memory filesystem"),
-            )
-        })?;
-
-        let lock = entry.lock_arc();
-
-        Ok(Box::new(MemoryFile { inner: lock }))
-    }
-}
-
 struct MemoryFile {
     inner: ArcMutexGuard<RawMutex, Vec<u8>>,
+    can_read: bool,
+    can_write: bool,
 }
 
 impl File for MemoryFile {
     fn read_to_string(&mut self, buffer: &mut String) -> io::Result<()> {
+        if !self.can_read {
+            return Err(io::Error::new(
+                io::ErrorKind::PermissionDenied,
+                "this file wasn't open with read access",
+            ));
+        }
+
         // Verify the stored byte content is valid UTF-8
         let content = str::from_utf8(&self.inner)
             .map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err))?;
@@ -142,6 +151,13 @@ impl File for MemoryFile {
     }
 
     fn set_content(&mut self, content: &[u8]) -> io::Result<()> {
+        if !self.can_write {
+            return Err(io::Error::new(
+                io::ErrorKind::PermissionDenied,
+                "this file wasn't open with write access",
+            ));
+        }
+
         // Resize the memory buffer to fit the new content
         self.inner.resize(content.len(), 0);
         // Copy the new content into the memory buffer
@@ -211,7 +227,7 @@ mod tests {
     use parking_lot::Mutex;
     use rome_diagnostics::Error;
 
-    use crate::fs::FileSystemExt;
+    use crate::{fs::FileSystemExt, OpenOptions};
     use crate::{FileSystem, MemoryFileSystem, PathInterner, RomePath, TraversalContext};
     use rome_diagnostics::location::FileId;
 
@@ -226,7 +242,7 @@ mod tests {
         fs.insert(path.into(), content_1.as_bytes());
 
         let mut file = fs
-            .open(path)
+            .open_with_options(path, OpenOptions::default().read(true).write(true))
             .expect("the file should exist in the memory file system");
 
         let mut buffer = String::new();
@@ -245,6 +261,82 @@ mod tests {
         assert_eq!(buffer, content_2);
     }
 
+    #[test]
+    fn file_create() {
+        let fs = MemoryFileSystem::default();
+
+        let path = Path::new("file.js");
+        let mut file = fs.create(path).expect("the file should not fail to open");
+
+        file.set_content(b"content".as_slice())
+            .expect("the file should be written without error");
+    }
+
+    #[test]
+    fn file_create_truncate() {
+        let mut fs = MemoryFileSystem::default();
+
+        let path = Path::new("file.js");
+        fs.insert(path.into(), b"content".as_slice());
+
+        let file = fs.create(path).expect("the file should not fail to create");
+
+        drop(file);
+
+        let mut file = fs.open(path).expect("the file should not fail to open");
+
+        let mut buffer = String::new();
+        file.read_to_string(&mut buffer)
+            .expect("the file should be read without error");
+
+        assert!(
+            buffer.is_empty(),
+            "fs.create() should truncate the file content"
+        );
+    }
+
+    #[test]
+    fn file_create_new() {
+        let fs = MemoryFileSystem::default();
+
+        let path = Path::new("file.js");
+        let content = "content";
+
+        let mut file = fs
+            .create_new(path)
+            .expect("the file should not fail to create");
+
+        file.set_content(content.as_bytes())
+            .expect("the file should be written without error");
+
+        drop(file);
+
+        let mut file = fs.open(path).expect("the file should not fail to open");
+
+        let mut buffer = String::new();
+        file.read_to_string(&mut buffer)
+            .expect("the file should be read without error");
+
+        assert_eq!(buffer, content);
+    }
+
+    #[test]
+    fn file_create_new_exists() {
+        let mut fs = MemoryFileSystem::default();
+
+        let path = Path::new("file.js");
+        fs.insert(path.into(), b"content".as_slice());
+
+        let result = fs.create_new(path);
+
+        match result {
+            Ok(_) => panic!("fs.create_new() for an existing file should return an error"),
+            Err(error) => {
+                assert_eq!(error.kind(), io::ErrorKind::AlreadyExists);
+            }
+        }
+    }
+
     #[test]
     fn missing_file() {
         let fs = MemoryFileSystem::default();

crates/rome_fs/src/fs/os.rs

@@ -1,6 +1,6 @@
 //! Implementation of the [FileSystem] and related traits for the underlying OS filesystem
 use super::{BoxedTraversal, ErrorKind, File, FileSystemDiagnostic};
-use crate::fs::{FileSystemExt, OpenOptions};
+use crate::fs::OpenOptions;
 use crate::{
     fs::{TraversalContext, TraversalScope},
     FileSystem, RomePath,
@@ -21,10 +21,13 @@ pub struct OsFileSystem;
 
 impl FileSystem for OsFileSystem {
     fn open_with_options(&self, path: &Path, options: OpenOptions) -> io::Result<Box<dyn File>> {
-        let mut fs_options = fs::File::options();
-        Ok(Box::new(OsFile {
-            inner: options.into_fs_options(&mut fs_options).open(path)?,
-        }))
+        tracing::debug_span!("OsFileSystem::open_with_options", path = ?path, options = ?options)
+            .in_scope(move || -> io::Result<Box<dyn File>> {
+                let mut fs_options = fs::File::options();
+                Ok(Box::new(OsFile {
+                    inner: options.into_fs_options(&mut fs_options).open(path)?,
+                }))
+            })
     }
 
     fn traversal(&self, func: BoxedTraversal) {
@@ -34,24 +37,6 @@ impl FileSystem for OsFileSystem {
     }
 }
 
-impl FileSystemExt for OsFileSystem {
-    fn create(&self, path: &Path) -> io::Result<Box<dyn File>> {
-        tracing::debug_span!("OsFileSystem::create", path = ?path).in_scope(
-            move || -> io::Result<Box<dyn File>> {
-                self.open_with_options(path, OpenOptions::default().write(true).create_new(true))
-            },
-        )
-    }
-
-    fn open(&self, path: &Path) -> io::Result<Box<dyn File>> {
-        tracing::debug_span!("OsFileSystem::open", path = ?path).in_scope(
-            move || -> io::Result<Box<dyn File>> {
-                self.open_with_options(path, OpenOptions::default().read(true).write(true))
-            },
-        )
-    }
-}
-
 struct OsFile {
     inner: fs::File,
 }