Skip to content

/ server

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

Jump to bottom

User changes e-mail, validation only partly triggered #21336

Open
ghost opened this issue Jun 9, 2020 · 7 comments
Open

User changes e-mail, validation only partly triggered #21336

ghost opened this issue Jun 9, 2020 · 7 comments
Labels
1. to develop 19-feedback bug good first issue

Comments

@ghost
Copy link

@ghost ghost commented Jun 9, 2020

How to use GitHub

  • Please use the 👍 reaction to show that you are affected by the same issue.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.

Steps to reproduce

  1. Login as user.
  2. Change e-mail address.
    3a. As user: Watch e-mail address getting stored, nothing more.
    3b. As admin: Check database to see additional job.

Expected behaviour

Either send verification e-mail or don't create verification job without any verification data.

Actual behaviour

No verification mail is sent but verification job without verification data is created.

Server configuration

Operating system: Linux, details unknown

Web server: Apache

Database: MySQL

PHP version: 7.4.5

Nextcloud version: 19.0.0 (but bug was present before; I have jobs for accounts that where deleted before 19.0.0 upgrade)

Updated from an older Nextcloud/ownCloud or fresh install: Update

Where did you install Nextcloud from: nextcloud.com / Updater

Signing status:

Signing status No errors have been found.

List of activated apps:

App list

Default
AppOrder
Deck
Forms
Polls

Nextcloud configuration:

