Allow hiding public albums from general public

[qwerty287]

This is a simpler variant of the "Additional login screen" PR (https://github.com/LycheeOrg/Lychee/pull/1325) by hfr which we closed. It allows to hide every public album and photo from users not logged in.

Unfortunately, the way how to get empty result sets are very bad. For example, $query2->where('null', '=', 'never-true');. Is there a way to get empty result sets with "native" code?

[codecov[bot]]

Codecov Report

Merging #1357 (6d1e287) into master (f39be7a) will decrease coverage by 0.98%. The diff coverage is 65.78%.

[nagmat84]

Unfortunately, the way how to get empty result sets are very bad. For example, $query2->where('null', '=', 'never-true');. Is there a way to get empty result sets with "native" code?

No there is not. I haven't had a look into your PR yet, but this sounds like a code smell. If you know in advance that you don't want a result, then you shouldn't make a DB query in the first place.

[qwerty287]

You right. Then this will need some more work to prevent DB queries.

[ildyria]

You right. Then this will need some more work to prevent DB queries.

You can do it that way:

• have a middleware which check if public_visible is true & user is logged in. if not, redirect to /gallery or returns 403
• on the api/albums::get() endpoint, check if public_visible is true, if not return empty collections instead of fetching Top.

[nagmat84]

Please don't do it on middleware. This is not a job for middlewares. A middleware should do things which are useful on some general level and apply to a wider set of requests without specific knowledge about the requests, like replacing all empty strings by null values like the Laravel middleware does. (I am not sure if I like this idea in general, because '' and null typically mean different things, but it is a good toy example.) If you need to do things which are specific for a certain kind of request, then do it in the request.

Actually, @qwerty287 initial idea to adopt the queries of the AlbumAuthorizationProvider and PhotoAuthorizationProvider was completely right in theory. :+1: He only got the implementation wrong. I agree the queries are not the easiest to read and understand not to speak about changing them.

Let me rewrite one of the queries next weekend as a model example. Then it should be easy to change the other queries accordingly.

[ildyria]

Please don't do it on middleware. This is not a job for middlewares.

I somewhat beg to differ:

From Laravel doc

Middleware provide a convenient mechanism for inspecting and filtering HTTP requests entering your application. For example, Laravel includes a middleware that verifies the user of your application is authenticated. If the user is not authenticated, the middleware will redirect the user to your application's login screen. However, if the user is authenticated, the middleware will allow the request to proceed further into the application.

Doing that in a middleware provides a simpler solution to the problem. No public albums for unlogged user is for me an access level problem.

However now that I think of it, this need a bit more thinking. E.g.

• Do we want to forbid any access to unlogged users ?
• or ... Do we want to hide all the public albums to unlogged users? => IF you have a direct link to an album, can you still access it (it is "hidden" therefore not "public" anymore) ?

[qwerty287]

Currently it is completely inaccessible and not only hidden, but maybe we can add another config option for this.

[nagmat84]

@qwerty287 I pushed a suggestion for a better approach. Note, that I only adopted AlbumAuthorizationProvider. I left the other parts untouched for you. I do not guarantee that is is already 100% bug or error free. This is only a sketch. This what I did:

• I created two "null" queries: \App\Database\Query\NullQuery and \App\Models\Extension\NullModelQuery. They are "no-op" builders, i.e. if you run a query they do not invoke the DB layer but constantly return "empty" results, i.e. 0, null, [] or false depending on the method. Note, that we need two different kind of "null" queries, because the original Eloquent Query Builder and the Laravel Query Builder of the framework are not inherited from each other, so we need a null query for each of them.
• I adopted AlbumAuthorizationProvider to return these null queries where necessary. No guarantees, that the logic is 100% correct, I simply took your conditions as a starting point. Moreover, I have not stepped through the code and checked where these methods are called. Maybe the calling code needs some adoptions as well, in particular because the return type has changed.

PS: I renamed the config option no_public to restrict_public_to_auth (short for authenticated). I deem no_public as to short and incomprehensible. At the moment, we may know what we meant by it, but nobody will remember in one year.

[qwerty287]

Do you forgot to commit \App\Database\Query\NullQuery? The file isn't included here.

[nagmat84]

Do you forgot to commit \App\Database\Query\NullQuery? The file isn't included here.

Oops. Added it and pushed a new commit. :see_no_evil:

[qwerty287]

I updated the PhotoAuthorisationProvider to use the NullBuilder. This should be everything that's missing, right?

[nagmat84]

Not entirely.

I know at least two places where the invocation of the changed methods needs to be adopted. The issue is that we now also return an entire new object (the NullQuery) instead of only modifying the passed query. Keep in mind that objects are passed by reference.

Everything is fine, if the methods are used as part of a chain, i.e. like this

applyFilter($query->doSomething())->doSomethingElse()->...->get()

Here, doSomethingElse() is invoked on the result of the filter. Great.

But it does not work for code like this

$query = Model::query()->doSomething();
applyFilter($query);
$query->doSomethingElse();
$query->get();

In this case the returned object of the 2nd line is not used and line 3 uses the original query. Line 2 must be replaced by $query = applyFilter($query).

Hence, the existing code needs to be checked.

Moreover, a change like this requires extensive testing. At the moment the whole authorization thing is not well covered by our test suite.

I would also like to wait until @ildyria's PR for PhpStan is merged. This change probably introduces new type errors which need to be dealt with.

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
0.0% Duplication

[pchampio]

Can it be possible to unlock the "Additional login screen" if the unlock_password_photos_with_url_param config is on and the ?password=xxxx url param is provided.

I'll like to have this feature. That way, I can only share one link, and people don't have to input anything.

(On older version of lychee, if albums had the same password and the unlock_password_photos_with_url_param config was on, it would unlock every albums, providing a similar functionality as what you are implementing).

[nagmat84]

@pchampio I understand what you want but this has nothing to do with this PR. It is not the same nor easier, it is simply something else.

Lychee provides password-protected albums. If a browser navigates to such an album, the server sends a 404 (or 403, I forgot) response and the client presents a password dialog. The user enters the password and if the password is correct, then all albums with the same password are unlocked (not only the current album). This behavior still exist and should work.

On top of that you want the option to put the password into the URL as a query parameter such that the user does not need to enter the password manually. Understood. I believe this feature accidentally got lost during the big refactoring. Please open an independent issue for that.

[pchampio]

Oh, ok, sorry, I thought it was related.

[qwerty287]

I fixed usages of these methods now, so this is ready for review again. But as @nagmat84 suggested, we may wait for #1336 with this.

[nagmat84]

Yes, I would like to get #1336 merged first, then #1351 and also get #1326 moved to upstream first. Moreover, I want to test the changes made here extensively and not just do a code review.

I also believe to have spotted a type error in the code which mixes Database\Query and Eloquent\Query in a wrong way. But I have to check that thoroughly. That is nothing which is done quickly.

[nagmat84]

As this PR requires thorough testing to ensure that there are no regression regarding authorization, I would like to have https://github.com/LycheeOrg/Lychee/compare/use_filestreams...authorization_tests in place first.

[nagmat84]

After re-considering this PR I came to the conclusion that another approach might be even easier and more user friendly. At the moment this PR hijacks the is_public attribute and adds a global configuration which specifies whether this attribute means public for everyone (incl. unauthenticated users) or only visible to authenticated users. I am not sure whether any user will ever find this configuration option.

In the light of https://github.com/LycheeOrg/Lychee/tree/fix-1411-visibility-propagation and my idea of rewriting authentication from ground up (incl. proper group support), I would suggest the following alternative approach:

Make this possible on a per-album level. Either we add another boolean attribute to the table base_albums like is_accessible_to_authenticated or we convert the current is_public into an tiny integer with the values 0, 1 and 2 with the following semantics.

is_public (if made tri-state)	is_accessible_to_authenticated	is_public (if kept as boolean)	Remark
0	false	---	Not public for everyone; note if is_accessible_to_authenticated equals false, then is_public (as a boolean) is not considered
1	true	false	Only public for authenticated users
2	true	true	Public to everyone, incl. anonymous users, same behaviour as before

The advantages are two-fold:

• More user friendly!
• Allows per-album settings
• The changes to the queries are much smaller, no need for the null query, no need to change the way how queries are built

[ildyria]

is_accessible_to_authenticated or requires_authentication ? :) Unless you meant is_accessible_to_unauthenticated :stuck_out_tongue:

