Add read-only device roles

src/main.rs

@@ -1,7 +1,7 @@
 use std::path::PathBuf;
 
 use anyhow::Result;
-use clap::{Parser, Subcommand};
+use clap::{Parser, Subcommand, ValueEnum};
 
 use git_remote_e2ee::crypto::{KeyFile, PublicDevice};
 use git_remote_e2ee::policy::DeviceRoles;
@@ -15,6 +15,23 @@ struct Cli {
     command: Command,
 }
 
+#[derive(Clone, Copy, Debug, ValueEnum)]
+enum DeviceRole {
+    Read,
+    Write,
+    Admin,
+}
+
+impl From<DeviceRole> for DeviceRoles {
+    fn from(role: DeviceRole) -> Self {
+        match role {
+            DeviceRole::Read => Self::reader(),
+            DeviceRole::Write => Self::collaborator(),
+            DeviceRole::Admin => Self::owner(),
+        }
+    }
+}
+
 #[derive(Subcommand)]
 enum Command {
     Keygen {
@@ -78,8 +95,8 @@ enum Command {
         key: PathBuf,
         #[arg(long)]
         device: PathBuf,
-        #[arg(long)]
-        admin: bool,
+        #[arg(long, value_enum, default_value = "write")]
+        role: DeviceRole,
     },
     DeviceRevoke {
         #[arg(long, conflicts_with = "remote", required_unless_present = "remote")]
@@ -178,16 +195,12 @@ fn main() -> Result<()> {
             remote,
             key,
             device,
-            admin,
+            role,
         } => {
             let pin = admin_pin_path(&key);
             let public = PublicDevice::read(&device)?;
             let device_id = public.device_id.clone();
-            let roles = if admin {
-                DeviceRoles::owner()
-            } else {
-                DeviceRoles::collaborator()
-            };
+            let roles = DeviceRoles::from(role);
             let (manifest, policy) = match (storage, remote) {
                 (Some(storage), None) => {
                     EncryptedRepository::new(FilesystemStorage::new(storage), KeyFile::read(&key)?)
@@ -239,11 +252,9 @@ fn main() -> Result<()> {
             };
             for device in devices {
                 println!(
-                    "{} reader={} writer={} admin={} status={}",
+                    "{} role={} status={}",
                     device.public.device_id,
-                    device.roles.reader,
-                    device.roles.writer,
-                    device.roles.administrator,
+                    role_name(&device.roles),
                     if device.active() { "active" } else { "revoked" }
                 );
             }
@@ -252,6 +263,18 @@ fn main() -> Result<()> {
     Ok(())
 }
 
+fn role_name(roles: &DeviceRoles) -> &'static str {
+    if roles.administrator {
+        "admin"
+    } else if roles.writer {
+        "write"
+    } else if roles.reader {
+        "read"
+    } else {
+        "none"
+    }
+}
+
 fn admin_pin_path(key: &std::path::Path) -> PathBuf {
     let mut value = key.as_os_str().to_owned();
     value.push(".admin-state.json");

src/manifest.rs

@@ -1122,6 +1122,30 @@ mod tests {
         assert!(error.contains("does not match its sealed audit entry"));
     }
 
+    #[test]
+    fn reader_only_device_cannot_sign_an_ordinary_manifest() {
+        let owner = KeyFile::generate();
+        let reader = KeyFile::generate_for_repository(owner.repository_root.clone()).unwrap();
+        let (genesis, _) = PolicyState::genesis(&owner).unwrap();
+        let mut devices = genesis.body.devices.clone();
+        devices.push(DeviceRecord {
+            public: reader.public_device().unwrap(),
+            roles: DeviceRoles::reader(),
+            revoked_at: None,
+        });
+        let (policy, _) = PolicyState::successor(&genesis, devices, &owner).unwrap();
+        let generation_key = random_key();
+
+        // seal_manifest can construct a cryptographically valid signature from
+        // any key. Verification must still enforce the signer's policy role.
+        let encrypted = writer_manifest(&reader, &policy, &generation_key);
+        let opened = open_manifest(&encrypted, &generation_key).unwrap();
+        let error = verify_manifest(&opened, &policy, None, &generation_key)
+            .unwrap_err()
+            .to_string();
+        assert!(error.contains("manifest was not signed by an active writer"));
+    }
+
     #[test]
     fn reader_trials_all_envelopes_and_reports_inactive_membership_clearly() {
         let owner = KeyFile::generate();

src/policy.rs

@@ -22,6 +22,14 @@ pub struct DeviceRoles {
 }
 
 impl DeviceRoles {
+    pub fn reader() -> Self {
+        Self {
+            reader: true,
+            writer: false,
+            administrator: false,
+        }
+    }
+
     pub fn owner() -> Self {
         Self {
             reader: true,
@@ -265,6 +273,11 @@ impl PolicyState {
             .is_some_and(|device| device.active() && device.roles.writer)
     }
 
+    pub fn is_active_reader(&self, device_id: &str) -> bool {
+        self.device(device_id)
+            .is_some_and(|device| device.active() && device.roles.reader)
+    }
+
     pub fn is_active_admin(&self, device_id: &str) -> bool {
         self.device(device_id)
             .is_some_and(|device| device.active() && device.roles.administrator)
@@ -342,6 +355,33 @@ mod tests {
         assert!(unauthorized.validate_successor(&parent).is_err());
     }
 
+    #[test]
+    fn writers_and_administrators_must_also_be_readers() {
+        let owner = KeyFile::generate();
+        let (parent, _) = PolicyState::genesis(&owner).unwrap();
+        for roles in [
+            DeviceRoles {
+                reader: false,
+                writer: true,
+                administrator: false,
+            },
+            DeviceRoles {
+                reader: false,
+                writer: false,
+                administrator: true,
+            },
+        ] {
+            let added = KeyFile::generate_for_repository(owner.repository_root.clone()).unwrap();
+            let mut devices = parent.body.devices.clone();
+            devices.push(DeviceRecord {
+                public: added.public_device().unwrap(),
+                roles,
+                revoked_at: None,
+            });
+            assert!(PolicyState::successor(&parent, devices, &owner).is_err());
+        }
+    }
+
     #[test]
     fn genesis_cannot_be_substituted_under_a_known_repository_root() {
         let expected = KeyFile::generate();

src/repository.rs

@@ -349,7 +349,16 @@ impl<S: Storage> EncryptedRepository<S> {
         git::ensure_repository(repo)?;
         let chain = self.current_chain()?;
         let current = chain.first().context("manifest chain is empty")?.clone();
-        if !current.policy.is_active_writer(&self.key.device_id()?) {
+        let device_id = self.key.device_id()?;
+        if !current.policy.is_active_writer(&device_id) {
+            if current.policy.is_active_reader(&device_id) {
+                if let Some(remote_name) = remote_name {
+                    bail!(
+                        "this device is read-only for {remote_name}; ask an administrator for the write role"
+                    )
+                }
+                bail!("this device is read-only; ask an administrator for the write role")
+            }
             bail!("device is not an active writer")
         }
         if let Some(remote_name) = remote_name {

tests/cli_roles.rs

@@ -0,0 +1,106 @@
+use std::path::Path;
+use std::process::{Command, Output};
+
+use git_remote_e2ee::crypto::KeyFile;
+
+fn run_cli(args: &[String]) -> Output {
+    Command::new(env!("CARGO_BIN_EXE_git-e2ee"))
+        .args(args)
+        .output()
+        .unwrap()
+}
+
+fn path_arg(path: &Path) -> String {
+    path.to_string_lossy().into_owned()
+}
+
+fn add_device_args(storage: &Path, key: &Path, public: &Path) -> Vec<String> {
+    vec![
+        "device-add".to_owned(),
+        "--storage".to_owned(),
+        path_arg(storage),
+        "--key".to_owned(),
+        path_arg(key),
+        "--device".to_owned(),
+        path_arg(public),
+    ]
+}
+
+#[test]
+fn device_add_parses_roles_and_device_list_uses_role_names() {
+    let temporary = tempfile::tempdir().unwrap();
+    let storage = temporary.path().join("storage");
+    let owner_path = temporary.path().join("owner.key.json");
+    let owner = KeyFile::generate();
+    owner.write_new(&owner_path).unwrap();
+
+    let initialized = run_cli(&[
+        "init".to_owned(),
+        "--storage".to_owned(),
+        path_arg(&storage),
+        "--key".to_owned(),
+        path_arg(&owner_path),
+    ]);
+    assert!(
+        initialized.status.success(),
+        "git-e2ee init: {}",
+        String::from_utf8_lossy(&initialized.stderr)
+    );
+
+    let roles = [
+        ("default", None, "write"),
+        ("reader", Some("read"), "read"),
+        ("writer", Some("write"), "write"),
+        ("admin", Some("admin"), "admin"),
+    ];
+    let mut device_ids = Vec::new();
+    for (name, selected_role, expected_role) in roles {
+        let device_key = KeyFile::generate_for_repository(owner.repository_root.clone()).unwrap();
+        let public = device_key.public_device().unwrap();
+        let public_path = temporary.path().join(format!("{name}.public.json"));
+        public.write_new(&public_path).unwrap();
+        let mut args = add_device_args(&storage, &owner_path, &public_path);
+        if let Some(role) = selected_role {
+            args.extend(["--role".to_owned(), role.to_owned()]);
+        }
+        let added = run_cli(&args);
+        assert!(
+            added.status.success(),
+            "git-e2ee device-add {name}: {}",
+            String::from_utf8_lossy(&added.stderr)
+        );
+        device_ids.push((public.device_id, expected_role));
+    }
+
+    let mut legacy_args = add_device_args(
+        &storage,
+        &owner_path,
+        &temporary.path().join("default.public.json"),
+    );
+    legacy_args.push("--admin".to_owned());
+    let legacy = run_cli(&legacy_args);
+    assert!(!legacy.status.success());
+    assert!(String::from_utf8_lossy(&legacy.stderr).contains("--admin"));
+
+    let listed = run_cli(&[
+        "device-list".to_owned(),
+        "--storage".to_owned(),
+        path_arg(&storage),
+        "--key".to_owned(),
+        path_arg(&owner_path),
+    ]);
+    assert!(
+        listed.status.success(),
+        "git-e2ee device-list: {}",
+        String::from_utf8_lossy(&listed.stderr)
+    );
+    let output = String::from_utf8(listed.stdout).unwrap();
+    for (device_id, role) in device_ids {
+        assert!(
+            output.contains(&format!("{device_id} role={role} status=active")),
+            "missing role {role} for {device_id} in:\n{output}"
+        );
+    }
+    let owner_id = owner.device_id().unwrap();
+    assert!(output.contains(&format!("{owner_id} role=admin status=active")));
+}

tests/multi_device.rs

@@ -341,6 +341,49 @@ fn non_admin_device_cannot_change_policy() {
     );
 }
 
+#[test]
+fn read_only_device_cannot_add_or_revoke_devices() {
+    let temp = tempfile::tempdir().unwrap();
+    let remote = temp.path().join("remote");
+    let owner_key = KeyFile::generate();
+    let reader_key = KeyFile::generate_for_repository(owner_key.repository_root.clone()).unwrap();
+    let candidate_key =
+        KeyFile::generate_for_repository(owner_key.repository_root.clone()).unwrap();
+    let admin_pin = temp.path().join("owner-admin-state.json");
+    let owner = EncryptedRepository::new(FilesystemStorage::new(&remote), owner_key);
+    owner.initialize().unwrap();
+    owner.pin_admin_state(&admin_pin).unwrap();
+    owner
+        .add_device(
+            reader_key.public_device().unwrap(),
+            DeviceRoles::reader(),
+            &admin_pin,
+        )
+        .unwrap();
+
+    let reader = EncryptedRepository::new(FilesystemStorage::new(&remote), reader_key.clone());
+    let before = snapshot_files(&remote);
+    assert!(
+        reader
+            .add_device(
+                candidate_key.public_device().unwrap(),
+                DeviceRoles::collaborator(),
+                &admin_pin,
+            )
+            .unwrap_err()
+            .to_string()
+            .contains("not an active administrator")
+    );
+    assert!(
+        reader
+            .revoke_device(&reader_key.device_id().unwrap(), &admin_pin)
+            .unwrap_err()
+            .to_string()
+            .contains("not an active administrator")
+    );
+    assert_eq!(snapshot_files(&remote), before);
+}
+
 fn copy_directory(source: &Path, destination: &Path) {
     fs::create_dir_all(destination).unwrap();
     for entry in fs::read_dir(source).unwrap() {