laravel-doctrine / orm

A drop-in Doctrine ORM 2 implementation for Laravel 5+ and Lumen
http://laraveldoctrine.org
MIT License
827 stars 179 forks source link

[request] integration with laravel passport #189

Closed Evertt closed 7 years ago

Evertt commented 7 years ago

The title says it all. We need a replacement for the Laravel\Passport\HasApiTokens trait for example.

edit

or would this better fit in the extensions repo?

patrickbrouwers commented 7 years ago

I have looked into it just after it was released and it seemed like a lot of work. (Maybe I'm wrong). I currently don't need it myself and I'm very busy at the moment. If anybody wants to PR it, I'm happy to setup a repository for it.

ghost commented 7 years ago

For a project of mine I have partially ported passport to doctrine... At least password grants and authentication works, but not much more as of now. Also, I am quite unexperienced with laravel... I could publish it, so we could work on this together.

But a short disclaimer: It was impossible to reuse anything from passport. More or less, I just copied the source and manually changed everything to use doctrine interfaces. So this project would have to be maintained completely independent from passport...

We would also have to rethink some aspects like configuration etc... I mostly hardwired everything to get the basics to work.

I think that it would be a good codebase to start with, but still, it needs one or two days of work to publish it. But I don't want to maintain this just by myself, so if anybody could help me, I would be grateful.

@patrickbrouwers If you're interessted, I could make a PR for the repo you mentioned (I have seen that a repo already exists, but it is empty and can't be forked therefore)

patrickbrouwers commented 7 years ago

@christiandreher if you can share the repo through your own github, I can have a look if it's in the right direction :) Ping me on our slack.

ghost commented 7 years ago

@patrickbrouwers All right, I will try to do that on monday. Thanks for the response

Toyinster commented 7 years ago

Hi, I wondered if there was any update in this please?

ghost commented 7 years ago

@Toyinster I have published my first codebase here, but I did not yet receive any feedback.

I will continue working on this as the requirements for my project change, but for now this is enough for me... Wish that I could invest more time in this project, but I cant, I'm sorry

ghost commented 7 years ago

Yes, the problem for me was that the API of the lib by phpleague is quite complex, and passport already figured out how to use it. I therefore thought that it would be the most simple, and the most save (!) approach to port it to doctrine. Passport seems to be well established...

I know that its not perfect, especially if there is a security leak or anything, but we could either implement everything by ourselves (on top of phpleagues lib of course), or use the existing, well-established approach by passport as skeleton and replace everything with doctrine-standards until we have our own OAuth2 service provider.

At least this is what I thought. Sorry for responding so late, but I was busy until now

Btw: I think that we (everyone interested in this, like me, @oddz, etc and especially laravel-doctrine) should combine our efforts instead of everyone working on it's own half-baked solution. I would therefore highly appreciate, if some admin of laravel-doctrine could supervise and coordinate this. I think that this would be best for all of us. I do not say that I have the right approach or anything, I am just interested in the result, so if anyone has a better idea I will be the last refusing to cooperate

CC @patrickbrouwers (sorry, I don't know how I should raise attention otherwise)

Edit: @oddz: The problem by itself is not with phpleagues lib, which already implements OAuth2 very good, but rather with the integration with laravel/lumen. Passport has a good integration with laravel/lumen, but it depends on eloquent. So the problem is one abstraction layer above the OAuth2 implementation, exactly where passport uses eloquent. But, to be fair, it would be strange if they would use a database-abstraction-abstraction-layer :D

ghost commented 7 years ago

I wanted to publish it in the end for composer, so it would actually work like that, but I thought that it is more important to get the basics running instead of publishing it prematurely

patrickbrouwers commented 7 years ago

I looked at Passport when it came out and already noticed it would be a lot of work, so didn't proceed bridging it.

I'd suggest joining our slack and ask for people's help there. Not everyone reads all github issues :)

SwenVanZanten commented 7 years ago

I need it too.. perhaps I can help

