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

Error on custom repository and model #385

Closed
nanoray8888 opened this issue Apr 26, 2023 · 17 comments
Closed

Error on custom repository and model #385

nanoray8888 opened this issue Apr 26, 2023 · 17 comments
Assignees
Labels
Scope: Documentation Proposed changes to the README or other documentation.

Comments

@nanoray8888
Copy link

Laravel Version

Laravel 10.x

SDK Version

SDK 7.7

PHP Version

PHP 8.3

What happened?

Hello,

I'm attempting to implement a custom repository that utilizes Eloquent ORM (from this link - https://github.com/auth0/laravel-auth0/blob/main/docs/User%20Models%20and%20Repositories.md), but I've encountered an error: Call to undefined method App\Models\User::where(). I believe the issue stems from the fact that the Auth0\Laravel\Model\User class does not extend the Laravel model, resulting in the absence of a ::where() method.

Could you please tell me what went wrong with the integration process based on the above documentation?

Thank you.

How can we reproduce this issue?

na

Additional context

na

@evansims
Copy link
Member

Hi @nanoray8888 👋 Whoops, you're totally correct, I forgot to include the Eloquent user model! That's an embarrassing oversight. 🤦 I'll get that fixed yet today, thanks for the heads up!

@evansims evansims self-assigned this Apr 26, 2023
@evansims evansims added the Scope: Documentation Proposed changes to the README or other documentation. label Apr 30, 2023
@spiro-bss
Copy link

spiro-bss commented May 7, 2023

Curious if there's any movement on this? Or a short term workaround? I'm having the same issue.

@FreekVR
Copy link
Contributor

FreekVR commented May 10, 2023

Also having issues with this in Laravel 9 and custom model/repository

Following this guide:
https://github.com/auth0/laravel-auth0/blob/main/docs/User%20Models%20and%20Repositories.md

I get the same issues as the OP

If I skip extending the default Auth0 model, and just directly implement the \Auth0\Laravel\Contract\Model\User contract on my Eloquent-based model, I get errors about several methods (like _get()) being incompatible with the Eloquent implementation in Laravel 9.

For now, I'll have to downgrade to < 7.5.0 as I'm also having memory limit issues which may or may not be related to differing user model implementations.

@evansims
Copy link
Member

Hey everyone 👋 Apologies for the delay, my attention was focused on getting 7.8 shipped alongside new authentication and authorization documentation.

With that backlog of items shipped, I'll be focusing on getting better solutions and guidance for Eloquent support next. If anyone has any specific implementation ideas they'd like me to consider, please do share that feedback.

@devjack
Copy link
Contributor

devjack commented May 20, 2023

Jumping in here to +1 w/ context @FreekVR's comment regarding Eloquent compatibility.

Simple type hinting is preventing having a application space model that extends Eloquent models + implements Auth0's UserContract; specifically defining __get(string $id) since Eloquent doesn't type hint the method signature.

A cursory search through the interface usage it could be easier for users of the SDK to have a non-type hinted method signature with an implementation trait (or non-final concrete implementation) that casts to string instead.

@devfrey
Copy link

devfrey commented May 23, 2023

I've found that implementing UserContract is not actually required, as the SDK typehints against Laravel's Authenticable contract. The former interface appears to be redundant. I suggest removing it to reduce the complexity.

One thing that really bothers me with the current implementation is that during the code exchange UserRepository::fromSession() receives the decoded JWT, while on subsequent calls (when authenticated) it receives a serialized version of the model that was returned from fromSession(). I'm doing a hacky isset($user[...]) workaround to determine which type it is. Ideally the SDK just stores the user identifier (Authenticatable::getAuthIdentifier()) in the session, similar to Laravel's SessionGuard.

By the way, the AuthenticationGuard doesn't implement Laravel's Illuminate\Contracts\Auth\Guard interface. So this potentially breaks code that depends on a Guard implementation when interacting with other parts of the framework.

@evansims
Copy link
Member

evansims commented May 28, 2023

Thanks for your suggestions @devjack @devfrey.

If we could, let's please create new issues for future discussions, as a lot of this doesn't really have any relation at all to the reporter's suggestion, and we're spamming their inbox in the process.

@devfrey, responding to your comments:

I've found that implementing UserContract is not actually required, as the SDK typehints against Laravel's Authenticable contract. The former interface appears to be redundant. I suggest removing it to reduce the complexity.

UserContract extends Authenticatable and is a shared interface between all of the various user model types expressed by the SDK. It is advisable to extend UserContract for future implementation considerations. Authenticatable will naturally work because that is the base interface, but this may not always be the case in all circumstances.

One thing that really bothers me with the current implementation is that during the code exchange UserRepository::fromSession() receives the decoded JWT, while on subsequent calls (when authenticated) it receives a serialized version of the model that was returned from fromSession().

