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

Require password confirmation before setting/removing superuser access. #13975

Merged
merged 20 commits into from May 16, 2019
Merged

Require password confirmation before setting/removing superuser access. #13975

merged 20 commits into from May 16, 2019

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Jan 18, 2019 •
edited

Adds the confirmation to listAllAPI.twig and to userSettings.twig. There is some code redundancy between the two which I couldn't find a good way of removing.

Fixes #13711

@diosmosis diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Jan 18, 2019
@diosmosis diosmosis added this to the 3.9.0 milestone Jan 18, 2019
@diosmosis diosmosis changed the title Require password confirmation before showing token auth in UI. Require password confirmation before showing token auth in UI and setting/removing superuser access. Jan 18, 2019
@tsteur
Copy link
Member

tsteur commented Jan 18, 2019

Not sure if it's needed in listAllApi? You could also see the token in export container, or just by typing piwik.token_auth or something in the console... also by monitoring network requests etc. I reckon we would only do it in the personal settings screen for now? Or maybe we even don't do it at all until we removed it everywhere and replaced token_auth with something better?

@diosmosis
Copy link
Member Author

cc @mattab

@mattab
Copy link
Member

mattab commented Jan 21, 2019

Or maybe we even don't do it at all until we removed it everywhere and replaced token_auth with something better?

Sounds good to me 👍

better API authentication will be done later in #6559

@diosmosis
Copy link
Member Author

Closing

@diosmosis diosmosis closed this Jan 21, 2019
@diosmosis
Copy link
Member Author

Whoops forgot there was another change here

@diosmosis diosmosis reopened this Jan 21, 2019
@diosmosis
Require password confirmation for changing superuser access and fix i…
f0c9e48 
…ssue where getSiteAccess is called w/ superuser when toggling superuser access.
@diosmosis diosmosis changed the title Require password confirmation before showing token auth in UI and setting/removing superuser access. Require password confirmation before setting/removing superuser access. Jan 22, 2019
@diosmosis
Copy link
Member Author

@tsteur removed the code to require password before showing token auth. UI tests may need to be fixed.

sgiehl
sgiehl requested changes Feb 25, 2019
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.

Due to the not existing method confirming the password currently returns a 500 - internal server error. Wondering if we should handle such issues in any way. Currently the popover is closed and nothing happens. Maybe it would be good to at least display an error message?

plugins/UsersManager/API.php Outdated Show resolved Hide resolved
plugins/UsersManager/API.php Outdated Show resolved Hide resolved
@diosmosis
Copy link
Member Author

CC @tsteur to make sure the breaking API change is ok

@tsteur
Copy link
Member

tsteur commented Feb 26, 2019

This breaking change should be fine 👍

@mattab mattab modified the milestones: 3.9.0, 3.10.0 Mar 18, 2019
@tsteur
Copy link
Member

tsteur commented Apr 11, 2019 •
edited

Looks good and works 👍 .
I looked at dependencies where we use this method and I think it be good if we could maybe disable the passwordConfirmation check when we are in CLI mode? This would eg help on the cloud.

Also I think Installation\Controller::createSuperUser needs to be adjusted and some tests need to be fixed.

@diosmosis diosmosis mentioned this pull request May 13, 2019
Merged
@diosmosis diosmosis merged commit 05017ba into 3.x-dev May 16, 2019
@diosmosis diosmosis deleted the 13710-token-auth-pwd-conf branch May 16, 2019 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl requested 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
3.10.0
Development

Successfully merging this pull request may close these issues.

Require password confirmation when giving someone super user access
4 participants
@diosmosis @tsteur @mattab @sgiehl