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

fix: LoginModel cannot use uuid for id column #1046

Merged
merged 3 commits into from
Feb 25, 2024
Merged

fix: LoginModel cannot use uuid for id column #1046

merged 3 commits into from
Feb 25, 2024

Conversation

MrFrost-Nv27
Copy link
Contributor

@MrFrost-Nv27 MrFrost-Nv27 commented Feb 24, 2024

Description
Supersedes #1044

i want use uuid for user id, so let set the validation customable or change the behavior.

my opinion is for improve jwt token security auth at sub payload not integer id, but uuid

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@MrFrost-Nv27 MrFrost-Nv27 changed the title change LoginModel validation behavior fix: LoginModel cannot use uuid for id column Feb 24, 2024
@kenjis kenjis added the bug Something isn't working label Feb 24, 2024
@kenjis
Copy link
Member

kenjis commented Feb 24, 2024

Thank you, but you missed GPG signing.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#gpg-signing-old-commits

@MrFrost-Nv27
Copy link
Contributor Author

Thank you, but you missed GPG signing. See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#gpg-signing-old-commits

Yes i forget, clear

@datamweb
Copy link
Collaborator

@MrFrost-Nv27 Can you use uuid instead of id only with this change? Have you done this practically?
As far as I remember, there were more changes needed to use uuid.

@datamweb datamweb added the stale Pull requests with conflicts label Feb 24, 2024
@MrFrost-Nv27
Copy link
Contributor Author

MrFrost-Nv27 commented Feb 24, 2024

@MrFrost-Nv27 Can you use uuid instead of id only with this change? Have you done this practically? As far as I remember, there were more changes needed to use uuid.

yes i do, LoginModel user_id get error only on jwt fail with user uuid, i delete integer and all clear, because the minimum scenario (i think) is only let validate the user id is not only integer, string so can be. But, The best way actually change the integer to uuid regex maybe, this the regex regex_match[/[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/] see https://www.codeigniter.com/user_guide/incoming/routing.html#custom-placeholders and for implement it on validation see https://www.codeigniter.com/user_guide/libraries/validation.html#rules-for-general-use but I haven't tried it yet

ahh sorry, I just meant it. If your intention is to apply UUID to the entire project, including changing the ID from the user table to UUID, that only requires 2 additional steps, namely:

  1. extending the migration file itself and changing all user id types to char(32) to prepare uuid
  2. extending the UserModel and UserEntity

fyi i use the eloquent model hehe 😄 from beginning is start with eloquent service with CI4, this my code to add a user with credential and group

UserModel::create([
            'username' => 'superadmin',
            'nama' => 'Super Admin',
        ])->setEmailIdentity([
                    'email' => 'superadmin@gmail.com',
                    'password' => "password",
                ])->addGroup('superadmin')->activate();

@datamweb
Copy link
Collaborator

But, The best way actually change the integer to uuid regex maybe

Honestly, I don't agree with the complete removal of the rule and I don't feel good. But it seems that using a regex is a good way.

And your branch is currently conflicting, please rebase and fix conflicts.

src/Models/LoginModel.php Outdated Show resolved Hide resolved
Co-authored-by: Pooya Parsa <pooya_parsa_dadashi@yahoo.com>
@datamweb datamweb removed the stale Pull requests with conflicts label Feb 24, 2024
@kenjis kenjis merged commit 904e3d1 into codeigniter4:develop Feb 25, 2024
32 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants