optional imap groups via domain & make domain striping optional - Githubissues

nextcloud / user_external

👥 External user authentication methods like IMAP, SMB and FTP

https://apps.nextcloud.com/apps/user_external

108 stars 64 forks source link

optional imap groups via domain & make domain striping optional #65

Closed lavdnone closed 5 years ago

lavdnone commented 5 years ago

imap groups via domain-part for #10 working with v15 from https://github.com/nextcloud/apps/compare/master...bjoern86:patch-1

[X] if someone can convert it to new way "$query->insert..."

Changes proposed in this pull request:

if there is no group equal to domain part of the new user create group
add new user to the group
make group and domain striping configurable via parameters

fixes #10 fixes #76 fixes #68

violoncelloCH commented 5 years ago

thank you for the PR! yes this would need some adaptions for the DB part (sorry I currently don't the time for it)... also simply adding this to the base.php could cause other problems if someone uses email addresses as usernames or simply usernames with @ s in an other backend... it would at least be needed to check if the backend is IMAP before applying this...

lavdnone commented 5 years ago

Updated to getGroupManager()->createGroup from OC_DB

Maybe you can add if backend is IMAP? something like:

if($createduser->getBackendClassName() === 'OC_User_IMAP')
if(get_class($backend) === 'OC_User_IMAP')

violoncelloCH commented 5 years ago

cool, thanks! I'll look into this as soon as I'll find some time...

hmm, we should probably also make this optional, because probably not everyone wants this behaviour and it would be an unexpected change... so having an optional parameter in the configuration that has to be set to true...

violoncelloCH commented 5 years ago

looking a little closer at this I noticed that this won't work if if the domain is striped away from the username :/ currently this is only the case if the domain is restricted, however I would like to make it possible to configure all this independently... so in the end it should also be possible to restrict to multiple domains, stripe the domain (admin would need to make sure there are no username conflicts) but still put users into a group according to the domain...

violoncelloCH commented 5 years ago

okay so I've moved this so it's only executed for IMAP... not sure though if it's the best way to do this... (feedback welcome) I've also added the possibility for two additional parameters whether to stripe the domain part and whether to create a group from the domain or not... also the generation of the group is not yet working correctly... need to investigate in how this works...

violoncelloCH commented 5 years ago

also the generation of the group is not yet working correctly... need to investigate in how this works...

found this... :see_no_evil: should be working now

violoncelloCH commented 5 years ago

please review @nextcloud/user_external

lavdnone commented 5 years ago

@violoncelloCH thanks, looks great

violoncelloCH commented 5 years ago

thanks for the feedback @lavdnone! could you test this? I know I could approve this PR myself because I didn't open it, but because I've changed quite a lot here, I would like to have a proper review (and test) from someone else :) (cc @nextcloud/user_external)

lavdnone commented 5 years ago

@violoncelloCH tested, put in prod, safe to merge

violoncelloCH commented 5 years ago

thanks for testing this @lavdnone !

kesselb commented 5 years ago

I would prefer "stripDomain" over "stripeDomain" ;)

violoncelloCH commented 5 years ago

@kesselb you're right... I'm not a native speaker and didn't check that, just took over how others called it... will probably change this naming with a future PR...