Feat: Refactor ldap-sync + implement new sync sources

[jannden]

Closes #2373

[CLAassistant]

All committers have signed the CLA.

[jannden]

This is the first proposition for how to approach extending the ldap-sync.

Here, using a partner's endpoint to get the emails of users that they want us to delete from Zitadel, we wouldn't need LDAP at all. The library zitadel_rust_client would need an additional method delete_user_by_email.

One thing I am pondering -> if taking this approach and sidestepping the LDAP altogether, this might eventually become a zitadel-sync instead of ldap-sync :)

[tlater-famedly]

Here, using a partner's endpoint to get the emails of users that they want us to delete from Zitadel, we wouldn't need LDAP at all. The library zitadel_rust_client would need an additional method delete_user_by_email.

Yeah, that's the idea. For situations where the deactivate-only source is used, LDAP isn't relevant. I'm also fairly certain that method exists already, I think I used it in tests?

One thing I am pondering -> if taking this approach and sidestepping the LDAP altogether, this might eventually become a zitadel-sync instead of ldap-sync :)

Yep, we agreed to rename this to famedly-sync-client.

---

I think the primary difficulty is finding the correct level of abstraction for the "source" concept. Maybe it's enough to just implement lots of ldap.rs, deactivate-list.rs, etc., and then manually call a method for each, and then doing a complex union between sets. otoh, perhaps we should implement a trait to do that properly, so we can implement new source without touching lib.rs. Maybe the config parsing for each source should live in its specific module, rather than in a top-level config.rs. These are all things to explore.

Tests will also likely become more complex. For this particular source, wiremock is probably the way to go, but for others we'll probably need other docker test environments, which should be launched when appropriate with test groups so that the services don't interfere with each other.

[jannden]

Okay, makes perfect sense then :)

[sirewix]

I think the better approach would be to not separate deactivate_only feature into "from ldap" and "from endpoint" (although according to the issue description this design would suffice), but rather abstract in a way that would allow to recieve changes from different source by using different adaptors.

trait DataSource {
  async fn get_data() -> Stream<DataEntry>; // or Vec
}

fn asdf() {
  let changes = match config.source {
    Ldap => LdapSource::new().get_data(),
    Endpoint => Endpoint::new().get_data(),
  };
  ...
}

However if we want to combine a few sources, that might be tricky:

struct DataSourcesConfig {
  get_new: DataSource,
  get_updated: DataSource,
  get_deleted: DataSource,
}

[jannden]

Major changes:

1. I untangled the User from LDAP, so that there is a clear distinction between the Zitadel part and the LDAP part.
2. Based on the comments, I evolved the trait to focus on the Source logic only, without touching anything Zitadel related. The Zitadel logic lives separately and is called directly from the lib.rs.

[sirewix]

The provided api spec for deactivation endpoint is extremely client specific, I think we should either name that data source using client name (like UKTCustomDataSource) or invent a generic configurable format to cover all use cases. The latter seems excessive, especially at this point, so I'd say we should implement simple ad-hoc data source and name it accordingly to client

[jannden]

Thanks for the comments. Valid points, good perspectives. I like how it's crystalizing. Will come up with next version soon-ish.

[jannden]

I will nudge the next version in a slightly new direction. To give it a fresh perspective. Because when we step back for a moment, do we really think that having a trait for unifying the sources actually makes sense?

Consider the fact that the sources are too diverse. They can supply a completely different matrix of data combinations. At this moment, we have only two sources, and already now it's tangled with added, changed, removed on one side VS for each of those sometimes it’s user ID, sometimes LDAP Search Entry, sometimes email address. That results in a huge amount of combinations and unnecessary complexity trying to tie it up with a single get_all method.

Let me present a simplified solution tomorrow and you will let me know how you feel about it.

[jannden]

There is a question we need to address.

