Use Laravel Auth facade.

[ildyria]

~This is preliminary work for some refactoring later removing the need of SessionFunctions in favor of the Auth facade.~ Remove the AccessControl.php facade in favor of the Auth:: and Gate:: Laravel facades ~~App/Auth/Authorization.php which is an extensions of Laravel Auth facade`~~.

Rationale of using Auth

Auth is unfortunately necessary for the use of https://github.com/Laragear/WebAuthn as it is used in the middleware provided. I would rather avoid to have a clone version of WebAuthn adapted to our need as I previously did for Larapass as it keeps us out of the loop for updates etc.

As for the reason to use Laragear/WebAuthn instead of Larapass: the later is abandoned. As a result in order to upgrade to Laravel 9.0, we need to support Laragear/WebAuthn. Do note that this will also enable us to use Safe functions 2.2 instead of locked at 1.3.

I know that @nagmat84 intends to rewrite the full Auth section. I would also like to point out that https://laravel.com/docs/9.x/passport would be an interesting addition in order to support Oauth2 in the future (and make some of our users happy in that regards).

With regard to Auth: https://laravel.com/docs/9.x/authentication With regard to Gate: https://laravel.com/docs/9.x/authorization

[ildyria]

Still need to fix the tests obviously.

[codecov[bot]]

Codecov Report

Merging #1403 (2a38942) into master (953050c) will decrease coverage by 0.51%. The diff coverage is 89.22%.

[ildyria]

I went for the quick&dirty solution as I would like to not take too much of your time with heavy code refactoring. I would say though that we now make use of Auth instead of Sesssion::get('user') and Session::get('id'). Which is a step forward in that regard.

[ildyria]

I really not to test that thoroughly

Fortunately, there is an extensive test suite in that regard see UsersTests.php

[ildyria]

https://github.com/LycheeOrg/Lychee/pull/1403/commits/6a8e9bba9ac54ba793c88b73542a6fe9f9202f7a : was was needed or the test under with $userId === null was useless.

[ildyria]

@nagmat84 don't hate me too much. I pretty much nuked all the logic out of Authorization. :'D I strongly suggest to read through: https://github.com/LycheeOrg/Lychee/pull/1403/commits/3f54e8eea32683ff41f6c033d20e9ebb1541021e The changes are pretty much search&replace.

[nagmat84]

Has anybody already checked whether the exception which are thrown by policies, guards and authenticator interact nicely with the exception handler such that the correct HTTP codes (i.e. 401 and 403) are returned. That also something which is on my list of tests.

[ildyria]

Has anybody already checked whether the exception which are thrown by policies, guards and authenticator interact nicely with the exception handler such that the correct HTTP codes (i.e. 401 and 403) are returned. That also something which is on my list of tests.

They mostly throw 403 Authorization Exceptions. However they also sometimes do redirect to a login page (if the user is not logged in) and try to be smarter than they should be. I haven't really dug much in it as I use the boolean check functions. :)

[nagmat84]

I will go over this entire PR once again the next weekend. :monocle_face:

But there are already two things I would like to throw into the discussion. The changes a trivial and has more to do with organizing things:

• AlbumAuthorizationProvider and PhotoAuthorizationProvider: I am fine that @ildyria moved all the methods which operate on hydrated models out of these files and moved them into independent files AlbumPolicy and PhotoPolicy, but left all query related methods in AlbumAuthorizationProvider and PhotoAuthorizationProvider (see https://github.com/LycheeOrg/Lychee/pull/1403#discussion_r931043851). I can go with this separation. However, these methods are strongly coupled, for example the methods isVisible($photo) and appendVisibilityFilter($photoQuery) essentially contain the same logic, except that the first one uses attributes which have already been fetched from the DB while the latter does the same thing on the SQL layer before the model is fetched. Having both methods next to each other in the same file increased the chance that whenever one method is changed the other may be, too. Keeping the logic of the methods consistent is crucial or funny things may happen. This connection is now lost, because the methods are moved to different files. The connection is even more lost, because the files are kept in completely different directories of the project. On the one hand we have app/Auth/AlbumAuthorizationProvider; on the other hand we have app/Auth/AlbumPolicy (same holds for photos). I would therefore advocate to rename AlbumAuthorizationProvider into AlbumQueryPolicy and move the file into app/Policies as well (same for photos). I know that this is not a policy in the Laravel sense (because it operates on queries and not on models), but keeping the files next to each other and giving them similar names may help maintainability.

• Currently, we have

  • app/Legacy/Legacy::loginAsAdmin()
  • app/Auth/Authorization::isAdminNotRegistered()
  • app/Auth/Authorization::loginAsAdminIfNotRegistered()

  In particular, the latter two methods are the only ones of Auth/Authorization. All three methods are somehow legagy. I would therefore pledge to move them to

  • app/Legacy/AdminAuthentication::loginAsAdmin()
  • app/Legacy/AdminAuthentication::isAdminNotRegistered()
  • app/Legacy/AdminAuthentication::loginAsAdminIfNotRegistered()

  In particular, the latter two methods are the only ones of Auth/Authorization. These are my reasons

  • In combination with the very first bullet point the additional directory Auth is removed from the root level. Personally, I believe that the number of directories is already too large and that things could be grouped better. Removing the directory Auth will at least not make things worse in this regard.
  • app/Legacy/Legacy.php always bothered me. Having a file named Legacy in a directory named Legacy feels like duplication.
  • Putting all three methods in the same class keeps them together. They are all three required for the similar reasons. (Strange handling of the admin account since Lychee v3.)

[ildyria]

• I would therefore advocate to rename AlbumAuthorizationProvider into AlbumQueryPolicy and move the file into app/Policies as well (same for photos). I know that this is not a policy in the Laravel sense (because it operates on queries and not on models), but keeping the files next to each other and giving them similar names may help maintainability.

Sure.

In particular, the latter two methods are the only ones of Auth/Authorization. All three methods are somehow legagy. I would therefore pledge to move them to

• app/Legacy/AdminAuthentication::loginAsAdmin()
• app/Legacy/AdminAuthentication::isAdminNotRegistered()
• app/Legacy/AdminAuthentication::loginAsAdminIfNotRegistered()

Also make sense to me.

[ildyria]

Why not simply throw new UnauthenticatedException(); and use the default message 'User is not authenticated'? I am not sure if I like this new message. "Id cannot be null" is simply a technical detail which happens to be equivalent to the fact that no user is authenticated.

In particular, keep in mind that this is the message which will show up in the logs and in the message section on the top of the site of the frontend. At least the message should be "User ID cannot be null", because every user will wonder what ID the message refers to when the user only sees the message on the frontend and has no access to the log. But I deem the default message 'User is not authenticated' much more telling.

Fixed.

[ildyria]

I would call this change in behavior surprising, because I assume users always expect to be able to do everything with photos (and album) owned by themselves.

agreed.

I need to think about it again. I didn't consider this edge case.

[nagmat84]

as far as I see it, the underlying problem is that previously the code distinguished between isDownloadable and isArchiveable.

I didn't realize that we had two separate methods like that. Can you explain the difference? I always thought that the server used the term "archive" and the front end "download" but that they were referring to the same thing?

What's the difference between downloading and archiving a photo?

[nagmat84]

@ildyria: You need to apply the bug fix of https://github.com/LycheeOrg/Lychee/pull/1430 to PhotoPolicy::visible as well.

[kamil4]

Let me repeat a comment I made yesterday as for some unfathomable reason it shows up as an edit of @nagmat84's comment rather than my response. Here it is once again:

as far as I see it, the underlying problem is that previously the code distinguished between isDownloadable and isArchiveable.

I didn't realize that we had two separate methods like that. Can you explain the difference? I always thought that the server used the term "archive" and the front end "download" but that they were referring to the same thing?

What's the difference between downloading and archiving a photo?

[nagmat84]

as far as I see it, the underlying problem is that previously the code distinguished between isDownloadable and isArchiveable.

I didn't realize that we had two separate methods like that. Can you explain the difference? I always thought that the server used the term "archive" and the front end "download" but that they were referring to the same thing?

What's the difference between downloading and archiving a photo?

A very good question. I guess we can agree that there shouldn't be two different methods for that.:

• The former is used when a user downloads a ZIP archive of individual photos, i.e. for the request Photo::getArchive.
• The latter is used when a user downloads a ZIP archive of albums, i.e. for the request Album::getArchive, and when the handling action needs to decide whether a photo inside an album may be downloaded.

Note: The problematic code which I reviewed was for the handling action of Album::getArchive (see https://github.com/LycheeOrg/Lychee/pull/1403#discussion_r933784489), while the code snippet from 4.4 which you posted was for Photo::getArchive (see https://github.com/LycheeOrg/Lychee/pull/1403#discussion_r933807527).

Downloadable photos within Photo::getArchive

This is PhotoAuthorizationProvider::isDownloadable

https://github.com/LycheeOrg/Lychee/blob/cd394af664e065fde79dc895a70803cf66655efd/app/Actions/PhotoAuthorisationProvider.php#L121-L131

unless I haven't missed something there is only a single call trace which leads to it. It is only indirectly called by ArchivePhotosRequest::authorize

Archivable photos within Album::getArchive

We do not have a dedicated method for that, but this is the relevant code

https://github.com/LycheeOrg/Lychee/blob/cd394af664e065fde79dc895a70803cf66655efd/app/Actions/Album/Archive.php#L179-L185

Summary

The decision logic is slightly different:

• Visibility check: The first also checks whether the photo is visible. This isn't necessary for the latter, because Album::getArchive only iterates over those albums which are accessible and hence the photo inside is visible.
• Fallback for unsorted photos: Here we have a different which probably should not be there. Inside Photos::getArchive we fallback to the global configuration option for unsorted photos. Inside Album::getArchive we fallback to the setting of the smart or tag album instead. Probably, the latter is wrong, but I am very sure that this has always been the case even before my refactoring.

However, within Photos::getArchive as well as Album::getArchive a photo gets a pass if is_current_user_or_admin returns true, i.e. if the current user owns the photo.

Conclusion

After @ildyria refactoring PhotoPolicy::download does not check wether the current user is the owner. As I already wrote in https://github.com/LycheeOrg/Lychee/pull/1403#discussion_r933800576 it is not sufficient that PhotoPolicy::visible performs an owner check, because PhotoPolicy::download only calls PhotoPolicy::visible to bail out early. I believe, it should be sufficient, if @ildyria adds another check for ownership directly to PhotoPolicy::download itself. Then we should be fine.

However, I wonder if the check for ownership can simply be done in PhotoPolicy::before together with the admin check. I cannot think of a possible use case where an owner of a photo is denied any action on this photo.

[nagmat84]

Github is mixing up posts again. Please read

1. https://github.com/LycheeOrg/Lychee/pull/1403#issuecomment-1200443624 by @kamil4
2. https://github.com/LycheeOrg/Lychee/pull/1403#issuecomment-1200456451 by me
3. https://github.com/LycheeOrg/Lychee/pull/1403#discussion_r934004802 by @kamil4

to make sense of the flow of conversation.

@kamil4 wrote:

So I agree with @nagmat84 that the old (4.4.0) behavior was different. However, it was different due to a bug. [...]

There is one more thing though that bothers me about the quoted excerpts in this thread. It looks like we're now consulting album's is_downloadable without checking first if the album is public. I believe that's wrong. is_downloadable can be set for public albums only, so its value is only valid in that case. For non-public albums, we should be checking other criteria (either the current user is the owner or the album is shared with them -- in the latter case, we need to consult the global config downloadable).

Luckily not. The point is that the photo must be visible in the first place to be downloadable at all. PhotoAutorisationProvider::isDownloadable

https://github.com/LycheeOrg/Lychee/blob/cd394af664e065fde79dc895a70803cf66655efd/app/Actions/PhotoAuthorisationProvider.php#L121-L131

calls PhotoAutorisationProvider::isVisible and bails out if not. PhotoAuthorisationProvider::isVisible

https://github.com/LycheeOrg/Lychee/blob/cd394af664e065fde79dc895a70803cf66655efd/app/Actions/PhotoAuthorisationProvider.php#L83-L97

checks among other things AlbumAuthorisationProvider::isAccessible. An album is accessible for the very reasons which @kamil4 mentioned above, i.e. the album must be public, shared, owned by the user, etc.

So we should be fine here.

[kamil4]

Re: https://github.com/LycheeOrg/Lychee/pull/1403#issuecomment-1200461760

I'm still unconvinced. Looking at the lines you already quoted: https://github.com/LycheeOrg/Lychee/blob/cd394af664e065fde79dc895a70803cf66655efd/app/Actions/PhotoAuthorisationProvider.php#L121-L131

Imagine a public photo in a private album. It will pass the visibility test via https://github.com/LycheeOrg/Lychee/blob/cd394af664e065fde79dc895a70803cf66655efd/app/Actions/PhotoAuthorisationProvider.php#L93

But then we will test its downloadability using https://github.com/LycheeOrg/Lychee/blob/cd394af664e065fde79dc895a70803cf66655efd/app/Actions/PhotoAuthorisationProvider.php#L129

That's what I'm concerned about. We are reading the value of is_downloadable without verifying that the album in question has is_public set. In the example I brought up, it won't. Unless things have changed while I wasn't paying attention, the value of is_downloadable is effectively undefined for albums that don't have is_public = 1. Same with grants_full_photo and is_share_button_visible. For such photos, we should be consulting the global config variables. In 4.4.0, we used to ensure that we are doing the right thing via the following access method: https://github.com/LycheeOrg/Lychee/blob/5b51765c7c66e6542ff2a6dcd3fb57393ff16717/app/Models/Extensions/AlbumBooleans.php#L38-L45

We don't do it anymore. This looks like a bug to me.

[nagmat84]

We still do. See here and the lines above and below:

https://github.com/LycheeOrg/Lychee/blob/cd394af664e065fde79dc895a70803cf66655efd/app/Models/BaseAlbumImpl.php#L235

This is Laravel magic. If you call an attribute like is_public (with snake case), Laravel intercepts that, converts it into camel case, puts a get in front as well as an Attribute at the end of it and looks around if it finds a callable method getIsPublicAttribute. In this case the methods are defined in BaseAlbumImpl, because they are used for regular albums and tag albums.

[ildyria]

I still need to rename the Policies method and I hope it should be good. :)

[kamil4]

This is Laravel magic. If you call an attribute like is_public (with snake case), Laravel intercepts that, converts it into camel case, puts a get in front as well as an Attribute at the end of it and looks around if it finds a callable method getIsPublicAttribute.

Oh, right. I totally forgot about that...

[ildyria]

However, I wonder if the check for ownership can simply be done in PhotoPolicy::before together with the admin check. I cannot think of a possible use case where an owner of a photo is denied any action on this photo.

Unfortunately no. The before can only apply checks on User as far as I know. :/

[ildyria]

While I tried to refactor the authorizeAlbumWrite, I realize that it was a lot of copy paste. I am wondering if it would not be better to create multiple authorization trait (in the same idea of HasPhotoTrait etc...) with the authorization method declared in them e.g.:

AuthorizeCanEditAlbumTrait.php

/**
* {@inheritDoc}
*/
public function authorize(): bool
{
    return $this->authorizeAlbumWrite($this->album);
}

AuthorizeCanEditAlbumsTrait.php

/**
* {@inheritDoc}
*/
public function authorize(): bool
{
    return $this->authorizeAlbumsWrite($this->albums);
}

Idem for the write access for the photo. Obviously the goal of such refactoring is to remove the authorizeAlbumsWrite etc. I just kept it here for simplicity.

This would allow use to uniformize the access rights rather than having to duplicate code.

@nagmat84 @kamil4 @d7415 @qwerty287 opinion ?

I know that @nagmat84 removed quite a few Traits from previous Lychee architecture, so I would rather avoid making the same mistakes again.

TL;DR: should we use traits for the different implementation of authorize() when used in similar fashion at multiple places? :)

I also considered using inheritance e.g. : AbstractCanEditAlbumRequest.php but I would get the feeling it adds yet another layer of complexity to the code and makes it hard to track what is actually being applied.

[nagmat84]

However, I wonder if the check for ownership can simply be done in PhotoPolicy::before together with the admin check. I cannot think of a possible use case where an owner of a photo is denied any action on this photo.

Unfortunately no. The before can only apply checks on User as far as I know. :/

Good point. Haven't thought about that.

[nagmat84]

Idem for the write access for the photo. Obviously the goal of such refactoring is to remove the authorizeAlbumsWrite etc. I just kept it here for simplicity.

This would allow use to uniformize the access rights rather than having to duplicate code.

@nagmat84 @kamil4 @d7415 @qwerty287 opinion ?

In general I am fine with that.

I only wonder what it would save us? At the moment you only call a single function authorizeAlbumWrite() and you want to replace it by using a trait which also needs a line like use AurhorizeAlbumWrite. I don't see the argument that the later helps to keep things consistent when we need to change the logic of whether an album is writable or not. Obviously, you would do that inside the method authorizeAlbumWrite() and therewith preserve consistency.

I also see a drawback. Consider the case of requests which do not only require to authorize write access to a single album (like the request to edit the album's title), but requests which need to authorize more, like a photo and an album (like the request which moves a photo into an album). In these cases we currently call two methods authorizeAlbumWrite and authorizePhotoWrite. This wouldn't be possible with traits. This means we would end up in a mixed case. Some requests which only need to authorize an album and hence use the trait, other requests which are more complicated and don't use the trait. For me this is even a loss of consistency.

Don't get me wrong. I am not against using a trait perse, but I don't see the advantage in this scenario.

I know that @nagmat84 removed quite a few Traits from previous Lychee architecture, so I would rather avoid making the same mistakes again.

Yes, I have removed those traits which were only used in a single place and were strongly coupled to the class in which they were used. But I also have added new traits.

A trait should have a general purpose and should (in principle) be usable in more than a specific class and should not make (too strong) assumptions about the class which uses it.

[ildyria]

I only wonder what it would save us? At the moment you only call a single function authorizeAlbumWrite() and you want to replace it by using a trait which also needs a line like use AurhorizeAlbumWrite. I don't see the argument that the later helps to keep things consistent when we need to change the logic of whether an album is writable or not. Obviously, you would do that inside the method authorizeAlbumWrite() and therewith preserve consistency.

No, I was thinking that the trait would only have the authorize() method, no longer AuthorizeAlbumWrite sub direction. :)

[nagmat84]

No, I was thinking that the trait would only have the authorize() method, no longer AuthorizeAlbumWrite sub direction. :)

I believe we are talking past each other. Currently we have a single method invocation to check wether a HTTP request which attempts to modify an album has been authorized for write access:

$this->authorizedAlbumWrite($album);

Of course this is somehow repetitive, because we have a lot of HTTP requests which modifies albums. This is somehow expected. You now want to replace this by a trait which essentially calls this method. Hence, afterwards you have something like

use NeedsAutorizedAlbumWriteTrait;

in each request class. How is this less repetitive? Yes, I have understood that the trait needs to contain the whole authorize method, because this is the only possible way. However, I don't understand your arguments

This would allow use to uniformize the access rights rather than having to duplicate code.

IMHO, the trait does not improve on uniformity or less duplicate code. You still have to repeat the usage of the trait in every request class, but so one has to do with the method.

[...] no longer AuthorizeAlbumWrite sub direction.

That not true, you still have the redirection to the method authorizedAlbumWrite. You have only moved it to an extra trait. So you could say that in some sense you have increased the number of redirections: from the class to the trait, from the trait to the method.

You still haven't answered my question how you would deal with the situations when a request needs to do more than only authorizing a single album, for example take the MovePhotosRequest which must authorize write access for a photo and an album. Let's assume you have written the traits

AuthorizeCanEditAlbumTrait

/**
* {@inheritDoc}
*/
public function authorize(): bool {
    return $this->authorizeAlbumWrite($this->album);
}

AuthorizeCanEditPhotoTrait

/**
* {@inheritDoc}
*/
public function authorize(): bool {
    return $this->authorizePhotoWrite($this->photo);
}

you cannot use both traits together in the same request as they both define the method authorize. Hence, you will still end up with

class MovePhotosRequest extends BaseApiRequest implements HasPhotos, HasAlbum {
  /**
   * {@inheritDoc}
   */
  public function authorize(): bool {
    return $this->authorizePhotosWrite($this->photos) && $this->authorizeAlbumWrite($this->album);
  }

Here, you still need to call the methods directly, because you need both in the same request. This leads to a situation where some requests use a trait (which calls a method) and some requests call the method directly. For me this leads to less uniformity. The point is: You have to think about the complicated cases first.

An alternative suggestion

However, there is one aspect which might happen to be improvable. I haven't thought it through yet, hence I do not know if it works satisfactorily or make things worse, too. Hence, I would probably postpone that to another PR. I already mentioned in https://github.com/LycheeOrg/Lychee/pull/1403#discussion_r933796320, but it seemingly got lost:

The class BaseApiRequest defines a lot of authorize... methods just like the authorizeAlbumWrite above which we are discussing. All these methods are simple another redirection to the album policy (or photo policy) like this:

abstrat class BaseApiRequest {
  protected function authorizeAlbumAccess(?AbstractAlbum $album): bool {
    return $this->albumAuthorisationProvider->isAccessible($album);
  }
}

At the time, when I wrote these methods, there were only meant for convenience to keep the code succint in the child classes, i.e. it allowed me to write

class MovePhotosRequest extends BaseApiRequest implements HasPhotos, HasAlbum {
  /**
   * {@inheritDoc}
   */
  public function authorize(): bool {
    return $this->authorizePhotosWrite($this->photos) && $this->authorizeAlbumWrite($this->album);
  }

as shown above instead of the ciumsy

class MovePhotosRequest extends BaseApiRequest implements HasPhotos, HasAlbum {
  /**
   * {@inheritDoc}
   */
  public function authorize(): bool {
    return $this->photoAuthorisationProvider->isEditable($this->photos) && $this->albumAuthorisationProvider->isEditable($this->album);
  }

Given the Gate face, this code isn't as clumsy as before, i.e. the syntax is already succinct as in Gate('isEditable', $photo). Hence, we might actually be able to remove all these short convenient methods from BaseApiRequest. This would really save one level of indirection.

[ildyria]

@nagmat84 See https://github.com/LycheeOrg/Lychee/pull/1435 for my proposition. :) It uses traits to defines the different most used authorize cases:

• Album (change properties)
• AlbumAlbums (merge, move)
• Photo (change properties)
• Photos (change properties)
• PhotosAlbum (move, duplicate)

[ildyria]

@nagmat84 I will have to trouble you with another round, because you requested some changes. :D

[ildyria]

@nagmat84 see last commit :)