That sounds like it might be a bug, I'll check into that. It should always be represented as a decoded JWT payload.

Ideally the SDK just stores the user identifier (Authenticatable::getAuthIdentifier()) in the session, similar to Laravel's SessionGuard.

This would simply not work with how sessions are handled. We store all the token data necessary to validate a session within the session itself. This is how all Auth0 SDKs function. To do otherwise would impose requirements of persistent databases for sessions/user storage.

We have plans for improvements to the next version of the underlying PHP SDK to implement a way of handling sessions that would make this possible, but until that is implemented, it's not really viable. On the roadmap, though!

By the way, the AuthenticationGuard doesn't implement Laravel's Illuminate\Contracts\Auth\Guard interface. So this potentially breaks code that depends on a Guard implementation when interacting with other parts of the framework.

Thanks for the heads up! Definitely a bug. Will address that.

@dutsik
Copy link

dutsik commented May 29, 2023

@evansims Can we quickly release a new version with support for the Illuminate\Contracts\Auth\Guard interface:

By the way, the AuthenticationGuard doesn't implement Laravel's Illuminate\Contracts\Auth\Guard interface. So this potentially breaks code that depends on a Guard implementation when interacting with other parts of the framework.

It is really pain to overcome this flaw

@dutsik
Copy link

dutsik commented May 29, 2023

@devfrey do you have any working solution for lacking of Laravel's Guard contract. I am trying to integration AUth0 with FIlament. which expects Guard contract from auth() method...

@dutsik
Copy link

dutsik commented May 29, 2023

@evansims all the Guard's interface methods are in place we just to an inheritance...

@evansims
Copy link
Member

evansims commented May 29, 2023

@dutsik I'm on vacation until next week, but I'll be happy to take a look at cutting a release when I'm back. Lets please create new issues for topics unrelated to this thread.

@squpshift
Copy link

We have an application that is ready to launch except it seems to be impossible to integrate auth0 with a Laravel eloquent user model at the moment.

I'm really struggling to understand why the current implementation of this library requires a \App\Models\User that does not extend \Illuminate\Database\Eloquent\Model

I've been battling with this for so many hours trying to make it work.

Does anyone have a workaround they could suggest or are we stuck waiting for auth0 team to fix this?

@devjack
Copy link
Contributor

devjack commented May 31, 2023

I share your frustration with recent regressions and incompatibilities @squpshift.

I'm working on a Laravel 10 upgrade which will take my auth0/login from 7.4 up to at least 7.5. I'd have loved to try the 7.8 series but its clearly not feasible right now in its current state.

My suggestion (and what I'll be doing in the next few days) is pinning my dependency to either 7.5.* or 7.6.* in an effort to avoid the 7.8 mess. Sadly there's not as clear documentation for those versions but the interfaces & abstractions are at least relatively clear once you dive into the code. Happy to share back if I find a happy path in doing so.

@squpshift
Copy link

Thanks Jack, I'll be curious to hear how you go. I'm away on leave for the next week... if the 7.8 mess isn't resolved when I get back I guess we'll have to do the same.

Best of luck!

@sclavijo9310
Copy link

sclavijo9310 commented May 31, 2023

We have an application that is ready to launch except it seems to be impossible to integrate auth0 with a Laravel eloquent user model at the moment.

I'm really struggling to understand why the current implementation of this library requires a \App\Models\User that does not extend \Illuminate\Database\Eloquent\Model

I've been battling with this for so many hours trying to make it work.

Does anyone have a workaround they could suggest or are we stuck waiting for auth0 team to fix this?

For my case I just skipped to implement the interface or extends the abstract class the documentation says, and it worked (Keep the User model as is), seems like internally they just validate the custom model implements the laravel Authenticatable contract.

I'm sorry for my bad english

@evansims
Copy link
Member

I'm really struggling to understand why the current implementation of this library requires a \App\Models\User that does not extend \Illuminate\Database\Eloquent\Model

There is no such requirement. I don't know what would give you that impression. The only requirement the SDK enforces — as does Laravel itself — is user models must inherit Illuminate\Contracts\Auth\Authenticatable. That's it. 🤷

As @sclavijo9310 noted, the simplest approach is to simply extend the Auth0\Laravel\Users\UserAbstract abstract class for your user models. This is covered in the documentation: https://github.com/auth0/laravel-auth0/blob/main/docs/Users.md#custom-user-models

@evansims
Copy link
Member

I'm going to close this thread for now, as it seems to be becoming a catch-all for feedback. Please create new issues with any specific concerns so we can address them more effectively.

I am preparing an update to the documentation regarding an example of Eloquent model support, which was the original purpose of this issue.

@auth0 auth0 locked as off-topic and limited conversation to collaborators Jun 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Documentation Proposed changes to the README or other documentation.
Projects
None yet
Development

No branches or pull requests

9 participants