Skip to content

Allow users to choose a different email for notifications - #28422

Merged
skjnldsv merged 8 commits into
masterfrom
enh/27465/notification-email
Sep 10, 2021
Merged

Allow users to choose a different email for notifications#28422
skjnldsv merged 8 commits into
masterfrom
enh/27465/notification-email

Conversation

@blizzz

@blizzzblizzz commented Aug 13, 2021•
edited
Loading

Copy link
Copy Markdown
Member

Screenshot_20210828_000844

Screenshot_20210820_210826

Getting and setting primary mail via provisioning API

curl -u $userid -X PUT -d 'key=notify_email' -d 'value=myname%40mydomain.com' -H 'OCS-APIRequest: true' https://my.nxt.cld/ocs/v2.php/cloud/users/$userid

(mind the address must be added as additional email address and also be confirmed)

curl -u $userid -X GET -H 'OCS-APIRequest: true' https://nc.zara/master/ocs/v2.php/cloud/users/$userid

@blizzzblizzz added this to the Nextcloud 23 milestone Aug 13, 2021
@blizzz
blizzzforce-pushed the enh/27465/notification-email branch 2 times, most recently from 6ede3a0 to 2e49000CompareAugust 18, 2021 11:21
@blizzz
blizzzforce-pushed the enh/27465/notification-email branch 3 times, most recently from 4ae2959 to 51aefa3CompareAugust 25, 2021 10:50
LukasReschke
LukasReschke reviewed Aug 25, 2021
View reviewed changes

@LukasReschkeLukasReschke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cool stuff and nice work 🎉

Comment threadapps/provisioning_api/appinfo/routes.php Outdated
Comment threadapps/provisioning_api/lib/Controller/VerificationController.php
throw new InvalidArgumentException('Logged in user is not mail address owner');
}
$email = $this->crypto->decrypt($key);
$ref = \substr(hash('sha256', $email), 0, 8);

@LukasReschkeLukasReschkeAug 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any specific reason we limit it to the first 8 chars here? :)

@blizzzblizzzAug 25, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I consider it sufficient to avoid collisions – this is just part of the configkey stored in the db and used to avoid collisions only. If you manage to craft a collision – mind it applies per user – you manage to overwrite a previously stored token. It could have a security implication, if you manage to create a token for a different user that fits the collision, and yet the payload still needs to be valid and pass the checks.

Comment threadlib/public/Accounts/IAccountProperty.php
@blizzz
blizzzforce-pushed the enh/27465/notification-email branch 3 times, most recently from 38a7645 to c05a302CompareAugust 30, 2021 22:33
@skjnldsv

skjnldsv commented Sep 1, 2021

Copy link
Copy Markdown
Member

/backport to stable22

This was referenced Sep 1, 2021
Merged
Merged
@blizzz
blizzzforce-pushed the enh/27465/notification-email branch from c05a302 to 1b7519fCompareSeptember 1, 2021 13:40
@blizzz
blizzz marked this pull request as ready for review September 1, 2021 13:41
@blizzz
blizzzforce-pushed the enh/27465/notification-email branch from 1b7519f to 0c4dcddCompareSeptember 1, 2021 13:49
@blizzzblizzz added the pending documentation This pull request needs an associated documentation update label Sep 1, 2021
artonge
artonge previously requested changes Sep 7, 2021
View reviewed changes
Comment threadapps/provisioning_api/lib/Controller/UsersController.php Outdated
Comment threadapps/provisioning_api/lib/Controller/VerificationController.php Outdated
Comment threadapps/provisioning_api/lib/Controller/VerificationController.php
Comment threadapps/provisioning_api/lib/Controller/VerificationController.php
Comment threadlib/private/Accounts/AccountPropertyCollection.php Outdated
Comment threadlib/private/Accounts/AccountPropertyCollection.php
Comment threadlib/private/User/User.php
@blizzz
move verification token logic out of lost password controller
19cc757
- to make it reusable
- needed for local email verification
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
implement verification for additional mails
aacaad2
- mails added by (sub)admins are automatically verified
- provisioning_api controller as verification endpoint
- IAccountProperty gets a locallyVerified property
- IPropertyCollection gets a method to fetch an IAccountProperty by value
- an remove equivalent was already present
- AccountManager always initiates mail verification on update if necessary
- add core success template for arbitrary title and message
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
fix parameter type hint in phpdoc
37f510c
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
add a job to clean up expired verification tokens
a20de15
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
blizzzforce-pushed the enh/27465/notification-email branch from beb22ea to 8c3553fCompareSeptember 9, 2021 13:20
@blizzz
blizzz requested review from a team, LukasReschke and artongeSeptember 9, 2021 13:21
@blizzz
blizzz requested review from ArtificialOwl, PVince81 and skjnldsv and removed request for a teamSeptember 9, 2021 13:22
@blizzzblizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 9, 2021
@blizzz
blizzzforce-pushed the enh/27465/notification-email branch from 8c3553f to d378fc7CompareSeptember 9, 2021 14:52
Pytal
Pytal reviewed Sep 9, 2021
View reviewed changes
Comment threadapps/provisioning_api/lib/Controller/VerificationController.php Outdated
@blizzz
blizzzforce-pushed the enh/27465/notification-email branch from d378fc7 to 763136aCompareSeptember 9, 2021 17:22
@blizzz
blizzz requested a review from PytalSeptember 9, 2021 17:23
@blizzz
Confirm mails only per POST
0dee717
- this is to avoid automatic confirmation by certain softwares that open
links
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
enable the user to set a primary (notification) email address (backend)
4461b9e
- specific getters and setters on IUser and implementation
- new notify_email field in provisioning API
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
use specific email getter where necessary
27865d0
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
also allow admins to set the primary email
763136a
- there will be times when it is necessary to reset this value for sure
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Pytal
Pytal approved these changes Sep 10, 2021
View reviewed changes

@PytalPytal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Comprehensive 👌

@skjnldsvskjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 10, 2021
@skjnldsv
skjnldsv merged commit 78d6206 into masterSep 10, 2021
@skjnldsv
skjnldsv deleted the enh/27465/notification-email branch September 10, 2021 06:34
@backportbot-nextcloudbackportbot-nextcloudBot mentioned this pull request Sep 10, 2021
Merged
This was referenced Sep 10, 2021
Merged
Open
@juliusknorrjuliusknorr mentioned this pull request Oct 27, 2021
Merged
@tcitworldtcitworld mentioned this pull request Mar 20, 2023
Merged
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skjnldsvskjnldsvskjnldsv approved these changes
@PytalPytalPytal approved these changes
@artongeartongeAwaiting requested review from artonge
@LukasReschkeLukasReschkeAwaiting requested review from LukasReschke
@PVince81PVince81Awaiting requested review from PVince81
@ArtificialOwlArtificialOwlAwaiting requested review from ArtificialOwl

Assignees

No one assigned

Labels

4. to releaseReady to be released and/or waiting for tests to finishenhancementfeature: users and groupspending documentationThis pull request needs an associated documentation update

Projects

None yet

Milestone

Nextcloud 23

Development

Successfully merging this pull request may close these issues.

5 participants

@blizzz@skjnldsv@LukasReschke@artonge@Pytal