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
invite user replace add user #18351
Conversation
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
add Linux
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
What's needed here to move this PR along @peterhashair? Can you update and request review? |
@justinvelluppillai nothing needed here, just code review :) |
I stopped for now with reviewing as it looks like there may be still some bigger things left to do |
@tsteur will do an update. :) |
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
If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'. |
There was a problem hiding this 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", |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
@@ -436,6 +443,26 @@ public function setIgnoreCookie() | |||
Piwik::redirectToModule('UsersManager', 'userSettings', array('token_auth' => false)); | |||
} | |||
|
|||
public function activeInviteUser() |
There was a problem hiding this comment.
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.
@@ -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, |
There was a problem hiding this comment.
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.
if (!Piwik::hasUserSuperUserAccess()) { | ||
if (empty($initialIdSite)) { | ||
throw new \Exception(Piwik::translate("UsersManager_AddUserNoInitialAccessError")); | ||
} | ||
|
||
Piwik::checkUserHasAdminAccess($initialIdSite); | ||
} |
There was a problem hiding this comment.
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.
// we reload the access list which doesn't yet take in consideration this new user | ||
Access::getInstance()->reloadAccess(); | ||
Cache::deleteTrackerCache(); | ||
|
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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.
@sgiehl that one is not ready yet :) , but will do a update |
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Co-authored-by: Stefan Giehl <stefan@matomo.org>
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. |
If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'. |
closing in favor of #18868 |
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 nullinvite_at
datetimeUsing 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:
Missing:
Review