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

fix(ProvisioningApi): only return verified additional mails per user #44341

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Mar 19, 2024

Summary

…it would not per se be bad to return all of them, however the meta data about the verified state is missing. Since the information may go out to connected clients, those may have wrong trust the returned email addresses.

Email verification still works with this change.

Checklist

@blizzz blizzz added this to the Nextcloud 29 milestone Mar 19, 2024
@blizzz blizzz requested review from nickvergessen, Pytal, a team, ArtificialOwl, icewind1991 and sorbaugh and removed request for a team March 19, 2024 20:50
@blizzz blizzz added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 20, 2024
@Altahrim Altahrim mentioned this pull request Mar 20, 2024
@Altahrim Altahrim mentioned this pull request Mar 25, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@blizzz
Copy link
Member Author

blizzz commented Apr 7, 2024

Integration tests fail, for no email verification was done there. That might be tricky. There is no way to modify it from outside, without exposing anything, so not the way to go. Maybe utilizing FakeSMTPHelper in the integration tests could help, did not have time for a closer look there yet (free time activity…).

@nickvergessen
Copy link
Member

There is no way to modify it from outside, without exposing anything, so not the way to go.

Can add any code directly to the apps/testing app.

It would not per se be bad to return all of them, however the meta data
about the verified state is missing. Since the information may go out to
connected clients, those may have wrong trust the returned email
addresses.

Email verification still works with this change.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the fix/noid/return-verified-email branch from 266a79a to 35a0ee2 Compare June 23, 2024 20:05

class MailVerificationTestController extends OCSController {
public function __construct(
$appName,

Check notice

Code scanning / Psalm

MissingParamType Note test

Parameter $appName has no provided type
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$appName,
string $appName,


public function verify(string $userId, string $email): DataResponse {
$user = $this->userManager->get($userId);
$userAccount = $this->accountManager->getAccount($user);

Check notice

Code scanning / Psalm

PossiblyNullArgument Note test

Argument 1 of OCP\Accounts\IAccountManager::getAccount cannot be null, possibly null value provided
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$userAccount = $this->accountManager->getAccount($user);
if ($user === null) {
throw new InvalidArgumentException('User not available.');
}
$userAccount = $this->accountManager->getAccount($user);

@@ -0,0 +1,35 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

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

Needs SPDX headers

This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024

class MailVerificationTestController extends OCSController {
public function __construct(
$appName,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$appName,
string $appName,


public function verify(string $userId, string $email): DataResponse {
$user = $this->userManager->get($userId);
$userAccount = $this->accountManager->getAccount($user);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$userAccount = $this->accountManager->getAccount($user);
if ($user === null) {
throw new InvalidArgumentException('User not available.');
}
$userAccount = $this->accountManager->getAccount($user);

@@ -0,0 +1,35 @@
<?php

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
declare(strict_types=1);
/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants