Page MenuHomePhorge

Fix blocking access to some APIs
ClosedPublic

Authored by machniak on May 19 2023, 3:58 PM.
Tags
None
Referenced Files
F11885901: D4328.diff
Wed, Apr 24, 4:44 PM
Unknown Object (File)
Thu, Apr 18, 1:35 PM
Unknown Object (File)
Wed, Apr 17, 10:37 AM
Unknown Object (File)
Wed, Apr 17, 10:37 AM
Unknown Object (File)
Wed, Apr 17, 10:36 AM
Unknown Object (File)
Wed, Apr 17, 10:36 AM
Unknown Object (File)
Mon, Apr 15, 4:03 AM
Unknown Object (File)
Fri, Apr 12, 11:37 AM
Subscribers
Restricted Project

Details

Reviewers
None
Group Reviewers
Restricted Project
Commits
rKdd0613b1c65c: Fix blocking access to some APIs
Summary

Non-controller users cannot change their own config, email aliases and subscriptions.

Test Plan

./phpunit

Diff Detail

Repository
rK kolab
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

machniak created this revision.
mollekopf added inline comments.
src/app/Http/Controllers/API/V4/UsersController.php
302โ€“304

It's a pure style issue, but I really don't like using strings where it can be avoided:

This could just be written as:

$canExecuteModification = $requires_controller ? $current_user->canDelete($user) : $current_user->canUpdate($user);

or a variation thereof.

It's IMO easier to read (I know, subjective), but it also allows typecheckers/linters/autocompletion etc to function.

Obviously not a blocker.

src/app/Http/Controllers/RelationController.php
294โ€“296

This seems like an indirect check. Non-controllers cannot delete, therefore we use this to check if they can modify the config here. I think it would be better to have an explicit isController check, or at least supply a comment what we mean to be checking here.

This revision was not accepted when it landed; it landed in state Needs Review.May 23 2023, 8:49 AM
This revision was automatically updated to reflect the committed changes.