So, UKT provides us with a list of email we should delete from Zitadel. In zitadel-rust-client, I see we use email interchangeably with login_name (at least in the e2e tests written for ldap-sync. So I want to confirm this.

We have three methods to get users at the moment in zitadel-rust-client:

• get_user_by_id
• get_user_by_login_name
• get_user_by_nick_name

I assume this might change quite soon, when the zitadel-rust-client gets updated to zitadel's V2.

But the question remains for this PR: is supplying an email to get_user_by_login_name find the users we want to delete as per UKT's request?

Is there a way to test this in a real environment to be sure?

[sirewix]

Maybe abstracting over identifiers can be an option?

enum Id {
  Login(String),
  Nick(String),
  Id(String),
}

or

enum IdType { Login, Nick, Id }
type Id = (String, IdType)

But it has to be tested whether this possible to implement

[jannden]

@sirewix I don't want to discuss the implementation in zitadel_rust_client. My question simply is (if you are familiar with the matter), whether this will work:

let user = self.zitadel_client.get_user_by_login_name(email).await?;

It most probably will work, because this is a code that has been in the zitadel.rs from before, so I base my logic on that:

    async fn get_user_id(&self, user: &User) -> Result<Option<String>> {
        let status =
            self.zitadel_client.get_user_by_login_name(&user.email.clone().to_string()).await;

        if let Err(ZitadelError::TonicResponseError(ref error)) = status {
            if error.code() == TonicErrorCode::NotFound {
                return Ok(None);
            }
        }

        Ok(status.map(|user| user.map(|u| u.id))?)
    }

[jannden]

So I pushed a new version.

This is a solution that makes more sense to me. That is:

• without using an unnecessary trait that tries to unify sources that are too diverse,
• without trying to make the endpoint generic in case this is a small and custom made implementation for UKT.

Please have a look and hopefully this makes sense to you as well.

[jannden]

Another question - is it necessary to update .env or config.yaml in Github, so that the tests start passing? They do pass when I run them locally.

[jannden]

I added CSV Source support as well as per famedly/ldap-sync/issues/52

[jannden]

Pending: e2e tests for new sources.

[nikzen]

There is a question we need to address.

So, UKT provides us with a list of email we should delete from Zitadel. In zitadel-rust-client, I see we use email interchangeably with login_name (at least in the e2e tests written for ldap-sync. So I want to confirm this.

We have three methods to get users at the moment in zitadel-rust-client:

• get_user_by_id
• get_user_by_login_name
• get_user_by_nick_name

I assume this might change quite soon, when the zitadel-rust-client gets updated to zitadel's V2.

But the question remains for this PR: is supplying an email to get_user_by_login_name find the users we want to delete as per UKT's request?

Is there a way to test this in a real environment to be sure?

I have setup the connection and the mail is used as loginname

[jannden]

Thanks for the confirmation, Niklas 👍

[tlater-famedly]

There is a question we need to address.

So, UKT provides us with a list of email we should delete from Zitadel. In zitadel-rust-client, I see we use email interchangeably with login_name (at least in the e2e tests written for ldap-sync. So I want to confirm this.

We have three methods to get users at the moment in zitadel-rust-client:

* get_user_by_id

* get_user_by_login_name

* get_user_by_nick_name

I assume this might change quite soon, when the zitadel-rust-client gets updated to zitadel's V2.

But the question remains for this PR: is supplying an email to get_user_by_login_name find the users we want to delete as per UKT's request?

Is there a way to test this in a real environment to be sure?

To be clear, the identifier we want to use is the email address, because that is the unique ID on the Zitadel end. The LDAP ID is only used because LDAP doesn't give us email addresses of users it considers deleted. We store it in the nickname for the time being because Zitadel cannot search metadata, at least with the v1 API.

The Zitadel ID (get_user_by_id) should only really be used in tests, unless I forgot about an edge case where it's required.

But ultimately, the point is we should use the email, and fall back to other IDs where there is no other option.

Another question - is it necessary to update .env or config.yaml in Github, so that the tests start passing? They do pass when I run them locally.

Did you create a local http server to test against and forget to add it to the test environment? Or maybe hand-edit a local config.yaml instead of the tests/environment/config.template.yaml?

[jannden]

But ultimately, the point is we should use the email

Thanks for the confirmation.

Did you create a local http server to test against and forget to add it to the test environment? Or maybe hand-edit a local config.yaml instead of the tests/environment/config.template.yaml?

Nope, I use only tests/environment/config.template.yaml which is updated here in my branch as well so it should work on Github if there are no other specific settings here. When I run the e2e tests locally with cargo nextest run, all of them pass except for the two with the certificate problem. So I am really not sure why they don't pass here on Github. Will re-examine the logs again, it's curious.

[github-actions[bot]]

LCOV of commit 62fe051 during Rust workflow #329

Summary coverage rate:
  lines......: 85.7% (956 of 1116 lines)
  functions..: 49.3% (230 of 467 functions)
  branches...: no data found

Files changed coverage rate:
                     |Lines       |Functions  |Branches    
  Filename           |Rate     Num|Rate    Num|Rate     Num
  =========================================================
  src/config.rs      |96.2%    209|54.1%    98|    -      0
  src/main.rs        | 0.0%     31| 0.0%    10|    -      0
  src/sources/ldap.rs|95.9%    319|59.3%   135|    -      0
  src/sources/ukt.rs |86.7%    211|59.5%    84|    -      0
  src/user.rs        |85.3%     68|45.5%    22|    -      0
  src/zitadel.rs     |74.8%    278|31.4%   118|    -      0

[sirewix]

LGTM except few nits :+1: