Skip to content

Conversation

@lonnieezell
Copy link
Member

@lonnieezell lonnieezell commented Jan 4, 2023

Fixes #459

Previously, the active column for the user was not being used except that the EmailActivator flow would set it. This PR addresses this by:

  • Adding a new Activatable trait that is used on the User entity. This provides methods to check if a user is activated or not, and utility methods to activate/deactivate that user.
  • The SessionAuth and TokenAuth filters are updated to check the user's active status and redirect to login/return a 403 status code, respectively, when they aren't active. This takes into account if no activator is required after registration.

@kenjis kenjis changed the title feat: User activation checks and utility functions. Fixes #459 feat: User activation checks and utility functions. Jan 4, 2023
@kenjis kenjis added the enhancement New feature or request label Jan 4, 2023
@kenjis kenjis added the breaking change Pull requests that may break existing functionalities label Jan 5, 2023
return redirect()->to('/login');
return service('response')
->setStatusCode(Response::HTTP_FORBIDDEN)
->setJson(['message' => lang('Auth.badToken')]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change. It is better to document it.

#433 suggests 401, not 403. Which is better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for 401. For example, I already using it in few deployments with own api-filter. I would like to switch to this one if it's will be suitable for me.

Copy link
Contributor

@jozefrebjak jozefrebjak Jan 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

401 Unauthorized is the status code to return when the client provides no credentials or invalid credentials. 403 Forbidden is the status code to return when a client has valid credentials but not enough privileges to perform an action on a resource.

Receiving a 403 response is the server telling you, “I’m sorry. I know who you are–I believe who you say you are–but you just don’t have permission to access this resource. Maybe if you ask the system administrator nicely, you’ll get permission. But please don’t bother me again until your predicament changes.”

In summary, a 401 Unauthorized response should be used for missing or bad authentication, and a 403 Forbidden response should be used afterwards, when the user is authenticated but isn’t authorized to perform the requested operation on the given resource.

@lonnieezell Here we are mixing authentication and authorization. Maybe we need to first check if a client provided a valid token and if not, return 401 and after that, check, scopes and if there is no valid scope, return 403.

Copy link
Member

@kenjis kenjis Jan 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HyperText Transfer Protocol (HTTP) 401 Unauthorized response status code indicates that the client request has not been completed because it lacks valid authentication credentials for the requested resource.

This status code is similar to the 403 Forbidden status code, except that in situations resulting in this status code, user authentication can allow access to the resource.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/401

The HTTP 403 Forbidden response status code indicates that the server understands the request but refuses to authorize it.

This status is similar to 401, but for the 403 Forbidden status code, re-authenticating makes no difference. The access is tied to the application logic, such as insufficient rights to a resource.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/403

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These make sense. First check will result in a 401, with the not-activated check being a 403.

@datamweb datamweb closed this Jan 12, 2023
@datamweb datamweb reopened this Jan 12, 2023
/**
* Activates the user.
*/
public function activate(): bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change the return type to void?
The UserModel::update() throws an exception when the update fails.
So returning bool does not make sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless it's changing in a PR, update() currently returns a boolean. Changing this to void throws errors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sent a PR #626

/**
* Deactivates the user.
*/
public function deactivate(): bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same above.

@datamweb
Copy link
Collaborator

datamweb commented Feb 3, 2023

We have a new language file pt-BR, please add:

    'activationBlocked'     => '(to be translated) You must activate your account before logging in.',

Also conflict in file src/Entities/User.php.

Copy link
Collaborator

@datamweb datamweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kenjis, we can address your opinion separately in PR. This PR has taken too long.

@kenjis kenjis merged commit 112df4a into codeigniter4:develop Feb 5, 2023
@lonnieezell lonnieezell deleted the active-check branch February 5, 2023 04:52
@lonnieezell
Copy link
Member Author

Thanks!

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

Labels

breaking change Pull requests that may break existing functionalities enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

4 participants