Add flag.FlagSet versions of common.MultiStringFlag functions.
Bug: b/249507110
Change-Id: Idf73436a2ff97f9e537b80eddf19d44e891c3895
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/586908
Reviewed-by: Ravi Mistry <rmistry@google.com>
Commit-Queue: Joe Gregorio <jcgregorio@google.com>
diff --git a/go/common/common.go b/go/common/common.go
index 604e8e8..e8cd3d7 100644
--- a/go/common/common.go
+++ b/go/common/common.go
@@ -130,21 +130,34 @@
}
}
+// FSNewMultiStringFlag returns a []string flag, loaded with the given
+// defaults, usage string and name.
+func FSNewMultiStringFlag(fs *flag.FlagSet, name string, defaults []string, usage string) *[]string {
+ var values []string
+ m := newMultiString(&values, defaults)
+ fs.Var(m, name, usage)
+ return &values
+}
+
+// FSMultiStringFlagVar defines a MultiString flag with the specified name,
+// defaults, and usage string. The argument target points to a []string
+// variable in which to store the values of the flag.
+func FSMultiStringFlagVar(fs *flag.FlagSet, target *[]string, name string, defaults []string, usage string) {
+ m := newMultiString(target, defaults)
+ fs.Var(m, name, usage)
+}
+
// NewMultiStringFlag returns a []string flag, loaded with the given
// defaults, usage string and name.
func NewMultiStringFlag(name string, defaults []string, usage string) *[]string {
- var values []string
- m := newMultiString(&values, defaults)
- flag.Var(m, name, usage)
- return &values
+ return FSNewMultiStringFlag(flag.CommandLine, name, defaults, usage)
}
// MultiStringFlagVar defines a MultiString flag with the specified name,
// defaults, and usage string. The argument target points to a []string
// variable in which to store the values of the flag.
func MultiStringFlagVar(target *[]string, name string, defaults []string, usage string) {
- m := newMultiString(target, defaults)
- flag.Var(m, name, usage)
+ FSNewMultiStringFlag(flag.CommandLine, name, defaults, usage)
}
// String() returns the current values of multiString, as a comma separated list
diff --git a/kube/go/authproxy/authproxy.go b/kube/go/authproxy/authproxy.go
index b5784ad..63cc29f 100644
--- a/kube/go/authproxy/authproxy.go
+++ b/kube/go/authproxy/authproxy.go
@@ -140,7 +140,7 @@
fs.BoolVar(&a.allowPost, "allow_post", false, "Allow POST requests to bypass auth.")
fs.StringVar(&a.allowedFrom, "allowed_from", "", "A comma separated list of of domains and email addresses that are allowed to access the site. Example: 'google.com'")
fs.BoolVar(&a.passive, "passive", false, "If true then allow unauthenticated requests to go through, while still adding logged in users emails in via the webAuthHeaderName.")
- common.MultiStringFlagVar(&a.roleFlags, "role", []string{}, "Define a role and the group (CRIA, domain, email list) that defines who gets that role via flags. For example: --role=viewer=@google.com OR --role=triager=cria_group:project-angle-committers")
+ common.FSMultiStringFlagVar(fs, &a.roleFlags, "role", []string{}, "Define a role and the group (CRIA, domain, email list) that defines who gets that role via flags. For example: --role=viewer=@google.com OR --role=triager=cria_group:project-angle-committers")
return fs
}
@@ -292,10 +292,10 @@
if len(a.roleFlags) > 0 && (a.criaGroup != "" || a.allowedFrom != "") {
return fmt.Errorf("Can not mix --role and [--auth_group, --allowed_from] flags.")
}
- if a.criaGroup != "" && a.allowedFrom != "" {
+ if len(a.roleFlags) == 0 && (a.criaGroup != "" && a.allowedFrom != "") {
return fmt.Errorf("Only one of the flags in [--auth_group, --allowed_from] can be specified.")
}
- if a.criaGroup == "" && a.allowedFrom == "" {
+ if len(a.roleFlags) == 0 && (a.criaGroup == "" && a.allowedFrom == "") {
return fmt.Errorf("At least one of the flags in [--auth_group, --allowed_from] must be specified.")
}
diff --git a/kube/go/authproxy/authproxy_test.go b/kube/go/authproxy/authproxy_test.go
index d851234..4a68299 100644
--- a/kube/go/authproxy/authproxy_test.go
+++ b/kube/go/authproxy/authproxy_test.go
@@ -179,7 +179,7 @@
allowedFrom: "",
}
- require.Error(t, app.validateFlags())
+ require.NoError(t, app.validateFlags())
}
func TestValidateFlags_BothLegacyAndRolesFlagsSpecified_ReturnsError(t *testing.T) {
diff --git a/perf/go/frontend/BUILD.bazel b/perf/go/frontend/BUILD.bazel
index 81035f4..eca6a7f 100644
--- a/perf/go/frontend/BUILD.bazel
+++ b/perf/go/frontend/BUILD.bazel
@@ -1,3 +1,4 @@
+load("//bazel/go:go_test.bzl", "go_test")
load("@io_bazel_rules_go//go:def.bzl", "go_library")
go_library(
@@ -45,3 +46,15 @@
"@io_opencensus_go//trace",
],
)
+
+go_test(
+ name = "frontend_test",
+ srcs = ["frontend_test.go"],
+ embed = [":frontend"],
+ deps = [
+ "//go/alogin",
+ "//go/alogin/mocks",
+ "//go/roles",
+ "@com_github_stretchr_testify//require",
+ ],
+)
diff --git a/perf/go/frontend/frontend.go b/perf/go/frontend/frontend.go
index ae602f6..ddb156a 100644
--- a/perf/go/frontend/frontend.go
+++ b/perf/go/frontend/frontend.go
@@ -815,8 +815,8 @@
func (f *Frontend) isEditor(w http.ResponseWriter, r *http.Request, action string, body interface{}) bool {
user := f.loginProvider.LoggedInAs(r)
- if f.loginProvider.HasRole(r, roles.Editor) {
- httputils.ReportError(w, fmt.Errorf("Not logged in."), "You must be logged in to complete this action.", http.StatusInternalServerError)
+ if !f.loginProvider.HasRole(r, roles.Editor) {
+ httputils.ReportError(w, fmt.Errorf("Not logged in."), "You must be logged in to complete this action.", http.StatusUnauthorized)
return false
}
auditlog.LogWithUser(r, user.String(), "triage", body)
diff --git a/perf/go/frontend/frontend_test.go b/perf/go/frontend/frontend_test.go
new file mode 100644
index 0000000..6327176
--- /dev/null
+++ b/perf/go/frontend/frontend_test.go
@@ -0,0 +1,37 @@
+// Package frontend contains the Go code that servers the Perf web UI.
+package frontend
+
+import (
+ "net/http"
+ "net/http/httptest"
+ "testing"
+
+ "github.com/stretchr/testify/require"
+ "go.skia.org/infra/go/alogin"
+ "go.skia.org/infra/go/alogin/mocks"
+ "go.skia.org/infra/go/roles"
+)
+
+func setupForTest(t *testing.T, userIsEditor bool) (*httptest.ResponseRecorder, *http.Request, *Frontend) {
+ login := mocks.NewLogin(t)
+ w := httptest.NewRecorder()
+ r := httptest.NewRequest("POST", "/not-used", nil)
+ login.On("LoggedInAs", r).Return(alogin.EMail("nobody@example.org"))
+ login.On("HasRole", r, roles.Editor).Return(userIsEditor)
+ f := &Frontend{
+ loginProvider: login,
+ }
+ return w, r, f
+}
+
+func TestFrontendIsEditor_UserIsEditor_ReportsStatusOK(t *testing.T) {
+ w, r, f := setupForTest(t, true)
+ f.isEditor(w, r, "my-test-action", nil)
+ require.Equal(t, http.StatusOK, w.Result().StatusCode)
+}
+
+func TestFrontendIsEditor_UserIsOnlyViewer_ReportsError(t *testing.T) {
+ w, r, f := setupForTest(t, false)
+ f.isEditor(w, r, "my-test-action", nil)
+ require.Equal(t, http.StatusUnauthorized, w.Result().StatusCode)
+}
