Page MenuHomePhorge

Fix blocking access to some APIs
ClosedPublic

Authored by machniak on May 19 2023, 3:58 PM.
Tags
None
Referenced Files
F12252939: D4328.id12401.diff
Sun, May 19, 4:23 PM
F12237291: D4328.id12413.diff
Sun, May 19, 3:50 AM
F12236763: D4328.id12428.diff
Sun, May 19, 2:50 AM
Unknown Object (File)
Thu, May 16, 10:27 AM
Unknown Object (File)
Wed, May 15, 2:56 AM
Unknown Object (File)
Tue, May 7, 1:29 AM
Unknown Object (File)
Sun, May 5, 12:39 AM
Unknown Object (File)
Wed, Apr 24, 4:44 PM
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
Branch
dev/non-controller-access-fix
Lint
Lint Skipped
Unit
No Test Coverage
Build Status
Buildable 42869
Build 17237: arc lint + arc unit

Event Timeline

machniak created this revision.
mollekopf added inline comments.
src/app/Http/Controllers/API/V4/UsersController.php
301

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

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.