Added (optional) morphable user #420

crashkonijn · 2018-05-09T12:34:35Z

Changes

Added config option for morphable mode. Everything defaults to 'single mode', to maintain backwards compatibility.
Added config option for guard order.
Changed current UserSolver to UserIdSolver.
Added UserIdSolver and UserClassSolver.
Added commented value to migration file, which needs to be uncommented for morphable to work.
Added tests to include new classes.
Adjusted tests to test both modes.

Morph user

quetzyg · 2018-05-09T13:00:00Z

Hi @crashkonijn,

Care to explain what exactly is the purpose of this PR? What does it solve, and so on?

Cheers

crashkonijn · 2018-05-09T13:14:57Z

Hello @quetzyg,

We really wanted to use this package in our project, but in almost all project we have different models for different roles within the project. However this project very clearly only supported a single user type, where feedback in previous requests was to change the projects to match this package by using just a single user model.

After some discussing within our team we decided to extend this package and implement this feature. Because we really like this package and love to give something back to the community we tried to implement it with as little impact on existing code as possible. The morphable user has been implemented in such a way that it should be backwards compatible, and is completely optional.

We really hope this PR will be accepted, because we feel that would benefit more people than hosting a separate version.

If there's any feedback or suggestions to improve this I'm very willing to make those changes.

Cheers!

Edit: Typo

quetzyg · 2018-05-09T13:48:25Z

However this project very clearly only supported a single user type, where feedback in previous requests was to change the projects to match this package by using just a single user model.

While I never actually tested this package with a polymorphic User model, I'm pretty sure you could use it as is, right now. I mean, you just have to resolve the User id and store it.

Then you should be able to get the user with: $audit->user->userable

Or am I missing something?

crashkonijn · 2018-05-09T16:00:41Z

That would indeed be an option yes, but unfortunately our users have different models and nothing in common in that regard. Just like a normal polymorphic relation we need to store the user type as well, which is what this PR does.

The user just remains $audit->user, and the other way around can stay model->audits this way, which keeps things simple.

remkobrenters · 2018-05-09T16:23:05Z

@quetzyg,

The use case is for example you have multiple userTypes in a platform. By separating them in different models and Laravel guards not only can you keep the data separate (to prevent endless empty columns for users with different datasets) but it also allows you to have multiple user accounts with the same email address and, even cooler, allows you to be logged in as various users at the same time (for example a admin that is acting as a front-end user for support purposes).

This requires a morphable solution as the userId is not 'unique' (as each entity has it's own table and unique Id's) so you need the userType to retreive the unique id + type identity.

quetzyg · 2018-05-09T16:30:56Z

@remkobrenters, yeah I'm aware, but given each morph user type does relate to a base User, that's the id I was referring to in my earlier comment.

