Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Change Proposal] Access Rights management: Users and Albums #1462

Closed
ildyria opened this issue Aug 16, 2022 · 17 comments · Fixed by #2035
Closed

[Change Proposal] Access Rights management: Users and Albums #1462

ildyria opened this issue Aug 16, 2022 · 17 comments · Fixed by #2035
Labels
enhancement New feature or request

Comments

@ildyria
Copy link
Member

ildyria commented Aug 16, 2022

Let's move out of the PR and discuss this here. As follows is the log of previous discussions.

TL;DR: Proposition by @nagmat84

  • move protection settings grants_full_photo, requires_link, is_downloadable, is_share_button_visible from base_albums to user_base_album
  • guest user becomes id = null
  • GUI change:
    single dialog instead of two: A imagine a tabular like dialog with one row per user with whom the album is shared (including the anonymous user) and then a tuple of check boxes in columns for the various attributes. This means it would somewhat look similar to the layout we have for the user setting page.

@nagmat84

Let me sketch what I am thinking of:

I would like to have a unified approach for what is currently called protection settings (i.e. is_public, is_downloadable, etc.) and sharing photos and albums with authenticated users. It bothers me, that we can set attributes like is_downloadable on a per-album level if the album is public, but that we use a globally defined setting for that when the album is shared.

My idea is to have these settings on a per-share level, too. On a technical level, I would like to move the attributes grants_full_photo, requires_link, is_downloadable, is_share_button_visible from base_albums to user_base_album. In order to model a public album, I would insert a share with the null user into user_base_album. This makes a lot of sense, if you consider that Auth::id also returns null for the unauthenticated user. Hence, the boolean attribute is_public would be removed completely from base_album without replacement. An album is public if and only if it is shared with the null user. I assume this would also simplify the SQL queries, because the conditions become more consistent.

On the GUI level, we would only have a single dialog instead of two: A imagine a tabular like dialog with one row per user with whom the album is shared (including the anonymous user) and then a tuple of check boxes in columns for the various attributes. This means it would somewhat look similar to the layout we have for the user setting page.

In a second step I imagine the same approach, but for user groups. You can also share albums with groups and have the same options. All users are automatically members of the null group which by definition represents the group of authenticated users.

However, this endeavor is big. I would probably start with the users in one PR and then add groups in another PR. However, if we now make is_public tri-state, then we cannot do this in two separate PRs anymore, because this would mean temporarily losing a feature which we must not do.

This approach would also prevent future questions like the issue #438 which repeatedly pops up and which also caught me by surprise during writing my many authorization tests. I think it is a bad UI experience to "hide" those fundamental settings in the global configuration panel. But I admit it is much easier than implementing a proper UI.

Recently, we also had a discussion (@kamil4 might remember) that certain boolean attributes of an album (such as is_downloadable) are only defined if the album is public, but are otherwise undefined. This makes things unnecessary complex, because we cannot use that attribute when an album is shared. Obviously, this complexity would also be gone with the proposed alternative.


@kamil4

