Groups should not contain duplicates

rfd-api/src/context.rs

@@ -25,7 +25,7 @@ use rfd_model::{
 };
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
-use std::{collections::HashMap, sync::Arc};
+use std::{collections::{HashMap, BTreeSet}, sync::Arc};
 use tap::TapFallible;
 use thiserror::Error;
 use tracing::{info_span, instrument, Instrument};
@@ -332,7 +332,7 @@ impl ApiContext {
         let groups = AccessGroupStore::list(
             &*self.storage,
             AccessGroupFilter {
-                id: Some(user.groups.clone()),
+                id: Some(user.groups.iter().copied().collect()),
                 ..Default::default()
             },
             &ListPagination::default().limit(UNLIMITED),
@@ -572,9 +572,9 @@ impl ApiContext {
         }
     }
 
-    async fn get_mapped_fields(&self, info: &UserInfo) -> Result<(ApiPermissions, Vec<Uuid>), StoreError> {
+    async fn get_mapped_fields(&self, info: &UserInfo) -> Result<(ApiPermissions, BTreeSet<Uuid>), StoreError> {
         let mut mapped_permissions = ApiPermissions::new();
-        let mut mapped_groups = vec![];
+        let mut mapped_groups = BTreeSet::new();
 
         // We optimistically load mappers here. We do not want to take a lock on the mappers and
         // instead handle mappers that become depleted before we can evaluate them at evaluation
@@ -623,7 +623,7 @@ impl ApiContext {
         &self,
         api_user_id: Uuid,
         mut mapped_permissions: ApiPermissions,
-        mut mapped_groups: Vec<Uuid>,
+        mut mapped_groups: BTreeSet<Uuid>,
     ) -> Result<User, ApiError> {
         match self.get_api_user(&api_user_id).await? {
             Some(api_user) => {
@@ -996,7 +996,7 @@ impl ApiContext {
 
         Ok(if let Some(user) = user {
             let mut update: NewApiUser<ApiPermission> = user.into();
-            update.groups.push(*group_id);
+            update.groups.insert(*group_id);
 
             Some(ApiUserStore::upsert(&*self.storage, update).await?)
         } else {
@@ -1078,7 +1078,7 @@ mod tests {
         storage::{AccessGroupFilter, ListPagination, MockAccessGroupStore, MockApiUserStore},
         AccessGroup, ApiUser,
     };
-    use std::{ops::Add, sync::Arc};
+    use std::{ops::Add, sync::Arc, collections::BTreeSet};
     use uuid::Uuid;
 
     use crate::{
@@ -1146,10 +1146,12 @@ mod tests {
 
         let user_id = Uuid::new_v4();
         let user_permissions: ApiPermissions = vec![ApiPermission::GetRfd(5)].into();
+        let mut groups = BTreeSet::new();
+        groups.insert(group_id);
         let user = ApiUser {
             id: user_id,
             permissions: user_permissions.clone(),
-            groups: vec![group_id],
+            groups,
             created_at: Utc::now(),
             updated_at: Utc::now(),
             deleted_at: None,

rfd-api/src/endpoints/api_user.rs

@@ -1,3 +1,5 @@
+use std::collections::BTreeSet;
+
 use chrono::{DateTime, Utc};
 use dropshot::{
     endpoint, HttpError, HttpResponseCreated, HttpResponseOk, Path, RequestContext, TypedBody,
@@ -87,7 +89,7 @@ async fn get_api_user_op(
 #[derive(Debug, Clone, PartialEq, Deserialize, JsonSchema)]
 pub struct ApiUserUpdateParams {
     permissions: ApiPermissions,
-    groups: Vec<Uuid>,
+    groups: BTreeSet<Uuid>,
 }
 
 /// Create a new user with a given set of permissions
@@ -470,7 +472,7 @@ pub async fn remove_api_user_from_group(
 
 #[cfg(test)]
 mod tests {
-    use std::sync::Arc;
+    use std::{sync::Arc, collections::BTreeSet};
 
     use chrono::{Duration, Utc};
     use http::StatusCode;
@@ -499,7 +501,7 @@ mod tests {
         ApiUser {
             id: Uuid::new_v4(),
             permissions: vec![].into(),
-            groups: vec![],
+            groups: BTreeSet::new(),
             created_at: Utc::now(),
             updated_at: Utc::now(),
             deleted_at: None,
@@ -510,12 +512,12 @@ mod tests {
     async fn test_create_api_user_permissions() {
         let successful_update = ApiUserUpdateParams {
             permissions: vec![ApiPermission::CreateApiUser.into()].into(),
-            groups: vec![],
+            groups: BTreeSet::new(),
         };
 
         let failure_update = ApiUserUpdateParams {
             permissions: vec![ApiPermission::GetApiUserAll.into()].into(),
-            groups: vec![],
+            groups: BTreeSet::new(),
         };
 
         let mut store = MockApiUserStore::new();
@@ -528,7 +530,7 @@ mod tests {
                 Ok(ApiUser {
                     id: user.id,
                     permissions: user.permissions,
-                    groups: vec![],
+                    groups: BTreeSet::new(),
                     created_at: Utc::now(),
                     updated_at: Utc::now(),
                     deleted_at: None,
@@ -586,13 +588,13 @@ mod tests {
         let success_id = Uuid::new_v4();
         let successful_update = ApiUserUpdateParams {
             permissions: Vec::new().into(),
-            groups: vec![],
+            groups: BTreeSet::new(),
         };
 
         let failure_id = Uuid::new_v4();
         let failure_update = ApiUserUpdateParams {
             permissions: Vec::new().into(),
-            groups: vec![],
+            groups: BTreeSet::new(),
         };
 
         let mut store = MockApiUserStore::new();
@@ -603,7 +605,7 @@ mod tests {
                 Ok(ApiUser {
                     id: user.id,
                     permissions: user.permissions,
-                    groups: vec![],
+                    groups: BTreeSet::new(),
                     created_at: Utc::now(),
                     updated_at: Utc::now(),
                     deleted_at: None,
@@ -821,7 +823,7 @@ mod tests {
         let api_user = ApiUser {
             id: api_user_id,
             permissions: vec![ApiPermission::GetApiUserToken(api_user_id).into()].into(),
-            groups: vec![],
+            groups: BTreeSet::new(),
             created_at: Utc::now(),
             updated_at: Utc::now(),
             deleted_at: None,

rfd-api/src/endpoints/login/oauth/client.rs

@@ -315,7 +315,7 @@ async fn delete_oauth_client_redirect_uri_op(
 
 #[cfg(test)]
 mod tests {
-    use std::sync::{Arc, Mutex};
+    use std::{sync::{Arc, Mutex}, collections::BTreeSet};
 
     use chrono::Utc;
     use rfd_model::{
@@ -338,7 +338,7 @@ mod tests {
         ApiUser {
             id: Uuid::new_v4(),
             permissions: vec![ApiPermission::CreateOAuthClient].into(),
-            groups: vec![],
+            groups: BTreeSet::new(),
             created_at: Utc::now(),
             updated_at: Utc::now(),
             deleted_at: None,
@@ -352,7 +352,7 @@ mod tests {
             Ok(ApiUser {
                 id: user.id,
                 permissions: user.permissions,
-                groups: vec![],
+                groups: user.groups,
                 created_at: Utc::now(),
                 updated_at: Utc::now(),
                 deleted_at: None,

rfd-api/src/mapper.rs

@@ -1,3 +1,5 @@
+use std::collections::BTreeSet;
+
 use async_trait::async_trait;
 use rfd_model::storage::StoreError;
 use serde::{Deserialize, Serialize};
@@ -12,7 +14,7 @@ pub trait MapperRule: Send + Sync {
         ctx: &ApiContext,
         user: &UserInfo,
     ) -> Result<ApiPermissions, StoreError>;
-    async fn groups_for(&self, ctx: &ApiContext, user: &UserInfo) -> Result<Vec<Uuid>, StoreError>;
+    async fn groups_for(&self, ctx: &ApiContext, user: &UserInfo) -> Result<BTreeSet<Uuid>, StoreError>;
 }
 
 #[derive(Debug, Serialize)]
@@ -45,7 +47,7 @@ impl MapperRule for EmailDomainMapper {
         Ok(ApiPermissions::new())
     }
 
-    async fn groups_for(&self, ctx: &ApiContext, user: &UserInfo) -> Result<Vec<Uuid>, StoreError> {
+    async fn groups_for(&self, ctx: &ApiContext, user: &UserInfo) -> Result<BTreeSet<Uuid>, StoreError> {
         let has_email_in_domain = user
             .verified_emails
             .iter()
@@ -63,10 +65,10 @@ impl MapperRule for EmailDomainMapper {
                         None
                     }
                 })
-                .collect::<Vec<_>>();
+                .collect();
             Ok(groups)
         } else {
-            Ok(vec![])
+            Ok(BTreeSet::new())
         }
     }
 }
@@ -83,7 +85,7 @@ impl MapperRule for MappingRules {
         }
     }
 
-    async fn groups_for(&self, ctx: &ApiContext, user: &UserInfo) -> Result<Vec<Uuid>, StoreError> {
+    async fn groups_for(&self, ctx: &ApiContext, user: &UserInfo) -> Result<BTreeSet<Uuid>, StoreError> {
         match self {
             Self::EmailDomain(rule) => rule.groups_for(ctx, user).await,
         }

rfd-model/src/lib.rs

@@ -1,4 +1,4 @@
-use std::{collections::BTreeMap, fmt::Display};
+use std::{collections::{BTreeMap, BTreeSet}, fmt::Display};
 
 use chrono::{DateTime, Utc};
 use db::{
@@ -164,7 +164,7 @@ impl From<JobModel> for Job {
 pub struct ApiUser<T: Ord> {
     pub id: Uuid,
     pub permissions: Permissions<T>,
-    pub groups: Vec<Uuid>,
+    pub groups: BTreeSet<Uuid>,
     #[partial(NewApiUser(skip))]
     pub created_at: DateTime<Utc>,
     #[partial(NewApiUser(skip))]

rfd-model/src/storage/postgres.rs

@@ -554,7 +554,7 @@ where
             .values((
                 api_user::id.eq(user.id),
                 api_user::permissions.eq(user.permissions.clone()),
-                api_user::groups.eq(user.groups.clone()),
+                api_user::groups.eq(user.groups.into_iter().collect::<Vec<_>>()),
             ))
             .on_conflict(api_user::id)
             .do_update()

rfd-model/tests/postgres.rs

@@ -1,3 +1,5 @@
+use std::collections::BTreeSet;
+
 use chrono::{Duration, Utc};
 use diesel::{
     migration::{Migration, MigrationSource},
@@ -126,7 +128,7 @@ async fn test_api_user() {
         NewApiUser {
             id: api_user_id,
             permissions: vec![TestPermission::CreateApiKey(api_user_id).into()].into(),
-            groups: vec![],
+            groups: BTreeSet::new(),
         },
     )
     .await
@@ -146,7 +148,7 @@ async fn test_api_user() {
         NewApiUser {
             id: api_user_id,
             permissions: vec![TestPermission::CreateApiKey(api_user_id).into()].into(),
-            groups: vec![],
+            groups: BTreeSet::new(),
         },
     )
     .await
@@ -165,7 +167,7 @@ async fn test_api_user() {
                 TestPermission::DeleteApiKey(api_user_id).into(),
             ]
             .into(),
-            groups: vec![],
+            groups: BTreeSet::new(),
         },
     )
     .await