From 2135096d886dad9055640edf881661004312c69b Mon Sep 17 00:00:00 2001 From: Arianna Vespri <36129782+vesari@users.noreply.github.com> Date: Sat, 23 May 2020 23:01:31 +0200 Subject: [PATCH] Convert bool string comparisons to strconv.ParseBool for REST parameters (#13650) * Convert bool string comparisons to strconv.ParseBool for REST parameters * Log failed bool conversions * Rename errors, changed log levels * drop strconv.ParseBool error handling If the query string parameter is omitted, strconv.ParseBool returns an error for the empty strings, which spams the logs. Instead, just assume the default semantics of a `false` return value if an error occurs. * allow randomized Client4 booleans It's hard to test api4's handling of the various boolean input values accepted. Extend Client4 with support for overriding how it builds those strings, and pick a random value on test startup. * gofmt -s Co-authored-by: mattermod Co-authored-by: Ben Schumacher Co-authored-by: Jesse Hallam --- api4/apitestlib.go | 11 ++++++++ api4/bot.go | 18 ++++++------ api4/channel.go | 41 ++++++++++++++------------- api4/command.go | 5 +--- api4/file.go | 17 ++---------- api4/plugin.go | 8 ++---- model/client4.go | 69 +++++++++++++++++++++++++++++++++------------- 7 files changed, 96 insertions(+), 73 deletions(-) diff --git a/api4/apitestlib.go b/api4/apitestlib.go index fa0aa18a51..1e62b64641 100644 --- a/api4/apitestlib.go +++ b/api4/apitestlib.go @@ -6,6 +6,7 @@ package api4 import ( "fmt" "io/ioutil" + "math/rand" "net" "net/http" "os" @@ -153,6 +154,16 @@ func setupTestHelper(dbStore store.Store, searchEngine *searchengine.Broker, ent th.Client = th.CreateClient() th.SystemAdminClient = th.CreateClient() + // Verify handling of the supported true/false values by randomizing on each run. + rand.Seed(time.Now().UTC().UnixNano()) + trueValues := []string{"1", "t", "T", "TRUE", "true", "True"} + falseValues := []string{"0", "f", "F", "FALSE", "false", "False"} + trueString := trueValues[rand.Intn(len(trueValues))] + falseString := falseValues[rand.Intn(len(falseValues))] + mlog.Debug("Configured Client4 bool string values", mlog.String("true", trueString), mlog.String("false", falseString)) + th.Client.SetBoolString(true, trueString) + th.Client.SetBoolString(false, falseString) + th.LocalClient = th.CreateLocalClient(*config.ServiceSettings.LocalModeSocketLocation) if th.tempWorkspace == "" { diff --git a/api4/bot.go b/api4/bot.go index 95d516bffc..af5d2208f6 100644 --- a/api4/bot.go +++ b/api4/bot.go @@ -115,11 +115,11 @@ func getBot(c *Context, w http.ResponseWriter, r *http.Request) { } botUserId := c.Params.BotUserId - includeDeleted := r.URL.Query().Get("include_deleted") == "true" + includeDeleted, _ := strconv.ParseBool(r.URL.Query().Get("include_deleted")) - bot, err := c.App.GetBot(botUserId, includeDeleted) - if err != nil { - c.Err = err + bot, appErr := c.App.GetBot(botUserId, includeDeleted) + if appErr != nil { + c.Err = appErr return } @@ -148,8 +148,8 @@ func getBot(c *Context, w http.ResponseWriter, r *http.Request) { } func getBots(c *Context, w http.ResponseWriter, r *http.Request) { - includeDeleted := r.URL.Query().Get("include_deleted") == "true" - onlyOrphaned := r.URL.Query().Get("only_orphaned") == "true" + includeDeleted, _ := strconv.ParseBool(r.URL.Query().Get("include_deleted")) + onlyOrphaned, _ := strconv.ParseBool(r.URL.Query().Get("only_orphaned")) var OwnerId string if c.App.SessionHasPermissionTo(*c.App.Session(), model.PERMISSION_READ_OTHERS_BOTS) { @@ -163,15 +163,15 @@ func getBots(c *Context, w http.ResponseWriter, r *http.Request) { return } - bots, err := c.App.GetBots(&model.BotGetOptions{ + bots, appErr := c.App.GetBots(&model.BotGetOptions{ Page: c.Params.Page, PerPage: c.Params.PerPage, OwnerId: OwnerId, IncludeDeleted: includeDeleted, OnlyOrphaned: onlyOrphaned, }) - if err != nil { - c.Err = err + if appErr != nil { + c.Err = appErr return } diff --git a/api4/channel.go b/api4/channel.go index 5b237ee548..d02de7eea6 100644 --- a/api4/channel.go +++ b/api4/channel.go @@ -6,6 +6,7 @@ package api4 import ( "encoding/json" "net/http" + "strconv" "strings" "github.com/mattermost/mattermost-server/v5/audit" @@ -959,18 +960,18 @@ func searchAllChannels(c *Context, w http.ResponseWriter, r *http.Request) { c.SetPermissionError(model.PERMISSION_MANAGE_SYSTEM) return } - + includeDeleted, _ := strconv.ParseBool(r.URL.Query().Get("include_deleted")) opts := model.ChannelSearchOpts{ NotAssociatedToGroup: props.NotAssociatedToGroup, ExcludeDefaultChannels: props.ExcludeDefaultChannels, - IncludeDeleted: r.URL.Query().Get("include_deleted") == "true", + IncludeDeleted: includeDeleted, Page: props.Page, PerPage: props.PerPage, } - channels, totalCount, err := c.App.SearchAllChannels(props.Term, opts) - if err != nil { - c.Err = err + channels, totalCount, appErr := c.App.SearchAllChannels(props.Term, opts) + if appErr != nil { + c.Err = appErr return } @@ -1037,11 +1038,10 @@ func getChannelByName(c *Context, w http.ResponseWriter, r *http.Request) { return } - includeDeleted := r.URL.Query().Get("include_deleted") == "true" - - channel, err := c.App.GetChannelByName(c.Params.ChannelName, c.Params.TeamId, includeDeleted) - if err != nil { - c.Err = err + includeDeleted, _ := strconv.ParseBool(r.URL.Query().Get("include_deleted")) + channel, appErr := c.App.GetChannelByName(c.Params.ChannelName, c.Params.TeamId, includeDeleted) + if appErr != nil { + c.Err = appErr return } @@ -1057,9 +1057,9 @@ func getChannelByName(c *Context, w http.ResponseWriter, r *http.Request) { } } - err = c.App.FillInChannelProps(channel) - if err != nil { - c.Err = err + appErr = c.App.FillInChannelProps(channel) + if appErr != nil { + c.Err = appErr return } @@ -1072,11 +1072,10 @@ func getChannelByNameForTeamName(c *Context, w http.ResponseWriter, r *http.Requ return } - includeDeleted := r.URL.Query().Get("include_deleted") == "true" - - channel, err := c.App.GetChannelByNameForTeamName(c.Params.ChannelName, c.Params.TeamName, includeDeleted) - if err != nil { - c.Err = err + includeDeleted, _ := strconv.ParseBool(r.URL.Query().Get("include_deleted")) + channel, appErr := c.App.GetChannelByNameForTeamName(c.Params.ChannelName, c.Params.TeamName, includeDeleted) + if appErr != nil { + c.Err = appErr return } @@ -1085,9 +1084,9 @@ func getChannelByNameForTeamName(c *Context, w http.ResponseWriter, r *http.Requ return } - err = c.App.FillInChannelProps(channel) - if err != nil { - c.Err = err + appErr = c.App.FillInChannelProps(channel) + if appErr != nil { + c.Err = appErr return } diff --git a/api4/command.go b/api4/command.go index 209f8599cc..27aa6aad07 100644 --- a/api4/command.go +++ b/api4/command.go @@ -213,10 +213,7 @@ func deleteCommand(c *Context, w http.ResponseWriter, r *http.Request) { } func listCommands(c *Context, w http.ResponseWriter, r *http.Request) { - customOnly, failConv := strconv.ParseBool(r.URL.Query().Get("custom_only")) - if failConv != nil { - customOnly = false - } + customOnly, _ := strconv.ParseBool(r.URL.Query().Get("custom_only")) teamId := r.URL.Query().Get("team_id") if len(teamId) == 0 { diff --git a/api4/file.go b/api4/file.go index 6fc596d949..f7e48393b1 100644 --- a/api4/file.go +++ b/api4/file.go @@ -464,10 +464,7 @@ func getFile(c *Context, w http.ResponseWriter, r *http.Request) { return } - forceDownload, convErr := strconv.ParseBool(r.URL.Query().Get("download")) - if convErr != nil { - forceDownload = false - } + forceDownload, _ := strconv.ParseBool(r.URL.Query().Get("download")) auditRec := c.MakeAuditRecord("getFile", audit.Fail) defer c.LogAuditRec(auditRec) @@ -508,11 +505,7 @@ func getFileThumbnail(c *Context, w http.ResponseWriter, r *http.Request) { return } - forceDownload, convErr := strconv.ParseBool(r.URL.Query().Get("download")) - if convErr != nil { - forceDownload = false - } - + forceDownload, _ := strconv.ParseBool(r.URL.Query().Get("download")) info, err := c.App.GetFileInfo(c.Params.FileId) if err != nil { c.Err = err @@ -591,11 +584,7 @@ func getFilePreview(c *Context, w http.ResponseWriter, r *http.Request) { return } - forceDownload, convErr := strconv.ParseBool(r.URL.Query().Get("download")) - if convErr != nil { - forceDownload = false - } - + forceDownload, _ := strconv.ParseBool(r.URL.Query().Get("download")) info, err := c.App.GetFileInfo(c.Params.FileId) if err != nil { c.Err = err diff --git a/api4/plugin.go b/api4/plugin.go index 3e4bc08442..fb10d86afe 100644 --- a/api4/plugin.go +++ b/api4/plugin.go @@ -106,7 +106,7 @@ func installPluginFromUrl(c *Context, w http.ResponseWriter, r *http.Request) { return } - force := r.URL.Query().Get("force") == "true" + force, _ := strconv.ParseBool(r.URL.Query().Get("force")) downloadURL := r.URL.Query().Get("plugin_download_url") auditRec.AddMeta("url", downloadURL) @@ -364,11 +364,7 @@ func parseMarketplacePluginFilter(u *url.URL) (*model.MarketplacePluginFilter, e filter := u.Query().Get("filter") serverVersion := u.Query().Get("server_version") - localOnly, err := strconv.ParseBool(u.Query().Get("local_only")) - if err != nil { - localOnly = false - } - + localOnly, _ := strconv.ParseBool(u.Query().Get("local_only")) return &model.MarketplacePluginFilter{ Page: page, PerPage: perPage, diff --git a/model/client4.go b/model/client4.go index 06e5fb2bcb..96392067dd 100644 --- a/model/client4.go +++ b/model/client4.go @@ -61,6 +61,40 @@ type Client4 struct { AuthToken string AuthType string HttpHeader map[string]string // Headers to be copied over for each request + + // TrueString is the string value sent to the server for true boolean query parameters. + trueString string + + // FalseString is the string value sent to the server for false boolean query parameters. + falseString string +} + +// SetBoolString is a helper method for overriding how true and false query string parameters are +// sent to the server. +// +// This method is only exposed for testing. It is never necessary to configure these values +// in production. +func (c *Client4) SetBoolString(value bool, valueStr string) { + if value { + c.trueString = valueStr + } else { + c.falseString = valueStr + } +} + +// boolString builds the query string parameter for boolean values. +func (c *Client4) boolString(value bool) string { + if value && c.trueString != "" { + return c.trueString + } else if !value && c.falseString != "" { + return c.falseString + } + + if value { + return "true" + } else { + return "false" + } } func closeBody(r *http.Response) { @@ -81,7 +115,7 @@ func (c *Client4) Must(result interface{}, resp *Response) interface{} { } func NewAPIv4Client(url string) *Client4 { - return &Client4{url, url + API_URL_SUFFIX, &http.Client{}, "", "", map[string]string{}} + return &Client4{url, url + API_URL_SUFFIX, &http.Client{}, "", "", map[string]string{}, "", ""} } func BuildErrorResponse(r *http.Response, err *AppError) *Response { @@ -650,7 +684,7 @@ func (c *Client4) LoginByLdap(loginId string, password string) (*User, *Response m := make(map[string]string) m["login_id"] = loginId m["password"] = password - m["ldap_only"] = "true" + m["ldap_only"] = c.boolString(true) return c.login(m) } @@ -1476,7 +1510,7 @@ func (c *Client4) GetBot(userId string, etag string) (*Bot, *Response) { // GetBot fetches the given bot, even if it is deleted. func (c *Client4) GetBotIncludeDeleted(userId string, etag string) (*Bot, *Response) { - r, err := c.DoApiGet(c.GetBotRoute(userId)+"?include_deleted=true", etag) + r, err := c.DoApiGet(c.GetBotRoute(userId)+"?include_deleted="+c.boolString(true), etag) if err != nil { return nil, BuildErrorResponse(r, err) } @@ -1497,7 +1531,7 @@ func (c *Client4) GetBots(page, perPage int, etag string) ([]*Bot, *Response) { // GetBotsIncludeDeleted fetches the given page of bots, including deleted. func (c *Client4) GetBotsIncludeDeleted(page, perPage int, etag string) ([]*Bot, *Response) { - query := fmt.Sprintf("?page=%v&per_page=%v&include_deleted=true", page, perPage) + query := fmt.Sprintf("?page=%v&per_page=%v&include_deleted="+c.boolString(true), page, perPage) r, err := c.DoApiGet(c.GetBotsRoute()+query, etag) if err != nil { return nil, BuildErrorResponse(r, err) @@ -1508,7 +1542,7 @@ func (c *Client4) GetBotsIncludeDeleted(page, perPage int, etag string) ([]*Bot, // GetBotsOrphaned fetches the given page of bots, only including orphanded bots. func (c *Client4) GetBotsOrphaned(page, perPage int, etag string) ([]*Bot, *Response) { - query := fmt.Sprintf("?page=%v&per_page=%v&only_orphaned=true", page, perPage) + query := fmt.Sprintf("?page=%v&per_page=%v&only_orphaned="+c.boolString(true), page, perPage) r, err := c.DoApiGet(c.GetBotsRoute()+query, etag) if err != nil { return nil, BuildErrorResponse(r, err) @@ -1648,7 +1682,7 @@ func (c *Client4) GetAllTeams(etag string, page int, perPage int) ([]*Team, *Res // GetAllTeamsWithTotalCount returns all teams based on permissions. func (c *Client4) GetAllTeamsWithTotalCount(etag string, page int, perPage int) ([]*Team, int64, *Response) { - query := fmt.Sprintf("?page=%v&per_page=%v&include_total_count=true", page, perPage) + query := fmt.Sprintf("?page=%v&per_page=%v&include_total_count="+c.boolString(true), page, perPage) r, err := c.DoApiGet(c.GetTeamsRoute()+query, etag) if err != nil { return nil, 0, BuildErrorResponse(r, err) @@ -1800,7 +1834,7 @@ func (c *Client4) SoftDeleteTeam(teamId string) (bool, *Response) { // PermanentDeleteTeam deletes the team, should only be used when needed for // compliance and the like. func (c *Client4) PermanentDeleteTeam(teamId string) (bool, *Response) { - r, err := c.DoApiDelete(c.GetTeamRoute(teamId) + "?permanent=true") + r, err := c.DoApiDelete(c.GetTeamRoute(teamId) + "?permanent=" + c.boolString(true)) if err != nil { return false, BuildErrorResponse(r, err) } @@ -1920,7 +1954,7 @@ func (c *Client4) AddTeamMembersGracefully(teamId string, userIds []string) ([]* members = append(members, member) } - r, err := c.DoApiPost(c.GetTeamMembersRoute(teamId)+"/batch?graceful=true", TeamMembersToJson(members)) + r, err := c.DoApiPost(c.GetTeamMembersRoute(teamId)+"/batch?graceful="+c.boolString(true), TeamMembersToJson(members)) if err != nil { return nil, BuildErrorResponse(r, err) } @@ -2038,7 +2072,7 @@ func (c *Client4) InviteGuestsToTeam(teamId string, userEmails []string, channel // InviteUsersToTeam invite users by email to the team. func (c *Client4) InviteUsersToTeamGracefully(teamId string, userEmails []string) ([]*EmailInviteWithError, *Response) { - r, err := c.DoApiPost(c.GetTeamRoute(teamId)+"/invite/email?graceful=true", ArrayToJson(userEmails)) + r, err := c.DoApiPost(c.GetTeamRoute(teamId)+"/invite/email?graceful="+c.boolString(true), ArrayToJson(userEmails)) if err != nil { return nil, BuildErrorResponse(r, err) } @@ -2053,7 +2087,7 @@ func (c *Client4) InviteGuestsToTeamGracefully(teamId string, userEmails []strin Channels: channels, Message: message, } - r, err := c.DoApiPost(c.GetTeamRoute(teamId)+"/invite-guests/email?graceful=true", guestsInvite.ToJson()) + r, err := c.DoApiPost(c.GetTeamRoute(teamId)+"/invite-guests/email?graceful="+c.boolString(true), guestsInvite.ToJson()) if err != nil { return nil, BuildErrorResponse(r, err) } @@ -2163,7 +2197,7 @@ func (c *Client4) GetAllChannels(page int, perPage int, etag string) (*ChannelLi // GetAllChannelsWithCount get all the channels including the total count. Must be a system administrator. func (c *Client4) GetAllChannelsWithCount(page int, perPage int, etag string) (*ChannelListWithTeamData, int64, *Response) { - query := fmt.Sprintf("?page=%v&per_page=%v&include_total_count=true", page, perPage) + query := fmt.Sprintf("?page=%v&per_page=%v&include_total_count="+c.boolString(true), page, perPage) r, err := c.DoApiGet(c.GetChannelsRoute()+query, etag) if err != nil { return nil, 0, BuildErrorResponse(r, err) @@ -2410,7 +2444,7 @@ func (c *Client4) GetChannelByName(channelName, teamId string, etag string) (*Ch // GetChannelByNameIncludeDeleted returns a channel based on the provided channel name and team id strings. Other then GetChannelByName it will also return deleted channels. func (c *Client4) GetChannelByNameIncludeDeleted(channelName, teamId string, etag string) (*Channel, *Response) { - r, err := c.DoApiGet(c.GetChannelByNameRoute(channelName, teamId)+"?include_deleted=true", etag) + r, err := c.DoApiGet(c.GetChannelByNameRoute(channelName, teamId)+"?include_deleted="+c.boolString(true), etag) if err != nil { return nil, BuildErrorResponse(r, err) } @@ -2430,7 +2464,7 @@ func (c *Client4) GetChannelByNameForTeamName(channelName, teamName string, etag // GetChannelByNameForTeamNameIncludeDeleted returns a channel based on the provided channel name and team name strings. Other then GetChannelByNameForTeamName it will also return deleted channels. func (c *Client4) GetChannelByNameForTeamNameIncludeDeleted(channelName, teamName string, etag string) (*Channel, *Response) { - r, err := c.DoApiGet(c.GetChannelByNameForTeamNameRoute(channelName, teamName)+"?include_deleted=true", etag) + r, err := c.DoApiGet(c.GetChannelByNameForTeamNameRoute(channelName, teamName)+"?include_deleted="+c.boolString(true), etag) if err != nil { return nil, BuildErrorResponse(r, err) } @@ -3048,7 +3082,7 @@ func (c *Client4) GetPing() (string, *Response) { // GetPingWithServerStatus will return ok if several basic server health checks // all pass successfully. func (c *Client4) GetPingWithServerStatus() (string, *Response) { - r, err := c.DoApiGet(c.GetSystemRoute()+"/ping?get_server_status=true", "") + r, err := c.DoApiGet(c.GetSystemRoute()+"/ping?get_server_status="+c.boolString(true), "") if r != nil && r.StatusCode == 500 { defer r.Body.Close() return STATUS_UNHEALTHY, BuildErrorResponse(r, err) @@ -4638,7 +4672,7 @@ func (c *Client4) uploadPlugin(file io.Reader, force bool) (*Manifest, *Response writer := multipart.NewWriter(body) if force { - err := writer.WriteField("force", "true") + err := writer.WriteField("force", c.boolString(true)) if err != nil { return nil, &Response{Error: NewAppError("UploadPlugin", "model.client.writer.app_error", nil, err.Error(), 0)} } @@ -4681,10 +4715,7 @@ func (c *Client4) uploadPlugin(file io.Reader, force bool) (*Manifest, *Response } func (c *Client4) InstallPluginFromUrl(downloadUrl string, force bool) (*Manifest, *Response) { - forceStr := "false" - if force { - forceStr = "true" - } + forceStr := c.boolString(force) url := fmt.Sprintf("%s?plugin_download_url=%s&force=%s", c.GetPluginsRoute()+"/install_from_url", url.QueryEscape(downloadUrl), forceStr) r, err := c.DoApiPost(url, "")