@nagmat84 I think these are great ideas. I too have been increasingly bothered by the limited applicability of the base_album flags and the cryptic global config variables used to augment them (I don't remember anymore but I may be responsible for the downloadable one; in retrospect, that was not a good name). In a way, the GUI has already been moving in the direction you outline, what with the "Share this album with users" having been added right next to the album visibility button.

A couple of points that pop in my head:

  • It would be great to also have an is_editable or some such for sharing. Users have been asking us about it for years. It's orthogonal to what you propose and would probably be left as "future work" past the two PRs that you outlined, but at least the path to get there would be a lot more clear.
  • What about password protection? Would that be per-share as well?
  • Come to think of it, what about requires_link and is_share_button_visible? It feels like some of them make sense primarily for public (unauthenticated) mode although maybe I'm lacking imagination here and we should stay as flexible as possible...

@nagmat84

  • It would be great to also have an is_editable or some such for sharing. Users have been asking us about it for years. It's orthogonal to what you propose and would probably be left as "future work" past the two PRs that you outlined, but at least the path to get there would be a lot more clear.

Exactly my thoughts. I did not mention write access, because there are a lot of follow-up questions to be solved. For example, let's we have a is_uploadable attribute, such that user may upload to other users' albums, what happens with ownership? Currently, we only support single-ownership albums. Does this mean users lose the right to edit the photos they have just uploaded, because ownership gets transferred? If there is a general is_editable attribute, does this mean that user are also allowed to delete photos? These are all very interesting questions and I assume that we have to give up the concept of single-ownership albums in the long run.

But as you said: I would like to start with what we already support now, but re-structure it and if the architecture is clear, then it will be easier to go on from there.

  • What about password protection? Would that be per-share as well?

My mental picture was to leave that as the only per-album attribute which only affects unauthenticated users. For me that would be an extra input field in the dialog just below the table of shares which is only editable if the anonymous user is included in the sharing. IMHO, it doesn't make sense to ask authenticated users for an extra password. They have already proven their identity though their own password. If I didn't want those users to access the album, I would not have shared the album with them in the first place.

Actually, I would like to drop the password-support completely. I don't see any benefit in it, because one can always create a "dummy" user, lock the user such that no one can change its password and then share those user's credentials with the people. That would make the code so much easier in so many ways. In particular, we could drop this inefficient feature of unlocking all albums with the same password at once.

But probably, I am lacking the imagination of why this password feature is required.

  • Come to think of it, what about requires_link and is_share_button_visible? It feels like some of them make sense primarily for public (unauthenticated) mode although maybe I'm lacking imagination here and we should stay as flexible as possible...

I would keep those attributes on a per-share level.


@kamil4

This is probably not the best place for this conversation (wasn't there a dedicated issue where you outlined your ideas and that I could never find time to comment on?) but I'll bite...

let's we have a is_uploadable attribute, such that user may upload to other users' albums, what happens with ownership? Currently, we only support single-ownership albums. Does this mean users lose the right to edit the photos they have just uploaded, because ownership gets transferred? If there is a general is_editable attribute, does this mean that user are also allowed to delete photos? These are all very interesting questions and I assume that we have to give up the concept of single-ownership albums in the long run.

In the long run, sure, everything is possible. In the short run, I would personally keep single-ownership and simply treat the user that an album was shared with in read/write mode as the co-owner of that album (just like the admin is). So the ownership of everything uploaded/copied/moved to that album changes to the owner of the album. What happens when the co-owner creates a sub-album, I have no idea (to be useful, I guess that one would also have to be shared).

I'm sure there would be users dissatisfied with that though, who would like "upload only" right so that the co-owner can't delete existing photos and such.

  • What about password protection? Would that be per-share as well?

[snip]

IMHO, it doesn't make sense to ask authenticated users for an extra password. They have already proven their identity though their own password. If I didn't want those users to access the album, I would not have shared the album with them in the first place.

Good point. I'm convinced.

Actually, I would like to drop the password-support completely. I don't see any benefit in it, because one can always create a "dummy" user, lock the user such that no one can change its password and then share those user's credentials with the people. That would make the code so much easier in so many ways. In particular, we could drop this inefficient feature of unlocking all albums with the same password at once.
But probably, I am lacking the imagination of why this password feature is required.

Well, I happen to be an active user of password protected albums on my production instance, so let me give you my perspective.

First, I'm pretty sure that the feature dates back to Lychee v3, i.e., before multi-user support was added. So then that was the only option. With v4, you are right that there is an alternative.

But from users' point of view, that alternative is pretty clunky. With a password-protected album, when my parents in their seventies want to see the pictures of the grandkids, they just click on a single URL to the password-protected album and when they do, a dialog box pops up asking for a password and as soon as they do, pictures show up. With multi-user, that's currently not the case. The URL needs to be to the top-level gallery, where they first need to click on the little arrow in the top-left, provide a login and password, then click on the album that shows up. It's a lot more... friction. No wonder we had this very complaint from a user recently -- that they would like an option to at least go straight to the login/password dialog box.

@ildyria
Copy link
Member Author

ildyria commented Aug 16, 2022

I have a few restraints with regard to such change. Don't get me wrong, I think it is a way forward and Lychee definitely need improvements in that regards. However I fear that this approach would only work if you have only few users. If you have a significant number of user then the sharing table actually becomes unusable, so I am not in favor of such radical GUI change.

I do not use the password functionality so I don't really give a F about it, I tend prefer sharing hidden albums rather than password protected ones.

However I do agree with @kamil4 with regard to password protections. You can also consider the following use case as a wedding photographer: you do not want to have to create a user for each and every client you have, lock their account, assign them access rights to specified album. This is just too many clicks for what could be solved in a single dialog box.

However, I do not think creating a null user is a good idea in such table, reason being that it breaks the foreign key constraints.

In the case of a public user the following attributes:

Attribute Useful misc
is_public ✔️
requires_link ✔️
password ✔️
is_share_button_visible 💭 Why do we need that button as the user can just copy the URL...
grants_full_photo ✔️
is_downloadable ✔️
can_upload
can_move
can_delete
can_admin

In the case of logged in user:

Attribute Useful misc
is_public
requires_link
password
is_share_button_visible 💭 Why do we need that button as the user can just copy the URL...
grants_full_photo ✔️
is_downloadable ✔️
can_upload ✔️ Logged user can upload pictures to said album (also moveTo as a target)
can_move ✔️ Logged user can upload pictures from said album.
can_delete ✔️ Logged user can delete pictures from said album.
can_admin ✔️ Logged user can modify properties from said album.

Not that is_uploadable does not mean anything for albums. I recently got complaints that there was preferences about wording such as "unlocked" vs. "isUnlocked"... In this specific case, I would be strongly in favor of can_upload and can_delete where the access right is considered from the User rather than the properties of the album.

I assume this would also simplify the SQL queries, because the conditions become more consistent.

To me adding more tables where the results need to be merged per album per user is not making things "simple" in SQL.
A logged in user should still be allowed to see public albums. I fail to see how adding such tables will make things easier.

Furthermore, if we extend on access rights, I would be more in favor for an hybrid approach:

  • drop is_share_button_visible

  • is_public, requires_link, password, grants_full_photo, is_downloadable stays on the album.

  • Create a new table user_base_album_rights Which is the point of view of what the user can do rather than album has as options:

    • id,
    • album_id,
    • user_id,
    • can_download,
    • can_upload,
    • can_move,
    • can_delete,
    • can_admin,
    • has_full_photo.

    has_full_photo and can_download obviously have precedence on the album properties.

Does this mean users lose the right to edit the photos they have just uploaded, because ownership gets transferred? If there is a general is_editable attribute, does this mean that user are also allowed to delete photos? These are all very interesting questions and I assume that we have to give up the concept of single-ownership albums in the long run.

It can be seen in two ways:

  • User loses ownership and cannot modify.
  • User keeps ownership, album owner has the right to delete/move said picture. i.e. Album access rights override the photo ownership. User can delete their own pictures.
    Note that this also implies that the user will lose access to the picture if that one is moved to an album they don't have access to.

while it makes things a bit more complex, I would be in favor of the later.

@nagmat84
Copy link
Collaborator

nagmat84 commented Aug 16, 2022

For the moment, I would like to keep anything which has to do something with write access out of the discussion. I believe we all agree that this needs extra attention and a lot of detail questions needs to be solved. So in a first step I would like to concentrate on read access, i.e. what the "protection policy" dialog and the "share" dialog already allow today. We should take one step after the other.

Re @kamil4:

@nagmat84 wrote:
Actually, I would like to drop the password-support completely. [...] one can always create a "dummy" user, lock the user such that no one can change its password and then share those user's credentials with the people. [...]

@kamil4 replied:

But from users' point of view, that alternative is pretty clunky. With a password-protected album, when my parents in their seventies want to see the pictures of the grandkids, they just click on a single URL to the password-protected album and when they do, a dialog box pops up asking for a password and as soon as they do, pictures show up. With multi-user, that's currently not the case. The URL needs to be to the top-level gallery, where they first need to click on the little arrow in the top-left, provide a login and password, then click on the album that shows up. It's a lot more... friction. No wonder we had this very complaint from a user recently -- that they would like an option to at least go straight to the login/password dialog box.

Totally agreed when in comes to UX. The current situation is awful. But this has not to be this way. IMHO, it should be possible to login with username and password from any page and the frontend should automatically show a login dialog if it gets a 401 response.

I remember to have already proposed that somewhere else. I guess the current difference in UX which we currently see is due to the unfortunate history that error messages in the past used to only differentiate between "API not found" and "server error". But now, the client gets proper, parseable JSON error objects. Hence, I imagine the following: A user goes to a URL, the backend throws a 401 error, and the frontend shows the login dialog (for username+password).

This can already be implemented now and is an issue of the front-end only. This should actually be an own PR and completely independent of the matter we discuss here, because it has a great benefit for UX on its own.

Re @ildyria

However I do agree with @kamil4 with regard to password protections. You can also consider the following use case as a wedding photographer: you do not want to have to create a user for each and every client you have, lock their account, assign them access rights to specified album. This is just too many clicks for what could be solved in a single dialog box.

That was not what I meant. I was thinking of a single dummy user account which mimics the behavior of a shared password except that the user account also has a username. I am not suggesting to create one user account for each person. If one would do so, then there would also be no need to lock the account, because the account would only be used by a single person anyway.

However, I do not think creating a null user is a good idea in such table, reason being that it breaks the foreign key constraints.

Eh? No. That is exactly makes this solution so appealing. Obviously, you don't want to have a "technical dummy" user in the users table which fictitiously represents the anonymous user. Let alone, that none of the entries in the users table makes sense for the anonymous user. Hence, you define a foreign constraint on the album_user table from album_user.user_id to users.id. album_user.user_id is a nullable column, users.id is not nullable, because every regular user must have an ID. For every non-null value in album_user.user_id the foreign constraint is enforced by the DBMS and hence the ID must exist in users.id. However, for any pair (album_id, null) in album_user the foreign constraint is not enforced, because null values are exempt from foreign key constraints. This is a good thing because this allows to add (album_id, null) to album_user, without having a null entry in users.

However I fear that this approach would only work if you have only few users. If you have a significant number of user then the sharing table actually becomes unusable, so I am not in favor of such radical GUI change.

I don't take that argument, because this essentially implies that user-object ACL can never be implemented. But this is the way, how any modern file system architecture or any other multi-user system works nowadays. And this is what users expect.

Of course there are always ways how to implement such a feature in unusable ways. For example, lets take our unfortunate dialog for the album tree as an anti-role model. This dialog presents the whole album tree at once in an expanded representation without any chance to collapse sub-trees or search for specific albums by key input. Obviously, this is unusable, if there are thousands of albums. This is the way how things should not be done. Similarly, one could present an seemingly unsorted list of all users at once (sorted by some accidental DB internal key). This will surely be unusable. But again this has not to be this way. I think such a dialog would be fine, if it only shows the users with whom an album is actually shared. If one wants to add a user one has to pick this user from an extra list which must be searchable be key input.

@kamil4
Copy link
Contributor

kamil4 commented Aug 17, 2022

If you have a significant number of user then the sharing table actually becomes unusable.

I agree with @nagmat84 here. It should be perfectly fine if it starts with just one row (the "public" row); more rows are added explicitly when the owner wants to share an album with those users. On the other hand, I do like how the current dialog provides one-line clarifications underneath each option; this is something harder to duplicate in a table layout as seen with "Users", where we keep getting asked what is a "Restricted account", for instance.

You can also consider the following use case as a wedding photographer: you do not want to have to create a user for each and every client you have, lock their account, assign them access rights to specified album. This is just too many clicks for what could be solved in a single dialog box.

That's an interesting perspective in the sense that it's opposite (but complementary) to what I meant. I complained about the burden on the viewer; you on the burden on the uploader. I'm honestly not convinced by the argument about the uploader, as uploading is a one-off operation (so additional overheads are easier to accept), whereas viewing will often repeat many times. Why wouldn't I want to create an account for every client? I think that's exactly what I would want to do! But sure, it's more clicks than either hiding the album or password-protecting it.

is_share_button_visible: Why do we need that button as the user can just copy the URL...

??? That button has been there since forever, and has only just been expanded with the QR code. Now you want to remove it? What I personally don't understand is how this became a per-album config in the first place. I can see how not everybody may be interested in that button, but wouldn't a global config have been sufficient in that case? I see that it was merged in #403. The server side looks OK, but the front end is definitely acting up, hiding the button even for the album owner, which is something we never do with other related options such as is_downloadable. I would call that part of the implementation broken.

has_full_photo and can_download obviously have precedence on the album properties.

But why have this duplication between album properties and user_base_album_rights at all though? It will be confusing and error-prone.

IMHO, it should be possible to login with username and password from any page and the frontend should automatically show a login dialog if it gets a 401 response. This can already be implemented now and is an issue of the front-end only. This should actually be an own PR and completely independent of the matter we discuss here, because it has a great benefit for UX on its own.

Agreed. The 401 case should be relatively easy. There is a practical difficulty of allowing to log in from any page, in that "since forever" the space where the login button is located in the top-level albums view is instead in the album view occupied by the back button. Any suggestions how to solve it nicely without breaking everybody's finger memory in the process? I think the suggestion we've had of automatically presenting a login dialog if there are no public albums to display is also a good one.

@ildyria
Copy link
Member Author

ildyria commented Aug 17, 2022

On the other hand, I do like how the current dialog provides one-line clarifications underneath each option; this is something harder to duplicate in a table layout as seen with "Users", where we keep getting asked what is a "Restricted account", for instance.

Agreed.

Now you want to remove it?

Noooo! I am wondering if the option whether to show it or not is necessary. :)

There is a practical difficulty of allowing to log in from any page, in that "since forever" the space where the login button is located in the top-level albums view is instead in the album view occupied by the back button. Any suggestions how to solve it nicely without breaking everybody's finger memory in the process? I think the suggestion we've had of automatically presenting a login dialog if there are no public albums to display is also a good one.

You can actually hit the L key anywhere and it will make the login dialog pop. :)

