create a dynamic default account #28

ChristophWurst · 2016-09-02T15:05:20Z

Alternative approach to #3

@LukasReschke this one is for you :-)

ChristophWurst · 2016-09-02T15:06:02Z

appinfo/application.php

@@ -43,8 +49,25 @@ public function __construct(array $urlParams = []) {
 		$container->registerParameter("appName", "mail");
 		$container->registerParameter("userFolder", $container->getServer()->getUserFolder($user));
 		$container->registerParameter("testSmtp", $testSmtp);
-		$container->registerParameter("referrer", isset($_SERVER['HTTP_REFERER']) ? $_SERVER['HTTP_REFERER'] : null);
+		$container->registerParameter("referrer", isset($_SERVER['HTTP_REFERER']) ? : null);


ChristophWurst · 2016-09-02T15:08:25Z

cc @pierreozoux since you've been waiting for this feature

ChristophWurst · 2016-09-02T15:09:26Z

lib/service/defaultaccount/defaultaccountmanager.php

+
+		$user = $this->userSession->getUser();
+		$this->logger->info('building default account for user ' . $user->getUID());
+		$password = $this->crypto->encrypt($this->session->get('mail_default_account_password'));


TODO: check if password is set (admins could set the config after their users logged in)

ChristophWurst · 2016-09-03T11:09:16Z

Sample config

  'app.mail.accounts.default' => [
    'email' => '%USERID%@yourdomain.at'',
    'imapHost' => 'imap.yourserver.com',
    'imapPort' => 993,
    'imapUser' => '%USERID%@yourdomain.at',
    'imapSslMode' => 'ssl',
    'smtpHost' => 'smtp.yourserver.com',
    'smtpPort' => 486,
    'smtpUser' => '%USERID%@yourdomain.at'',
    'smtpSslMode' => 'none',
  ],

ChristophWurst · 2016-09-03T11:35:58Z

seems to work. @LukasReschke @jancborchardt @pierreozoux @nextcloud/mail review please :-)

pierreozoux · 2016-09-05T14:21:23Z

Hum... Not sure if related to this specific PR, but, I followed instructions from the README to "compile" this PR to test it. at the following step, I get an error:

www-data@43e47166ee64:~/html/apps/mail$ make optimize-js
npm install --production
./node_modules/bower/bin/bower install
./node_modules/requirejs/bin/r.js -o build/build.js

/var/www/html/apps/mail/node_modules/requirejs/bin/r.js:22358
        function addFile(file, fileUrl) {
        ^^^^^^^^
SyntaxError: In strict mode code, functions can only be declared at top level or immediately within another function.
    at Module._compile (module.js:439:25)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:906:3
Makefile:38: recipe for target 'optimize-js' failed
make: *** [optimize-js] Error 8

Can you tell me what I can do to fix it? thanks!

www-data@43e47166ee64:~/html/apps/mail$ node -v
v0.10.29
www-data@43e47166ee64:~/html/apps/mail$ npm -v
1.4.21

ChristophWurst · 2016-09-05T14:29:06Z

Strange. Update to a recent version of node/npm maybe? 🙈

I have node v6.5.0 and npm 3.10.6.

NPM can be updated with npm install -g npm

pierreozoux · 2016-09-05T14:44:03Z

Yes, it is working now, the issue in the README is this line: https://github.com/nextcloud/mail#developer-setup-info

should we point to https://nodejs.org/en/download/package-manager/#debian-and-ubuntu-based-linux-distributions instead?

ChristophWurst · 2016-09-05T14:54:57Z

should we point to https://nodejs.org/en/download/package-manager/#debian-and-ubuntu-based-linux-distributions instead?

Sounds good. If we point to https://nodejs.org/en/download/package-manager/ it's distribution independent

Could you please create a simple PR to fix that? :-)

pierreozoux · 2016-09-05T15:38:23Z

Ok, again, not sure if related to this PR, but I get the following js error:

Error: An "el" #app-navigation #app-navigation-accounts must exist in DOM

And the left side bar is empty, no folders are displayed.
https://cloud.pierre-o.fr/index.php/s/GZkjaf9a0H2nD7B

ChristophWurst · 2016-09-14T11:41:05Z

