Skip to content

Add configurable LDAP members and single-run mode - #33

Open
sylvesterkaczmarek wants to merge 1 commit into
alan-turing-institute:mainfrom
sylvesterkaczmarek:feature/29-ldap-member-single-run
Open

Add configurable LDAP members and single-run mode#33
sylvesterkaczmarek wants to merge 1 commit into
alan-turing-institute:mainfrom
sylvesterkaczmarek:feature/29-ldap-member-single-run

Conversation

@sylvesterkaczmarek

@sylvesterkaczmarek sylvesterkaczmarek commented Aug 21, 2026

Copy link
Copy Markdown

Summary

Implement the two configuration options requested in #29 while keeping the existing synchronization and PostgreSQL interfaces intact.

Changes

  • add LDAP_GROUP_MEMBER_ATTR, defaulting to memberUid for backward compatibility;
  • read group membership through the configured LDAP attribute;
  • normalize UID-based distinguished names such as FreeIPA member values before matching users;
  • add SINGLE_RUN_MODE, which performs one synchronization cycle and exits before sleeping;
  • document both settings;
  • add regression tests for custom group membership and single-run behavior.

Scope

Normal repeating mode remains unchanged; synchronise() now reports success/failure internally so single-run mode can return the appropriate process exit status.

There is an existing implementation in #30. This is a smaller, tested alternative focused on the requested behavior and backward compatibility.

Closes #29

@llewelld llewelld left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution which is great to have received.

I've made a few comments, most of which are about aligning your implementation with the behaviour of #30. I'm taking #30 to be a good match for the behaviour intended from the description in #29, so it would make sense to try to match it unless there's a case not to.

Broadly the implementation here looks sensible to me.

It's likely that #32 will be merged in prior to your changes here, so you may prefer to wait until that's been done in case it results in conflicts. and to avoid having to rebase again

Comment thread guacamole_user_sync/ldap/ldap_client.py Outdated
Comment thread guacamole_user_sync/ldap/ldap_client.py Outdated
Comment thread tests/test_ldap.py
Comment thread synchronise.py Outdated
Comment thread README.md Outdated
@llewelld

Copy link
Copy Markdown
Contributor

Ah, I forgot to mention in my review, but would it also be possible to add a little more detail to your commit message please? The changes are non-trivial in terms of functionality and it would help to clarify that this incorporates essentially two separate features (configurable LDAP members and single-run mode).

Assuming we end up going with this implementation rather than #30, it might also be appropriate to give partial credit to @LorenzoGParisi, especially if you're incorporating some of the changes I suggested in my review.

@sylvesterkaczmarek

Copy link
Copy Markdown
Author

Thanks for your contribution which is great to have received.

I've made a few comments, most of which are about aligning your implementation with the behaviour of #30. I'm taking #30 to be a good match for the behaviour intended from the description in #29, so it would make sense to try to match it unless there's a case not to.

Broadly the implementation here looks sensible to me.

It's likely that #32 will be merged in prior to your changes here, so you may prefer to wait until that's been done in case it results in conflicts. and to avoid having to rebase again

Thanks. I've aligned #33 with #30 where requested: missing optional LDAP attributes now fall back safely, single-run mode returns a non-zero exit status on synchronisation failure, and the README notes that REPEAT_INTERVAL is ignored in single-run mode.

I've also squashed the branch to one descriptive commit and credited @LorenzoGParisi. The updated lint/test workflows are currently awaiting maintainer approval to run.

@llewelld

Copy link
Copy Markdown
Contributor

Thanks for making the changes @sylvesterkaczmarek; I appreciate you being so receptive. As feared though the merging of #32 has resulted in conflicts. Would you mind please taking a look to see if they can be resolved?

@sylvesterkaczmarek

Copy link
Copy Markdown
Author

Thanks for making the changes @sylvesterkaczmarek; I appreciate you being so receptive. As feared though the merging of #32 has resulted in conflicts. Would you mind please taking a look to see if they can be resolved?

On it.

- add LDAP_GROUP_MEMBER_ATTR with a backward-compatible memberUid default
- normalise UID-based LDAP distinguished-name members before user matching
- tolerate missing optional LDAP membership/display attributes
- add SINGLE_RUN_MODE with non-zero exit status on failed synchronisation
- preserve GUACAMOLE_GROUP_PERMISSIONS behaviour introduced by alan-turing-institute#32
- document single-run interaction with REPEAT_INTERVAL
- add regression coverage for LDAP attribute variants and single-run success/failure

Incorporates compatible behaviour from alan-turing-institute#30 following maintainer review and is rebased onto alan-turing-institute#32.

Co-authored-by: LorenzoGParisi <117413030+LorenzoGParisi@users.noreply.github.com>
@sylvesterkaczmarek
sylvesterkaczmarek force-pushed the feature/29-ldap-member-single-run branch from 1b660ea to 6311173 Compare August 25, 2026 16:06
@sylvesterkaczmarek

Copy link
Copy Markdown
Author

@llewelld thanks. I've rebased #33 onto current main after #32 and resolved the conflicts, preserving the new GUACAMOLE_GROUP_PERMISSIONS flow alongside LDAP_GROUP_MEMBER_ATTR and SINGLE_RUN_MODE.

The branch is back to a single descriptive commit and GitHub now reports the PR as mergeable. The lint/test workflows are awaiting maintainer approval to run.

@LorenzoGParisi

Copy link
Copy Markdown

@sylvesterkaczmarek Thanks for incorporating those changes, all good on my side @llewelld

@sylvesterkaczmarek

Copy link
Copy Markdown
Author

Thanks, Lorenzo — appreciate the confirmation. @llewelld, from the contributor side #33 now includes the #30 functionality, Lorenzo is happy with the combined implementation, and the branch is mergeable; the remaining workflows are still awaiting maintainer approval to run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enhancement: Add configurable LDAP group member attribute and single-run mode

3 participants