@ildyria
Copy link
Member Author

ildyria commented Aug 17, 2022

Okay, I think I am starting to see the way @nagmat84 wants to implement it.
However I don't really like that we have a table user_base_album without a model attached to it and would be more in favor to add a model AlbumAccessRights or something of similar naming and make it a base_album 1 ->* album_access_right relationship.

The model AlbumAccessRights having the following attributes:

Column Guest User Comment 
id ✔️ ✔️ id the the row obviously
album_id ✔️  ✔️ id of the base album 
user_id ✔️ nullable - link towards user
requires_link ✔️   Only used in the guest user case
password ✔️  ❌  Only used in the guest user case
is_share_button_visible 💭  💭 Why do we need that button as the user can just copy the URL...
grants_full_photo ✔️  ✔️  Access to full size
is_downloadable ✔️ ✔️ While for a Photo it is not really useful, for albums there is the rationale that you do not necessarily want to allow your users to download all the pictures in one go.

✔️ & ❌ representing the usefulness of the attribute.

I also can see how this can later be extended to support write access etc.

@nagmat84 can you confirm this is what you have in mind or are we not on the same wavelength yet ?

@kamil4
Copy link
Contributor

kamil4 commented Aug 17, 2022

You can actually hit the L key anywhere and it will make the login dialog pop. :)

