[IMAP] login without domain is alowed and creates new users

DJCrashdummy commented 4 years ago

Steps to reproduce

setup nextcloud and configure user_external as further below (especially with false for domain stripping).
login as user@domain.tld and everything seems to work as it should: new users get automatically created with uid user@domain.tld. also the tables users_external, accounts and credentials seem fine...
now try to login as user with the same (correct) password: then a new user with uid user gets created and seems to work like a normal nextcloud-user, although a login without the domain does not work at the plain IMAP-server! :astonished:
_{first i thought this issue occurres because this instance has accidentally created new users without domain while migrating from user_external <0.6.0.
but i've deleted them all and this bug seems to "work" also for completely new users.}

Expected behaviour

user_external should IMHO not add the domain for authentication and even more important not strip it for the nextcloud uid especially if it is configured to have uids with domain! in best case this creates confusion or panic at the users and is just annoying for admins... but in the worst case with more than one user_backends resp. domain, this creates a hell of a mess and serious trouble!

Actual behaviour

see Steps to reproduce

Affected Authentication backend

at least IMAP (i don't use other)

Server configuration

User External App version: (see Nextcloud apps page) 0.8.0

Operating system: ubuntu

Web server: apache

Database: mysql 5.7.28

PHP version: 7.2.24

Nextcloud version: (see Nextcloud admin page) 16.0.8.1

Updated from an older Nextcloud/ownCloud or fresh install: updated from at least NC13

Where did you install Nextcloud from: hoster script

Signing status:

Signing status

``` No errors have been found. ```

List of activated apps:

App list

``` quite a bunch, but IMHO in this case irrelevant. ```

Nextcloud configuration:

Config report

``` ... 'user_backends' => array ( 0 => array ( 'class' => 'OC_User_IMAP', 'arguments' => array ( 0 => 'mail.server.tld', 1 => 993, 2 => 'ssl', 3 => 'domain.tld', 4 => false, 5 => false, ), ), ), ... ```

Logs

Web server error log

``` nothing regarding this issue ```

Nextcloud log (data/nextcloud.log)

Nextcloud log

``` nothing regarding this issue ```

Browser log

``` nothing regarding this issue ```

DJCrashdummy commented 4 years ago

don't get me wrong, it may be ok adding the domain to be more relaxed for login, but then it must also add the domain for the uid when the config says to not strip the domain.

but IMHO this is not a good practice, if your IMAP-server refuses connection without a domain... because i prefer a consistent UX over one service with a more relaxed login. because why you would restrict the nextcloud-login to one domain of an IMAP-server? ...right, in case there is more than one domain allowed to login into that IMAP-server. so it is pretty safe to assume, that you can't login there without a domain. so why should it work at nextcloud?!?

yes, there may be corner cases where this behavior may be interesting resp. nice... like the same users with different domains (but then you would set domain stripping to true) or if you are doing really everything via nextcloud, but then if you start using an other IMAP-client after all, the confusion starts there.

so bottom line: i would kick the (not expected) adding of the domain at all to be on the safe side and don't further fiddle around with it.

DJCrashdummy commented 4 years ago

btw: can someone give me a hint/advise to clean up the incurred mess? after deleting the wrong user without the domain via the nextcloud-GUI it is also deleted at the table users_external and accounts, but at credentials an entry for the deleted user is still there!?! can/should i manually delete this? is this intended? or is this perhaps another bug (maybe of nextcloud core) i stumbled across? :thinking:

violoncelloCH commented 4 years ago

so bottom line: i would kick the (not expected) adding of the domain at all to be on the safe side and don't further fiddle around with it.

yes, I guess this makes sense! would you be up for a PR?

can/should i manually delete this? is this intended? or is this perhaps another bug (maybe of nextcloud core) i stumbled across?

actually this I don't know... we would need a more experienced Nextcloud core dev for this... I just know that user_external doesn't handle more than the authentication and it's database for which account comes from which backend...

violoncelloCH commented 4 years ago

well taking a closer look at the code it seems this (adding the domain part to the uid for the authentication against the imap server) was indeed implemented intentionally... I guess it was thought for the scenario where you only have one domain for your nextclouds users but on a (shared) IMAP server. In this case it's convenient and to some extend logical to have only the user part of the mail as uid in Nextcloud

violoncelloCH commented 4 years ago

so I guess the best thing to do would be to only add the domain to the user part if Domain stripping is active... right?

violoncelloCH commented 4 years ago

maybe @kosli is still around and remembers the decisions made when implementing this?

kosli commented 4 years ago

well taking a closer look at the code it seems this (adding the domain part to the uid for the authentication against the imap server) was indeed implemented intentionally... I guess it was thought for the scenario where you only have one domain for your nextclouds users but on a (shared) IMAP server. In this case it's convenient and to some extend logical to have only the user part of the mail as uid in Nextcloud

@violoncelloCH I think that is what exactly was the purpose of that functionality. e.g. I have one Nextcloud instance for a specific association, and only users from the associated mail-domain should be able to login to the nextcloud server. and I like to have the uid created without the domain.

I see that there are to options on the IMAP module: $domain If provided, loging will be restricted to this domain $stripeDomain (whether to stripe the domain part from the username or not) So I assume if none of this is configured, then the scenario of @DJCrashdummy where the user can login with and without domain and is two different users, would not work?

DJCrashdummy commented 3 years ago

first of all sorry for being silent so long, but i lost track of this issue until some of my users experienced it yet again. :frowning_face:

would you be up for a PR?

well... i would already have done it myself, if i only could! - unfortunately i'm just an admin, not a developer and everything above (excessive) shell-scripting exceeds my skills. :disappointed:

I guess it was thought for the scenario where you only have one domain for your nextclouds users but on a (shared) IMAP server. In this case it's convenient and to some extend logical to have only the user part of the mail as uid in Nextcloud

@violoncelloCH i also guessed something like that... but with more than one user with the same username (ahead of the domain), this becomes a serious security issue (beside of creating new uids when users just enter their username without the domain)! and IMHO convenience features must never automatically/unattended threaten security: so i would remove or at least disable this feature until it is sure to work in a proper way. ...especially in a 1.x release! :stuck_out_tongue_winking_eye:

I have one Nextcloud instance for a specific association, and only users from the associated mail-domain should be able to login to the nextcloud server. and I like to have the uid created without the domain.

that's not the topic... i've got at one instance indeed a similar setup. that's what domain stripping is for, but the issue is the "domain adding"!

and if @kosli meant the convenience for users to just type in their username (without the domain), i'm still not sure about this use-case... as already mentioned: i would strongly advise against creating a different UX if the IMAP server doesn't allow a login without the domain!

and even more important: if you are using more the one domain (resp. the IMAP backend more than once), which domain gets added?!? :confused: -> kind of, which one is the master? the first domain in the config or the last? or does it try all domains everytime? ...which can also be in some cases a convenience feature, but in most cases (especially if you are not aware of it) this is a security nightmare!

DJCrashdummy commented 3 years ago

after deleting the wrong user without the domain via the nextcloud-GUI it is also deleted at the table users_external and accounts, but at credentials an entry for the deleted user is still there!?! can/should i manually delete this? is this intended? or is this perhaps another bug (maybe of nextcloud core) i stumbled across? :thinking:

actually this I don't know... we would need a more experienced Nextcloud core dev for this...

perhaps @jospoortvliet can give a hint whom to ask resp. raise someone’s attention directly who can determine if this is a nextcloud core issue or intended (and if so, why)?

j0rgan commented 2 years ago

This is still an issue when trying to connect via caldav. Although it denies the login, the account is created. Steps to reproduce:

configure as intended (after login the account is created as user@example.com)
try to sync calendar (with iCal for example) automatically - the result will be: the apple ical will assume it is allowed to login, and then it will attempt to sync. It will not be able to login, however new account with stripped domain is created. Same for syncing carddav.

j0rgan commented 2 years ago

So i have multiple domains on my email server. I created two same usernames on two different domains. Logging in with the username only with two different passwords will log me in on the same account. Definitely this plugin is not intended to be used on the same email server having multiple domains.

j0rgan commented 2 years ago

I found a workaround to the issue. Configure like this: 'user_backends' => array ( 0 => array ( 'class' => 'OC_User_IMAP', 'arguments' => array ( 0 => 'mail.example.com', 1 => 993, 2 => 'ssl', 3 => 'firstdomain.com', 4 => false, 5 => true, ), ), 1 => array ( 'class' => 'OC_User_IMAP', 'arguments' => array ( 0 => 'mail.example.com', 1 => 993, 2 => 'ssl', 3 => 'seconddomain.org', 4 => false, 5 => true, ), ), ),

Then, edit the lib/imap.php: line 66 - $username = $uid . '@' . $this->domain; line 66 + return false;

This way it will not attempt to create another username nor will check valid password against other domains.

Himmelxd commented 6 months ago

Since the uid is returned and this one is used afterwards, I am able to change it inside of the script. Having good experience so far.

Changing line 67 in apps/user_external/lib/IMAP.php

- $username = $uid . '@' . $this->domain;
+ $uid = $uid . '@' . $this->domain;
+ $username = $uid;

Of course this means that the uid will always contain the domain it was first successfully checked against. As there always had been one domain for me and all used to register including the domain, I had no issues so far. If someone knows where this can bring issues, I am happy to hear. Interestingly if a user account without the @domain exists and that one is deactivated, they are also not allowed to login without @domain, pointing that there is at least one small issue (but that account probably should not exists in the first place).

nextcloud / user_external