So, you would resolve the base User id, which would be stored, and then you would get model via:
$audit->user->userable (or whatever method name you have defined for the morphTo() and the inverse would be like $user->user->audits.

But I get your point.

quetzyg

Left a few comments.

quetzyg · 2018-05-09T16:32:08Z

config/audit.php

+    |
+    */
+
+    'morphable' => false,


Move this to the user config down bellow, a line after 'model' => App\User::class,

quetzyg · 2018-05-09T16:33:36Z

config/audit.php

+    |--------------------------------------------------------------------------
+    |
+    | Define the User guards, in the order they should be checked.
+    | This is used when in 'morphable mode'.


We could actually use this even when not in morphable mode.

quetzyg · 2018-05-09T16:35:02Z

database/migrations/audits.stub

@@ -28,6 +28,7 @@ class CreateAuditsTable extends Migration
        Schema::create('audits', function (Blueprint $table) {
            $table->increments('id');
            $table->unsignedInteger('user_id')->nullable();
+            //$table->string('user_type')->nullable();


This should not be commented, since we want the column.

The reason that I left it commented out is because you don't need it for the 'normal' mode, just when you need it morphable. Still want me to uncomment this?

quetzyg · 2018-05-09T16:38:15Z

config/audit.php

@@ -49,6 +76,8 @@
    |
    */
    'resolver' => [
+        'user_id'    => OwenIt\Auditing\Resolvers\UserIdResolver::class,


Just one resolver, since both are doing pretty much the same thing, just calling two different methods (->getMorphClass() and ->getAuthIdentifier()).

quetzyg · 2018-05-09T16:41:06Z

src/Auditable.php

@@ -282,18 +283,24 @@ public function toAudit(): array

        $tags = implode(',', $this->generateTags());

-        return $this->transformAudit([


With just one User resolver, we would get the user at this point, like:

$user = $this->resolveUser();

quetzyg · 2018-05-09T16:43:47Z

src/Contracts/Audit.php

@@ -43,9 +42,9 @@ public function auditable(): MorphTo;
    /**
     * User responsible for the changes.
     *
-     * @return \Illuminate\Database\Eloquent\Relations\BelongsTo
+     * @return \Illuminate\Database\Eloquent\Relations\MorphTo | \Illuminate\Database\Eloquent\Relations\BelongsTo


No space before/after |, please

quetzyg · 2018-05-09T16:44:31Z

src/Resolvers/UserClassResolver.php

+     */
+    protected static function resolveUser()
+    {
+        $userResolver = \Config::get('audit.resolver.user');


Import Config and remove \

quetzyg · 2018-05-09T16:45:21Z

src/Resolvers/UserIdResolver.php

+     */
+    protected static function resolveUser()
+    {
+        $userResolver = \Config::get('audit.resolver.user');


Import Config and remove \

quetzyg · 2018-05-09T16:47:07Z

src/Resolvers/UserResolver.php

@@ -23,6 +23,36 @@ class UserResolver implements \OwenIt\Auditing\Contracts\UserResolver
     */
    public static function resolve()
    {
-        return Auth::check() ? Auth::user()->getAuthIdentifier() : null;
+        return \Config::get('audit.morphable', false) ? static::resolveMorphable() : static::resolveSingle();


Import Config and remove \

quetzyg · 2018-05-09T16:49:08Z

src/Resolvers/UserResolver.php

+     *
+     * @return \Illuminate\Contracts\Auth\Authenticatable|null
+     */
+    private static function resolveMorphable()


Like I stated earlier, we can default to this behaviour, even when not using a morph user.

remkobrenters · 2018-05-09T17:43:11Z

@quetzyg,

I'm afraid you do not understand the whole case. When you have multiple user entities there is no main user table or id. They all live in separate tables without relations in between. Without the userType there is no way of telling in which table to look for the user who triggered the audit-event.

quetzyg · 2018-05-09T20:36:57Z

I'm afraid you do not understand the whole case. When you have multiple user entities there is no main user table or id. They all live in separate tables without relations in between. Without the userType there is no way of telling in which table to look for the user who triggered the audit-event.

But then, are you actually using Laravel's polymorphic relationships, or do you have something else which you're also calling polymorphic?

Because the whole point of Laravel's polymorphic relationships is having a base User (in this case), and then you have the extended models (Admin, Manager or whatever) with their specific properties.

The idea is that they share basic functionality (that the User model has), like id, email, name, surname, etc.

PS: I'm aware that each other model (Admin, Manager or whatever) will have their own id too, but the User id is what I'm referring to in my example.

crashkonijn · 2018-05-09T21:44:40Z

A polymorphic relation doesn't rely on that at all, the whole point is that it can be any model.

Even the example in the docs uses 2 totally unrelated models (posts and videos) and adds comments to them.

https://laravel.com/docs/5.6/eloquent-relationships#polymorphic-relations

This package even uses it for the object it's watching. The whole reason for these changes is to lift the limitation of a single object/model being the user.

quetzyg · 2018-05-09T23:39:55Z

A polymorphic relation doesn't rely on that at all, the whole point is that it can be any model.

Yes, I'm fully aware.

Even the example in the docs uses 2 totally unrelated models (posts and videos) and adds comments to them.

You would think they're unrelated, but the fact is that you can comment on both models, in the same way a generic User can have different properties/roles through user types.

This package even uses it for the object it's watching. The whole reason for these changes is to lift the limitation of a single object/model being the user.

My point with this conversation is to understand how you're using such relation in your user models, since the whole base User model and extended (polymorphic) models, doesn't seem to apply.

remkobrenters · 2018-05-10T08:46:31Z

@quetzyg,

I think we're not on the same page. Since L5.3 (correct me if i'm wrong on the exact version) Laravel allows you to create separate authenticable entities with their own class and table. For example if you have a huge web-shop with customers which contain a lot of customer-related data (addresses, orders, invoices, wish-lists) and you have a admin environment that has content managers, admins etc. You can create two different user tables, customers & managers for example. They do not share a central users table or Id and do not extend each other in any way.

Laravel allows you to have a guard for each authenticable entity with it's own configuration. Allowing you for example to load the managers from another database, using another session 'container', authentication method (maybe two-factor for your managers) or whatever you like. Separating them completely from the other user accounts.

In this case both customers and managers can perform auditable actions and without the morphable user_type (which contains the entity class) there is no way of telling what entity belongs to the identity that performed the action.

I think this tutorial shows a really simple but clear explanation of this 'new' feature and it's implementation: https://scotch.io/@sukelali/how-to-create-multi-table-authentication-in-laravel

quetzyg · 2018-05-10T08:58:48Z

Ahh OK, I finally understood what you have going there. The Authenticatable interface is just a contract, that doesn't have anything to do with polymorphic relations.

But since you kept referring to it as such (morphable), that's what threw me off.

remkobrenters · 2018-05-10T09:00:14Z

@quetzyg,

Cool. Sorry for the confusion. We think the suggested PR could be a very welcome addition for parties that, like us, use a lot of separate authenticable entities to keep various datasets separate.

quetzyg · 2018-05-10T16:59:05Z

Actually, if you don't mind, I'm gonna simplify this a little bit in another branch.

crashkonijn · 2018-05-10T17:01:19Z

Alright, I'm very curious to see what you changed!

quetzyg · 2018-05-10T21:29:08Z

So basically, I got rid of unnecessary configurations. We're always gonna store the User as a morph, which avoids the programmer from choosing between normal and morphable modes.

This simplifies tests, overall logic and migrations (no need to comment user_type column, since it's always used).

Great job @crashkonijn / @remkobrenters !

Peter Klooster and others added 11 commits May 8, 2018 14:21

Basic version of morphable user

4d29aa9

Updated readme

17f1007

Made morphable optional, project should now be backwards compatible

d2fa829

Readme reset, updated config

65b6545

Added tests for morphable code coverage

c6bb784

Increaded tests

0503af8

Added missing docblocks

6549e62

Merge pull request #1 from Webparking/morph-user

1934875

Morph user

Implemented styleci feedback

c4b0de7

Some more styleci feedback fixes

aa7ca40

Even more styleci fixes

a2076fc

quetzyg suggested changes May 9, 2018

View reviewed changes

Peter Klooster added 3 commits May 10, 2018 16:39

Changed config.audit.morphable to config.audit.user.morphable

c4a3c6f

Implemented PR feedback

d0f2cd7

Remaining feedback changes

5d7bfe3

quetzyg mentioned this pull request May 10, 2018

[FEAT] V7 - Multi user audits #421

Merged

6 tasks

quetzyg closed this May 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added (optional) morphable user #420

Added (optional) morphable user #420

crashkonijn commented May 9, 2018

quetzyg commented May 9, 2018

crashkonijn commented May 9, 2018 •

edited

Loading

quetzyg commented May 9, 2018

crashkonijn commented May 9, 2018

remkobrenters commented May 9, 2018

quetzyg commented May 9, 2018

quetzyg left a comment

quetzyg May 9, 2018

quetzyg May 9, 2018

quetzyg May 9, 2018

crashkonijn May 10, 2018

quetzyg May 10, 2018

quetzyg May 9, 2018

quetzyg May 9, 2018

quetzyg May 9, 2018

quetzyg May 9, 2018

quetzyg May 9, 2018

quetzyg May 9, 2018

quetzyg May 9, 2018

remkobrenters commented May 9, 2018

quetzyg commented May 9, 2018 •

edited

Loading

crashkonijn commented May 9, 2018 •

edited

Loading

quetzyg commented May 9, 2018

remkobrenters commented May 10, 2018

quetzyg commented May 10, 2018 •

edited

Loading

remkobrenters commented May 10, 2018

quetzyg commented May 10, 2018

crashkonijn commented May 10, 2018

quetzyg commented May 10, 2018 •

edited

Loading

		@@ -282,18 +283,24 @@ public function toAudit(): array

		$tags = implode(',', $this->generateTags());

		return $this->transformAudit([

Added (optional) morphable user #420

Added (optional) morphable user #420

Conversation

crashkonijn commented May 9, 2018

Changes

quetzyg commented May 9, 2018

crashkonijn commented May 9, 2018 • edited Loading

quetzyg commented May 9, 2018

crashkonijn commented May 9, 2018

remkobrenters commented May 9, 2018

quetzyg commented May 9, 2018

quetzyg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remkobrenters commented May 9, 2018

quetzyg commented May 9, 2018 • edited Loading

crashkonijn commented May 9, 2018 • edited Loading

quetzyg commented May 9, 2018

remkobrenters commented May 10, 2018

quetzyg commented May 10, 2018 • edited Loading

remkobrenters commented May 10, 2018

quetzyg commented May 10, 2018

crashkonijn commented May 10, 2018

quetzyg commented May 10, 2018 • edited Loading

crashkonijn commented May 9, 2018 •

edited

Loading

quetzyg commented May 9, 2018 •

edited

Loading

crashkonijn commented May 9, 2018 •

edited

Loading

quetzyg commented May 10, 2018 •

edited

Loading

quetzyg commented May 10, 2018 •

edited

Loading