-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Adds checking of reserved keywords against team names #22
Conversation
LGTM but needs to be rebased |
Please reopen against master. |
Would it make sense for the empty name case to be handled by the IsUsableTeamName directly ? |
Full link to original issue: gogs/gogs#3789 |
I agree with @strk , if the team name is empty, don't enter on Maybe forbidden team names could be move to |
var reservedTeamNames = []string{"new"} | ||
|
||
for i := range reservedTeamNames { | ||
if name == reservedTeamNames[i] { |
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.
case sensitive?
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 did it in the same manner as models/user.go#L504:L508
.
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.
Macaron isn't (apparently) case insensitive ( test by going to /org/CREATE
instead of /org/create
)
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.
@lunny is right, you should always downcase the name.
This list contains only names that are forbidden to prevent issues with the routes. |
// NewTeam creates a record of new team. | ||
// It's caller's responsibility to assign organization ID. | ||
func NewTeam(t *Team) error { | ||
func NewTeam(t *Team) (err error) { | ||
if err = IsUsableTeamName(t.Name); err != nil { |
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.
Yes, the others are right, move that below the if len(t.Name) == 0 {
check.
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.
My suggestion was actually to move the len(t.Name) == 0
check inside IsUsableTeamName
(as empty is clearly not a usable team name)
@tboerger Fixed. |
Current coverage is 2.18% (diff: 0.00%)@@ master #22 diff @@
========================================
Files 31 31
Lines 7508 7516 +8
Methods 0 0
Messages 0 0
Branches 0 0
========================================
Hits 164 164
- Misses 7327 7335 +8
Partials 17 17
|
LGTM |
var reservedTeamNames = []string{"new"} | ||
|
||
for i := range reservedTeamNames { | ||
if name == reservedTeamNames[i] { |
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.
@lunny is right, you should always downcase the name.
func IsUsableTeamName(name string) (err error) { | ||
var reservedTeamNames = []string{"new"} | ||
|
||
for i := range reservedTeamNames { |
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.
for _, reservedName := range reservedTeamNames {
if reservedName == strings.ToLower(name) {
}
}
Sorry, but after seeing the comment of @lunny I changed my opinion :D |
@tboerger changed your mind so you want to |
Assuming everyone is happy with this, merging (since github lets me) |
I didn't realized that before, but why have that been merged when there are still requests for changes left? I requested to do always a downcase check... |
* Don't trim trailing whitespaces for markdown * Update .drone.yml.sig
This fixes and solves gogit/gogs#3789