🤯

Is that something you added as part of U2F support? Because it's not documented at https://lycheeorg.github.io/docs/keyboard.html, and I know that back when I was updating that page, I went through the source code to make sure that all the shortcuts were covered...

Anyway, having slept on it, I think we should add the log-in button to the right of the "back" button. That way it will be there without shifting existing buttons.

@ildyria
Copy link
Member Author

ildyria commented Aug 17, 2022

Is that something you added as part of U2F support? Because it's not documented at

Don't remember, that's too old. I was just lazy to put my mouse in the top left corner ... :'D

@nagmat84
Copy link
Collaborator

Okay, I think I am starting to see the way @nagmat84 wants to implement it. However I don't really like that we have a table user_base_album without a model attached to it and would be more in favor to add a model AlbumAccessRights or something of similar naming and make it a base_album 1 ->* album_access_right relationship.

Well we already have the table user_base_album today without a model attached to it, because it implements the (m:n)-relationship between shared base albums and users. This is nothing new. Adding more attributes to it is also fine. There only two different implementation approaches you can take.

One can either consider the table user_base_album to model an attributed (m:n)-relationship. In theory, Eloquent supports that. But I don't now how much PITA an attributed (m:n)-relationship will be. In UML this gives the following picture:

+------+                         +-----------+
| User | (0..*)-----+---- (0..*) | BaseAlbum |
+------+            |            +-----------+
                    |
     +------------------------------+
     | # requires_direct_link: bool |
     | # grants_full_photo: bool    |
     | ...                          |
     +------------------------------+

