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

[5.6] Add ability to determine if the current user is ALREADY authenticated without triggering side effects #24518

Merged
merged 3 commits into from
Jun 9, 2018
Merged

Conversation

mpyw
Copy link
Contributor

@mpyw mpyw commented Jun 8, 2018

  • Auth::check() internally calls Auth::user(). It may trigger side effects.
    (e.g. accessing database or making an HTTP request for external Web API)
  • Auth::guard()->user is protected.

We currently do not have an ability to determine if Auth::guard()->user is already set without using reflection. This PR adds Auth::alreadyAuthenticated() which returns boolean value.

It will be useful in models or custom relations.

Example

class Post extends Model
{
    protected $visible = ['id', 'title', 'body', 'created_at', 'likes', 'liked'];
    protected $with = ['likeByCurrentUser'];
    protected $appends = ['liked'];

    public function likes()
    {
        return $this->hasMany(Like::class);
    }

    public function likeByCurrentUser()
    {
        return $this->hasOne(Like::class)->where('user_id', Auth::alreadyAuthenticated() ? Auth::id() : null);
    }

    public function getLikedAttribute()
    {
        return Auth::alreadyAuthenticated() ? ! is_null($this->likeByCurrentUser) : null;
    }
}

mpyw added 2 commits June 8, 2018 23:47
It behave similarly to `GuardHelpers::check()`, but it doesn't trigger any side effects.
@mpyw mpyw changed the title [5.6] Add ability to check the current user is ALREADY authenticated without triggering side effects [5.6] Add ability to determine if the current user is ALREADY authenticated without triggering side effects Jun 8, 2018
@browner12
Copy link
Contributor

I not sure you can rely on the $this->user property being set as an indicator of whether or not the user is authenticated or not. If the property is set, you know the user is logged in, but if the property is not set, the user may not be logged in, or the class just might not know about the user yet, and needs to run through it's driver specific code to check if the user is authenticated.

@mpyw
Copy link
Contributor Author

mpyw commented Jun 8, 2018

@browner12 I know. In some environment running driver specific code through Auth::user() may throw exceptions or trigger unexpected behaviors. In my project, for instance, development environment is separated from the authentication platform API so making HTTP requests through Auth::user() triggers exceptions.

Please tell me if you have the proper name for this method. (e.g. Auth::alreadyChecked())
Or just make Auth::guard()->user public?

@taylorotwell
Copy link
Member

I would rather just say something like Auth::hasUser()

@mpyw
Copy link
Contributor Author

mpyw commented Jun 8, 2018

@taylorotwell Nice

class Post extends Model
{
    protected $visible = ['id', 'title', 'body', 'created_at', 'likes', 'liked'];
    protected $with = ['likeByCurrentUser'];
    protected $appends = ['liked'];

    public function likes()
    {
        return $this->hasMany(Like::class);
    }

    public function likeByCurrentUser()
    {
        return $this->hasOne(Like::class)->where('user_id', Auth::hasUser() ? Auth::id() : null);
    }

    public function getLikedAttribute()
    {
        return Auth::hasUser() ? ! is_null($this->likeByCurrentUser) : null;
    }
}

@browner12
Copy link
Contributor

I guess I'm a little confused. If your local environment throws exceptions when trying to authenticate over HTTP with your auth API, how does this change help you? Wouldn't $this->user always be null then?

@mpyw
Copy link
Contributor Author

mpyw commented Jun 8, 2018

@browner12 In this example, fetching Post instances always fails in my local environment regardless of whether the endpoint requires authentication or not.

class Post extends Model
{
    protected $visible = ['id', 'title', 'body', 'created_at', 'likes', 'liked'];
    protected $with = ['likeByCurrentUser'];
    protected $appends = ['liked'];

    public function likes()
    {
        return $this->hasMany(Like::class);
    }

    public function likeByCurrentUser()
    {
        return $this->hasOne(Like::class)->where('user_id', Auth::id());
    }

    public function getLikedAttribute()
    {
        return ! is_null($this->likeByCurrentUser);
    }
}
class PostContrller extends Controller
{
    public function index()
    {
        // Always fails in my local environment
        return Post::all();
    }
}

Note that this is just an example. Generally I think loading or formatting models should not trigger side effects except accessing database to fetch their rows.

@taylorotwell taylorotwell merged commit 6a9d9e3 into laravel:5.6 Jun 9, 2018
@mpyw mpyw deleted the auth-already-authenticated branch June 9, 2018 15:38
@mpyw
Copy link
Contributor Author

mpyw commented Jun 11, 2018

@taylorotwell Should missing public methods such as authenticate() hasUser() be added to Guard contract at the next major release? (6.0? 5.7)

@antonkomarev
Copy link
Contributor

antonkomarev commented Jun 11, 2018

@mpyw next major release will be 5.7. Laravel isn't following SemVer.

TBlindaruk added a commit to TBlindaruk/laravel-framework that referenced this pull request Sep 15, 2018
 - laravel#24518 in this PR was added method `hasUser` to the GuardHelper trait. As for me it will be useful to have this method in the contract.
taylorotwell added a commit that referenced this pull request Oct 12, 2021
* [9.x] Add `hasUser` method to Guard contract

- #24518 in this PR was added method `hasUser` to the GuardHelper trait. As for me it will be useful to have this method in the contract.

* Update Guard.php

Co-authored-by: Tetiana Blindaruk <t.blindaruk@gmail.com>
Co-authored-by: Taylor Otwell <taylor@laravel.com>
taylorotwell added a commit to illuminate/contracts that referenced this pull request Oct 12, 2021
* [9.x] Add `hasUser` method to Guard contract

- laravel/framework#24518 in this PR was added method `hasUser` to the GuardHelper trait. As for me it will be useful to have this method in the contract.

* Update Guard.php

Co-authored-by: Tetiana Blindaruk <t.blindaruk@gmail.com>
Co-authored-by: Taylor Otwell <taylor@laravel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants