Remove IdentityOrRemoteIdentity (#2220)

* wip

* wip

* fix migrations

* fix

* Fix Prisma migrations + fire new app migration

---------

Co-authored-by: jake <77554505+brxken128@users.noreply.github.com>

core/prisma/migrations/20240319065147_remove_remoteidentity/migration.sql

@@ -0,0 +1,57 @@
+/*
+  Warnings:
+
+  - The primary key for the `cloud_crdt_operation` table will be changed. If it partially fails, the table could be left without primary key constraint.
+  - You are about to alter the column `id` on the `cloud_crdt_operation` table. The data in that column could be lost. The data in that column will be cast from `Binary` to `Int`.
+  - The primary key for the `crdt_operation` table will be changed. If it partially fails, the table could be left without primary key constraint.
+  - You are about to alter the column `id` on the `crdt_operation` table. The data in that column could be lost. The data in that column will be cast from `Binary` to `Int`.
+  - Added the required column `remote_identity` to the `instance` table without a default value. This is not possible if the table is not empty.
+
+ - @oscartbeaumont modified the migration Prisma generated to fill the `NOT NULL` `remote_identity` field with the existing IdentityOrRemoteIdentity value so we can handle it in the app migrations.
+
+*/
+-- RedefineTables
+PRAGMA foreign_keys=OFF;
+CREATE TABLE "new_cloud_crdt_operation" (
+    "id" INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT,
+    "timestamp" BIGINT NOT NULL,
+    "model" TEXT NOT NULL,
+    "record_id" BLOB NOT NULL,
+    "kind" TEXT NOT NULL,
+    "data" BLOB NOT NULL,
+    "instance_id" INTEGER NOT NULL,
+    CONSTRAINT "cloud_crdt_operation_instance_id_fkey" FOREIGN KEY ("instance_id") REFERENCES "instance" ("id") ON DELETE RESTRICT ON UPDATE CASCADE
+);
+INSERT INTO "new_cloud_crdt_operation" ("data", "id", "instance_id", "kind", "model", "record_id", "timestamp") SELECT "data", "id", "instance_id", "kind", "model", "record_id", "timestamp" FROM "cloud_crdt_operation";
+DROP TABLE "cloud_crdt_operation";
+ALTER TABLE "new_cloud_crdt_operation" RENAME TO "cloud_crdt_operation";
+CREATE TABLE "new_instance" (
+    "id" INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT,
+    "pub_id" BLOB NOT NULL,
+    "identity" BLOB,
+    "remote_identity" BLOB NOT NULL,
+    "node_id" BLOB NOT NULL,
+    "metadata" BLOB,
+    "last_seen" DATETIME NOT NULL,
+    "date_created" DATETIME NOT NULL,
+    "timestamp" BIGINT
+);
+INSERT INTO "new_instance" ("date_created", "id", "identity", "remote_identity", "last_seen", "metadata", "node_id", "pub_id", "timestamp") SELECT "date_created", "id", "identity", "identity", "last_seen", "metadata", "node_id", "pub_id", "timestamp" FROM "instance";
+DROP TABLE "instance";
+ALTER TABLE "new_instance" RENAME TO "instance";
+CREATE UNIQUE INDEX "instance_pub_id_key" ON "instance"("pub_id");
+CREATE TABLE "new_crdt_operation" (
+    "id" INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT,
+    "timestamp" BIGINT NOT NULL,
+    "model" TEXT NOT NULL,
+    "record_id" BLOB NOT NULL,
+    "kind" TEXT NOT NULL,
+    "data" BLOB NOT NULL,
+    "instance_id" INTEGER NOT NULL,
+    CONSTRAINT "crdt_operation_instance_id_fkey" FOREIGN KEY ("instance_id") REFERENCES "instance" ("id") ON DELETE RESTRICT ON UPDATE CASCADE
+);
+INSERT INTO "new_crdt_operation" ("data", "id", "instance_id", "kind", "model", "record_id", "timestamp") SELECT "data", "id", "instance_id", "kind", "model", "record_id", "timestamp" FROM "crdt_operation";
+DROP TABLE "crdt_operation";
+ALTER TABLE "new_crdt_operation" RENAME TO "crdt_operation";
+PRAGMA foreign_key_check;
+PRAGMA foreign_keys=ON;

core/prisma/schema.prisma

@@ -50,10 +50,12 @@ model Node {
 // represents a single `.db` file (SQLite DB) that is paired to the current library.
 // A `LibraryInstance` is always owned by a single `Node` but it's possible for that node to change (or two to be owned by a single node).
 model Instance {
-  id       Int   @id @default(autoincrement()) // This is is NOT globally unique
-  pub_id   Bytes @unique // This UUID is meaningless and exists soley cause the `uhlc::ID` must be 16-bit. Really this should be derived from the `identity` field.
-  // Enum: sd_core::p2p::IdentityOrRemoteIdentity
-  identity Bytes
+  id              Int    @id @default(autoincrement()) // This is is NOT globally unique
+  pub_id          Bytes  @unique // This UUID is meaningless and exists soley cause the `uhlc::ID` must be 16-bit. Really this should be derived from the `identity` field.
+  // Enum: sd_p2p::Identity (or sd_core::p2p::IdentityOrRemoteIdentity in early versions)
+  identity        Bytes?
+  // Enum: sd_core::node::RemoteIdentity
+  remote_identity Bytes
 
   node_id  Bytes
   metadata Bytes? // TODO: This should not be optional

core/src/cloud/sync/receive.rs

@@ -6,7 +6,7 @@ use crate::{
 use super::{err_break, CompressedCRDTOperations};
 use sd_cloud_api::RequestConfigProvider;
 use sd_core_sync::NTP64;
-use sd_p2p::{IdentityOrRemoteIdentity, RemoteIdentity};
+use sd_p2p::RemoteIdentity;
 use sd_prisma::prisma::{cloud_crdt_operation, instance, PrismaClient, SortOrder};
 use sd_sync::CRDTOperation;
 use sd_utils::uuid_to_bytes;
@@ -241,7 +241,7 @@ pub async fn create_instance(
 			instance::pub_id::equals(uuid_to_bytes(uuid)),
 			instance::create(
 				uuid_to_bytes(uuid),
-				IdentityOrRemoteIdentity::RemoteIdentity(identity).to_bytes(),
+				identity.get_bytes().to_vec(),
 				node_id.as_bytes().to_vec(),
 				Utc::now().into(),
 				Utc::now().into(),

core/src/custom_uri/mod.rs

@@ -11,7 +11,7 @@ use http_body::combinators::UnsyncBoxBody;
 use hyper::{header, upgrade::OnUpgrade};
 use sd_file_ext::text::is_text;
 use sd_file_path_helper::{file_path_to_handle_custom_uri, IsolatedFilePathData};
-use sd_p2p::{IdentityOrRemoteIdentity, RemoteIdentity, P2P};
+use sd_p2p::{RemoteIdentity, P2P};
 use sd_prisma::prisma::{file_path, location};
 use sd_utils::db::maybe_missing;
 
@@ -174,9 +174,8 @@ async fn get_or_init_lru_entry(
 		let path = Path::new(path)
 			.join(IsolatedFilePathData::try_from((location_id, &file_path)).map_err(not_found)?);
 
-		let identity = IdentityOrRemoteIdentity::from_bytes(&instance.identity)
-			.map_err(internal_server_error)?
-			.remote_identity();
+		let identity =
+			RemoteIdentity::from_bytes(&instance.remote_identity).map_err(internal_server_error)?;
 
 		let lru_entry = CacheValue {
 			name: path,

core/src/library/config.rs

@@ -3,7 +3,7 @@ use crate::{
 	util::version_manager::{Kind, ManagedVersion, VersionManager, VersionManagerError},
 };
 
-use sd_p2p::{Identity, IdentityOrRemoteIdentity};
+use sd_p2p::{Identity, RemoteIdentity};
 use sd_prisma::prisma::{file_path, indexer_rule, instance, location, node, PrismaClient};
 use sd_utils::{db::maybe_missing, error::FileIOError};
 
@@ -70,10 +70,11 @@ pub enum LibraryConfigVersion {
 	V7 = 7,
 	V8 = 8,
 	V9 = 9,
+	V10 = 10,
 }
 
 impl ManagedVersion<LibraryConfigVersion> for LibraryConfig {
-	const LATEST_VERSION: LibraryConfigVersion = LibraryConfigVersion::V9;
+	const LATEST_VERSION: LibraryConfigVersion = LibraryConfigVersion::V10;
 
 	const KIND: Kind = Kind::Json("version");
 
@@ -265,7 +266,8 @@ impl LibraryConfig {
 
 						instance::Create {
 							pub_id: instance_id.as_bytes().to_vec(),
-							identity: node
+							// WARNING: At this stage in the migration this field *should* be an `Identity` not a `RemoteIdentityOrIdentity` (as that was introduced later on).
+							remote_identity: node
 								.and_then(|n| n.identity.clone())
 								.unwrap_or_else(|| Identity::new().to_bytes()),
 							node_id: node_config.id.as_bytes().to_vec(),
@@ -374,16 +376,20 @@ impl LibraryConfig {
 								.map(|i| {
 									db.instance().update(
 										instance::id::equals(i.id),
-										vec![instance::identity::set(
-									// This code is assuming you only have the current node.
-									// If you've paired your node with another node, reset your db.
-									IdentityOrRemoteIdentity::Identity(
-										Identity::from_bytes(&i.identity).expect(
-											"Invalid identity detected in DB during migrations",
-										),
-									)
-									.to_bytes(),
-								)],
+										vec![
+											// In earlier versions of the app this migration would convert an `Identity` in the `identity` column to a `IdentityOrRemoteIdentity::Identity`.
+											// We have removed the `IdentityOrRemoteIdentity` type so we have disabled this change and the V9 -> V10 will take care of it.
+											// instance::identity::set(
+											// 	// This code is assuming you only have the current node.
+											// 	// If you've paired your node with another node, reset your db.
+											// 	IdentityOrRemoteIdentity::Identity(
+											// 		Identity::from_bytes(&i.identity).expect(
+											// 			"Invalid identity detected in DB during migrations",
+											// 		),
+											// 	)
+											// 	.to_bytes(),
+											// ),
+										],
 									)
 								})
 								.collect::<Vec<_>>(),
@@ -391,6 +397,57 @@ impl LibraryConfig {
 						.await?;
 					}
 
+					(LibraryConfigVersion::V9, LibraryConfigVersion::V10) => {
+						db._batch(
+							db.instance()
+								.find_many(vec![])
+								.exec()
+								.await?
+								.into_iter()
+								.filter_map(|i| {
+									let Some(identity) = i.identity else {
+										return None;
+									};
+
+									let (remote_identity, identity) = if identity[0] == b'I' {
+										// We have an `IdentityOrRemoteIdentity::Identity`
+										let identity = Identity::from_bytes(&identity[1..]).expect(
+											"Invalid identity detected in DB during migrations - 1",
+										);
+
+										(identity.to_remote_identity(), Some(identity))
+									} else if identity[0] == b'R' {
+										// We have an `IdentityOrRemoteIdentity::RemoteIdentity`
+										let identity = RemoteIdentity::from_bytes(&identity[1..])
+											.expect(
+											"Invalid identity detected in DB during migrations - 2",
+										);
+
+										(identity, None)
+									} else {
+										// We have an `Identity` or an invalid column.
+										let identity = Identity::from_bytes(&identity).expect(
+											"Invalid identity detected in DB during migrations - 3",
+										);
+
+										(identity.to_remote_identity(), Some(identity))
+									};
+
+									Some(db.instance().update(
+										instance::id::equals(i.id),
+										vec![
+											instance::identity::set(identity.map(|i| i.to_bytes())),
+											instance::remote_identity::set(
+												remote_identity.get_bytes().to_vec(),
+											),
+										],
+									))
+								})
+								.collect::<Vec<_>>(),
+						)
+						.await?;
+					}
+
 					_ => {
 						error!("Library config version is not handled: {:?}", current);
 						return Err(VersionManagerError::UnexpectedMigration {

core/src/library/manager/error.rs

@@ -3,7 +3,7 @@ use crate::{
 	location::{indexer, LocationManagerError},
 };
 
-use sd_p2p::IdentityOrRemoteIdentityErr;
+use sd_p2p::IdentityErr;
 use sd_utils::{
 	db::{self, MissingFieldError},
 	error::{FileIOError, NonUtf8PathError},
@@ -35,7 +35,7 @@ pub enum LibraryManagerError {
 	#[error("failed to watch locations: {0}")]
 	LocationWatcher(#[from] LocationManagerError),
 	#[error("failed to parse library p2p identity: {0}")]
-	Identity(#[from] IdentityOrRemoteIdentityErr),
+	Identity(#[from] IdentityErr),
 	#[error("failed to load private key for instance p2p identity")]
 	InvalidIdentity,
 	#[error("current instance with id '{0}' was not found in the database")]

core/src/library/manager/mod.rs

@@ -12,7 +12,7 @@ use crate::{
 };
 
 use sd_core_sync::SyncMessage;
-use sd_p2p::{Identity, IdentityOrRemoteIdentity};
+use sd_p2p::Identity;
 use sd_prisma::prisma::{crdt_operation, instance, location, SortOrder};
 use sd_utils::{
 	db,
@@ -203,16 +203,20 @@ impl Libraries {
 				self.libraries_dir.join(format!("{id}.db")),
 				config_path,
 				Some({
+					let identity = Identity::new();
 					let mut create = instance.unwrap_or_else(|| instance::Create {
 						pub_id: Uuid::new_v4().as_bytes().to_vec(),
-						identity: IdentityOrRemoteIdentity::Identity(Identity::new()).to_bytes(),
+						remote_identity: identity.to_remote_identity().get_bytes().to_vec(),
 						node_id: node_cfg.id.as_bytes().to_vec(),
 						last_seen: now,
 						date_created: now,
-						_params: vec![instance::metadata::set(Some(
-							serde_json::to_vec(&node.p2p.peer_metadata())
-								.expect("invalid node metadata"),
-						))],
+						_params: vec![
+							instance::identity::set(Some(identity.to_bytes())),
+							instance::metadata::set(Some(
+								serde_json::to_vec(&node.p2p.peer_metadata())
+									.expect("invalid node metadata"),
+							)),
+						],
 					});
 					create._params.push(instance::id::set(config.instance_id));
 					create
@@ -423,14 +427,11 @@ impl Libraries {
 			})?
 			.clone();
 
-		let identity = Arc::new(
-			match IdentityOrRemoteIdentity::from_bytes(&instance.identity)? {
-				IdentityOrRemoteIdentity::Identity(identity) => identity,
-				IdentityOrRemoteIdentity::RemoteIdentity(_) => {
-					return Err(LibraryManagerError::InvalidIdentity)
-				}
-			},
-		);
+		let identity = match instance.identity.as_ref() {
+			Some(b) => Arc::new(Identity::from_bytes(&b)?),
+			// We are not this instance, so we don't have the private key.
+			None => return Err(LibraryManagerError::InvalidIdentity),
+		};
 
 		let instance_id = Uuid::from_slice(&instance.pub_id)?;
 		let curr_metadata: Option<HashMap<String, String>> = instance

core/src/p2p/libraries.rs

@@ -1,8 +1,6 @@
 use std::{collections::HashMap, sync::Arc};
 
-use sd_p2p::{
-	flume::bounded, HookEvent, HookId, IdentityOrRemoteIdentity, PeerConnectionCandidate, P2P,
-};
+use sd_p2p::{flume::bounded, HookEvent, HookId, PeerConnectionCandidate, RemoteIdentity, P2P};
 use tracing::error;
 
 use crate::library::{Libraries, LibraryManagerEvent};
@@ -38,9 +36,8 @@ pub fn libraries_hook(p2p: Arc<P2P>, libraries: Arc<Libraries>) -> HookId {
 							};
 
 							for i in instances.iter() {
-								let identity = IdentityOrRemoteIdentity::from_bytes(&i.identity)
-									.expect("lol: invalid DB entry")
-									.remote_identity();
+								let identity = RemoteIdentity::from_bytes(&i.remote_identity)
+									.expect("lol: invalid DB entry");
 
 								// Skip self
 								if identity == library.identity.to_remote_identity() {
@@ -68,9 +65,8 @@ pub fn libraries_hook(p2p: Arc<P2P>, libraries: Arc<Libraries>) -> HookId {
 							};
 
 							for i in instances.iter() {
-								let identity = IdentityOrRemoteIdentity::from_bytes(&i.identity)
-									.expect("lol: invalid DB entry")
-									.remote_identity();
+								let identity = RemoteIdentity::from_bytes(&i.remote_identity)
+									.expect("lol: invalid DB entry");
 
 								let peers = p2p.peers();
 								let Some(peer) = peers.get(&identity) else {

crates/file-path-helper/src/lib.rs

@@ -93,6 +93,7 @@ file_path::select!(file_path_to_handle_custom_uri {
 		path
 		instance: select {
 			identity
+			remote_identity
 		}
 	}
 });

crates/p2p/src/identity.rs

@@ -170,48 +170,3 @@ impl From<ed25519_dalek::SigningKey> for Identity {
 		Self(value)
 	}
 }
-
-#[derive(Debug, Error)]
-pub enum IdentityOrRemoteIdentityErr {
-	#[error("IdentityErr({0})")]
-	IdentityErr(#[from] IdentityErr),
-	#[error("InvalidFormat")]
-	InvalidFormat,
-}
-
-/// TODO: Remove this. I think it make security issues far too easy.
-#[derive(Debug, PartialEq)]
-pub enum IdentityOrRemoteIdentity {
-	Identity(Identity),
-	RemoteIdentity(RemoteIdentity),
-}
-
-impl IdentityOrRemoteIdentity {
-	pub fn remote_identity(&self) -> RemoteIdentity {
-		match self {
-			Self::Identity(identity) => identity.to_remote_identity(),
-			Self::RemoteIdentity(identity) => {
-				RemoteIdentity::from_bytes(identity.get_bytes().as_slice()).expect("unreachable")
-			}
-		}
-	}
-}
-
-impl IdentityOrRemoteIdentity {
-	pub fn from_bytes(bytes: &[u8]) -> Result<Self, IdentityOrRemoteIdentityErr> {
-		match bytes[0] {
-			b'I' => Ok(Self::Identity(Identity::from_bytes(&bytes[1..])?)),
-			b'R' => Ok(Self::RemoteIdentity(RemoteIdentity::from_bytes(
-				&bytes[1..],
-			)?)),
-			_ => Err(IdentityOrRemoteIdentityErr::InvalidFormat),
-		}
-	}
-
-	pub fn to_bytes(&self) -> Vec<u8> {
-		match self {
-			Self::Identity(identity) => [&[b'I'], &*identity.to_bytes()].concat(),
-			Self::RemoteIdentity(identity) => [[b'R'].as_slice(), &identity.get_bytes()].concat(),
-		}
-	}
-}

crates/p2p/src/lib.rs

@@ -11,9 +11,7 @@ mod smart_guards;
 mod stream;
 
 pub use hooks::{HookEvent, HookId, ListenerId, ShutdownGuard};
-pub use identity::{
-	Identity, IdentityErr, IdentityOrRemoteIdentity, IdentityOrRemoteIdentityErr, RemoteIdentity,
-};
+pub use identity::{Identity, IdentityErr, RemoteIdentity};
 pub use mdns::Mdns;
 pub use p2p::{Listener, P2P};
 pub use peer::{ConnectionRequest, Peer, PeerConnectionCandidate};

crates/utils/src/db.rs

@@ -7,12 +7,11 @@ use thiserror::Error;
 pub enum MigrationError {
 	#[error("An error occurred while initialising a new database connection: {0}")]
 	NewClient(#[from] Box<NewClientError>),
-	#[cfg(debug_assertions)]
-	#[error("An error occurred during migration: {0}")]
-	MigrateFailed(#[from] DbPushError),
-	#[cfg(not(debug_assertions))]
 	#[error("An error occurred during migration: {0}")]
 	MigrateFailed(#[from] MigrateDeployError),
+	#[cfg(debug_assertions)]
+	#[error("An error occurred during migration: {0}")]
+	DbPushFailed(#[from] DbPushError),
 }
 
 /// load_and_migrate will load the database from the given path and migrate it to the latest version of the schema.
@@ -21,6 +20,8 @@ pub async fn load_and_migrate(db_url: &str) -> Result<PrismaClient, MigrationErr
 		.await
 		.map_err(Box::new)?;
 
+	client._migrate_deploy().await?;
+
 	#[cfg(debug_assertions)]
 	{
 		let mut builder = client._db_push();
@@ -51,9 +52,6 @@ pub async fn load_and_migrate(db_url: &str) -> Result<PrismaClient, MigrationErr
 		}
 	}
 
-	#[cfg(not(debug_assertions))]
-	client._migrate_deploy().await?;
-
 	Ok(client)
 }
 

packages/client/src/core.ts

@@ -393,7 +393,7 @@ instance_id: number;
  */
 cloud_id?: string | null; generate_sync_operations?: boolean; version: LibraryConfigVersion }
 
-export type LibraryConfigVersion = "V0" | "V1" | "V2" | "V3" | "V4" | "V5" | "V6" | "V7" | "V8" | "V9"
+export type LibraryConfigVersion = "V0" | "V1" | "V2" | "V3" | "V4" | "V5" | "V6" | "V7" | "V8" | "V9" | "V10"
 
 export type LibraryConfigWrapped = { uuid: string; instance_id: string; instance_public_key: RemoteIdentity; config: LibraryConfig }