Or one can break up the (n:m)-relationship into two (1:n)-relationships like this

+------+                   +------------------------------+                   +-----------+
| User | (0..1)-----(0..*) |              Share           | (0..*)-----(0..*) | BaseAlbum |
+------+                   +------------------------------+                   +-----------+
                           | # requires_direct_link: bool |
                           | # grants_full_photo: bool    |
                           | ...                          |
                           +------------------------------+

On the SQL level the different approaches will even result into isomorphic table structures. I probably would have gone for the latter approach anyway, because I fear that Laravel/Eloquent will be a PITA if we tried the first approach. Moreover, the second approach also gives @ildyria his model ;-)

The model AlbumAccessRights having the following attributes:
Column Guest User Comment
id heavy_check_mark heavy_check_mark id the the row obviously
album_id heavy_check_mark heavy_check_mark id of the base album
user_id x heavy_check_mark nullable - link towards user
requires_link heavy_check_mark x Only used in the guest user case
password heavy_check_mark x Only used in the guest user case
is_share_button_visible thought_balloon thought_balloon Why do we need that button as the user can just copy the URL...
grants_full_photo heavy_check_mark heavy_check_mark Access to full size
is_downloadable heavy_check_mark heavy_check_mark While for a Photo it is not really useful, for albums there is the rationale that you do not necessarily want to allow your users to download all the pictures in one go.