LavaToaster commented 7 years ago

Hey All - I've made a PR that contains what I consider to be a near MVP version of a Laravel doctrine port. https://github.com/laravel-doctrine/passport/pull/1

I'd appreciate it if you could review it. ❤️

Evertt commented 7 years ago

@Lavoaster I don't have time to really review your code, but I just want to say I very much appreciate the work you've put into it!

patrickbrouwers commented 7 years ago

We have decided to not support any Doctrine implementation of Passport. Instead we suggest using it with Eloquent, and mapping it to Doctrine after the authentication is done.

Link to slack discussion: https://laraveldoctrine.slack.com/archives/C07HD2RS7/p1488372497003072

I'll be updating the readme/docs soon to indicate how you should integrate with Passport.

patrickbrouwers commented 7 years ago

Please offer your apologies for your response. I don't accept being responded to in this way.

patrickbrouwers commented 7 years ago

I won't help you anymore until you apologize. Your response is completely ridiculous and childish. This decision wasn't taken lightly and you are mocking us for it. You are the one being unacceptable. We are in no way required to offer and support a doctrine implementation of passport. You are not paying us to do it. Nothing is stopping you from starting your own passport repo. We decided against supplying one for good reasons. You should respect that decision and don't respond in this disrespectful way anymore.

patrickbrouwers commented 7 years ago

Please stop responding, you are not helping yourself.

sisve commented 7 years ago

@oddz The way to proceed in this issue would be to change one of the large underlying issues in Laravel's passport code.

laravel code is horribly coupled to eloquent and keeping up with the BC is not fun Source: https://laraveldoctrine.slack.com/archives/C07HD2RS7/p1488372589003080

The decision has been made on the current state of affairs. The best way to change the decision would be to change the underlying issue, the coupling between Passport and Eloquent.

Passport doesn't support MongoDB. It only supports Eloquent. Source: https://github.com/laravel/passport/pull/306#issuecomment-285879831

So, boot up that propaganda- and lobby machine, and change this underlying problem.

patrickbrouwers commented 7 years ago

Let me start clearing up some things.

but you have Lavoaster whom has a pretty good solution to this problem.

Never stated he didn't write a good implementation of Passport with Doctrine.

However, you won't accept or even look at it because it completely rewrites the module.

I have looked at it and decided against it.

If you actually take anytime to look at how passport is written you will see that it is TIGHTLY coupled to eloquent.

Trust me, i know how tightly it's coupled to Eloquent.

Perhaps in the next version of Laravel we try to Negev ideas. Create a version of passport that is flexible enough to have multiple pesistence laters?

In that case (case of rewrite by Laravel) we can re-evaluate this decision.

My passion for this project may come off offensive

Very offensive. And disrespectful against the people spending a lot of time building and maintaining this set of packages and community.

Patrick, your programming and/or leadership skills are much better in code than in person.

Very offensive. And very childish. So everybody who disagrees with you is per definition a bad person?

It's unacceptable to have such a tight coupling to an authentication layer that is meant to make the framework more flexible... Easier to use with other technologies.

You are putting the problem in the wrong community. Laravel-Doctrine can't be helt accountable for decisions made by Laravel.

sisve commented 7 years ago

@oddz I've added links to the sources for those statements.

If you want to put your energy into this issue, boot up the propaganda machine and start lobbying a change in the laravel/passport repository to allow persistence-ignorance so that we can easily support it without rewriting everything.

patrickbrouwers commented 7 years ago

This is not the location for this dicussion. It's up to Taylor to provide interchangeablity in Passport. You should open a ticket there, instead of blaming us for it.

Evertt commented 7 years ago

@patrickbrouwers @oddz @christiandreher @Toyinster @sisve @HDsign as you can see I opened an issue on the Laravel Passport repo. Maybe you can show your support and/or add some ideas or information to the thread. :-)

patrickbrouwers commented 7 years ago

I have tested the integration and it's actually achievable without any customizations in Laravel Passport and Laravel Doctrine.

