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

Check websites existence for super-admin #14408

Closed
Valair opened this issue May 3, 2019 · 1 comment
Closed

Check websites existence for super-admin #14408

Valair opened this issue May 3, 2019 · 1 comment
Labels
duplicate For issues that already existed in our issue tracker and were reported previously.

Comments

@Valair
Copy link

Valair commented May 3, 2019

This is related to this issue #8697, but with wider implications.

Steps to reproduce: the same as the previous issue, but it works even with plugins (I'm currently working on extending the DisableTracking for example.
Results: if a call to API with not existing idSites is made using super-admin auth token, there is no error message for not existing IDs, as the permission check successfully complete, and when a website ID with that ID is created, all the options are applied.
Expected result: it should not be possible, even for super-admin to use not existing IDs and an exception should be raised.

I think to have tracked the cause of this behaviour on how the access check is made in Piwik::Access class; if the user is a super-admin, all Piwik::Access.checkUserHas*Access($idSites) return as $this->hasSuperUserAccess(), without checking the given IDs.

@tsteur
Copy link
Member

tsteur commented May 5, 2019

@Valair I'll mark this as a duplicate of #8697 as it's pretty much the same issue / root cause. Indeed removing eg changing

    public function checkUserHasViewAccess($idSites)
    {
        if ($this->hasSuperUserAccess()) {
            return;
        }
  . ..

to

    public function checkUserHasViewAccess($idSites)
    {
  ...

should work. The only possible big problem that may happen is that quite a few automated tests may fail (integration tests, system tests) and we would need to adjust them to ensure the sites exist. wouldn't be surprised if some tests only worked due to this bug. We would just remove those lines with if ($this->hasSuperUserAccess()) { and see if many tests fail. Would you be keen to create a PR?

@tsteur tsteur closed this as completed May 5, 2019
@tsteur tsteur added the duplicate For issues that already existed in our issue tracker and were reported previously. label May 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate For issues that already existed in our issue tracker and were reported previously.
Projects
None yet
Development

No branches or pull requests

2 participants