Skip to content

Consider admin defaults when creating shares - #43024

Merged
nfebe merged 3 commits into
masterfrom
42835-use-default-perms-4new-shares
Feb 1, 2024
Merged

Consider admin defaults when creating shares#43024
nfebe merged 3 commits into
masterfrom
42835-use-default-perms-4new-shares

Conversation

@nfebe

Copy link
Copy Markdown
Contributor

The current share logic always uses the default BUNDLED_PERMISSIONS.ALL which includes everything. This commit updates share creation logic to use defaultPermissions if set by admin for the creation of new shares.

Resolves: #42835

@nfebe
nfebeforce-pushed the 42835-use-default-perms-4new-shares branch 2 times, most recently from f09aea3 to e8ff76fCompareJanuary 22, 2024 17:39

@nfebenfebe left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default permissions might not always be same as "Allow editing" (or basically all permissions)

Screenshot from 2024-01-23 12-27-32

Comment threadapps/files_sharing/src/views/SharingDetailsTab.vue Outdated
@nfebe

Copy link
Copy Markdown
ContributorAuthor

Something else that has come up while working on this in relation to maintaining "Allow editing" and "Default permissions"

  • It turns out when no permissions are set on the admin, the default permission is "1" which is ready only. So changing "Allow editing" to "Default permissions" which has the probability to be 1, most of the time would make little sense with read only just at the top, that is, "Read only" (first option), second option (Default permissions, still read only)

Screenshot from 2024-01-25 11-29-58

So, the brainstorming is not over it seems, the second path, is simply to add "Default permissions" as a new option IF it is different from READ only (1) as well as different from "Allow editing" (31)

@nfebe

Copy link
Copy Markdown
ContributorAuthor

cc: @jancborchardt

@jancborchardt

Copy link
Copy Markdown
Member

As far as I can see, the issue reported at #42835 should be fixed via code and does not need a new entry in this list of presets.
These are merely default share permissions set by the admin and that’s that.

  • If the permissions set by the admin are the same as "View only", preselect that
  • If the permissions set by the admin are the same as "Allow editing", preselect that
  • If the permissions set by the admin are anything else, preselect "Custom permissions" with the relevant subline

But we should not replace an entry of those with "Default permissions" or anything like that.

@nfebe

Copy link
Copy Markdown
ContributorAuthor

As far as I can see, the issue reported at #42835 should be fixed via code and does not need a new entry in this list of presets.
These are merely default share permissions set by the admin and that’s that.

Sure, it does not necessarily need a new entry in the list of presets. But definitely needs making changes to how the presets work to avoid confusion. Adding another option or updating language are just other solutions paths suggested.

So the new implication is that "Allow editing" is not always the selected/default option as is the case now. It would be dynamically selected.

The only difference between this and my last suggestion is that no new entry would be added. To mitigate the issue of clearly understanding what default permissions are, I think it would be nice to have a way for the user to know that whatever permission was pres-elected for them is the default. This can be done by simply adding something like Default in parenthesis. (Default)

@nfebe
nfebeforce-pushed the 42835-use-default-perms-4new-shares branch 2 times, most recently from 8e3a248 to db03ac1CompareJanuary 30, 2024 23:00
@nfebe
nfebe marked this pull request as ready for review January 30, 2024 23:02
Comment threadapps/files_sharing/lib/Listener/LoadSidebarListener.php Outdated
@nfebe
nfebeforce-pushed the 42835-use-default-perms-4new-shares branch 3 times, most recently from 4a4c59c to 388fd55CompareJanuary 31, 2024 20:08
@nfebe
nfebe requested a review from skjnldsvJanuary 31, 2024 20:08
@nfebe
nfebeforce-pushed the 42835-use-default-perms-4new-shares branch 2 times, most recently from eed249c to 0662682CompareJanuary 31, 2024 20:34
@nfebe
nfebeforce-pushed the 42835-use-default-perms-4new-shares branch from 0662682 to cd58945CompareFebruary 1, 2024 14:05
@nfebe

nfebe commented Feb 1, 2024

Copy link
Copy Markdown
ContributorAuthor

/compile /

@susnux

Copy link
Copy Markdown
Contributor

/compile

CI is a bit slow at the moment due to 28.0.2 release I guess, you could short cut by compiling locally and submit the assets.

@nfebe
nfebe enabled auto-merge February 1, 2024 14:36
The current share logic always uses the default `BUNDLED_PERMISSIONS.ALL`
which includes everything.
This commit updates share creation logic to use `defaultPermissions` if set
by admin for the creation of new shares.
Signed-off-by: fenn-cs <fenn25.fn@gmail.com>
- Remove redundant initial state added
- Call `getCapabilities()` in share config file.
Signed-off-by: fenn-cs <fenn25.fn@gmail.com>
@nfebe
nfebeforce-pushed the 42835-use-default-perms-4new-shares branch from bde1408 to 9cff27bCompareFebruary 1, 2024 16:53
@nfebe

nfebe commented Feb 1, 2024

Copy link
Copy Markdown
ContributorAuthor

/compile amend /

Signed-off-by: fenn-cs <fenn25.fn@gmail.com>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@nextcloud-command
nextcloud-commandforce-pushed the 42835-use-default-perms-4new-shares branch from 9cff27b to f944d26CompareFebruary 1, 2024 18:25
@nfebe
nfebe disabled auto-merge February 1, 2024 23:42
@nfebe
nfebe merged commit c5c4840 into masterFeb 1, 2024
@nfebe
nfebe deleted the 42835-use-default-perms-4new-shares branch February 1, 2024 23:43
@nfebe

nfebe commented Feb 1, 2024

Copy link
Copy Markdown
ContributorAuthor

PHP unit failures unrelated.

@nfebe

nfebe commented Feb 1, 2024

Copy link
Copy Markdown
ContributorAuthor

/backport to stable28

@nfebe

nfebe commented Feb 1, 2024

Copy link
Copy Markdown
ContributorAuthor

/backport to stable27

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Creating new shares doesn't use default admin sharing permissions

8 participants

@nfebe@jancborchardt@susnux@JSoko@artonge@skjnldsv@sorbaugh@emoral435