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.

Sign up for GitHub

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 Write permission for adding annotations through API #22605

Draft
wants to merge 3 commits into
base: 5.x-dev
Choose a base branch
from
Draft

Require Write permission for adding annotations through API #22605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be6feb5
Require Write permission for adding annotations through API
sgiehl Sep 19, 2024
dbcee0f
Add changelog
sgiehl Sep 19, 2024
0f109ab
adjust tests
sgiehl Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@ This is the Developer Changelog for Matomo platform developers. All changes in o

The Product Changelog at **[matomo.org/changelog](https://matomo.org/changelog)** lets you see more details about any Matomo release, such as the list of new guides and FAQs, security fixes, and links to all closed issues.

## Matomo 6.0.0

### Breaking Changes

* The API methods `Annotations.add` will now require `Write` permission, instead of `View` permission.

## Matomo 5.2.0

### Breaking Changes
Expand Down
2 changes: 1 addition & 1 deletion plugins/Annotations/API.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ private function checkUserCanModifyOrDelete($annotation): void
*/
private static function checkUserCanAddNotesFor($idSite): void
{
if (!Piwik::isUserHasViewAccess($idSite) || Piwik::isUserIsAnonymous()) {
if (!AnnotationList::canUserAddNotesFor($idSite)) {
throw new Exception("The current user is not allowed to add notes for site #$idSite.");
}
}
Expand Down
91 changes: 61 additions & 30 deletions plugins/Annotations/tests/System/AnnotationsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,51 +232,82 @@ public function testDeleteSuccess()
API::getInstance()->get(self::$fixture->idSite1, 1);
}

public function getPermissionsFailData()
public function getPermissionsChecks(): iterable
{
return array(
// getAll
array(false, false, "module=API&method=Annotations.getAll&idSite=1&date=2012-01-01&period=year", true, "getAll should throw if user does not have view access"),
yield 'Annotations.getAll should throw if user does not have view access' => [
null, 'module=API&method=Annotations.getAll&idSite=1&date=2012-01-01&period=year', true
];

// get
array(false, false, "module=API&method=Annotations.get&idSite=1&idNote=0", true, "get should throw if user does not have view access"),
yield 'Annotations.get should throw if user does not have view access' => [
null, 'module=API&method=Annotations.get&idSite=1&idNote=0', true
];

// getAnnotationCountForDates
array(false, false, "module=API&method=Annotations.getAnnotationCountForDates&idSite=1&date=2012-01-01&period=year", true, "getAnnotationCountForDates should throw if user does not have view access"),

// add
array(false, false, "module=API&method=Annotations.add&idSite=1&date=2011-02-01&note=whatever", true, "add should throw if user does not have view access"),
array(false, true, "module=API&method=Annotations.add&idSite=1&date=2011-02-01&note=whatever2", false, "add should not throw if user has view access"),
array(true, true, "module=API&method=Annotations.add&idSite=1&date=2011-02-01&note=whatever3", false, "add should not throw if user has admin access"),

// save
array(false, false, "module=API&method=Annotations.save&idSite=1&idNote=0&date=2011-03-01&note=newnote", true, "save should throw if user does not have view access"),
array(false, true, "module=API&method=Annotations.save&idSite=1&idNote=0&date=2011-03-01&note=newnote", true, "save should throw if user has view access but did not edit note"),
array(true, true, "module=API&method=Annotations.save&idSite=1&idNote=0&date=2011-03-01&note=newnote", false, "save should not throw if user has admin access"),

// delete
array(false, false, "module=API&method=Annotations.delete&idSite=1&idNote=0", true, "delete should throw if user does not have view access"),
array(false, true, "module=API&method=Annotations.delete&idSite=1&idNote=0", true, "delete should throw if user does not have view access"),
array(true, true, "module=API&method=Annotations.delete&idSite=1&idNote=0", false, "delete should not throw if user has admin access"),
);
yield 'Annotations.getAnnotationCountForDates should throw if user does not have view access' => [
null, 'module=API&method=Annotations.getAnnotationCountForDates&idSite=1&date=2012-01-01&period=year', true
];

yield 'Annotations.add should throw if user has view access' => [
'view', 'module=API&method=Annotations.add&idSite=1&date=2011-02-01&note=whatever', true
];

yield 'Annotations.add should not throw if user has write access' => [
'write', 'module=API&method=Annotations.add&idSite=1&date=2011-02-01&note=whatever', false
];

yield 'Annotations.add should not throw if user has admin access' => [
'admin', 'module=API&method=Annotations.add&idSite=1&date=2011-02-01&note=whatever', false
];

yield 'Annotations.save should throw if user does not have view access' => [
null, 'module=API&method=Annotations.save&idSite=1&idNote=0&date=2011-03-01&note=newnote', true
];

yield 'Annotations.save should throw if user has view access but did not edit note' => [
'view', 'module=API&method=Annotations.save&idSite=1&idNote=0&date=2011-03-01&note=newnote', true
];

yield 'Annotations.save should not throw if user has write access' => [
'write', 'module=API&method=Annotations.save&idSite=1&idNote=0&date=2011-03-01&note=newnote', false
];

yield 'Annotations.save should not throw if user has admin access' => [
'admin', 'module=API&method=Annotations.save&idSite=1&idNote=0&date=2011-03-01&note=newnote', false
];

yield 'Annotations.delete should throw if user does not have view access' => [
null, 'module=API&method=Annotations.delete&idSite=1&idNote=0', true
];

yield 'Annotations.delete should throw if user has view access but did not edit note' => [
'view', 'module=API&method=Annotations.delete&idSite=1&idNote=0', true
];

yield 'Annotations.delete should not throw if user has write access' => [
'write', 'module=API&method=Annotations.delete&idSite=1&idNote=0', false
];

yield 'Annotations.delete should not throw if user has admin access' => [
'admin', 'module=API&method=Annotations.delete&idSite=1&idNote=2', false
];
}

/**
* @dataProvider getPermissionsFailData
* @dataProvider getPermissionsChecks
*/
public function testMethodPermissions($hasAdminAccess, $hasViewAccess, $request, $checkException, $failMessage)
public function testMethodPermissions($permissionLevel, $request, $shouldThrowException)
{
if (true === $checkException) {
if (true === $shouldThrowException) {
self::expectException(Exception::class);
} else {
self::expectNotToPerformAssertions();
}

// create fake access that denies user access
FakeAccess::clearAccess(false);
FakeAccess::$identity = 'user' . (int)$hasAdminAccess . (int)$hasViewAccess;
FakeAccess::$idSitesAdmin = $hasAdminAccess ? array(self::$fixture->idSite1) : array();
FakeAccess::$idSitesView = $hasViewAccess ? array(self::$fixture->idSite1) : array();
FakeAccess::$identity = 'user' . $permissionLevel;
FakeAccess::$idSitesAdmin = $permissionLevel === 'admin' ? array(self::$fixture->idSite1) : [];
FakeAccess::$idSitesWrite = $permissionLevel === 'write' ? array(self::$fixture->idSite1) : [];
FakeAccess::$idSitesView = $permissionLevel === 'view' ? array(self::$fixture->idSite1) : [];

$request = new Request($request . '&format=original');
$request->process();
Expand Down
Loading