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

invite user replace add user #18351

Closed
wants to merge 340 commits into from
Closed

invite user replace add user #18351

wants to merge 340 commits into from

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Nov 19, 2021

Description:

Just a note, I think this PR will be put on pause for now, currently is on the transaction from angular to VUE. Once the Vue implementation is finished, will convert it to VUE and fix the remaining task.

Add Two table column

  • invite_status sent or null
  • invite_at datetime

Using currently token from user_auth_token table. Resend invite will send a link to the user, clicking the link will mark the user active and land on the password setting page.

Email template:

image

Missing:

  • UI Test
  • Integration Test

Review

Peter Zhang added 30 commits October 27, 2021 15:53
disable fast cgi
copy config file
update UI test to apache
update ui and php config
enable UI tests
update cache permission
update folder permission for apache
update file access permission
update redis and update gloable ini
update apache perimssion
remove permission on fonts etc
use php instead of package
remove apache, set nginx
only test UI
make it fast only test one
only test nginx
check apache status
tests nginx
typo
add full path
remove error conf
update bracks
add events fields
use php 8.0
apache ui 0 test
# Conflicts:
#	config/global.ini.php
use php -S
test github action php
@justinvelluppillai
Copy link
Contributor

What's needed here to move this PR along @peterhashair? Can you update and request review?

@peterhashair
Copy link
Contributor Author

@justinvelluppillai nothing needed here, just code review :)

@tsteur
Copy link
Member

tsteur commented Jan 19, 2022

  • In the Invite User UI before the "Invite" button can we show some text what will happen after clicking on the button? Like we can mention that the user will receive an email, that the user has X days to confirm this email. And we can also inform them they can reject an unaccepted invite by doing xyz? What else would be important?

image

  • The email contains %s text
    image

  • Can we disable maybe the "Password" form field (maybe also in the API) until a user confirmed the invite? Currently a super user can set the password for a person who was invited but has not accepted anything yet. I checked it wasn't possible to log in but may be still good to prevent this?

  • I'm not sure but it looks like I was automatically logged in if after accepting the invite without having to confirm the password. Possible that this was the case because the super user set a password on my behalf but doesn't look like it. I tested it with another user as well and I did not have to configure any password I was directly logged in and could click around in the UI and it counted as accepted.
    Instead it was accepted that we're showing the user a screen where the user confirms the invite, configures password, and also we need to make sure that they'll know what their login is. They won't be automatically logged in. If a privacy or terms or imprint URL is configured in general settings then they would need to agree to the configured terms and policy to confirm the invite. And if an Imprint URL is configured, then we would need to show this URL too but they would not need to agree to it. This could be shown at the bottom.

I stopped for now with reviewing as it looks like there may be still some bigger things left to do

@peterhashair
Copy link
Contributor Author

@tsteur will do an update. :)

@peterhashair peterhashair removed the Needs Review PRs that need a code review label Jan 20, 2022
Peter added 4 commits January 20, 2022 14:49
update user
# Conflicts:
#	plugins/TwoFactorAuth/tests/UI/expected-screenshots/TwoFactorAuthUsersManager_list.png
#	plugins/UsersManager/tests/UI/expected-screenshots/UsersManager_all_rows_deselected.png
#	plugins/UsersManager/tests/UI/expected-screenshots/UsersManager_all_rows_in_search.png
#	plugins/UsersManager/tests/UI/expected-screenshots/UsersManager_all_rows_selected.png
#	plugins/UsersManager/tests/UI/expected-screenshots/UsersManager_bulk_remove_access.png
#	plugins/UsersManager/tests/UI/expected-screenshots/UsersManager_bulk_set_access.png
#	plugins/UsersManager/tests/UI/expected-screenshots/UsersManager_delete_bulk_access.png
#	plugins/UsersManager/tests/UI/expected-screenshots/UsersManager_delete_single.png
#	plugins/UsersManager/tests/UI/expected-screenshots/UsersManager_load.png
#	plugins/UsersManager/tests/UI/expected-screenshots/UsersManager_manage_users_back.png
#	plugins/UsersManager/tests/UI/expected-screenshots/UsersManager_next_click.png
#	plugins/UsersManager/tests/UI/expected-screenshots/UsersManager_previous.png
#	plugins/UsersManager/tests/UI/expected-screenshots/UsersManager_role_for.png
#	plugins/UsersManager/tests/UI/expected-screenshots/UsersManager_rows_selected.png
#	tests/UI/expected-screenshots/UIIntegrationTest_admin_home.png
#	tests/UI/expected-screenshots/UIIntegrationTest_api_error.png
update password field
update condition on password
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2022

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Feb 8, 2022
@peterhashair peterhashair removed the Stale The label used by the Close Stale Issues action label Feb 8, 2022
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Had rough look through the code and left various comments. Didn't go on with the review and UI testing as actually various requirements aren't obviously met (by looking at the code only). Please review the requirements here #13321 (comment) and adjust the implementation accordingly.

