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

Remove the LDAP on/off switch #4093

Merged
merged 5 commits into from
Nov 23, 2020

Conversation

matthias-ronge
Copy link
Collaborator

@matthias-ronge matthias-ronge commented Nov 11, 2020

Fixes #3336, part of #456

Note: The ldap_use parameter in the kitodo_config.properties file should initially be retained so that users can initially remove their LDAP group after a migration. However, the default for this attribute can be set to true for new installations.

Follow-up pull request to #4092 (immediate diff)

# Logins ueber LDAP verwenden
ldap_use=false
# Use logins via LDAP
ldap_use=true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you enabling LDAP authentication by default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See here:

Procedure proposal:

1. Enable individual users are authenticated against database even when LDAP is on. (#4014)

2. Deactivate the LDAP group for the users supplied with the basic database. (Cf. also #3304)

3. Remove the LDAP on/off switch. (#3336)
   (The ldap_use parameter in the kitodo_config.properties file should initially be retained
   so that users can initially remove their LDAP group after a migration. However, the
   default for this attribute can be set to true for new installations.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Than add this as explanation. Without this I would deactivate this again after this pull request is merged!

I hope that your migration way of LDAP usage will be documented somewhere in an official way as issues and pull requests are not a long term documentation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I wrote: You would want to leave the setting unchanged after a migration. It is only about empty new installations, where this can be true in the future (does not have to be. Anyone can set it to false by hand. If someone did that, they should be aware that LDAP does not work.)

Copy link
Collaborator

@henning-gerhardt henning-gerhardt left a comment

Choose a reason for hiding this comment

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

I can not test your changes and your changes are not using any kind of test, so I can not judge if your changes are correct or not.

@Kathrin-Huber Kathrin-Huber merged commit 7606f93 into kitodo:master Nov 23, 2020
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.

What about the LDAP on/off switch?
3 participants