@pierreozoux that looks unrelated, as I did not touch any of that client-side code. Does building of the master branch work for you?

ChristophWurst · 2016-09-14T11:42:55Z

/var/www/html/apps/mail/node_modules/requirejs/bin/r.js:22358

Hm, that's actually a 3rd-party file. I've never seen something like that and the build also succeeds on travis. Maybe a environment-specific error?

pierreozoux · 2016-09-14T12:48:19Z

Ok, I used node v6, and it is now working \o/

ChristophWurst · 2016-09-14T13:05:00Z

@LukasReschke @jancborchardt mind giving this another test run? :-)

jancborchardt · 2016-09-14T17:47:30Z

Yo @LukasReschke can you enable this for us please? ;)

ChristophWurst · 2016-09-14T17:50:00Z

Be aware that once we have the remember-me functionality back, this way of storing the login password will break. Then the account will magically disappear as soon as the user is logged in again via the remember-me cookie 💩

jancborchardt · 2016-09-15T15:14:44Z

Wait, what? :D So this will break anyway?

ChristophWurst · 2016-09-15T16:38:21Z

Wait, what? :D So this will break anyway?

well, not as long as remember-me is broken 🚀

ChristophWurst · 2016-09-19T14:49:31Z

Error: An "el" #app-navigation #app-navigation-accounts must exist in DOM

@Gomez has the same problem on a production system. @pierreozoux which server/core did you use for testing?

pierreozoux · 2016-09-19T15:31:45Z

The way to solve it I guess was to use node v6.
I use server v10.0

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

jancborchardt · 2017-06-12T14:26:27Z

@LukasReschke @pierreozoux @rakshazi please go ahead and test it. :)

lcalaresu

Oh, that one is really nice!

No problem using the %USERID%, but I have some problem with the %EMAIL%... Will double check my configuration.

When I add another mail account, I have some js TypeError problem, but everything works fine when I reload the page... But I reproduce it on master branch as well so, it does not seem related to the PR. Is it a known issue?
(edit) It is indeed a known issue #320, sorry.

lcalaresu · 2017-06-12T14:58:37Z

lib/Service/AccountService.php

 	/**
 	 * @param MailAccountMapper $mapper
 	 */