That being said, I do like this idea more. Wouldn't go for a three state though.

[nagmat84]

Why don't you like the tree-state? It makes the DB queries more easy (and probably faster), because you don't have two attributes and it avoids the problem of an undefined state.

[ildyria]

I find it makes the code a bit less readable: you need to remember what 0, 1, 2 corresponds to. That's all. That being said it would allow filtering queries such as is_public > 0 etc.

[qwerty287]

You can always define some consts for them.

[ildyria]

You can always define some consts for them.

Or enum: https://github.com/LycheeOrg/Lychee/pull/1357#issuecomment-1207200446 / https://www.php.net/manual/en/language.enumerations.php (php 8.1 from end November 2022)

[qwerty287]

Yes, but then we have to drop PHP 8.0 support.

[ildyria]

Yes, but then we have to drop PHP 8.0 support.

php 8.0 is active support until 26 Nov 2022 https://www.php.net/supported-versions.php :imp:

[nagmat84]

Please don't.

Personally, I believe we are a little bit too aggressive in our PHP policy. Even if a PHP version is not under active feature development anymore, but is still supported in the sense that it gets bug fixes and security fixes, we should try to support it unless there are good reasons to drop it (i.e. like 3rd party dependencies which only support a newer PHP version). Of course there might be exceptions, like a transition to a new major version as we did from PHP 7.4 to 8.0. But I would like to support PHP 8.0 as long as we can without extra effort.

