Apply naming convention for Rights, Capabilites etc. #1511

ildyria · 2022-09-07T10:23:21Z

This PR builds on #1508 so merging this one will automatically merge the previous one.

Content:

Drops the Admin Middleware
move authorization to Gates on Requests
Increase number of Policy & granularity of Rights
creates DTO for:
- user rights
- settings rights
- photo rights
- album rights
- album protection policy
- user (for admin editing)
renames capabilities with regard to [Enhancement] Giving security attributes better names #1481
drops visibility for sharing links per album in favor of the already existing global setting.
adds stricter tests on Locale and remove extra keys in other languages than English

TODO (maybe future PR?) :

Photo protection policy ?
make use of rights in front end (currently mostly depending of admin/user can upload/nothing).

nagmat84

I appreciate the endeavor. However, this PR still mixes capabilities and rights. If we go for it (we should), we should try to get it correct this time.

app/Actions/Settings/SetLogin.php

app/DTO/UserCapabilities.php

app/Http/Controllers/Administration/SharingController.php

app/Http/Controllers/SessionController.php

app/Policies/UserPolicy.php

database/migrations/2022_09_07_141854_rename_capabilities.php

ildyria · 2022-09-07T18:41:20Z

And back to the drawing boards. 😆

codecov · 2022-09-12T07:20:53Z

Codecov Report

Merging #1511 (3b0d491) into master (f905e22) will decrease coverage by 0.42%.
The diff coverage is 89.93%.

Additional details and impacted files

ildyria · 2022-09-12T19:19:51Z

This PR is getting "a bit" bigger than I expected. Sorry @nagmat84

ildyria · 2022-09-21T20:23:55Z

I am not sure yet if this is 100% merge ready. I tried to cover quite a few of the corner cases.
The front-end still depends heavily on the global right to upload instead of per album... but I would rather do those change subsequently.

Nevertheless, as always I expect a lots of complaints of @nagmat84 so here we go for a first round of reviews. :D
Hence yes, @nagmat84, this is a review request. ;)

nagmat84

Reviewed 22 of 91 files.

app/Actions/Album/SetProtectionPolicy.php

nagmat84 · 2022-09-25T09:11:12Z

app/Actions/Albums/Top.php

