Major changes, please review code by fl0eb · Pull Request #1 · gillg/OctoPrint-LDAP

fl0eb · 2018-01-06T22:43:58Z

No description provided.

gillg · 2018-01-07T14:29:41Z

Thanks for your improvments ! This module is in testing phase, and I had some issues with configuration settings.
Your PR contains some good things but also other wired. Why do you add user and password in settings ? This is not the goal of an LDAP auth ?
Another thing, you add a syscall "kill" with undefined var !?? Why ?

gillg · 2018-01-07T14:40:49Z

See my comments in your changes, I think we could have a good LDAP module after that ! :) 👍

fl0eb · 2018-01-07T15:56:09Z

Sorry, I was a little lazy while committing... Unfortunately i cannot see any comments.
Anyway I will write some more information why I changed specific parts in the code.
For me the plugin is right now working as it is (I'm not using TLS).
Yes, it is far from perfect, but it is working (at least for me).
I just wanted to share it, so you can take the good parts and leave the rest :-)
I wouldn't mind if you just reject it.

Regarding the LDAP Credentials:
It is necessary to have a binding user, which is used to fetch data from LDAP.
You can not just query if a user exists without any sort oft authentication.
This is only a service account and is not used for logging into OctoPrint.
And yes, the password should be stored somehow differently.

Yes the kill() in init is only a workaround and surely should be improved.
Problem here is, that if the settings of the LDAP are wrong and the initial configuration test fails, the only two options are to

just proceed and have a broken login mechanism which locks out users
raise an exception, so Octoprint will fall back to default authentication, but then the Plugins settings are not loaded and the plugin can only be fixed by manually editing the conf.yaml

With the settings().remove() and kill, you just can just restart with an disabled plugin

fl0eb · 2018-01-07T15:57:52Z


 # The plugin's identifier, has to be unique
-plugin_identifier = "auth_ldap"
+plugin_identifier = "authldap"


Had to rename the plugin, as the settings could not be stored otherwise.

fl0eb · 2018-01-07T15:59:35Z

+
+        <label class="control-label">{{ _('Search base pattern') }}</label>
+        <div class="controls">
+            <input type="text" class="input-block-level" data-bind="value: settings.plugins.authldap.ldap_search_base"


This setting controls where to look for Users

fl0eb · 2018-01-07T16:02:50Z

+
+        <label class="control-label">{{ _('LDAP Filter') }}</label>
+        <div class="controls">
+            <input type="text" class="input-block-level" data-bind="value: settings.plugins.authldap.ldap_query"


As you maybe want to restrict which users can access OctoPrint, you should be able to set a filter.
If this filter returns a result, the user can log in.
{uid} will be replaced with the user trying to login.
I saw this kind of configuration also on other systems.

fl0eb · 2018-01-07T16:04:21Z

+        <label class="control-label">{{ _('Default Roles:') }}</label>
+        <div class="controls">
+            <input type="text" class="input-block-level" data-bind="value: settings.plugins.authldap.roles"
+                   placeholder="Comma separated list of roles: user,admin">


Self explanatory.
This will give the newly created users specific roles. Currently Octoprint only differs between user and administrator rights

fl0eb · 2018-01-07T16:06:18Z

+        </div>
+        <div class="controls">
+            <label class="checkbox">
+                <input type="checkbox" data-bind="checked: settings.plugins.authldap.auto_activate">Automatically activate users?


In case you want new users to be created, but not able to login.
I'm not sure if it makes sense to disable this, maybe this functionality could be removed

fl0eb · 2018-01-07T16:08:41Z

+        <h4>Authentication</h4>
+        <label class="control-label">{{ _('Binding User DN') }}</label>
+        <div class="controls">
+            <input type="text" class="input-block-level" data-bind="value: settings.plugins.authldap.ldap_bind_user"


This is the service user, which is used for querying the LDAP. Do not confuse it with the user trying to login later.
Once OctoPrint has a more granular user access control, you could also query for groups, emails, etc.

fl0eb · 2018-01-07T16:09:15Z

+        </div>
+        <label class="control-label">{{ _('Password') }}</label>
+        <div class="controls">
+            <input type="password" class="input-block-level" data-bind="value: settings.plugins.authldap.ldap_bind_password">


Password of service user. Should be stored differently...

fl0eb · 2018-01-07T16:10:40Z

+        <label class="control-label">{{ _('Method:') }}</label>
+        <div class="controls">
+            <select data-bind="value: settings.plugins.authldap.ldap_method">
+                <option value="BASIC">Basic</option>


Only used to differ between LDAPs and LDAP, this helped a little bit debugging, but could be replaced with a regex on the uri

fl0eb · 2018-01-07T16:10:53Z

+        </div>
+        <label class="control-label">{{ _('TLS certification check') }}</label>
+        <div class="controls">
+            <select data-bind="value: settings.plugins.authldap.ldap_tls_reqcert">


Never tested this...

fl0eb · 2018-01-07T16:13:27Z