heavy_check_mark & x representing the usefulness of the attribute.

I am not 100% sure how I am supposed to interpret that table. Of course the table can only have one set of attributes for both the guest and authenticated users. That is why I would keep the password column in base_album. It does not make much sense for any other user and hence it should not be repeated for every user even if it is null "presque partout" for efficiency reasons.

I also agree on is_share_button_visible. Maybe we should really convert this into a global configuration option.

I also can see how this can later be extended to support write access etc.

@nagmat84 can you confirm this is what you have in mind or are we not on the same wavelength yet ?

Yes

@nagmat84
Copy link
Collaborator

@ildyria and @kamil4 I would like to pull out the discussion about the login dialog intro an extra issue. As I haven't found my old suggestion, I created a new enhancement issue LycheeOrg/Lychee-front#312. I guess I would start with working on that anyway, because it is rather independent of the matter discussed here and a worthy PR on its own.

@nagmat84
Copy link
Collaborator

Just a little heads-up. This is the direction into which the dialog is developing:

Screenshot_20220821_204935

At the moment it is only a hard-coded frontend UI. I took care of tooltips, such that users have a chance to understand what the symbol means and the drop-down menu at the bottom allows to add new users.

While I tried to code this UI, I remembered that I have one open PR for the modal dialog LycheeOrg/basicModal#3 and I a demo branch https://github.com/LycheeOrg/basicModal/tree/lychee_demo_revised which shows how the dialogs are going to look. In theory they should still look the same, but as some of the margin/paddings, etc. have been unified to make it a lot easier to code dialogs, the one or other dialog looks slightly different.

@ildyria
Copy link
Member Author

ildyria commented Aug 23, 2022

Back-end wise, I am convinced.
Front-end wise, I disagree with merging the two : guest user / share user.

My rationale is are follows:

  • it is a break in UI compared to what we currently have (not that it is a bad thing per say) and thus may confuse users: this complicates the simple sharing window with guest users.
  • I would believe most users of Lychee do not use the user functionalities (I may be wrong).
  • I would rather keep those separate in order to have more flexibility later when we implement write access for example.
  • just because we merge them in the back-end does not means that we need to merge them in the front-end.
  • adding more users to the shared list changes the size of the window (and thus lower the guest accessibility setting and may induce a scroll).

I am not opposed to changes, I am just trying to keep it simple and easily usable/accessible for the user.

That being said, I do like the presentation of the upper half part, I just think it must not be merged with the guest user side.
That is why I am more in favor of a sharing modal or a double tabs: one for guest, one for sharing with users.

FYI with livewire, I do plan to remove some of the multiple modals that we have. That one is one that will for sure disappear. :)

@nagmat84
Copy link
Collaborator

nagmat84 commented Aug 23, 2022

My rationale is are follows:

  • it is a break in UI compared to what we currently have (not that it is a bad thing per say) and thus may confuse users: this complicates the simple sharing window with guest users.

Au contraire. It makes matter much clearer. In particular, it is much more obvious which option affects what in which way. At the moment we have two dialogs, which options that "somehow" interact with each other in sometimes intuitive ways. Just see the discussion in #1478.

If we keep the things separate, then the share dialog will always be the third wheel, because it will always stay the unbeloved child.

  • I would believe most users of Lychee do not use the user functionalities (I may be wrong).