@@ -64,7 +64,7 @@ public function get(): TopAlbums
 		$smartAlbums = $this->albumFactory
 			->getAllBuiltInSmartAlbums(false)
 			->map(
-				fn ($smartAlbum) => Gate::check(AlbumPolicy::IS_VISIBLE, $smartAlbum) ? $smartAlbum : null
+				fn ($smartAlbum) => Gate::check(AlbumPolicy::CAN_SEE, $smartAlbum) ? $smartAlbum : null


I need to think some time, if I like this or should hate it.

This is perfectly fine with respect to the systematics to start the name of a right with CAN.... CAN_VISIBLE obviously makes no sense and CAN_SEE is in perfect line with CAN_DOWNLOAD, CAN_ACCESS, CAN_ARCHIVE, ..., etc. So actually I should like it.

But we also have a "visibility filter" in the query policy which is strongly related to this right and hence the verb should not change. :(

nagmat84 · 2022-09-25T09:33:58Z

app/Actions/User/ResetAdmin.php

+		$user->may_edit_own_settings = true;
+		$user->may_administrate = true;


Given this two lines, I need to share a (fun) story which happened to me recently during a discussion with a CC evaluator.

In CC language "administration" (or "administrate") is nothing technical, but conducted by the operating organization. For example, the HR department administrates matters related to employees and employments. "Configuration" (or "configure") is something which happens during manufacturing or procurement. For example, a firewall may have different form factors (19" rack mountable with 48 ports, table-top small form factor with 8 ports, etc.) but still be the same product from evaluation perspective with different configurations. However, settings which are changed by a privilege user role at runtime are managed.

We also used the terms in different meanings and the evaluator was completely derailed. He was completely puzzled how a user should be able to configure settings at runtime. (We used "configure settings" instead of "manage settings"). In the end we had to change our wording. Luckily we do not plan to get a CC evaluation for Lychee, otherwise it should probably be may_self_manage and may_manage.

app/Contracts/AbstractAlbum.php

app/DTO/AlbumDTO.php

app/DTO/LycheeGitInfo.php

app/DTO/SortingCriterion.php

app/DTO/User.php

app/DTO/UserRights.php

app/DTO/Version.php

app/Http/Controllers/SessionController.php

app/Http/Livewire/Sidebar.php

app/Http/Requests/Album/SetAlbumCoverRequest.php

app/Http/Requests/Album/SetAlbumProtectionPolicyRequest.php

nagmat84

57 of 91 files.

nagmat84 · 2022-09-26T09:19:38Z

app/Http/Requests/Settings/ChangeLoginRequest.php

@@ -26,7 +26,7 @@ class ChangeLoginRequest extends BaseApiRequest implements HasPassword
 	 */
 	public function authorize(): bool
 	{
-		return Gate::check(UserPolicy::CAN_EDIT_SETTINGS, User::class);
+		return Gate::check(UserPolicy::CAN_EDIT_OWN_SETTINGS, User::class);


In order to follow the general systematics we need something like this

return Gate::check(UserPolicy::CAN_EDIT_SETTINGS, [User::class, Auth::user()]);

This might look a little bit weird on the first glance, but makes a lot of sense if one keeps in mind that the currently authenticated user has rights on some object (and this object happens to be the user itself). The UserPolicy has to look like this

class UserPolicy { public function canEditSettings(User $user, User $otherUser): void { // Bypass for administrators is granted by `before`, so we only need to deal with the case of self-editing here return $user->id === $otherUser->id; } }

nagmat84 · 2022-09-26T09:22:20Z

app/Http/Requests/User/SetEmailRequest.php

@@ -15,7 +15,7 @@ class SetEmailRequest extends BaseApiRequest

 	public function authorize(): bool
 	{
-		return Gate::check(UserPolicy::CAN_EDIT_SETTINGS, User::class);
+		return Gate::check(UserPolicy::CAN_EDIT_OWN_SETTINGS, User::class);


See some comments above. In order to stay systematic we should use

return Gate::check(UserPolicy::CAN_EDIT_SETTINGS, [User::class, Auth::user()]);

app/Locale/ChineseSimplified.php

app/Models/BaseAlbumImpl.php

app/Policies/AlbumPolicy.php

nagmat84

74 of 91 files. That is everything except for the tests.

app/Policies/AlbumPolicy.php

app/Policies/AlbumQueryPolicy.php

app/Policies/PhotoPolicy.php

app/Policies/UserPolicy.php

nagmat84 · 2022-09-26T11:55:23Z

database/migrations/2022_09_07_141854_rename_capabilities.php

+
+		// rename the column
+		Schema::table('users', function (Blueprint $table) {
+			$table->renameColumn('is_locked', 'may_edit_own_settings');


I need to check whether renaming a columns breaks foreign constraints for SQLite. I have that feeling that renaming was mimicked by deleting and re-creating the table and forgetting foreign key constraints on that way.

database/migrations/2022_09_12_175257_rename_attributes_grants.php

ildyria · 2022-09-26T19:50:38Z

Rebased on top of #1529

app/DTO/AlbumProtectionPolicy.php

ildyria · 2022-09-28T21:17:45Z

@nagmat84 I would suggest you have another look. There should not be many changes left to be done hopefully.
I tried to go in most corner cases.

ildyria · 2022-09-29T12:17:40Z

@nagmat84 I tried to cover all your comments. :)

So far, the only part missing is on CAN_EDIT_OWN_SETTINGS.

@kamil4 @qwerty287 @d7415 Enjoy the review. :)
LycheeOrg/Lychee-front#320 < also needed.

ildyria · 2022-09-29T14:26:00Z

Considering closing this, squash and open a new PR in order to have a clean history and remove resolved comments. :')

nagmat84 · 2022-09-29T16:39:52Z

Considering closing this, squash and open a new PR in order to have a clean history and remove resolved comments. :')

I don't mind either way. But in case you decide to "restart", I would not review this PR but wait until you have decided.

ildyria · 2022-09-29T16:52:25Z

Considering closing this, squash and open a new PR in order to have a clean history and remove resolved comments. :')

I don't mind either way. But in case you decide to "restart", I would not review this PR but wait until you have decided.

I'll do that then.

ildyria changed the title ~~Apply rights naming convention~~ Apply rights naming convention for Users Sep 7, 2022

ildyria marked this pull request as ready for review September 7, 2022 16:08

ildyria requested review from nagmat84 and a team September 7, 2022 16:09

ildyria added the Requires Lychee-Front Sync label Sep 7, 2022

This was referenced Sep 7, 2022

Apply rights naming convention LycheeOrg/Lychee-front#320

Merged

Ensure that Admin rights are overloading others #1508

Merged

ildyria force-pushed the apply-rights-naming-convention branch from 7d3359a to b205fbb Compare September 7, 2022 17:45

nagmat84 requested changes Sep 7, 2022

View reviewed changes

ildyria marked this pull request as draft September 7, 2022 18:41

nagmat84 mentioned this pull request Sep 9, 2022

[Enhancement] Giving security attributes better names #1481

Closed

ildyria force-pushed the apply-rights-naming-convention branch from 23278b2 to 191afa8 Compare September 12, 2022 06:44

ildyria changed the title ~~Apply rights naming convention for Users~~ Apply naming convention for Rights, Capabilites etc. Sep 12, 2022

ildyria mentioned this pull request Sep 15, 2022

Fix 1411 visibility propagation #1454

Closed

ildyria marked this pull request as ready for review September 21, 2022 20:19

ildyria requested review from nagmat84 and a team September 21, 2022 20:19

nagmat84 requested changes Sep 25, 2022

View reviewed changes

ildyria added 2 commits September 25, 2022 15:03

Arrayable DTO using reflection

e188b29

add comment

b758e39

nagmat84 reviewed Sep 26, 2022

View reviewed changes

ildyria added 3 commits September 26, 2022 16:42

with respect to comments

b59943b

phpstan

aa47c07

this seems to be php8.1 only

46756df

ildyria marked this pull request as draft September 26, 2022 19:48

WIP

16c9949

ildyria force-pushed the apply-rights-naming-convention branch from f2dce40 to 16c9949 Compare September 26, 2022 19:49

ildyria added 9 commits September 26, 2022 21:52

remove debug

1dcb617

WIP

364126e

more work

cd428f3

hopefully ready for a new round of reviews

83944d9

update front

69a5586

remove isAdmin

8af2356

with respect to comments

8817abc

merge with Master & solve conflicts

91c9c5f

isOwner helpers are private

bcc346e

ildyria marked this pull request as ready for review September 28, 2022 21:12

ildyria requested a review from nagmat84 September 28, 2022 21:12

ildyria commented Sep 28, 2022

View reviewed changes

app/DTO/AlbumProtectionPolicy.php Outdated Show resolved Hide resolved

ildyria added 6 commits September 29, 2022 10:26

fix tests

55d3cce

ensure that updates cannot be run by anyone :)

1d3a176

drop sharing link at album level

f8485db

removing ofAlbum() in favor to virtual attributes

d649c78

improve contract

97e99f7

typos

37f1836

fix 500

3b0d491

ildyria closed this Sep 29, 2022

ildyria mentioned this pull request Oct 23, 2022

Apply rights naming convention #1539

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply naming convention for Rights, Capabilites etc. #1511

Apply naming convention for Rights, Capabilites etc. #1511

ildyria commented Sep 7, 2022 •

edited

Loading

nagmat84 left a comment

ildyria commented Sep 7, 2022

codecov bot commented Sep 12, 2022 •

edited

Loading

ildyria commented Sep 12, 2022 •

edited

Loading

ildyria commented Sep 21, 2022 •

edited

Loading

nagmat84 left a comment

nagmat84 Sep 25, 2022

nagmat84 Sep 25, 2022

nagmat84 left a comment

nagmat84 Sep 26, 2022

nagmat84 Sep 26, 2022

nagmat84 left a comment

nagmat84 Sep 26, 2022

ildyria commented Sep 26, 2022

ildyria commented Sep 28, 2022

ildyria commented Sep 29, 2022

ildyria commented Sep 29, 2022

nagmat84 commented Sep 29, 2022

ildyria commented Sep 29, 2022

		$user->may_edit_own_settings = true;
		$user->may_administrate = true;

Apply naming convention for Rights, Capabilites etc. #1511

Apply naming convention for Rights, Capabilites etc. #1511

Conversation

ildyria commented Sep 7, 2022 • edited Loading

Content:

TODO (maybe future PR?) :

nagmat84 left a comment

Choose a reason for hiding this comment

ildyria commented Sep 7, 2022

codecov bot commented Sep 12, 2022 • edited Loading

Codecov Report

ildyria commented Sep 12, 2022 • edited Loading

ildyria commented Sep 21, 2022 • edited Loading

nagmat84 left a comment

Choose a reason for hiding this comment

nagmat84 Sep 25, 2022

Choose a reason for hiding this comment

nagmat84 Sep 25, 2022

Choose a reason for hiding this comment

nagmat84 left a comment

Choose a reason for hiding this comment

nagmat84 Sep 26, 2022

Choose a reason for hiding this comment

nagmat84 Sep 26, 2022

Choose a reason for hiding this comment

nagmat84 left a comment

Choose a reason for hiding this comment

nagmat84 Sep 26, 2022

Choose a reason for hiding this comment

ildyria commented Sep 26, 2022

ildyria commented Sep 28, 2022

ildyria commented Sep 29, 2022

ildyria commented Sep 29, 2022

nagmat84 commented Sep 29, 2022

ildyria commented Sep 29, 2022

ildyria commented Sep 7, 2022 •

edited

Loading

codecov bot commented Sep 12, 2022 •

edited

Loading

ildyria commented Sep 12, 2022 •

edited

Loading

ildyria commented Sep 21, 2022 •

edited

Loading