For example, my Linux distribution is Gentoo which normally is very fast in adopting new upstream versions, because the packages are effectively compiled from upstream source. But even with Gentoo, I have to use PHP 8.0 at the moment, because the Imagick extension is not available for 8.1. Moreover, PHP 8.0 is still the recommended version for Nextcloud.

I am not saying that we should actively take care of older versions, but neither should we try to actively break them.

Dropping support for an older version just because there is a newer version is nothing which I would prefer. For me, using a higher version number has no value in itself. Sometimes I get the feeling that it is a sport for @ildyria to migrate to a higher version and drop support for lower versions merely because we can. (I also have got this feeling with respect to @ildyria's eagerness to migrate to Laravel 9.) But we also had complaints about our approach by users in the past and I would like to listen to our users.

[ildyria]

I also have got this feeling with respect to @ildyria's eagerness to migrate to Laravel 9.

I don't like being in the "security fixes only" section: https://laravel.com/docs/9.x/releases#support-policy :(

still supported in the sense that it gets bug fixes and security fixes

That is incorrect. It is only security fixes for php. No bug fixes.

Moreover, PHP 8.0 is still the recommended version for Nextcloud.

I could use the argument that Spatie on the other hand pretty much forced all their package users to move o php 8 as soon as it was released. I am not that extreme. :) And I would not drop support for php 8.0 if we don't have a release for something else at the same time.

[nagmat84]

There more I am thinking about this PR I would prefer to not follow it any further for a far more better and complete approach. As everybody knows I am in favor to replace the whole authentication logic by something which provides more features than now and this PR would make it more difficult to get there. Moreover, after #1414 is out of the door, I finally have time for it.

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 https://github.com/LycheeOrg/Lychee/issues/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.