From e0ebff4f613956266c1bb974f4833576cea96f5c Mon Sep 17 00:00:00 2001
From: Jordan Doyle <jordan@doyle.la>
Date: Sat, 18 Sep 2021 03:15:46 +0100
Subject: [PATCH] Pass UUIDs when the frontend and backend are communicating rather than the primary key

---
 Cargo.lock                                              |  1 +
 chartered-db/Cargo.toml                                 |  1 +
 chartered-db/src/lib.rs                                 |  1 +
 chartered-db/src/schema.rs                              |  2 ++
 chartered-db/src/users.rs                               | 33 ++++++++++++++++++++++++++++++++-
 chartered-db/src/uuid.rs                                | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 migrations/2021-08-31-214501_create_crates_table/up.sql |  4 ++++
 chartered-frontend/src/pages/crate/Members.tsx          | 28 ++++++++++++++++------------
 chartered-frontend/src/pages/ssh-keys/ListSshKeys.tsx   |  6 +++---
 chartered-web/src/endpoints/cargo_api/owners.rs         |  5 +++--
 chartered-web/src/endpoints/web_api/search_users.rs     |  4 ++--
 chartered-web/src/endpoints/web_api/ssh_key.rs          |  9 +++++----
 chartered-web/src/endpoints/web_api/crates/members.rs   | 29 +++++++++++++++++++----------
 13 files changed, 133 insertions(+), 43 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index ba708b9..344bbaa 100644
--- a/Cargo.lock
+++ a/Cargo.lock
@@ -210,6 +210,7 @@
  "thiserror",
  "thrussh-keys",
  "tokio",
+ "uuid",
 ]
 
 [[package]]
diff --git a/chartered-db/Cargo.toml b/chartered-db/Cargo.toml
index 04422f0..62e8f9b 100644
--- a/chartered-db/Cargo.toml
+++ a/chartered-db/Cargo.toml
@@ -22,5 +22,6 @@
 serde_json = "1"
 thiserror = "1"
 tokio = "1"
+uuid = "0.8"
 dotenv = "0.15"
 thrussh-keys = "0.21"
diff --git a/chartered-db/src/lib.rs b/chartered-db/src/lib.rs
index 37d3bf7..45aeffb 100644
--- a/chartered-db/src/lib.rs
+++ a/chartered-db/src/lib.rs
@@ -32,6 +32,7 @@
 pub mod crates;
 pub mod schema;
 pub mod users;
+pub mod uuid;
 
 #[macro_use]
 extern crate diesel;
diff --git a/chartered-db/src/schema.rs b/chartered-db/src/schema.rs
index df2edd6..af75a13 100644
--- a/chartered-db/src/schema.rs
+++ a/chartered-db/src/schema.rs
@@ -48,6 +48,7 @@
 table! {
     user_ssh_keys (id) {
         id -> Integer,
+        uuid -> Binary,
         name -> Text,
         user_id -> Integer,
         ssh_key -> Binary,
@@ -59,6 +60,7 @@
 table! {
     users (id) {
         id -> Integer,
+        uuid -> Binary,
         username -> Text,
     }
 }
diff --git a/chartered-db/src/users.rs b/chartered-db/src/users.rs
index be6de05..0c3434f 100644
--- a/chartered-db/src/users.rs
+++ a/chartered-db/src/users.rs
@@ -1,5 +1,6 @@
 use super::{
     schema::{user_crate_permissions, user_sessions, user_ssh_keys, users},
+    uuid::SqlUuid,
     ConnectionPool, Result,
 };
 use bitflags::bitflags;
@@ -12,6 +13,7 @@
 #[derive(Identifiable, Queryable, Associations, PartialEq, Eq, Hash, Debug)]
 pub struct User {
     pub id: i32,
+    pub uuid: SqlUuid,
     pub username: String,
 }
 
@@ -51,6 +53,23 @@
         .await?
     }
 
+    pub async fn find_by_uuid(
+        conn: ConnectionPool,
+        given_uuid: uuid::Uuid,
+    ) -> Result<Option<User>> {
+        use crate::schema::users::dsl::uuid;
+
+        tokio::task::spawn_blocking(move || {
+            let conn = conn.get()?;
+
+            Ok(crate::schema::users::table
+                .filter(uuid.eq(SqlUuid(given_uuid)))
+                .get_result(&conn)
+                .optional()?)
+        })
+        .await?
+    }
+
     pub async fn find_by_session_key(
         conn: ConnectionPool,
         given_session_key: String,
@@ -68,7 +87,7 @@
                 )
                 .filter(session_key.eq(given_session_key))
                 .inner_join(users::table)
-                .select((users::dsl::id, users::dsl::username))
+                .select(users::all_columns)
                 .get_result(&conn)
                 .optional()?)
         })