+    </div>
+    <div class="control-group">
+        <h4>Activation</h4>
+            <p>{% trans %}


It makes sense to have a way to activate/deactivate the plugin in the settings.
As I'm not aware of any easy way to directly set the userManager setting from here, I implemented it with a local property and on_settings_save.

fl0eb · 2018-01-07T16:14:53Z

+
+	global __plugin_hooks__
+	__plugin_hooks__ = {
+		"octoprint.plugin.softwareupdate.check_config":


Removed the octoprint.users.factory hook and switched to the userManager setting.

fl0eb · 2018-01-07T16:17:26Z

+
+	_localUserManager = FilebasedUserManager()
+
+	def __init__(self):


This function should check if the plugin is working as expected while loading.
If not it should fall back to the FilebasedUserManager.
This could be achieved by raising an exception, which however disables also the settings page, so fixing the configuration from the UI would not be possible.

fl0eb · 2018-01-07T16:17:59Z

+					return
+				except:
+					pass
+			settings().remove(["accessControl", "userManager"])


This is the fallback!

fl0eb · 2018-01-07T16:18:27Z

+			settings().remove(["accessControl", "userManager"])
+			settings().remove(["plugins", "authldap", "active"])
+			settings().save()
+			os.system('kill $PPID')


This is only to not start OctoPrint with a broken configuration.

fl0eb

Added comments to the code

fl0eb · 2018-01-07T16:22:43Z

@@ -0,0 +1,271 @@
+# coding=utf-8


Had to change folder name due to renaming the plugin...

fl0eb · 2018-01-07T16:23:42Z

@@ -0,0 +1,79 @@
+<form class="form-horizontal">


Changed filename to to renaming of the plugin, also implemented settings as proposed in the best practice

fl0eb · 2018-01-07T16:24:15Z

+
+	def get_update_information(self):
+		return dict(
+			authldap=dict(


I guess this part was copied over. Corrected it.

fl0eb · 2018-01-07T16:25:58Z

+			self._logger.debug("LDAP is using TLS, setting ldap options...")
+			ldap_verifypeer = settings().get(
+				["plugins", "authldap", "ldap_tls_reqcert"])
+			verifypeer = ldap.OPT_X_TLS_HARD


Added all other kinds of TLS Verification.
If OPT_X_TLS_* will be directly written in the settings, the if/elif could be removed

fl0eb · 2018-01-07T16:29:17Z

+			ldap_bind_password = None
+		)
+
+	def on_settings_save(self, data):


This part only registers/unregisters the UserManager in OctoPrint. A better solution would be to directly write into the accessControl.userManager setting, this is only quick and dirty, but redundant...

fl0eb · 2018-01-07T16:33:22Z

+				password = ''.join([random.choice(string.lowercase) for i in range(10)])
+				if settings().get(["plugins", "authldap", "roles"]) is not None:
+					roles = [x.strip() for x in settings().get(["plugins", "authldap", "roles"]).split(',')]
+				self._localUserManager.addUser(userid, password, active=settings().getBoolean(["plugins", "authldap", "auto_activate"]), roles=roles)


findUser is creating LDAP users once searched.
If (for some strange reasons) Octoprint is searching for random people, they all would be added to the Users list with default permissions.
I'm not sure if this could lead into a problem...
However once OctoPrint as a more sophisticated user account control, you could control LDAP users without them logged in once or manually created.

fl0eb · 2018-01-07T16:34:45Z

+	def findUser(self, userid=None, apikey=None, session=None):
+		user = UserManager.findUser(self, userid=userid, session=session)
+
+		if user is not None:


Just some checks for already logged in users or api access
"Copied" from FilebasedUserManager

fl0eb

Added Comments

fl0eb · 2018-01-07T16:39:11Z

I hope this helps to understand my code.
Code without proper documentation isn't worth anything... 😪

gillg · 2018-01-08T12:34:54Z

Many thanks for all your comments !!! I have no time to read all immediately but I will check this soon !

gillg · 2018-01-28T11:18:50Z

@Flow555 I add you to main project collabs, so you can participate to the new PR #3 and push directly on branch https://github.com/gillg/OctoPrint-LDAP/tree/initial-refactoring

Major changes, please review code

97a9027

fl0eb commented Jan 7, 2018

View reviewed changes

gillg closed this Jan 28, 2018

gillg mentioned this pull request Jan 28, 2018

Major changes #3

Open


		_localUserManager = FilebasedUserManager()

		def __init__(self):

Conversation

fl0eb commented Jan 6, 2018

Uh oh!

gillg commented Jan 7, 2018

Uh oh!

gillg commented Jan 7, 2018

Uh oh!

fl0eb commented Jan 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fl0eb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fl0eb left a comment

Choose a reason for hiding this comment

Uh oh!

fl0eb commented Jan 7, 2018

Uh oh!

gillg commented Jan 8, 2018

Uh oh!

gillg commented Jan 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gillg commented Jan 28, 2018 •

edited

Loading