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

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

Merged
merged 7 commits into from
Jun 2, 2019

Conversation

lavdnone
Copy link
Contributor

@lavdnone lavdnone commented Mar 31, 2019

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

  • 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
Copy link
Member

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...

@violoncelloCH violoncelloCH added enhancement New feature or request help wanted Extra attention is needed labels Apr 14, 2019
@lavdnone
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

violoncelloCH commented May 22, 2019

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...

lib/imap.php Outdated Show resolved Hide resolved
@violoncelloCH violoncelloCH changed the title imap groups via domain optional imap groups via domain & make domain striping optional May 22, 2019
@violoncelloCH violoncelloCH added this to the 0.6.2 milestone May 22, 2019
@violoncelloCH
Copy link
Member

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

found this... 🙈
should be working now

@violoncelloCH
Copy link
Member

please review @nextcloud/user_external

@lavdnone
Copy link
Contributor Author

lavdnone commented Jun 1, 2019

@violoncelloCH thanks, looks great

@violoncelloCH
Copy link
Member

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)

@violoncelloCH violoncelloCH modified the milestones: 0.6.2, 0.6.3 Jun 1, 2019
@lavdnone
Copy link
Contributor Author

lavdnone commented Jun 2, 2019

@violoncelloCH tested, put in prod, safe to merge

lavdnone and others added 7 commits June 2, 2019 13:49
imap groups via domain-part

Signed-off-by: none <vlad@teksperts.nyc>
Signed-off-by: none <vlad@teksperts.nyc>
Signed-off-by: Jonas Sulzer <jonas@violoncello.ch>
Signed-off-by: none <vlad@teksperts.nyc>
…nal parameters wheter to stripe domain and create group based on domain or not

Signed-off-by: Jonas Sulzer <jonas@violoncello.ch>
Signed-off-by: none <vlad@teksperts.nyc>
Signed-off-by: Jonas Sulzer <jonas@violoncello.ch>
Signed-off-by: none <vlad@teksperts.nyc>
…and domain based group creation

Signed-off-by: Jonas Sulzer <jonas@violoncello.ch>
Signed-off-by: none <vlad@teksperts.nyc>
…r later extendability e.g. nextcloud#69)

Signed-off-by: Jonas Sulzer <jonas@violoncello.ch>
Signed-off-by: none <vlad@teksperts.nyc>
lib/imap.php Outdated
@@ -37,7 +37,7 @@ class OC_User_IMAP extends \OCA\user_external\Base {
* @param boolean $stripeDomain (whether to stripe the domain part from the username or not)
* @param boolean $groupDomain (whether to add the usere to a group corresponding to the domain of the address)
*/
public function __construct($mailbox, $port = null, $sslmode = null, $domain = null, $stripeDomain = true, $groupDomain = false) {
public function __construct($mailbox, $port = null, $sslmode = null, $domain = null, $stripeDomain = false, $groupDomain = false) {
Copy link
Member

Choose a reason for hiding this comment

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

the thing is the current behaviour is to stripe the domain...
that's why I've put this to true by default, so we don't break anything for current setups...

Copy link
Member

Choose a reason for hiding this comment

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

also we only apply it if a restrictig domain is specified...

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't set it as default either, but as we do it this way in the existing versions, we shouldn't change it now, because it would break existing setups... so I've reverted your last commit, hope it's clear now why :)

@violoncelloCH
Copy link
Member

thanks for testing this @lavdnone !

@violoncelloCH violoncelloCH modified the milestones: 0.6.3, 0.6.2 Jun 2, 2019
@violoncelloCH violoncelloCH merged commit 406e41c into nextcloud:master Jun 2, 2019
@kesselb
Copy link

kesselb commented Jun 4, 2019

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

@violoncelloCH
Copy link
Member

@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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement New feature or request
Projects
None yet
4 participants