Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature: Banning Users #650
Feature: Banning Users #650
Changes from 19 commits
a2db120
0293227
9886eb4
cae9014
1af7fd3
2bb6a4f
d40e090
ddfed19
00654ed
b8625de
c9963dd
d6c2425
5cddb94
28206d1
1bc78de
0e27d3b
7088052
ad2984f
70ab02f
f256b69
a186793
8591f67
5fdc155
040f17a
9d54ac5
6371431
df48197
206fe9a
c00b84e
ed74635
88bb918
524a6c9
fb1e851
7134871
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
banned
andbanned_message
is going to be used instead ofstatus
andstatus_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
andstatus_message
before.This is my opinion, see what others say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thestatus
is not clear.If the
status
is the status of the user for administration, ban should use thestatus
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 thestatus
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thestatus
for banning, account locking with too many login failures.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are right there @kenjis. This means that the 2 new columns introduced for
banned
andbanned_message
are redundant but it now allows us to use thestatus
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 thestatus
field would containbanned
in a different scenario it may containlocked
etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lonnieezell See this thread.
I hope this PR uses the
status
column for banning like Myth::Auth.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change? Is this related to PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, I'll revert then add that to another pull request.