Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Syncs should be limited to the specified organization/project #103

Open
tlater-famedly opened this issue Jan 15, 2025 · 0 comments
Open
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@tlater-famedly
Copy link
Contributor

tlater-famedly commented Jan 15, 2025

Currently, when given a service user with sufficient permissions to do this, users who are not in the set organization/project will also be subjected to the sync, and therefore usually simply deleted. This is obviously very hazardous behavior, we should not just do that.

To limit the sync to just one organization is pretty simple:

diff --git a/src/zitadel.rs b/src/zitadel.rs
index ae205be..5c9c7e1 100644
--- a/src/zitadel.rs
+++ b/src/zitadel.rs
@@ -10,9 +10,10 @@ use zitadel_rust_client::{
 	v1::Zitadel as ZitadelClientV1,
 	v2::{
 		users::{
-			AddHumanUserRequest, IdpLink, InUserEmailsQuery, ListUsersRequest, Organization,
-			SearchQuery, SetHumanEmail, SetHumanPhone, SetHumanProfile, SetMetadataEntry,
-			TypeQuery, UpdateHumanUserRequest, User as ZitadelUser, UserFieldName, Userv2Type,
+			AddHumanUserRequest, AndQuery, IdpLink, InUserEmailsQuery, ListUsersRequest,
+			Organization, OrganizationIdQuery, SearchQuery, SetHumanEmail, SetHumanPhone,
+			SetHumanProfile, SetMetadataEntry, TypeQuery, UpdateHumanUserRequest,
+			User as ZitadelUser, UserFieldName, Userv2Type,
 		},
 		Zitadel as ZitadelClient,
 	},
@@ -95,9 +96,14 @@ impl Zitadel {
 	pub fn list_users(&mut self) -> Result<impl Stream<Item = Result<(User, String)>> + Send> {
 		self.zitadel_client
 			.list_users(
-				ListUsersRequest::new(vec![
-					SearchQuery::new().with_type_query(TypeQuery::new(Userv2Type::Human))
-				])
+				ListUsersRequest::new(vec![SearchQuery::new().with_and_query(
+					AndQuery::new().with_queries(vec![
+						SearchQuery::new().with_type_query(TypeQuery::new(Userv2Type::Human)),
+						SearchQuery::new().with_organization_id_query(OrganizationIdQuery::new(
+							self.zitadel_config.organization_id.clone(),
+						)),
+					]),
+				)])
 				.with_asc(true)
 				.with_sorting_column(UserFieldName::NickName),
 			)

However, limiting this to just the project grants is a bit more complex. This information is not tied to the user, but the project, so at the very least we would need an additional query for every user to check which projects they are in (i.e., search for the user's ID with https://zitadel.com/docs/apis/resources/mgmt/management-service-list-project-grant-members, or get a list of users with that search and add their IDs to the AndQuery - actually, the latter just seems better, hopefully there is not a limit to the number of IDs that can be added to a query).

These limits also add an issue in cases where an existing user is added to a new project, since it will create a new user instead of simply adding a new scope; this is especially problematic since we made the Zitadel ID a foreign key. I believe that that should never be the case in a famedly context, but we should confirm this.

@tlater-famedly tlater-famedly added bug Something isn't working good first issue Good for newcomers labels Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

1 participant