-	public function __construct(MailAccountMapper $mapper, IL10N $l10n) {
+	public function __construct(MailAccountMapper $mapper, IL10N $l10n,
+		Manager $defaultAccountManager) {


Are the @param important?
They were not consistent on that file before the PR anyway, so maybe it is not important... but I'm asking coz I am tempted to change some headers on a file I'm changing on another PR 😋

My bad, of course they should be in sync with the actual parameters of the method :-)

Although one could argue that the @param annotation has no added value when the parameter is type hinted anyway 😉

lcalaresu · 2017-06-12T18:51:01Z

I finally got it working with the %EMAIL% in the config file. 😵
It's easier to catch by reading the code 😝

Actually, my test user had a different email than what was supposed to be (from the config file perspective). Do not know if it's clear...

Here is the story of John (my user):

He put john@doe.com in his profile because he prefer using his personal email.
The administrator of his nextcloud instance wrote the following configuration file:

'app.mail.accounts.default' => [
    'email' => '%USERID%@domain.tld',
    'imapHost' => 'imap.domain.tld',
    'imapPort' => 993,
    'imapUser' => '%EMAIL%',
    'imapSslMode' => 'ssl',
    'smtpHost' => 'smtp.domain.tld',
    'smtpPort' => 465,
    'smtpUser' => '%EMAIL%',
    'smtpSslMode' => 'ssl',
],

When he goes to the mail app, the mailbox john@domain.tld has been created, but the app is trying to connect to the server using his personal email address and his nextcloud password... John (actually, that was me) is confused: he cannot access his professional mailbox, and cannot even delete it...

Actually, I thought the %EMAIL% keyword was just to avoid repeating the email written just above in the config file.

But that let me think: is it safe to rely on the user email? (that could be changed by him at any time).
If the admin chooses the host, the port and the ssl mode, shouldn't he be certain the email will not change over time?

Except that, I love that feature! And everything else works perfectly fine for me.

ChristophWurst · 2017-06-13T06:28:09Z

I finally got it working with the %EMAIL% in the config file. 😵
It's easier to catch by reading the code 😝

Yeah, there should be more documentation on how to use this new feature.

Regarding the email issue: good catch! I didn't think about that scenario. However, it's the responsibility of the admin to set the default account settings to something that actually makes sense and works. On most instances the %USERID% replacement is all that is actually needed, because you can easily build credentials for a single domain.
Therefore we have two possible paths to take: first, remove this %EMAIL% replacement feature right away. second, leave it for complex setups. I could make sense to use a user-provided mail address, for example on instances where users are connected to the same user back-end, but with different email domains.

lcalaresu · 2017-06-13T07:22:14Z

@ChristophWurst Yeah, that could be convenient for that admin indeed.
I am just afraid end users would have the power to (silently) break their mail app.

Actually, I don't use LDAP or others on NC. But in that case, can the end user modify his email or is it "forced" by the admin?
If it cannot be changed, I would say that is indeed not really a problem if the "limitation" is documented for those not using domain users (who will be less likely to use that feature anyway I think).

ChristophWurst · 2017-06-13T10:53:35Z

Actually, I don't use LDAP or others on NC. But in that case, can the end user modify his email or is it "forced" by the admin?

I don't know.

ChristophWurst · 2017-06-13T11:35:36Z

If it cannot be changed, I would say that is indeed not really a problem if the "limitation" is documented for those not using domain users (who will be less likely to use that feature anyway I think).

IMO having a centralized user back-end that is both used by your Nextcloud and IMAP server is the only realistic use case for this feature 😉

lcalaresu · 2017-06-13T14:05:06Z

IMO having a centralized user back-end that is both used by your Nextcloud and IMAP server is the only realistic use case for this feature 😉

I agree.

Concerning the "Delete account", do you think it could be possible to disable or hide it easily?
Nothing really important here since the user cannot delete the account anyway... But if the button does not work, maybe it could be seen as a bug for some people. Maybe in another PR...

Everything else worked well for me, it works as expected. 👍 👍

ChristophWurst · 2017-06-13T14:06:18Z

But if the button does not work, maybe it could be seen as a bug for some people. Maybe in another PR...

Ah yes, we should hide it ofc.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

ChristophWurst · 2017-06-13T14:33:26Z

Fixed 🚀

lcalaresu

Looks good to me 😃

ChristophWurst · 2017-06-13T16:20:12Z

Thanks a bunch for testing and reviewing 😀

lcalaresu · 2017-06-13T18:38:50Z

I did the easy part 😉

fsedarkalex · 2017-06-14T08:31:52Z

Just one question...

first of all it seems working but what should happen if the email password differs from the NC-Password?
Currently endless loading occurs. Do you see any chance to temporarily disable the mailbox (until next login) or even better ask for the password?

ChristophWurst · 2017-06-14T08:35:07Z

first of all it seems working but what should happen if the email password differs from the NC-Password?
Currently endless loading occurs. Do you see any chance to temporarily disable the mailbox (until next login) or even better ask for the password?

Yes, a bit of failure tolerance is needed regardless of an automatically created account because any IMAP account can be temporarily unavailable. Luckily, the code in #326 already comes with a bit of tolerance when one of the accounts fails to load. You still see a warning, but other accounts are usable.

sim6 · 2017-06-14T14:48:07Z

Please, could you release a version with this changes?

ChristophWurst · 2017-06-14T15:55:14Z

@sim6 sure, as soon as we've got into a more or less stable state again. Currently there are some issues, e.g. with the background synchronization that make your browser freeze in certain circumstances. #326 tackles some of the issues. Any help (bugfixing, testing, issue reporting, reviewing) is very much appreciated.

Extarys · 2017-09-26T08:52:44Z

Since I'm using 3 different domain name, I would like the login to be the email address of the account.
Instead of 'email' => '%USERID%@domain.tld', => 'email' => '%EMAIL%',, But it doesn't work. :\

ChristophWurst · 2017-09-26T08:54:49Z

Hi,

please file a new ticket if you have found bugs and do not comment on old and closed pull requests. Thanks!

ChristophWurst added enhancement 2. developing labels Sep 2, 2016

ChristophWurst added this to the 0.6 milestone Sep 2, 2016

ChristophWurst reviewed Sep 2, 2016
View reviewed changes

ChristophWurst force-pushed the dynamic-default-account branch 2 times, most recently from 8c3a403 to 46184a4 Compare September 3, 2016 11:25

ChristophWurst added 3. to review and removed 2. developing labels Sep 3, 2016

ChristophWurst mentioned this pull request Sep 3, 2016

Create default account automatically #3

Closed

6 tasks

ChristophWurst force-pushed the dynamic-default-account branch from 46184a4 to aec8810 Compare September 14, 2016 11:43

Fix ternary operator expression to actually return the array value

2ff8a0f

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

lcalaresu reviewed Jun 12, 2017

View reviewed changes

Hide 'Delete' button for non-deletable accounts

8ffdbf3

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

lcalaresu approved these changes Jun 13, 2017

View reviewed changes

ChristophWurst merged commit 525d210 into master Jun 13, 2017

ChristophWurst deleted the dynamic-default-account branch June 13, 2017 16:19

lcalaresu mentioned this pull request Jun 14, 2017

[Request] Default Account #391

Closed

This was referenced Sep 19, 2017

Configure mail app for IMAP authenticated users #536

Closed

General question: Quota and mail nextcloud/server#6629

Closed

Enhanced doc #544

Merged

nextcloud locked and limited conversation to collaborators Sep 26, 2017

create a dynamic default account #28

create a dynamic default account #28

Conversation

ChristophWurst commented Sep 2, 2016

ChristophWurst Sep 2, 2016 • edited Loading

Choose a reason for hiding this comment

ChristophWurst commented Sep 2, 2016

ChristophWurst Sep 2, 2016 • edited Loading

Choose a reason for hiding this comment

ChristophWurst commented Sep 3, 2016

ChristophWurst commented Sep 3, 2016

pierreozoux commented Sep 5, 2016

ChristophWurst commented Sep 5, 2016

pierreozoux commented Sep 5, 2016

ChristophWurst commented Sep 5, 2016

pierreozoux commented Sep 5, 2016 • edited Loading

ChristophWurst commented Sep 14, 2016

ChristophWurst commented Sep 14, 2016

pierreozoux commented Sep 14, 2016

ChristophWurst commented Sep 14, 2016

jancborchardt commented Sep 14, 2016

ChristophWurst commented Sep 14, 2016

jancborchardt commented Sep 15, 2016

ChristophWurst commented Sep 15, 2016

ChristophWurst commented Sep 19, 2016

pierreozoux commented Sep 19, 2016

jancborchardt commented Jun 12, 2017

lcalaresu left a comment • edited Loading

Choose a reason for hiding this comment

lcalaresu Jun 12, 2017

Choose a reason for hiding this comment

ChristophWurst Jun 12, 2017

Choose a reason for hiding this comment

ChristophWurst Jun 12, 2017

Choose a reason for hiding this comment

lcalaresu commented Jun 12, 2017 • edited Loading

ChristophWurst commented Jun 13, 2017

lcalaresu commented Jun 13, 2017

ChristophWurst commented Jun 13, 2017

ChristophWurst commented Jun 13, 2017

lcalaresu commented Jun 13, 2017

ChristophWurst commented Jun 13, 2017

ChristophWurst commented Jun 13, 2017

lcalaresu left a comment

Choose a reason for hiding this comment

ChristophWurst commented Jun 13, 2017

lcalaresu commented Jun 13, 2017

fsedarkalex commented Jun 14, 2017

ChristophWurst commented Jun 14, 2017

sim6 commented Jun 14, 2017

ChristophWurst commented Jun 14, 2017

Extarys commented Sep 26, 2017

ChristophWurst commented Sep 26, 2017

ChristophWurst Sep 2, 2016 •

edited

Loading

ChristophWurst Sep 2, 2016 •

edited

Loading

pierreozoux commented Sep 5, 2016 •

edited

Loading

lcalaresu left a comment •

edited

Loading

lcalaresu commented Jun 12, 2017 •

edited

Loading