@@ -5,10 +5,11 @@
"TwoFactorAuthentication": "Two-factor authentication",
"ResetTwoFactorAuthentication": "Reset two-factor authentication",
"ResetTwoFactorAuthenticationInfo": "If the user can no longer log in due to lost recovery codes or a lost authentication device, you can reset two-factor authentication for the user, so they can log in again.",
"AddUser": "Add a new user",
"AddUser": "Invite a new user",
Copy link
Member

Choose a reason for hiding this comment

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

Please use new translation keys in case the sense of the message changed to ensure existing translations won't be used anymore.

/**
* Update for version 4.7.0-b1.
*/
class Updates_4_7_0_b1 extends PiwikUpdates
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be changed to 4.8.0-b1

plugins/CoreAdminHome/lang/en.json Outdated Show resolved Hide resolved
@@ -436,6 +443,26 @@ public function setIgnoreCookie()
Piwik::redirectToModule('UsersManager', 'userSettings', array('token_auth' => false));
}

public function activeInviteUser()
Copy link
Member

Choose a reason for hiding this comment

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

activeInviteUser sounds like a bad method name to me. It actually doesn't really say what it does. I guess something like acceptInvitation would be a lot more meaningful.

plugins/UsersManager/Controller.php Outdated Show resolved Hide resolved
@@ -52,6 +52,8 @@ public function getTablesCreateSql()
superuser_access TINYINT(2) unsigned NOT NULL DEFAULT '0',
date_registered TIMESTAMP NULL,
ts_password_modified TIMESTAMP NULL,
invite_status varchar(40) DEFAULT NULL,
Copy link
Member

Choose a reason for hiding this comment

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

Is there any benefit and storing the invite_status at all? Actually if there is an invited_at timestamp that means, that the user has been invited. And after he accepted, we can simply remove the timestamp and it indicates the users is active.

Comment on lines +788 to +794
if (!Piwik::hasUserSuperUserAccess()) {
if (empty($initialIdSite)) {
throw new \Exception(Piwik::translate("UsersManager_AddUserNoInitialAccessError"));
}

Piwik::checkUserHasAdminAccess($initialIdSite);
}
Copy link
Member

Choose a reason for hiding this comment

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

Does that mean that a super user can invite someone without any initial access to a website?
Btw. might also be good to check if a site with the id $initialIdSite actually exists.

Comment on lines +811 to +814
// we reload the access list which doesn't yet take in consideration this new user
Access::getInstance()->reloadAccess();
Cache::deleteTrackerCache();

Copy link
Member

Choose a reason for hiding this comment

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

Why should that be needed? Adding a new user shouldn't change anything on the currents user auth. Not should it cause any changes to the tracker, so no need to clear the cache.

//add token
$this->model->deleteAllTokensForUser($userLogin);
$generatedToken = $this->model->generateRandomTokenAuth();
$this->model->addTokenAuth($userLogin, $generatedToken, "Invite Token", Date::now()->getDatetime(), Date::now()->addDay(7)->getDatetime());
Copy link
Member

Choose a reason for hiding this comment

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

The following requirements aren't fulfilled here:

By default, the invite is valid for 7 days. This needs to be configurable using the API parameter and as a configuration option.

@peterhashair
Copy link
Contributor Author

@sgiehl that one is not ready yet :) , but will do a update

Peter Zhang and others added 2 commits February 10, 2022 12:53
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
@peterhashair
Copy link
Contributor Author

Just a note, I think this PR will be put on pause for now, currently is on the transaction from angular to VUE. Once the Vue implementation is finished, will transfer it to VUE and fix the remaining task.

@peterhashair peterhashair modified the milestones: 4.8.0, 4.9.0 Feb 23, 2022
@peterhashair peterhashair mentioned this pull request Mar 4, 2022
11 tasks
@github-actions
Copy link
Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Mar 10, 2022
@justinvelluppillai justinvelluppillai modified the milestones: 4.9.0, 4.10.0 Apr 12, 2022
@sgiehl
Copy link
Member

sgiehl commented Apr 26, 2022

closing in favor of #18868

@sgiehl sgiehl closed this Apr 26, 2022
@sgiehl sgiehl deleted the m-13321-invite-user branch April 5, 2023 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Stale The label used by the Close Stale Issues action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants