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

Using UUid instead of Native ID's #793

Closed
sineld opened this issue Jul 11, 2018 · 4 comments · Fixed by #2089
Closed

Using UUid instead of Native ID's #793

sineld opened this issue Jul 11, 2018 · 4 comments · Fixed by #2089

Comments

@sineld
Copy link

sineld commented Jul 11, 2018

Hi,

I have updated my migrations to accept uuids instead of ids but when giving roles to users or giving permissions to roles, these two pieces occur issue because uuids appear like names:

Spatie\Permission\Traits\HasRoles

        if (is_string($role)) {
            return app(Role::class)->findByName($role, $this->getDefaultGuardName());
        }

and
Spatie\Permission\Traits\HasPermissions

        if (is_string($permissions)) {
            return app(Permission::class)->findByName($permissions, $this->getDefaultGuardName());
        }

So I added a custom trait to my Role model as below to solve this issue:

    namespace App\Traits\Acl;

    use App\Models\Role;
    use App\Models\Permission;

    trait UuidInsteadOfId
    {
        protected function getStoredRole($role) : Role
        {
            if (strlen($role) == 36) {
                return app(Role::class)
                    ->where('id', $role)
                    ->where('guard_name', $this->getDefaultGuardName())
                    ->first();
            }

            if (is_numeric($role)) {
                return app(Role::class)->findById($role, $this->getDefaultGuardName());
            }

            if (is_string($role)) {
                return app(Role::class)->findByName($role, $this->getDefaultGuardName());
            }

            return $role;
        }

        protected function getStoredPermission($permissions)
        {
            if (strlen($permissions) == 36) {
                return app(Permission::class)
                    ->where('id', $permissions)
                    ->where('guard_name', $this->getDefaultGuardName())
                    ->first();
            }

            if (is_numeric($permissions)) {
                return app(Permission::class)->findById($permissions, $this->getDefaultGuardName());
            }

            if (is_string($permissions)) {
                return app(Permission::class)->findByName($permissions, $this->getDefaultGuardName());
            }

            if (is_array($permissions)) {
                return app(Permission::class)
                    ->whereIn('name', $permissions)
                    ->whereIn('guard_name', $this->getGuardNames())
                    ->get();
            }

            return $permissions;
        }
    }

I am sure if (strlen($permissions) == 36) { is not the best practice, but as a temporary solution it worked for me.

Thanks for this great work.

@vpratfr
Copy link
Contributor

vpratfr commented Aug 13, 2018

A better check would be if (Uuid::isValid($role))

Would be nice to be able to have a PK name uuid instead of id too.

@drbyte
Copy link
Collaborator

drbyte commented Aug 13, 2018

@vpratfr with #827 merged, what do you think of a PR to update the above methods, to test for UUID/GUID and lookup accordingly?

@vpratfr
Copy link
Contributor

vpratfr commented Aug 13, 2018

I had a quick look while doing the other PR, but that is clearly another story :)

One would need:

  • Ability to provide custom PK name
  • Clean way of integrating the UUID generation code
  • Checking all queries which are currently done on the model class, with the hardcoded id field which is supposed to be the model PK

Currently slightly out of time for that. But I will consider having a look when I get some time.

Our current application is using regular IDs for roles/permissions. And on the user side, we only refer to names (never IDs), hence the switch to IDs for those 2 models is not critical to us for now.

@drbyte
Copy link
Collaborator

drbyte commented Nov 20, 2018

Closing because there are no plans to implement this at this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants