Skip to content

Add LDAP test settings command #50567

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.

Sign up for GitHub

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

Jump to bottom
Merged
come-nc merged 4 commits into from
Feb 6, 2025
Merged

Add LDAP test settings command #50567

come-nc merged 4 commits into from
Feb 6, 2025
+299 −50

Conversation

@come-nc
Copy link
Contributor

@come-nc come-nc commented Jan 30, 2025 •
edited
Loading

Summary

To help debugging issues related to LDAP configuration, this command shows how Nextcloud sees a specific LDAP user, and if provided an associated group.
This should help debugging situations where group<>member association is not working as expected, and may also help in any situation where LDAP configuration is involved.

Copie d'écran_20250130_120330

Checklist

@come-nc come-nc added the 2. developing Work in progress label Jan 30, 2025
@come-nc come-nc added this to the Nextcloud 32 milestone Jan 30, 2025
@come-nc come-nc self-assigned this Jan 30, 2025
@come-nc come-nc requested a review from blizzz as a code owner January 30, 2025 11:03
@come-nc
Copy link
Contributor Author

come-nc commented Jan 30, 2025

I’m quite happy with current state, only thing I would like to add is clearing the cache maybe?

@blizzz
Copy link
Member

blizzz commented Jan 30, 2025

Nifty stuff 🤩 The execute() method is a bit long maybe.

Clearing the cache is useful, but would give it a new command.

@sorbaugh sorbaugh requested a review from Altahrim January 30, 2025 13:03
@come-nc
Copy link
Contributor Author

come-nc commented Jan 30, 2025

Nifty stuff 🤩 The execute() method is a bit long maybe.

Well yeah but it’s not that easy to break into submethods because there are some early return in there to stop earlier.
The failures could be replaced by Exception throwing but the success are harder to do in a clean way. I will keep it like that to avoid spending too much time on this.

Clearing the cache is useful, but would give it a new command.

I now added a --clearcache option because, especially when used together with --debug-log, it makes a lot of sense to do these tests with a clean cache.

@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 30, 2025
@come-nc come-nc requested review from a team, provokateurin and skjnldsv and removed request for a team January 30, 2025 15:00
kesselb
kesselb reviewed Feb 6, 2025
View reviewed changes
apps/user_ldap/lib/Command/TestUserSettings.php
protected function configure(): void {
$this
->setName('ldap:test-user-settings')
->setDescription('Runs tests and show information about user related LDAP settings')
Copy link
Contributor

@kesselb kesselb Feb 6, 2025

Choose a reason for hiding this comment

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

Could we add a better description? I'm unsure what you mean with "Runs tests" 🤔

kesselb
kesselb approved these changes Feb 6, 2025
View reviewed changes
Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

LGTM

AndyScherzinger
AndyScherzinger approved these changes Feb 6, 2025
View reviewed changes
Copy link
Member

@AndyScherzinger AndyScherzinger left a comment

Choose a reason for hiding this comment

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

🐘

@come-nc come-nc merged commit 9c3ef8e into master Feb 6, 2025
189 checks passed
@come-nc come-nc deleted the enh/ldap-add-test-settings-command branch February 6, 2025 11:23
@come-nc
Copy link
Contributor Author

come-nc commented Feb 6, 2025

/backport to stable31

@backportbot backportbot bot mentioned this pull request Feb 6, 2025
Merged
@come-nc
Copy link
Contributor Author

come-nc commented Feb 6, 2025

/backport to stable30

@backportbot backportbot bot mentioned this pull request Feb 6, 2025
Merged
@come-nc come-nc mentioned this pull request Mar 3, 2025
Closed
8 tasks
@joshtrichards joshtrichards added the pending documentation This pull request needs an associated documentation update label Mar 19, 2025
@joshtrichards joshtrichards mentioned this pull request Mar 25, 2025
Open
8 tasks
@skjnldsv skjnldsv mentioned this pull request Mar 27, 2025
Merged
2 tasks
@skjnldsv skjnldsv removed the pending documentation This pull request needs an associated documentation update label Mar 27, 2025
@skjnldsv
Copy link
Member

skjnldsv commented Mar 27, 2025

Docs in nextcloud/documentation#12940

@nextcloud-bot nextcloud-bot mentioned this pull request Aug 19, 2025
Merged
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AndyScherzinger AndyScherzinger AndyScherzinger approved these changes

@kesselb kesselb kesselb approved these changes

@blizzz blizzz Awaiting requested review from blizzz blizzz is a code owner

@Altahrim Altahrim Awaiting requested review from Altahrim

@skjnldsv skjnldsv Awaiting requested review from skjnldsv skjnldsv is a code owner automatically assigned from nextcloud/server-backend

@provokateurin provokateurin Awaiting requested review from provokateurin provokateurin is a code owner automatically assigned from nextcloud/server-backend

Assignees

@come-nc come-nc

Labels

3. to review Waiting for reviews enhancement feature: ldap

Projects

None yet

Milestone

Nextcloud 32

Development

Successfully merging this pull request may close these issues.

Add debug occ command to check that the LDAP group membership is correctly configured

7 participants

@come-nc @blizzz @skjnldsv @AndyScherzinger @kesselb @joshtrichards