codeigniter4 / shield

Authentication and Authorization for CodeIgniter 4
https://shield.codeigniter.com
MIT License
351 stars 123 forks source link

Bug: when active field in users table is `false` users can login #459

Closed datamweb closed 1 year ago

datamweb commented 1 year ago

PHP Version

8.1.5

CodeIgniter4 Version

4.2.5

Shield Version

dev

Which operating systems have you tested for this bug?

Windows

Which server did you use?

apache

Database

MySQL 5.6

Did you customize Shield?

No.

What happened?

The login operation for a user who is not active is completed successfully. In general, a user who is not active for any reason should not login to the system.

Steps to Reproduce

  1. Register a new user.
  2. Change the value of active true to false.
  3. Perform the login operation.
  4. A user who is inactive login without a problem!!

Expected Output

Failure of the user login to the system and display the error message "Your account is inactive".

Anything else?

see https://forum.codeigniter.com/showthread.php?tid=83481

kenjis commented 1 year ago

I don't know this is a bug.

$user->active is never used (read) in the current codebase. So we could just remove the column.

When we need user activation, we are doing it by an Auth Action (and a UserIdentiy record).

datamweb commented 1 year ago

@kenjis, I think using the active column is necessary. In a real project, the site administrator may want to deactivate users. This feature is one of the most basic features of any AUTH. In my opinion, removing a necessary feature is not an interesting task, if it is not implemented, there is no reason to remove it.

MGatner commented 1 year ago

I think @kenjis' point was: "this isn't a bug, it is an extra unused column". If we want to turn it into a feature request I think that it would be a great feature to have. It is probably a port from Myth where you could "ban" a user.

kenjis commented 1 year ago

@MGatner Yes.

Currently, Shield does not have the feature for an administrator to ban a user from logging in. Also, there is no exact specification of what the active column in the users table represents. The column will be 1 after email activation when we use email activation.

The feature to ban a user is probably required for many sites, so adding it is not a problem. Yes, it would be great.

However, I think we need to consider whether to use users.active for its implementation.

grisinformatica commented 1 year ago

see https://forum.codeigniter.com/showthread.php?tid=83481

Hi there, I open that Topic in Codeigniter Forums, and made more posts days ago (The post made by you is under moderation and currently not visible publicly. The post will be visible to everyone once a moderator approves it) :(

It is probably a port from Myth where you could "ban" a user.

I think so, also with users.status and users.status_message, both seems unused by the system.

datamweb commented 1 year ago

Guys, I think we should go the following route.

Use the users.active column to check whether the user's email (or mobile number) is verified or not. Note : For more readability, we can rename the activecolumn to is_verified.

Use the users.status to check the user's status, if it is true, it can be login, if it is false, display the message of inactivity to the user. users.status_message can also contain the text of the message, for example, the administrator explains why the user is false.

do you agree Or do you have any additional comments?

grisinformatica commented 1 year ago

Use the users.status to check the user's status, if it is true, it can be login, if it is false, display the message of inactivity to the user. users.status_message can also contain the text of the message, for example, the administrator explains why the user is false.

users.status is varchar, was used in myth-auth for banned tag. I assume type is varchar for expanding the status to multiple uses.

kenjis commented 1 year ago

Yes, I think users.status is effectively an enum, not a bool.

lonnieezell commented 1 year ago

Yes, sorry, I seem to have forgotten to implement that. But the idea was that active was for email validation status. We should definitely check that during filters if we're not already.

status was intended to be able to use for multiple states of the user depending on the application. It could hold a warning with a warning message in status_message, or banning a user as is being discussed here. I intended to implement banning but look to have forgotten to.