It is extremely obvious, that user asks for such features. Of course, user won't use them, if you keep them hidden in some obscure dialog and if you make them unusable. But that is a self-fulfilling prophecy: If a feature is not found in the first place or unintuitive/unclear to use, then user won't use it. But taking that as an argument to not improve it, will keep it in that state.

  • I would rather keep those separate in order to have more flexibility later when we implement write access for example.

We can simply add more columns at the right. At the moment there are only two columns (for download and full photo), there can be more.

  • just because we merge them in the back-end does not means that we need to merge them in the front-end.

Correct, the frontend design should not be governed by backend decisions on the relational DB model. But in the specific case at hand, the reason of merging it, is another one and is the common root cause for changing both the frontend and backend: It is simply the right way to think about it. Keeping the sharing dialog for anonymous users and for authenticated users separately, introduces an artificial boundary between both. This will always lead to problems of the kind that some feature is blindfoldly implemented for the one aspect without taking the other aspect into consideration.

  • adding more users to the shared list changes the size of the window (and thus lower the guest accessibility setting and may induce a scroll).

Nope. At the moment I intent to keep the entire dialog at a fix height and reserve, four/five rows for the body of the table. If more users are added to the table, the body but only the body of the table becomes scrollable.

I am not opposed to changes, I am just trying to keep it simple and easily usable/accessible for the user.

Agreed, but easy access in this case means that user can in particular find it and gain an understanding on how the various options work together. This isn't achieved by splitting things apart which logically depend on each other.

@kamil4
Copy link
Contributor

kamil4 commented Aug 23, 2022

I actually agree with both of you. Or, to make my position more clear, I think @ildyria's concerns are valid, but that they can (and probably should) be addressed within a single dialog, because I also agree with @nagmat84 that having all visibility settings in one place is a plus, though only if the dialog is easy to use.

The above screenshot is a little unfortunate because it doesn't show how sharing with anonymous users is to be achieved. Is there still a big "Public" slider? If not, what replaces it? Do we instead need to add some pseudo-user via the GUI? If so, I can see how that would make it more logically consistent and better optimized in terms of screen real estate usage, but perhaps it is better to keep the current style for selecting options for the anonymous users and only use the table for sharing with regular users?

Either way, I think that the options that apply to anonymous users should be the most prominent ones because I agree with @ildyria that they are used the most. So they should be on top (so that if the dialog box becomes too tall and gets a scrollbar, they are still visible by default) or, if one went with tabs instead, they should be in the tab that is displayed when the dialog box opens.

@nagmat84
Copy link
Collaborator

The above screenshot is a little unfortunate because it doesn't show how sharing with anonymous users is to be achieved. Is there still a big "Public" slider? If not, what replaces it? Do we instead need to add some pseudo-user via the GUI? If so, I can see how that would make it more logically consistent and better optimized in terms of screen real estate usage,

Exactly. The anonymous user is part the first entry in the drop-down list and the top entry of the table, if added. Sorry, that I picked an unfortunate screenshot.

The idea is that the options which are meaningful for the anonymous user and a known user are presented in the same table. Thats also why the special options for the anonymous user are reduced to two.

And I am still unsure if the the option whether a direct link is required should be an option for the anonymous user only (as it is now) or be an option for each sharing.

Either way, I think that the options that apply to anonymous users should be the most prominent ones [...]. So they should be on top

That's not possible with the approach talen here. Because to make the additional, special options at the bottom useful (and not greyed out) the use has to select the anonymous user in the table above first.

The slider for "is public" which you are missing is essentially replaced by the fact whether the anonymous user is part of the sharing table or not.

However, logically this means that the table must come top, because users have to select the anonymous user first.

(so that if the dialog box becomes too tall and gets a scrollbar, they are still visible by default)

That's why the table itself is made to have a fixed height and become scrollable, if necessary.

I will post new screenshots soon, then it will become more clearer.

@ildyria
Copy link
Member Author

ildyria commented Aug 24, 2022

That's not possible with the approach taken here. Because to make the additional, special options at the bottom useful (and not greyed out) the use has to select the anonymous user in the table above first.

However, logically this means that the table must come top, because users have to select the anonymous user first.

I strongly disagree. I believe that there should not be a fusion of anonymous user and normal user. This is the case in the background but I believe the user do not need to be aware of this.

