Refactor permissions #3700
Refactor permissions #3700
Conversation
…igin/datetime-attributes
… origin/datetime-attributes
Co-authored-by: Damodar Lohani <lohanidamodar@users.noreply.github.com>
…write/appwrite into refactor-permissions-inc-console-fix
| @@ -59,13 +61,14 @@ protected function createPerPeriodMetric(string $projectId, string $metric, int | |||
|
|
|||
| // Required for billing | |||
| if ($monthly) { | |||
| $time = strtotime("first day of the month"); | |||
| $time = DateTime::createFromFormat('Y-m-d\TH:i:s.v', \date('Y-m-01\T00:00:00.000'))->format(DateTime::RFC3339); | |||
There was a problem hiding this comment.
Would have been great if we had a single consistent class to handle dates. We could also address later with @fogelito
| if (!Auth::isAppUser($roles) && !Auth::isPrivilegedUser($roles)) { | ||
| foreach (Database::PERMISSIONS as $type) { | ||
| foreach ($permissions as $permission) { | ||
| $permission = Permission::parse($permission); |
There was a problem hiding this comment.
This is OK, as its similar to the previous implementation. We could consider to have this logic available on the database library to reduce complexity in this layer. We can handle it individually later.
| "utopia-php/cache": "0.6.*", | ||
| "utopia-php/cli": "0.13.*", | ||
| "utopia-php/config": "0.2.*", | ||
| "utopia-php/database": "0.20.*", | ||
| "utopia-php/database": "dev-refactor-permissions as 0.22.0", |
There was a problem hiding this comment.
Reminder to use released version here
There was a problem hiding this comment.
I think we are already on 0.23.x 😎
There was a problem hiding this comment.
I tried to make these descriptions more concise. See if you like the suggestions and accept the comments you agree with :)
- I think "and get a full list of available permissions" is redundant after "Learn more about permissions".
- I suggested a reword for documentSecurity and fileSecurity.
| 'permissions' => [ | ||
| Permission::create(Role::any()), | ||
| Permission::read(Role::any()), | ||
| ], |
There was a problem hiding this comment.
Do empty permissions map to any ?
There was a problem hiding this comment.
Omitted individual permissions are not granted to anyone. If the permissions parameter is not passed at all, all permissions are granted to the current user. Empty array means no one has permission for anything (excluding admin/API keys)
There was a problem hiding this comment.
Empty array means no one has permission for anything (excluding admin/API keys)
I assumed so too.
If you look at the diff here. Previously the permissions were empty which means no one has access to the document. Now its been replaced with ::any which means anyone can access it. Which is the complete opposite of what it was previously. Is this expected ?
There was a problem hiding this comment.
We used to skip checking collection permissions if document-level were enabled, so having empty permissions on the collection worked in later tests like createDocument. In the new model, the collection permissions are always checked, and the document permissions are additionally checked if enabled.
So for the test referenced, we needed to add at least create permission for the current user, or no one could create any documents. I will update as such.
What does this PR do?
Test Plan
Existing tests + new tests to cover changed functionality
Related PRs and Issues
Original PR
#3668