The only thing that doesn't work out of the box are the listing of "personal access tokens". With a small change in Laravel-Passport that would also work.

You should just embrace Eloquent for this OAuth stuff. By setting the user provider on "doctrine", the oauth token gets automatically converted to a Doctrine instance.

Currently it's necessary to have$id public (because Laravel-Passport uses it) and have a getKey() method.

<?php

namespace App\Entities;

use Doctrine\ORM\Mapping AS ORM;
use Laravel\Passport\HasApiTokens;

/**
 * @ORM\Entity
 * @ORM\Table(name="users")
 */
class User
{
    use HasApiTokens;

    /**
     * @ORM\Id
     * @ORM\GeneratedValue
     * @ORM\Column(type="integer")
     */
    public $id;

    /**
     * @ORM\Column(type="string")
     */
    protected $name;

    /**
     * @return mixed
     */
    public function getId()
    {
        return $this->id;
    }

    public function getKey()
    {
        return $this->id;
    }
}

config/auth.php:

<?php

return [

    /*
    |--------------------------------------------------------------------------
    | Authentication Defaults
    |--------------------------------------------------------------------------
    |
    | This option controls the default authentication "guard" and password
    | reset options for your application. You may change these defaults
    | as required, but they're a perfect start for most applications.
    |
    */

    'defaults' => [
        'guard' => 'web',
        'passwords' => 'users',
    ],

    /*
    |--------------------------------------------------------------------------
    | Authentication Guards
    |--------------------------------------------------------------------------
    |
    | Next, you may define every authentication guard for your application.
    | Of course, a great default configuration has been defined for you
    | here which uses session storage and the Eloquent user provider.
    |
    | All authentication drivers have a user provider. This defines how the
    | users are actually retrieved out of your database or other storage
    | mechanisms used by this application to persist your user's data.
    |
    | Supported: "session", "token"
    |
    */

    'guards' => [
        'web' => [
            'driver' => 'session',
            'provider' => 'users',
        ],

        'api' => [
            'driver' => 'passport',
            'provider' => 'users',
        ],
    ],

    /*
    |--------------------------------------------------------------------------
    | User Providers
    |--------------------------------------------------------------------------
    |
    | All authentication drivers have a user provider. This defines how the
    | users are actually retrieved out of your database or other storage
    | mechanisms used by this application to persist your user's data.
    |
    | If you have multiple user tables or models you may configure multiple
    | sources which represent each model / table. These sources may then
    | be assigned to any extra authentication guards you have defined.
    |
    | Supported: "database", "eloquent"
    |
    */

    'providers' => [
        'users' => [
            'driver' => 'doctrine',
            'model' => App\Entities\User::class,
        ],

        // 'users' => [
        //     'driver' => 'database',
        //     'table' => 'users',
        // ],
    ],

    /*
    |--------------------------------------------------------------------------
    | Resetting Passwords
    |--------------------------------------------------------------------------
    |
    | You may specify multiple password reset configurations if you have more
    | than one user table or model in the application and you want to have
    | separate password reset settings based on the specific user types.
    |
    | The expire time is the number of minutes that the reset token should be
    | considered valid. This security feature keeps tokens short-lived so
    | they have less time to be guessed. You may change this as needed.
    |
    */

    'passwords' => [
        'users' => [
            'provider' => 'users',
            'table' => 'password_resets',
            'expire' => 60,
        ],
    ],

];
patrickbrouwers commented 7 years ago

Changes for Laravel Passport to make it fully compatible: https://github.com/laravel/passport/pull/327

patrickbrouwers commented 7 years ago

Above PR was merged. You can now use Passport together with Laravel Doctrine with the minimal change that you need to implement the getKey() method in your User entity.

Evertt commented 7 years ago

Patrick, you are amazing. :-D

mauserrifle commented 7 years ago

Great work!

I just tested the password grant but resulted in error:

Call to undefined method Cms3\Core\Domain\User\Entity\User::where()' in vendor/laravel/passport/src/Bridge/UserRepository.php:44

