Adds checking of reserved keywords against team names #22
Conversation
ghost
added
the
|
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.
I did it in the same manner as models/user.go#L504:L508.
There was a problem hiding this comment.
Macaron isn't (apparently) case insensitive ( test by going to /org/CREATE instead of /org/create )
There was a problem hiding this comment.
@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.
Yes, the others are right, move that below the if len(t.Name) == 0 { check.
There was a problem hiding this comment.
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.
@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.
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... |
ghost
deleted the
tboerger
added
the
lgtm/done
* Don't trim trailing whitespaces for markdown * Update .drone.yml.sig
This fixes and solves gogit/gogs#3789