@@ -112,12 +131,13 @@
         let parsed_name = split.next().unwrap_or("(none)").to_string();
 
         tokio::task::spawn_blocking(move || {
-            use crate::schema::user_ssh_keys::dsl::{name, ssh_key, user_id};
+            use crate::schema::user_ssh_keys::dsl::{name, ssh_key, user_id, uuid};
 
             let conn = conn.get()?;
 
             insert_into(crate::schema::user_ssh_keys::dsl::user_ssh_keys)
                 .values((
+                    uuid.eq(SqlUuid::random()),
                     name.eq(parsed_name),
                     ssh_key.eq(parsed_key.public_key_bytes()),
                     user_id.eq(self.id),
@@ -129,12 +149,12 @@
         .await?
     }
 
-    pub async fn delete_user_ssh_key_by_id(
+    pub async fn delete_user_ssh_key_by_uuid(
         self: Arc<Self>,
         conn: ConnectionPool,
-        ssh_key_id: i32,
+        ssh_key_id: uuid::Uuid,
     ) -> Result<bool> {
-        use crate::schema::user_ssh_keys::dsl::{id, user_id, user_ssh_keys};
+        use crate::schema::user_ssh_keys::dsl::{uuid, user_id, user_ssh_keys};
 
         tokio::task::spawn_blocking(move || {
             let conn = conn.get()?;
@@ -142,7 +162,7 @@
             let rows = diesel::delete(
                 user_ssh_keys
                     .filter(user_id.eq(self.id))
-                    .filter(id.eq(ssh_key_id)),
+                    .filter(uuid.eq(SqlUuid(ssh_key_id))),
             )
             .execute(&conn)?;
 
@@ -317,6 +337,7 @@
 #[belongs_to(User)]
 pub struct UserSshKey {
     pub id: i32,
+    pub uuid: SqlUuid,
     pub name: String,
     pub user_id: i32,
     pub ssh_key: Vec<u8>,
diff --git a/chartered-db/src/uuid.rs b/chartered-db/src/uuid.rs
new file mode 100644
index 0000000..bf3771a 100644
--- /dev/null
+++ a/chartered-db/src/uuid.rs
@@ -1,0 +1,53 @@
+use diesel::sql_types::Binary;
+use std::fmt;
+use std::fmt::{Display, Formatter};
+use std::io::prelude::*;
+pub use uuid::Uuid;
+
+#[derive(Debug, Clone, Copy, FromSqlRow, AsExpression, Hash, Eq, PartialEq)]
+#[sql_type = "Binary"]
+pub struct SqlUuid(pub uuid::Uuid);
+
+impl SqlUuid {
+    pub fn random() -> Self {
+        Self(uuid::Uuid::new_v4())
+    }
+}
+
+impl From<SqlUuid> for uuid::Uuid {
+    fn from(s: SqlUuid) -> Self {
+        s.0
+    }
+}
+
+impl Display for SqlUuid {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        write!(f, "{}", self.0)
+    }
+}
+
+impl<B: diesel::backend::Backend> diesel::deserialize::FromSql<Binary, B> for SqlUuid
+where
+    Vec<u8>: diesel::deserialize::FromSql<Binary, B>,
+{
+    fn from_sql(bytes: Option<&B::RawValue>) -> diesel::deserialize::Result<Self> {
+        let value = <Vec<u8>>::from_sql(bytes)?;
+        uuid::Uuid::from_slice(&value)
+            .map(SqlUuid)
+            .map_err(|e| e.into())
+    }
+}
+
+impl<B: diesel::backend::Backend> diesel::serialize::ToSql<Binary, B> for SqlUuid
+where
+    [u8]: diesel::serialize::ToSql<Binary, B>,
+{
+    fn to_sql<W: Write>(
+        &self,
+        out: &mut diesel::serialize::Output<W, B>,
+    ) -> diesel::serialize::Result {
+        out.write_all(self.0.as_bytes())
+            .map(|_| diesel::serialize::IsNull::No)
+            .map_err(Into::into)
+    }
+}
diff --git a/migrations/2021-08-31-214501_create_crates_table/up.sql b/migrations/2021-08-31-214501_create_crates_table/up.sql
index 787c099..67c5cdb 100644
--- a/migrations/2021-08-31-214501_create_crates_table/up.sql
+++ a/migrations/2021-08-31-214501_create_crates_table/up.sql
@@ -24,11 +24,15 @@
 
 CREATE TABLE users (
     id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT,
+    uuid BINARY(128) NOT NULL UNIQUE,
     username VARCHAR(255) NOT NULL UNIQUE
 );
 
+INSERT INTO users (id, uuid, username) VALUES (1, X'936DA01F9ABD4D9D80C702AF85C822A8', "admin");
+
 CREATE TABLE user_ssh_keys (
     id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT,
+    uuid BINARY(128) NOT NULL UNIQUE,
     name VARCHAR(255) NOT NULL,
     user_id INTEGER NOT NULL,
     ssh_key BLOB NOT NULL,
diff --git a/chartered-frontend/src/pages/crate/Members.tsx b/chartered-frontend/src/pages/crate/Members.tsx
index f6dca03..f4c0258 100644
--- a/chartered-frontend/src/pages/crate/Members.tsx
+++ a/chartered-frontend/src/pages/crate/Members.tsx
@@ -14,7 +14,7 @@
 }
 
 interface Member {
-    id: number,
+    uuid: string,
     username: string,
     permissions: string[],
 }
@@ -31,7 +31,7 @@
     React.useEffect(() => {
         if (response && response.members) {
             setProspectiveMembers(prospectiveMembers.filter((prospectiveMember) => {
-                _.findIndex(response.members, (responseMember) => responseMember.id === prospectiveMember.id) === -1
+                _.findIndex(response.members, (responseMember) => responseMember.uuid === prospectiveMember.uuid) === -1
             }));
         }
     }, [response])
@@ -75,10 +75,10 @@
                     )}
 
                     <MemberListInserter
-                        onInsert={(username, userId) => setProspectiveMembers([
+                        onInsert={(username, userUuid) => setProspectiveMembers([
                             ...prospectiveMembers,
                             {
-                                id: userId,
+                                uuid: userUuid,
                                 username,
                                 permissions: ["VISIBLE"],
                             }
@@ -111,7 +111,7 @@
                     'Content-Type': 'application/json',
                 },
                 body: JSON.stringify({
-                    user_id: member.id,
+                    user_uuid: member.uuid,
                     permissions: selectedPermissions,
                 }),
             });
@@ -140,7 +140,7 @@
                     'Content-Type': 'application/json',
                 },
                 body: JSON.stringify({
-                    user_id: member.id,
+                    user_uuid: member.uuid,
                 }),
             });
             let json = await res.json();
@@ -195,7 +195,7 @@
                 <RenderPermissions
                     allowedPermissions={allowedPermissions}
                     selectedPermissions={selectedPermissions}
-                    userId={member.id}
+                    userUuid={member.uuid}
                     onChange={setSelectedPermissions}
                 />
             </td>
@@ -207,7 +207,7 @@
     </>;
 }
 
-function MemberListInserter({ onInsert, existingMembers }: { existingMembers: Member[], onInsert: (username, user_id) => any }) {
+function MemberListInserter({ onInsert, existingMembers }: { existingMembers: Member[], onInsert: (username, user_uuid) => any }) {
     const auth = useAuth();
     const searchRef = React.useRef(null);
     const [loading, setLoading] = useState(false);
@@ -235,7 +235,7 @@
     };
 
     const handleChange = (selected) => {
-        onInsert(selected[0].username, selected[0].user_id);
+        onInsert(selected[0].username, selected[0].user_uuid);
         searchRef.current.clear();
     }
 
@@ -253,7 +253,7 @@
             <AsyncTypeahead
                 id="search-new-user"
                 onSearch={handleSearch}
-                filterBy={(option) => _.findIndex(existingMembers, (existing) => option.user_id === existing.id) === -1}
+                filterBy={(option) => _.findIndex(existingMembers, (existing) => option.user_uuid === existing.uuid) === -1}
                 labelKey="username"
                 options={options}
                 isLoading={loading}
@@ -284,16 +284,16 @@
     </tr>;
 }
 
-function RenderPermissions({ allowedPermissions, selectedPermissions, userId, onChange }: { allowedPermissions: string[], selectedPermissions: string[], userId: number, onChange: (permissions) => any }) {
+function RenderPermissions({ allowedPermissions, selectedPermissions, userUuid, onChange }: { allowedPermissions: string[], selectedPermissions: string[], userUuid: number, onChange: (permissions) => any }) {
     return (
         <div className="row ms-2">
             {allowedPermissions.map((permission) => (
-                <div key={permission + userId} className="form-check col-12 col-md-6">
+                <div key={permission + userUuid} className="form-check col-12 col-md-6">
                     <input
                         className="form-check-input"
                         type="checkbox"
                         value="1"
-                        id={`checkbox-${userId}-${permission}`}
+                        id={`checkbox-${userUuid}-${permission}`}
                         checked={selectedPermissions.indexOf(permission) > -1}
                         onChange={(e) => {
                             let newUserPermissions = new Set(selectedPermissions);
@@ -307,7 +307,7 @@
                             onChange(Array.from(newUserPermissions));
                         }}
                     />
-                    <label className="form-check-label" htmlFor={`checkbox-${userId}-${permission}`}>
+                    <label className="form-check-label" htmlFor={`checkbox-${userUuid}-${permission}`}>
                         {permission}
                     </label>
                 </div>
diff --git a/chartered-frontend/src/pages/ssh-keys/ListSshKeys.tsx b/chartered-frontend/src/pages/ssh-keys/ListSshKeys.tsx
index 7a410a4..c7b17a9 100644
--- a/chartered-frontend/src/pages/ssh-keys/ListSshKeys.tsx
+++ a/chartered-frontend/src/pages/ssh-keys/ListSshKeys.tsx
@@ -17,7 +17,7 @@
 }
 
 interface SshKeysResponseKey {
-    id: number,
+    uuid: string,
     name: string,
     fingerprint: string,
     created_at: string,
@@ -46,7 +46,7 @@
         setError("");
 
         try {
-            let res = await fetch(authenticatedEndpoint(auth, `ssh-key/${deleting.id}`), {
+            let res = await fetch(authenticatedEndpoint(auth, `ssh-key/${deleting.uuid}`), {
                 method: 'DELETE',
                 headers: {
                     'Content-Type': 'application/json',
@@ -88,7 +88,7 @@
                         <table className="table table-striped">
                             <tbody>
                                 {sshKeys.keys.map(key => (
-                                    <tr key={key.id}>
+                                    <tr key={key.uuid}>
                                         <td className="align-middle">
                                             <h6 className="m-0 lh-sm">{key.name}</h6>
                                             <pre className="m-0">{key.fingerprint}</pre>
diff --git a/chartered-web/src/endpoints/cargo_api/owners.rs b/chartered-web/src/endpoints/cargo_api/owners.rs
index bdd9d96..f194d70 100644
--- a/chartered-web/src/endpoints/cargo_api/owners.rs
+++ a/chartered-web/src/endpoints/cargo_api/owners.rs
@@ -36,7 +36,8 @@
 
 #[derive(Serialize)]
 pub struct GetResponseUser {
-    id: i32,
+    // cargo spec says this should be an unsigned 32-bit integer
+    // uuid: chartered_db::uuid::Uuid,
     login: String,
     name: Option<String>,
 }
@@ -60,7 +61,7 @@
         .await?
         .into_iter()
         .map(|user| GetResponseUser {
-            id: user.id,
+            // uuid: user.uuid.0,
             login: user.username,
             name: None,
         })
diff --git a/chartered-web/src/endpoints/web_api/search_users.rs b/chartered-web/src/endpoints/web_api/search_users.rs
index 7fddc45..9278150 100644
--- a/chartered-web/src/endpoints/web_api/search_users.rs
+++ a/chartered-web/src/endpoints/web_api/search_users.rs
@@ -15,7 +15,7 @@
 
 #[derive(Serialize)]
 pub struct ResponseUser {
-    user_id: i32,
+    user_uuid: chartered_db::uuid::Uuid,
     username: String,
 }
 
@@ -27,7 +27,7 @@
         .await?
         .into_iter()
         .map(|user| ResponseUser {
-            user_id: user.id,
+            user_uuid: user.uuid.0,
             username: user.username,
         })
         .collect();
diff --git a/chartered-web/src/endpoints/web_api/ssh_key.rs b/chartered-web/src/endpoints/web_api/ssh_key.rs
index 272f170..2231390 100644
--- a/chartered-web/src/endpoints/web_api/ssh_key.rs
+++ a/chartered-web/src/endpoints/web_api/ssh_key.rs
@@ -6,6 +6,7 @@
 use serde::{Deserialize, Serialize};
 use std::sync::Arc;
 use thiserror::Error;
+use chartered_db::uuid::Uuid;
 
 use crate::endpoints::ErrorResponse;
 
@@ -16,7 +17,7 @@
 
 #[derive(Serialize)]
 pub struct GetResponseKey {
-    id: i32, // TODO: this should be a UUID so we don't leak incremental IDs
+    uuid: Uuid,
     name: String,
     fingerprint: String,
     created_at: NaiveDateTime,
@@ -32,7 +33,7 @@
         .await?
         .into_iter()
         .map(|key| GetResponseKey {
-            id: key.id,
+            uuid: key.uuid.0,
             fingerprint: key.fingerprint().unwrap_or_else(|e| {
                 warn!("Failed to parse key with id {}: {}", key.id, e);
                 "INVALID".to_string()
@@ -66,9 +67,9 @@
 pub async fn handle_delete(
     extract::Extension(db): extract::Extension<ConnectionPool>,
     extract::Extension(user): extract::Extension<Arc<User>>,
-    extract::Path((_session_key, ssh_key_id)): extract::Path<(String, i32)>,
+    extract::Path((_session_key, ssh_key_id)): extract::Path<(String, Uuid)>,
 ) -> Result<Json<ErrorResponse>, Error> {
-    let deleted = user.delete_user_ssh_key_by_id(db, ssh_key_id).await?;
+    let deleted = user.delete_user_ssh_key_by_uuid(db, ssh_key_id).await?;
 
     if deleted {
         Ok(Json(ErrorResponse { error: None }))
diff --git a/chartered-web/src/endpoints/web_api/crates/members.rs b/chartered-web/src/endpoints/web_api/crates/members.rs
index 4252017..29bc45a 100644
--- a/chartered-web/src/endpoints/web_api/crates/members.rs
+++ a/chartered-web/src/endpoints/web_api/crates/members.rs
@@ -1,9 +1,5 @@
 use axum::{extract, Json};
-use chartered_db::{
-    crates::Crate,
-    users::{User, UserCratePermissionValue as Permission},
-    ConnectionPool,
-};
+use chartered_db::{ConnectionPool, crates::Crate, users::{User, UserCratePermissionValue as Permission}, uuid::Uuid};
 use serde::{Deserialize, Serialize};
 use std::sync::Arc;
 use thiserror::Error;
@@ -18,7 +14,7 @@
 
 #[derive(Deserialize, Serialize)]
 pub struct GetResponseMember {
-    id: i32,
+    uuid: Uuid,
     username: String,
     permissions: Permission,
 }
@@ -39,7 +35,7 @@
         .await?
         .into_iter()
         .map(|(user, permissions)| GetResponseMember {
-            id: user.id,
+            uuid: user.uuid.0,
             username: user.username,
             permissions,
         })
@@ -53,7 +49,7 @@
 
 #[derive(Deserialize)]
 pub struct PutOrPatchRequest {
-    user_id: i32,
+    user_uuid: chartered_db::uuid::Uuid,
     permissions: Permission,
 }
 
@@ -69,8 +65,10 @@
         .map(std::sync::Arc::new)?;
     ensure_has_crate_perm!(db, user, crate_, Permission::VISIBLE | -> Error::NoCrate, Permission::MANAGE_USERS | -> Error::NoPermission);
 
+    let action_user = User::find_by_uuid(db.clone(), req.user_uuid).await?.ok_or(Error::InvalidUserId)?;
+
     let affected_rows = crate_
-        .update_permissions(db, req.user_id, req.permissions)
+        .update_permissions(db, action_user.id, req.permissions)
         .await?;
     if affected_rows == 0 {
         return Err(Error::UpdateConflictRemoved);
@@ -91,8 +89,10 @@
         .map(std::sync::Arc::new)?;
     ensure_has_crate_perm!(db, user, crate_, Permission::VISIBLE | -> Error::NoCrate, Permission::MANAGE_USERS | -> Error::NoPermission);
 
+    let action_user = User::find_by_uuid(db.clone(), req.user_uuid).await?.ok_or(Error::InvalidUserId)?;
+
     crate_
-        .insert_permissions(db, req.user_id, req.permissions)
+        .insert_permissions(db, action_user.id, req.permissions)
         .await?;
 
     Ok(Json(ErrorResponse { error: None }))
@@ -100,7 +100,7 @@
 
 #[derive(Deserialize)]
 pub struct DeleteRequest {
-    user_id: i32,
+    user_uuid: chartered_db::uuid::Uuid,
 }
 
 pub async fn handle_delete(
@@ -114,8 +114,10 @@
         .ok_or(Error::NoCrate)
         .map(std::sync::Arc::new)?;
     ensure_has_crate_perm!(db, user, crate_, Permission::VISIBLE | -> Error::NoCrate, Permission::MANAGE_USERS | -> Error::NoPermission);
+
+    let action_user = User::find_by_uuid(db.clone(), req.user_uuid).await?.ok_or(Error::InvalidUserId)?;
 
-    crate_.delete_member(db, req.user_id).await?;
+    crate_.delete_member(db, action_user.id).await?;
 
     Ok(Json(ErrorResponse { error: None }))
 }
@@ -130,6 +132,8 @@
     NoPermission,
     #[error("Permissions update conflict, user was removed as a member of the crate")]
     UpdateConflictRemoved,
+    #[error("An invalid user id was given")]
+    InvalidUserId,
 }
 
 impl Error {
@@ -141,6 +145,7 @@
             Self::NoCrate => StatusCode::NOT_FOUND,
             Self::NoPermission => StatusCode::FORBIDDEN,
             Self::UpdateConflictRemoved => StatusCode::CONFLICT,
+            Self::InvalidUserId => StatusCode::BAD_REQUEST,
         }
     }
 }
--
rgit 0.1.3