KITPraktomatTeam / Praktomat

quality control for programming assignments
http://pp.ipd.kit.edu/projects/praktomat/praktomat.php
GNU General Public License v2.0
46 stars 22 forks source link

Fix LDAP authentication #322

Closed hannesbraun closed 2 years ago

hannesbraun commented 2 years ago
ifrh commented 2 years ago

"Auto create local user on first login" is a nice thing, but this can result in Praktomat-Course-Members which do not take part in current lecture. Perhaps it should be configurable i.e. in settings/local.py if Praktomat should create a local account while first login. :-)

hannesbraun commented 2 years ago

That's a good point. I left the default setting to True but I don't really care what's the default setting. I'm also totally fine with changing it to False :)

ifrh commented 2 years ago

@hannesbraun 👍

ratefuchs commented 2 years ago

There is a setting called new_users_via_sso from the configuration module that currently decides if new accounts are created while logging in with shibboleth (see accounts/shib_views.py line 85 as an example).

I think re-using this setting for LDAP would make sense, since I don't think there will be much of a use case of having both Shibboleth and LDAP while only auto-creating accounts for just one of these methods. I also think that it makes more sense to have it in the configuration module as opposed to the settings module: This way, it can be changed by a superuser on the admin interface of Praktomat, no need to change files on the server. And it makes it easier to allow auto-creation only for a certain time frame during courses (e.g. at the beginning).

ifrh commented 2 years ago

Well, in my option it seems to be a good feature, if Praktomat Admins could choose from Praktomat-Adminsites to query LDAP or shibboleth.

But in both cases it should configurable

hannesbraun commented 2 years ago

Using new_users_via_sso instead of intruducing a new setting makes sense to me.

  • if students accounts get automaticaly created in Praktomat

If I understand you correctly, this can be achieved by moving the setting in the configuration module. So I'll fix that in a minute.

if students can change their passworts (at our site no student should be able to change their LDAP password via Praktomat. For password changing we have an other way.)

@ifrh I'm not entirely sure about the current state, but at least from what I've seen, it doesn't look like it's possible to change an external password from within Praktomat. I think you're only able to change the password of the local user, right? At least, that should apply for authentication via LDAP. (I haven't used Shibboleth so far.) I'd personally leave it as it is right now and this can be fixed separately later. Or is this an important feature that is directly affected by this Pull Request? Or did I miss something else?😅

ifrh commented 2 years ago

@hannesbraun You asked:

I think you're only able to change the password of the local user, right?

Because we at H-BRS use LDAP and we allways import users "on block" via
https://github.com/KITPraktomatTeam/Praktomat/blob/master/src/accounts/admin.py#L110 , we allways deactivate register and password change urls: https://github.com/KITPraktomatTeam/Praktomat/blob/master/src/accounts/urls.py#L28-L44

So I haven't look very deep into code related to shibboleth or individual user registration.

Our LDAP login feature require that the Praktomat internal password has an unchanged value: