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

Jump to bottom

invite user replace add user #18868

Merged
merged 132 commits into from Jun 6, 2022
Merged

invite user replace add user #18868

merged 132 commits into from Jun 6, 2022

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Mar 2, 2022 •
edited

Description:

fixes: #13321

Since we move UsersManager to VUE. Redo the entire PR in the VUE version and fixes the remaining comments

This PR is VUE version of #18351

This probably needs docs for it, still working on the docs, not sure we want people inject their only term & condition on the set password page.

Changes

add one table column

  • invite_at datetime

Review

@peterhashair
Copy link
Contributor Author

@sgiehl I got a question for the structure, is that ok for me to move some of the API.php private methods to a different class, just the public method remains in the API.php. In this case, is the UsersManager/API.php

Peter and others added 2 commits March 7, 2022 15:05
@sgiehl
Copy link
Member

sgiehl commented Mar 7, 2022

@sgiehl I got a question for the structure, is that ok for me to move some of the API.php private methods to a different class, just the public method remains in the API.php. In this case, is the UsersManager/API.php

If private API methods could be reused somewhere else, it makes sense to move them to another class.

Peter added 14 commits March 8, 2022 14:09
add invite page
229610d 
add invite page
add invite page
5989648 
add invite page
add some api back
a452ab4 
add some api back
Merge branch '4.x-dev' into m-13321
6230239
Update UserRepository.php
1738799 
update tests
update all xml in the tests
375b90c 
update all xml in the tests
update tests
445d3fe 
update tests
update tests
0685806 
update tests
Update LoginLdap
679552b 
update tests
Update UsersManagerTest.php
a9c5b9c 
update tests
update tests
4e327a4 
update tests
Merge branch '4.x-dev' into m-13321
532bd1c 
# Conflicts:
#	plugins/Login/Controller.php
Update APITest.php
3344e1d 
update tests
Update UserInviteEmail.php
86fb58f 
fix cs
@peterhashair peterhashair added the Needs Review PRs that need a code review label May 30, 2022
bx80
bx80 suggested changes Jun 1, 2022
View reviewed changes
Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

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

@peterhashair Looking good overall 👍

I noticed a couple of issues:

The Resend Invite action shows for all users even if they haven't been invited, this could be confusing and an error is shown when attempting to reinvite existing users. It would be good to hide Resend Invite if the user hasn't been invited.
image

After creating an invite for a new user, following the accept link and entering a new password an error is shown:
image
This happens when already logged in as one user who creates the invite, then the accept link is opened in the same browser. Probably not a common scenario but it could happen, especially when testing.

@bx80 bx80 removed the Needs Review PRs that need a code review label Jun 1, 2022
@peterhashair
Copy link
Contributor Author

@bx80 fixed it.

peterhashair and others added 2 commits June 2, 2022 02:51
@peterhashair peterhashair added the Needs Review PRs that need a code review label Jun 6, 2022
bx80
bx80 approved these changes Jun 6, 2022
View reviewed changes
Copy link
Contributor

@bx80 bx80 left a comment

Choose a reason for hiding this comment

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

The resend invite action is only showing when an invite has been sent 👍

The defaultReportDate was due to an out of date plugin I had installed - no issue once the plugin was deactivated 👍

@peterhashair peterhashair merged commit d036954 into 4.x-dev Jun 6, 2022
@peterhashair peterhashair deleted the m-13321 branch June 6, 2022 23:47
sgiehl
sgiehl reviewed Jun 7, 2022
View reviewed changes
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.

Here would have really a lot changes been required before I would have considered this PR as mergeable.

The most important ones are:

  • Using a token_auth as invite token is kind of a security problem, as it allows using the API without even accepting the invite
  • The tests are not covering the whole process. It's nice that all screens are covered by a UI test, but it would also be important to really click the accept or decline button and see what happens afterwards. Not having tests for that has the risk, that this might regress in the future and we won't see that.
  • The user experience of this new feature is quite bad, as in a lot places the success notifications are missing and the users doesn't see if his action actually worked or not.
  • Some Most of the requirements of the original issue aren't fulfilled. To mention some important ones of the list here: Invite new users in Matomo, rather than creating them directly #13321 (comment)
    • Activity log should recognise "invite user" and "invite user accepted".
      --> see comments about missing / changed events
    • A user can also decline the invite in which case the person that created the invite is being notified by email and we remove the user entry to have no personal data in there. (we might need to store in the user table who invited the user, if that user login no longer exists then we don't notify anyone)
      --> currently the user remains in the database
    • When a user clicks on accept invite, they enter their password. If privacy policy or terms is configured, then we show these links. We say eg if privacy policy is configured: By signing up, I accept the Privacy Policy. If terms is configured we say it similarly for terms. If both are configured then we mention both.
      --> currently there is always Terms & Conditions displayed.
    • In addition the UI currently doesn't allow to see when a invite expire, nor is it possible to show all invited users (e.g. by a filter).
    • Invited users can be seen by super users, and the person that invited that user.
      --> currently everyone who has permission can see the user, as invites are created as a normal users.

core/Db/Schema/Mysql.php Show resolved Hide resolved
core/Validators/Login.php Show resolved Hide resolved
plugins/Login/Controller.php Show resolved Hide resolved
plugins/Login/Controller.php Show resolved Hide resolved
plugins/Login/Controller.php Show resolved Hide resolved
plugins/UsersManager/vue/src/PagedUsersList/PagedUsersList.vue Show resolved Hide resolved
plugins/UsersManager/tests/UI/UsersManager_spec.js Show resolved Hide resolved
plugins/UsersManager/vue/src/User.ts Show resolved Hide resolved
plugins/UsersManager/vue/src/UserEditForm/UserEditForm.vue Show resolved Hide resolved
plugins/UsersManager/vue/src/UserEditForm/UserEditForm.vue Show resolved Hide resolved
@bx80
Copy link
Contributor

bx80 commented Jun 8, 2022

Thanks for that @sgiehl, you are right - there are several issues here that should have been caught during review and will need to be addressed with a fix PR.

With the auth token issue, what do you think about adding a boolean flag invite_token to the user_token_auth table to indicate that the token is only valid for user invitation, similar to the existing system_token option? Any attempt to use an invite_token for general API access could then be rejected in addition to removing the token entirely once the invite is accepted / rejected?

@peterhashair peterhashair mentioned this pull request Jun 8, 2022
Closed
11 tasks
@sgiehl
Copy link
Member

sgiehl commented Jun 8, 2022

@bx80 using the token auth table for that might not have been the best choice, as it will require to change all the existing code, that by default when checking for tokens it will need to ignore such special tokens.

Personally I might have added two fields to the user table. invite_token and invite_expire
I'm not sure if we would need a status field at all.
If someone is invited, the generated token will be stored in invite_token, together with the expire datetime in invite_expire.
Every user where both fields are filled is in the state pending.
If the expire datetime passes, we can show expired as status.
If a user declines the request, the requirements actually defined, that the user should be removed, to avoid storing any personal data, so this status isn't needed.
If a user accepts the invite, both invite fields would be cleared and such users will have the status active.

@peterhashair
Copy link
Contributor Author

@sgiehl that makes sense, I will rework the table structure on that one.

This was referenced Jun 14, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl left review comments

@bx80 bx80 bx80 approved these changes

Assignees
No one assigned
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.11.0
Development

Successfully merging this pull request may close these issues.

Invite new users in Matomo, rather than creating them directly
4 participants
@peterhashair @sgiehl @diosmosis @bx80