This bridge is still tight to eloquent. Or did I forget to setup something maybe?

patrickbrouwers commented 7 years ago

Maybe they broke it again? I thought I tested password grant too and it work last month.

aftabnaveed commented 7 years ago

@patrickbrouwers does the integration work? If yes is it fully stable?

provun commented 7 years ago

@patrickbrouwers has the code been stabilised as yet? Or should we be looking at other avenues?

jorygeerts commented 7 years ago

The current implementation seems broken. Laravel\Passport\Bridge\UserRepository::getUserEntityByUserCredentials() reads the auth.providers.users.model config value and assumes it is the name of a class that has a constructor without arguments and either findForPassport of where as method.

Since my entities do have constructors (with their non-nullable properties as parameters) and I do not need a findForPassport or where method in them, I created a simple class (App\Auth\PassportDoctrineBridge) that implements only findForPassport (using App::make(UserRepository::class) to fetch my repository) and configured that as "model". This, however, leads to an exception "No EntityManager is set-up for App\Auth\PassportDoctrineBridge".

The issue here is that Laravel Passport has one configuration option, model, that it uses for both the entity and the repository (which is valid for active record, because the entity and repository are the same class). In my opinion, the "proper" solution would be to get Laravel Passport changed so that it uses two different configuration options and then also support that second property in LaravelDoctrine. However, the more pragmatic approach might be to just do that second part.

I've solved this by extending DoctrineServiceProvider, overwriting extendAuthManager to change $entity = $config['model']; to $entity = $config['entity'] ?? $config['model'];. That + my PassportDoctrineBridge + the following configuration in config/auth.php works for me. 'providers' => [ 'users' => [ 'driver' => 'doctrine', 'model' => App\Auth\PassportDoctrineBridge::class, 'entity' => App\Entities\User::class, ], ]

isaackearl commented 7 years ago

So what is the current state of this? Is it possible to use Laravel Doctrine with Passport? I'm looking to start a new project and I definitely want to try using Laravel Doctrine for it. However I'm thinking of creating an API that would highly benefit from passport.

So if I understand correctly, there was a change to the passport code base which made it possible to integrate with Laravel Doctrine... but further changes to the package once again broke it?

It also seemed like there is a workaround from reading through this thread, that I can just handle oauth with eloquent and then also map entities to the same tables etc using doctrine?

If anybody with a bit more experience can point me in the right direction that would be great. If it is not tenable at the moment then I'll just try and think of a new project to work on with Doctrine that doesn't need oauth. Thanks!

isaackearl commented 7 years ago

So to answer my own question... I went ahead and gave the integration a try.