One of the reason is that the meaning of the tick box under the table and the disabling of them is somewhat lost because no longer "attached" to the guest user.
One proof is that you had to clarify it: "These options only applies if the album is is shared with anonymous users."

The user of Lychee should (I would make it a must) not need use a drop down menu to make an album public.

But in the specific case at hand, the reason of merging it, is another one and is the common root cause for changing both the frontend and backend: It is simply the right way to think about it. Keeping the sharing dialog for anonymous users and for authenticated users separately, introduces an artificial boundary between both.

I agree, but we MUST lower the cognitive burden of the user to use the interface. Adding this virtual barrier simplifies the thoughts process when not using User. I really don't think that merging the User functionality with public sharing is the good way to go. Yes, once you understand how it works it obviously makes sense, but
there is an entry barrier which I am not comfortable to give to the user.

This will always lead to problems of the kind that some feature is blindfoldly implemented for the one aspect without taking the other aspect into consideration.

The back-end somewhat makes sure that it does not happen. :|

And I am still unsure if the the option whether a direct link is required should be an option for the anonymous user only (as it is now) or be an option for each sharing.

I don't think the direct link required is meaningful in the case of user sharing. :)

The reason why I am strongly against this merge is because:

  • Lychee is at first a single user gallery. Which I admit is not necessarily a strong argument as we expend the multiple user functionality, I still believe that the admin should be able to use Lychee while ignoring the User functionality it has: it makes it simpler for the user.
  • the functionalities for Public user and Normal user are very different: require links & password protected vs read access (for now but later: admin, edit, etc.)

@ildyria
Copy link
Member Author

ildyria commented Aug 24, 2022

Here is what I would be quite happier with.
It keeps the Public section obvious while also making the sharing part more prominent and merge the 2 in on modal.

I also added fictive option such as Upload, Delete, Manage, etc. and the inheritance parameter for easier propagation to give an idea of the direction I was thinking of.

┌──────────────────────────────────────────────────────┐
│                                                      │
│  ┌───┐                                               │
│  │o  │ :Inherit from parent                          │
│  └───┘                                               │
│------------------------------------------------------│
│  ┌───┐                                               │
│  │o  │  :Public:                                     │
│  └───┘                                               │
│   [ ]  Original                                      │
│   [ ]  Hidden                                        │
│   [ ]  Downloadable                                  │
│   [ ]  Password Protected                            │
│        Password: [____________________]              │
│------------------------------------------------------│
│                                                      │
│   Share with                                         │
│                                                      │
│                Orig  Dwnld Upld  Del   Manag         │
│   User 1       [ ]   [ ]   [ ]   [ ]   [ ]    [Del]  │
│   User 2       [ ]   [ ]   [ ]   [ ]   [ ]    [Del]  │
│  [User 3  v]   [ ]   [ ]   [ ]   [ ]   [ ]    [Add]  │
│                                                      │
│                                                      │
│------------------------------------------------------│
│  ┌───┐                                               │
│  │o  │ :Sensitive:                                   │
│  └───┘                                               │
│------------------------------------------------------│
│                                                      │
│  ┌───────────────┐          [ +Save & Refresh    v]  │
│  │ Cancel        │          [ +Save Only          ]  │
│  └───────────────┘          [ +Save & Propagate   ]  │
│                                                      │
└──────────────────────────────────────────────────────┘

@kamil4
Copy link
Contributor

kamil4 commented Aug 24, 2022

@nagmat84 What you are saying is logically correct but I share @ildyria's concerns about the UX. In my view, a successful interface should "keep simple things simple". Making an album public is one of the most common operations in Lychee, right after creating an album and uploading pictures. Folding that functionality into the user sharing UI, while logical on the face of it, feels like a step backwards in terms of ease-of-use. And we can't even fold it completely, because there are options that only apply to the anonymous users (requires_link, password) and presumably there will in the future be options that only apply to individual users (editing). These prominent exceptions make me think that we should keep them separate.

Regarding the requires_link for albums shared with users, I think that the same argument applies as you used to convince me about the password protection. The user is already authenticated and the album owner made an explicit decision to share the album with them -- there's no need for additional obfuscation/verification.

@ildyria's outline makes me concerned about the size of the resulting dialog box though. The current visibility settings box doesn't fit vertically on my 13" laptop screen; I'm honestly beginning to wonder if the new one will fit on my 27" external screen?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants