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 <mattermod@users.noreply.github.com>
Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>
Co-authored-by: Jesse Hallam <jesse.hallam@gmail.com>

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 == "" {

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)
+	bot, appErr := c.App.GetBot(botUserId, includeDeleted)
-	if err != nil {
+	if appErr != nil {
-		c.Err = err
+		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"
+	includeDeleted, _ := strconv.ParseBool(r.URL.Query().Get("include_deleted"))
-	onlyOrphaned := r.URL.Query().Get("only_orphaned") == "true"
+	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 {
+	if appErr != nil {
-		c.Err = err
+		c.Err = appErr
 		return
 	}
 

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)
+	channels, totalCount, appErr := c.App.SearchAllChannels(props.Term, opts)
-	if err != nil {
+	if appErr != nil {
-		c.Err = err
+		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"
+	includeDeleted, _ := strconv.ParseBool(r.URL.Query().Get("include_deleted"))
-
+	channel, appErr := c.App.GetChannelByName(c.Params.ChannelName, c.Params.TeamId, includeDeleted)
-	channel, err := c.App.GetChannelByName(c.Params.ChannelName, c.Params.TeamId, includeDeleted)
+	if appErr != nil {
-	if err != nil {
+		c.Err = appErr
-		c.Err = err
 		return
 	}
 
@@ -1057,9 +1057,9 @@ func getChannelByName(c *Context, w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
-	err = c.App.FillInChannelProps(channel)
+	appErr = c.App.FillInChannelProps(channel)
-	if err != nil {
+	if appErr != nil {
-		c.Err = err
+		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"
+	includeDeleted, _ := strconv.ParseBool(r.URL.Query().Get("include_deleted"))
-
+	channel, appErr := c.App.GetChannelByNameForTeamName(c.Params.ChannelName, c.Params.TeamName, includeDeleted)
-	channel, err := c.App.GetChannelByNameForTeamName(c.Params.ChannelName, c.Params.TeamName, includeDeleted)
+	if appErr != nil {
-	if err != nil {
+		c.Err = appErr
-		c.Err = err
 		return
 	}
 
@@ -1085,9 +1084,9 @@ func getChannelByNameForTeamName(c *Context, w http.ResponseWriter, r *http.Requ
 		return
 	}
 
-	err = c.App.FillInChannelProps(channel)
+	appErr = c.App.FillInChannelProps(channel)
-	if err != nil {
+	if appErr != nil {
-		c.Err = err
+		c.Err = appErr
 		return
 	}
 

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"))
+	customOnly, _ := strconv.ParseBool(r.URL.Query().Get("custom_only"))
-	if failConv != nil {
-		customOnly = false
-	}
 
 	teamId := r.URL.Query().Get("team_id")
 	if len(teamId) == 0 {

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"))
+	forceDownload, _ := strconv.ParseBool(r.URL.Query().Get("download"))
-	if convErr != nil {
-		forceDownload = false
-	}
 
 	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"))
+	forceDownload, _ := strconv.ParseBool(r.URL.Query().Get("download"))
-	if convErr != nil {
-		forceDownload = false
-	}
-
 	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"))
+	forceDownload, _ := strconv.ParseBool(r.URL.Query().Get("download"))
-	if convErr != nil {
-		forceDownload = false
-	}
-
 	info, err := c.App.GetFileInfo(c.Params.FileId)
 	if err != nil {
 		c.Err = err

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"))
+	localOnly, _ := strconv.ParseBool(u.Query().Get("local_only"))
-	if err != nil {
-		localOnly = false
-	}
-
 	return &model.MarketplacePluginFilter{
 		Page:          page,
 		PerPage:       perPage,

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"
+	forceStr := c.boolString(force)
-	if force {
-		forceStr = "true"
-	}
 
 	url := fmt.Sprintf("%s?plugin_download_url=%s&force=%s", c.GetPluginsRoute()+"/install_from_url", url.QueryEscape(downloadUrl), forceStr)
 	r, err := c.DoApiPost(url, "")