It ended up being fairly easy to get passport to work with doctrine. I would go so far as to say, if you are thinking of integrating passport in your doctrine laravel app, then just go ahead and try and do it, it works well out of the box after a few fixes. Here are the things I had to do to get things to work:

  1. I had to create my own tables/migrations to create the tables needed for passport. I created an entity mapping for each table so I could have doctrine automatically create the migration for me (that's how I did it, but you just need the tables created). I had some issues with trying to create database columns of type "TIMESTAMP", but ended up just doing DATETIME instead and it worked just fine. In the end that migration looked like this (I am using mysql):
    public function up(Schema $schema)
    {
        $this->abortIf($this->connection->getDatabasePlatform()->getName() != 'mysql', 'Migration can only be executed safely on \'mysql\'.');

        $this->addSql('CREATE TABLE oauth_access_tokens (id VARCHAR(100) NOT NULL, user_id INT DEFAULT NULL, client_id INT NOT NULL, name VARCHAR(255) DEFAULT NULL, scopes LONGTEXT DEFAULT NULL, revoked TINYINT(1) NOT NULL, created_at DATETIME DEFAULT NULL, updated_at DATETIME DEFAULT NULL, expires_at DATETIME DEFAULT NULL, INDEX user_id_index (user_id), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB');
        $this->addSql('CREATE TABLE oauth_auth_codes (id VARCHAR(100) NOT NULL, user_id INT NOT NULL, client_id INT NOT NULL, scopes LONGTEXT DEFAULT NULL, revoked TINYINT(1) NOT NULL, expires_at DATETIME DEFAULT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB');
        $this->addSql('CREATE TABLE oauth_clients (id INT AUTO_INCREMENT NOT NULL, user_id INT DEFAULT NULL, name VARCHAR(255) NOT NULL, secret VARCHAR(100) NOT NULL, redirect LONGTEXT NOT NULL, personal_access_client TINYINT(1) NOT NULL, password_client TINYINT(1) NOT NULL, revoked TINYINT(1) NOT NULL, created_at DATETIME DEFAULT NULL, updated_at DATETIME DEFAULT NULL, INDEX user_id_index (user_id), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB');
        $this->addSql('CREATE TABLE oauth_personal_access_clients (id INT AUTO_INCREMENT NOT NULL, client_id INT NOT NULL, created_at DATETIME DEFAULT NULL, updated_at DATETIME DEFAULT NULL, INDEX client_id_index (client_id), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB');
        $this->addSql('CREATE TABLE oauth_refresh_tokens (id VARCHAR(100) NOT NULL, access_token_id VARCHAR(100) NOT NULL, revoked TINYINT(1) NOT NULL, expires_at DATETIME DEFAULT NULL, INDEX access_token_index (access_token_id), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB');
    }
  1. I had to add one function to my User Entity....
    /**
     * @return int
     */
    public function getKey()
    {
        return $this->getId();
    }

That is pretty much it if I'm remembering properly. It also failed once I tried testing my API endpoint because the default endpoint in laravel tries to return the entire user... which fails because the entities don't have a __toString() method... You can fix this and get your api endpoint working by implementing a way to serialize yoru entity or you can just change the endpoint to return something else.

Route::middleware('auth:api')->get('/user', function (Request $request) {
    return $request->user()->getName();
});

So behind the scenes passport is still using Eloquent models to manipulate the data on the oauth tables that get created, but I don't see this as a problem at all. The rest of your application doesn't have to worry or think about that at all and it should have no affect.

Software used:

laravel-doctrine/orm                  1.3.6   A Doctrine ORM bridge for Laravel 5
laravel/framework                     v5.4.27 The Laravel Framework.
laravel/passport                      v2.0.11 Laravel Passport
KPKoch commented 7 years ago

Currently I am trying to integrate Passport with Laravel. Situation seems: not that easy with Passport 3.0. At least the solution suggested by isaackearl doesn't work this way for me.

It get stucks at the "Repos" and the access via Active Record. This seem to me like some major work to integrate. The exact problem I am facing is the missing "where", like in "Call to undefined method App\Models\User\User::where()" Is there some "easy" way to just overwrite it or solve this with another non-source modifying way?

Also I think passport is in modern days no optional feature, as most projects will need OAuth. Modern webapps are nowdays also an API for mobile apps - maybe for distributed services. This change is nowadays much more important then 10 years ago. This said: Issues like this could be a real deal breaker for me to use laraveldoctrine, as the more of a time invest is a true thing with doctrine instead of eloquent. Even if I really don't want to use an active record pattern.

isaackearl commented 7 years ago

@KPKoch

So I went ahead and upgraded my install to Passport 3.0 to see what the issues were, and it actually worked for me. I think I must have done something else that I didn't document properly in my above example.

I think I'll spend some time this week setting up another project with doctrine and passport, and I'll try and do a better job of documenting it and maybe do a little write-up.

The repo shouldn't be trying to use the "Model" for user at all.. so I'm thinking part of your configuration may be wrong. Have you changed your user provider in auth.php ? It should look something like this:

    'providers' => [
        'users' => [
            'driver' => 'doctrine',
            'model' => App\Entities\User::class,
        ],
    ],

Let me know if that fixes your problem at all.

KPKoch commented 7 years ago

I consider that there may be some configuration error. I feel this happens sometimes while upgrading and the project is up-to-date but had a minor history (think it could have had been 5.2) and the recent change in the frontend (decision to move from a laravel web app to a API only) At least the config is right: 'providers' => [ 'users' => [ 'driver' => 'doctrine', 'model' => App\Models\User\User::class, ],

I made this part work, by editing laravel/src/Bridge/UserRepository, getUserEntityByUserCredentials and added as quick fix a repomanager. So the question remains, why had I?

After this, there seem problems with the guard.... at least auth is empty, when asked later. I'll still have to check if there is something within the frontend that I have to adapt (vue with vue-passport).

Currently it breaks after the token authorization when the get to api/user is done.

AP-Hunt commented 7 years ago

I just want to say a massive thank you to everybody who's contributed to this issue over the last 9 months. I expect a lot of people have benefited from reading through your discussions and attempts to get Laravel-Doctrine working with Laravel Passport.

isaackearl commented 7 years ago

@KPKoch I went ahead and went through the process again and did a better job of documenting each thing I did. Here is the resultant guide:

https://isaacearl.com/blog/laravel-doctrine-setup

I go through step by step. and got a fully working passport demo at the end with doctrine etc. I also uploaded the result to a repository here: https://github.com/isaackearl/doctrine-passport-demo

patrickbrouwers commented 7 years ago

@isaackearl thanks for the article! Would you mind it if I would link to it in our documentation?

isaackearl commented 7 years ago

@patrickbrouwers No I don't mind at all. Sounds awesome! I'll try and keep it up to date for those checking it out.

patrickbrouwers commented 7 years ago

thanks @isaackearl :)

KPKoch commented 7 years ago

Thx guys for the effort. Currently I still do have the problem, as I:

Same place as before. passport/src/Bridge/UserRepository.php, line 44. If I modify this class to use the entity manager, this part works fine.

isaackearl commented 7 years ago

@KPKoch

So after scratching my head for a little while I finally realized you are trying to do a Password Grant instead of an Authorization Code Grant.

I was finally able to reproduce your issue while trying to do a password grant. I then found a fix which I will attempt to describe to you now. I will also update my repo and add a note to the guide I created.

You'll see in the code for the UserRepository (where it breaks) that there is a line included to allow for a custom function to get the user.. it looks like this:

if (method_exists($model, 'findForPassport')) {
    $user = (new $model)->findForPassport($username);
} else {
    $user = (new $model)->where('email', $username)->first();
}

So basically if you have a method "findForPassport" defined on your entity, it will do that instead of trying to use active record ->where... which is what is causing it to fail for you on line 44.

To fix it I created the findForPassport method on my User entity... and then turned it into a trait because it looks a bit nicer.

This is what that trait looks like for me right now:

trait CanUsePasswordGrant
{

    /**
     * @param string $userIdentifier
     * @return User
     */
    public function findForPassport($userIdentifier)
    {
        $userRepository = EntityManager::getRepository(get_class($this));
        return $userRepository->findOneByEmail($userIdentifier);
    }

}

I then just add that to my entity, and I don't have to worry about modifying any vendor files or anything like that

class User implements AuthenticatableContract, CanResetPasswordContract
{

    use Authenticatable, CanResetPassword, Notifiable, HasApiTokens, Timestamps, CanUsePasswordGrant;

Using this approach I got the Password Grant stuff to work as well.

Let me know if you have any other questions!

Note: I have updated the repo, you can pull latest change and it should work. https://github.com/isaackearl/doctrine-passport-demo

KPKoch commented 7 years ago

I realy like your example and it helped me to find sort out a "tiny" misconfiguration from old times. My first source currently to hunt bugs. This is why it is so important to me, that examples given on the internet are just working. If they don't it adds the factor of guessing. As said, the project is a bit older and so well the last updates with laravel were, it is always somewhat tricky to follow up all changes in the app folder - I had a depricated auth middleware.