@diosmosis opened this Pull Request on March 15th 2021 Member

Description:

Adding requirement for password confirmation for: activating a plugin, deactivating a plugin, uninstalling a plugin. And for CorePluginsAdmin.setSystemSettings.

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@flamisz commented on April 20th 2021 Contributor

Is it common for asking password as a URL parameter when we already have the token validation? Is it necessary? I can't remember using API like this.

@diosmosis commented on April 20th 2021 Member

@flamisz if you grep through API files for 'passwordConfirmation', you'll find similar methods. It is an extra security precaution for sensitive settings and changes.

Also just realized I forgot to the UI related changes, will move this back to a draft.

@diosmosis commented on April 21st 2021 Member

Added the UI changes. Also noticed we were using the old ready: event for materializecss modals, should be using onOpenEnd now.

@diosmosis commented on April 23rd 2021 Member

@sgiehl this should be ready for another review

This Pull Request was closed on April 24th 2021
Powered by GitHub Issue Mirror