Feature: Banning Users #650
Conversation
| 'banned' => ['type' => 'tinyint', 'after' => 'active', 'constraint' => 1, 'null' => false, 'default' => 0], | ||
| 'banned_message' => ['type' => 'varchar', 'after' => 'banned', 'constraint' => 255, 'null' => true], |
There was a problem hiding this comment.
If banned and banned_message is going to be used instead of status and status_message, it is better, rename the previous field ($forge->renameTable) and use command ($forge->modifyColumn()) to change the properties.
This may seem like a breaking change, but I don't think it will bother many users. Because we have not been able to use status and status_message before.
This is my opinion, see what others say.
There was a problem hiding this comment.
I was thinking the status can be utilised in the future for setting the status of a user. For example, a user may say set his status to away and then have a message to add context to the status that they set.
There was a problem hiding this comment.
It seems Myth::Auth uses status for banning.
https://github.com/lonnieezell/myth-auth/blob/1c646c58e8b9b956b2163ebda8e5ec7e9ed609ce/src/Entities/User.php#L196-L202
Basically, Shield should use the minimum number of columns required.
We are hesitant to use the status because the use of the status is not clear.
If the status is the status of the user for administration, ban should use the status.
There was a problem hiding this comment.
@datamweb Renaming the column is a breaking change. If a dev already uses the status for some reason, the app will break.
But if a dev uses the status for user's current status (away, busy, etc.), then the app will break even if we use the status for banning.
After all, it is not good to have columns with unclear uses that are not being used.
There was a problem hiding this comment.
I thought about this.
Shield is an Authentication and Authorization library.
If the status is for user's current status (away, busy, etc.), it is not for Authentication,
nor for Authorization. We should not provide the status.
Essentially, banning is about authorization. So it can be implemented using the authorization function.
However, authorization does not have the ability to provide (have) a message for a user in case of non-authorization.
It seems no problem that we use the status for the user status for users who can be authenticated but cannot login now in some reason. In this case, we can use the status for banning, account locking with too many login failures.
There was a problem hiding this comment.
I think you are right there @kenjis. This means that the 2 new columns introduced for banned and banned_message are redundant but it now allows us to use the status field for a multitude of features without having to define a new column for every new feature that may be added in future. So in this case the status field would contain banned in a different scenario it may contain locked etc.
There was a problem hiding this comment.
@lonnieezell I just need a resolution on whether I should get rid of the banned and banned_message fields and use the status and status_message fields then the feature should be ready.
There was a problem hiding this comment.
@lonnieezell See this thread.
I hope this PR uses the status column for banning like Myth::Auth.
There was a problem hiding this comment.
I had read this last night. I thought it was resolved to use the status column, which I agree with. That was the original intention for that column - to be used for a variety of system-related statuses, like banning, suspension, etc.
src/Filters/SessionAuth.php
Outdated
| if ($user !== null && ! $user->isActivated()) { | ||
| $authenticator->logout(); | ||
|
|
||
| return redirect()->route('login') | ||
| return redirect()->to(config('Auth')->logoutRedirect()) |
There was a problem hiding this comment.
Why did you change? Is this related to PR?
There was a problem hiding this comment.
No it isn't related. I changed it because I felt that a the route for login may not always be named that way and may be set in the configurations. Having the named of the route hardcoded may lead to 404 errors in future
There was a problem hiding this comment.
OK, so it shouldn't be changed in this PR.
Please revert, if you think we need this change, please send in a new PR thread.
There was a problem hiding this comment.
Noted, I'll revert then add that to another pull request.
| @@ -326,4 +326,24 @@ public function testCreatedAtIfDefaultLocaleSetFaWithAddGroup(): void | |||
| Locale::setDefault($currentLocale); | |||
| Time::setTestNow(); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The test doesn't cover fully or at least 80%, however I don't mind.
Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>
Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>
Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>
Thank you for your translation Co-authored-by: kenjis <kenji.uui@gmail.com>
|
@davidnsai @kenjis How close are we to getting this one in? Looks very close. I know we'd like to get a new release out, and I'm hoping to get this one in there. Can we make this happen? |
|
@davidnsai Will you be able to make the change to use the |
I'll be done with this today. I had a busy schedule lately. The changes are not much |
Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>
Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>
Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>
Co-authored-by: Pooya Parsa Dadashi <pooya_parsa_dadashi@yahoo.com>
There was a problem hiding this comment.
@davidnsai Thank you! 🥀
@lonnieezell I think this PR is ready to merge, if there is no case, please merge.
|
@davidnsai Thanks for carrying this all of the way through! Merging. |
Supersedes #617
Closes #509
The banning user feature has been completed. All recommendations from the previous pull request have been applied. @kenjis