Config report ‘…’, 'passwordsalt' => ‘…’, 'secret' => ‘…’, 'trusted_domains' => array ( 0 => ‘xyz.example.com', ), 'datadirectory' => '/www/htdocs/xyz.example.com/data', 'dbtype' => 'mysql', 'version' => '19.0.0.12', 'overwrite.cli.url' => 'https://xyz.example.com/', 'htaccess.RewriteBase' => '/', 'htaccess.IgnoreFrontController' => true, 'dbname' => ‘…’, 'dbhost' => 'localhost', 'dbport' => '', 'dbtableprefix' => 'oc_', 'mysql.utf8mb4' => true, 'dbuser' => ‘…’, 'dbpassword' => ‘…’, 'installed' => true, 'mail_smtpmode' => 'sendmail', 'mail_sendmailmode' => 'smtp', 'mail_from_address' => 'noreply', 'mail_domain' => ‘example.com’, 'tempdirectory' => '/www/htdocs/tmp', 'updater.secret' => ‘…’, 'maintenance' => false, 'theme' => '', 'loglevel' => 2, 'defaultapp' => 'calendar', 'skeletondirectory' => '', 'filesystem_check_changes' => 1, 'default_language' => 'de', 'default_locale' => 'de', 'updater.release.channel' => 'stable', );

Are you using external storage, if yes which one: no

Are you using encryption: no

Are you using an external user-backend, if yes which one: no

Client configuration

Browser: any

Operating system: any

Logs

Web server error log

no access

Nextcloud log (data/nextcloud.log)

Nextcloud log

https://pastebin.com/YiX1BWS5

Browser log

as before: any browser/os
@ghost ghost added 0. Needs triage bug labels Jun 9, 2020
@ghost
Copy link
Author

@ghost ghost commented Jun 9, 2020

In AccountManager.php#173 an empty verificationCode is set. If I add something here it's added to the job as well, but nothing happens. Seems to me like this feature is not fully implemented anymore.
@kesselb
Copy link
Contributor

@kesselb kesselb commented Jun 9, 2020

Either send verification e-mail

Why do you think a verification email should be sent?
@ghost
Copy link
Author

@ghost ghost commented Jun 9, 2020

Well, why would a job be generated for every e-mail change including an empty "verificationCode"?
It may well be that the intention of that code is different and my assumption that it's used as a validation for e-mail is wrong.
But nevertheless: For every e-mail change I get a new entry in oc_jobs that never runs and that's never removed from the table. So even if my assumption is wrong, there's still someting wrong.
@kesselb
Copy link
Contributor

@kesselb kesselb commented Jun 9, 2020

For every e-mail change I get a new entry in oc_jobs that never runs and that's never removed from the table.

That would be the actual behaviour than. Are you sure it's the same job or just another one for the same email?

So even if my assumption is wrong

https://github.com/nextcloud/server/blob/caff1023ea72bb2ea94130e18a2a6e2ccf819e5f/apps/settings/lib/BackgroundJobs/VerifyUserData.php

It seems nextcloud tries to validate a email against a lookup server. I don't know what that is and why but it also includes some retry logic to do this multiple times.

The job queue itself is working? Cron runs very 5 minutes and work on some jobs? I think in oc_jobs the last execution is logged.

If you change the loglevel to 0 the job execution is logged. Run xyz job with ID. That might help to find running / not running jobs.
@kesselb
Copy link
Contributor

@kesselb kesselb commented Jun 9, 2020

If you run a similar setup like #21212 (lookupServerUploadEnabled == 'no') you may end up with a job for each email change that stays in the queue for at least 24 hours. I guess we could tweak that a bit by not re scheduling the job if the configuration disables it anyway.
@kesselb
Copy link
Contributor

@kesselb kesselb commented Jun 9, 2020

https://github.com/nextcloud/server/blob/caff1023ea72bb2ea94130e18a2a6e2ccf819e5f/apps/settings/lib/BackgroundJobs/VerifyUserData.php

Is the code in question. The idea is to not add a verify user data job again if the preconditions are not fulfilled:

  • Precondition for verifyWebsite and verifyViaLookupServer: $this->config->getSystemValue('has_internet_connection', true) we should do that in the run method, set $this->retainJob = false; and early exit if internet connection is disabled.

  • Precondition for verifyViaLookupServer: There is already a check for lookupServer. Remove the has_internet_connection condition. Also change the return false to return true. If the method returns true the job is not added again. We also do that if the user is null already.
@ghost
Copy link
Author

@ghost ghost commented Jun 9, 2020

That would be the actual behaviour than. Are you sure it's the same job or just another one for the same email?

My apologies, you're right: That is the actual behavior.

I verified again (also with the code you referred to): The old job is removed and a new job with argument try+1 is created. So after a while they should be altogether gone.

The job queue itself is working? Cron runs very 5 minutes and work on some jobs? I think in oc_jobs the last execution is logged.

Yes, the queue is working and all jobs but the ones with class "OCA\Settings\BackgroundJobs\VerifyUserData" update their last_run date. The VerifyUserData job stores that information in the arguments column:
{"verificationCode":"","data":"","type":"email","uid":"NPbRszfRTOrqomqxXxhRpEnzkIVXNm","try":10,"lastRun":1591707291}

If you change the loglevel to 0 the job execution is logged. Run xyz job with ID. That might help to find running / not running jobs.

Did that. The jobs are running with the only result the re-scheduling.

{"reqId":"Xt@GlLBg50J281CMJgu8qwAAAAE","level":0,"time":"2020-06-09T12:54:44+00:00","remoteAddr":"95.114.125.119","user":"--","app":"cron","method":"GET","url":"/cron.php","message":"Run OCA\\Settings\\BackgroundJobs\\VerifyUserData job with ID 288","userAgent":"curl/7.64.1","version":"19.0.0.12"}
{"reqId":"Xt@GlLBg50J281CMJgu8qwAAAAE","level":0,"time":"2020-06-09T12:54:44+00:00","remoteAddr":"95.114.125.119","user":"--","app":"cron","method":"GET","url":"/cron.php","message":"Finished OCA\\Settings\\BackgroundJobs\\VerifyUserData job with ID 288 in 0 seconds","userAgent":"curl/7.64.1","version":"19.0.0.12"}

Re: "(lookupServerUploadEnabled == 'no')"
Yes that was disabled for our site as well.

Thank you already for your help & improvement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
1. to develop 19-feedback bug good first issue
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
@kesselb and others
You can’t perform that action at this time.