Skip to content

Commit

Permalink
Merge #45127
Browse files Browse the repository at this point in the history 
45127: server: make UI customizations work for non-admin users r=dhartunian a=knz

Fixes  #45126.

cc @piyush-singh  - I am considering this for backporting in 19.1/19.2 since it fixes a regression introduced in a previous patch revision.

Summary table:

| Time frame  | R/W of UI customization | different customizations per user |
|-------------|-------------------------|-----------------------------------|
| Pre-#42563  | all users               | broken (shared between users)     |
| Post-#42563 | admin only (regression) | broken                            |
| This commit | all users               | yes - each user has its own       |

The separation of customization between users is done by mixing the
username with the key with a `$` character when looking up rows in
`system.ui`.

This approach can look perhaps baroque when SQL would prefer us to
have two separate *columns* in the PK of `system.ui` and use that for
lookups and updates. Unfortunately PK changes are not yet supported,
and a PK change would incur a complex story in mixed-version clusters.

Additionally, this approach is suitable for backporting in 2.1, 19.1
and 19.2 clusters where the functionality was broken by #42563.

Release note (admin ui change): The display options are now saved
separately for each authenticated user. Note: when upgrading to a
version with this change, all current display customizations for admin
users are lost.

Release note (bug fix): Customizations of the Admin UI are again
properly saved across sessions.

Co-authored-by: Raphael 'kena' Poss <[email protected]>
  • Loading branch information
@knz
craig[bot] and knz committed Mar 4, 2020
2 parents 614b049 + 33d3a6f commit 4d24a25
Show file tree
Hide file tree
Showing 3 changed files with 158 additions and 85 deletions.
26 changes: 22 additions & 4 deletions pkg/server/admin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1066,15 +1066,15 @@ func (s *adminServer) getUIData(
if i != 0 {
query.Append(",")
}
query.Append("$", tree.NewDString(key))
query.Append("$", tree.NewDString(makeUIKey(userName, key)))
}
query.Append(");")
if err := query.Errors(); err != nil {
return nil, s.serverErrorf("error constructing query: %v", err)
}
rows, err := s.server.internalExecutor.QueryEx(
ctx, "admin-getUIData", nil, /* txn */
sqlbase.InternalExecutorSessionDataOverride{User: userName},
sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
query.String(), query.QueryArguments()...,
)
if err != nil {
Expand All @@ -1088,6 +1088,9 @@ func (s *adminServer) getUIData(
if !ok {
return nil, s.serverErrorf("unexpected type for UI key: %T", row[0])
}
_, key := splitUIKey(string(dKey))
dKey = tree.DString(key)

dValue, ok := row[1].(*tree.DBytes)
if !ok {
return nil, s.serverErrorf("unexpected type for UI value: %T", row[1])
Expand All @@ -1105,6 +1108,21 @@ func (s *adminServer) getUIData(
return &resp, nil
}

// makeUIKey combines username and key to form a lookup key in
// system.ui.
// The username is combined to ensure that different users
// can use different customizations.
func makeUIKey(username, key string) string {
return username + "$" + key
}

// splitUIKey is the inverse of makeUIKey.
// The caller must ensure that the value was produced by makeUIKey.
func splitUIKey(combined string) (string, string) {
pair := strings.SplitN(combined, "$", 2)
return pair[0], pair[1]
}

// SetUIData is an endpoint that stores the given key/value pairs in the
// system.ui table. See GetUIData for more details on semantics.
func (s *adminServer) SetUIData(
Expand All @@ -1128,9 +1146,9 @@ func (s *adminServer) SetUIData(
rowsAffected, err := s.server.internalExecutor.ExecEx(
ctx, "admin-set-ui-data", nil, /* txn */
sqlbase.InternalExecutorSessionDataOverride{
User: userName,
User: security.RootUser,
},
query, key, val)
query, makeUIKey(userName, key), val)
if err != nil {
return nil, s.serverError(err)
}
Expand Down
206 changes: 129 additions & 77 deletions pkg/server/admin_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,16 @@ func getAdminJSONProtoWithAdminOption(
func postAdminJSONProto(
ts serverutils.TestServerInterface, path string, request, response protoutil.Message,
) error {
return serverutils.PostJSONProto(ts, adminPrefix+path, request, response)
return postAdminJSONProtoWithAdminOption(ts, path, request, response, true)
}

func postAdminJSONProtoWithAdminOption(
ts serverutils.TestServerInterface,
path string,
request, response protoutil.Message,
isAdmin bool,
) error {
return serverutils.PostJSONProtoWithAdminOption(ts, adminPrefix+path, request, response, isAdmin)
}

// getText fetches the HTTP response body as text in the form of a
Expand Down Expand Up @@ -1027,107 +1036,150 @@ func TestAdminAPISettings(t *testing.T) {
})
}

// TestAdminAPIUIData checks that UI customizations are properly
// persisted for both admin and non-admin users.
func TestAdminAPIUIData(t *testing.T) {
defer leaktest.AfterTest(t)()
s, _, _ := serverutils.StartServer(t, base.TestServerArgs{})
defer s.Stopper().Stop(context.TODO())

start := timeutil.Now()
testutils.RunTrueAndFalse(t, "isAdmin", func(t *testing.T, isAdmin bool) {
start := timeutil.Now()

mustSetUIData := func(keyValues map[string][]byte) {
if err := postAdminJSONProto(s, "uidata", &serverpb.SetUIDataRequest{
KeyValues: keyValues,
}, &serverpb.SetUIDataResponse{}); err != nil {
t.Fatal(err)
mustSetUIData := func(keyValues map[string][]byte) {
if err := postAdminJSONProtoWithAdminOption(s, "uidata", &serverpb.SetUIDataRequest{
KeyValues: keyValues,
}, &serverpb.SetUIDataResponse{}, isAdmin); err != nil {
t.Fatal(err)
}
}
}

expectKeyValues := func(expKeyValues map[string][]byte) {
var resp serverpb.GetUIDataResponse
queryValues := make(url.Values)
for key := range expKeyValues {
queryValues.Add("keys", key)
}
url := "uidata?" + queryValues.Encode()
if err := getAdminJSONProto(s, url, &resp); err != nil {
t.Fatal(err)
}
// Do a two-way comparison. We can't use reflect.DeepEqual(), because
// resp.KeyValues has timestamps and expKeyValues doesn't.
for key, actualVal := range resp.KeyValues {
if a, e := actualVal.Value, expKeyValues[key]; !bytes.Equal(a, e) {
t.Fatalf("key %s: value = %v, expected = %v", key, a, e)
expectKeyValues := func(expKeyValues map[string][]byte) {
var resp serverpb.GetUIDataResponse
queryValues := make(url.Values)
for key := range expKeyValues {
queryValues.Add("keys", key)
}
}
for key, expVal := range expKeyValues {
if a, e := resp.KeyValues[key].Value, expVal; !bytes.Equal(a, e) {
t.Fatalf("key %s: value = %v, expected = %v", key, a, e)
url := "uidata?" + queryValues.Encode()
if err := getAdminJSONProtoWithAdminOption(s, url, &resp, isAdmin); err != nil {
t.Fatal(err)
}
}

// Sanity check LastUpdated.
for _, val := range resp.KeyValues {
now := timeutil.Now()
if val.LastUpdated.Before(start) {
t.Fatalf("val.LastUpdated %s < start %s", val.LastUpdated, start)
// Do a two-way comparison. We can't use reflect.DeepEqual(), because
// resp.KeyValues has timestamps and expKeyValues doesn't.
for key, actualVal := range resp.KeyValues {
if a, e := actualVal.Value, expKeyValues[key]; !bytes.Equal(a, e) {
t.Fatalf("key %s: value = %v, expected = %v", key, a, e)
}
}
for key, expVal := range expKeyValues {
if a, e := resp.KeyValues[key].Value, expVal; !bytes.Equal(a, e) {
t.Fatalf("key %s: value = %v, expected = %v", key, a, e)
}
}
if val.LastUpdated.After(now) {
t.Fatalf("val.LastUpdated %s > now %s", val.LastUpdated, now)

// Sanity check LastUpdated.
for _, val := range resp.KeyValues {
now := timeutil.Now()
if val.LastUpdated.Before(start) {
t.Fatalf("val.LastUpdated %s < start %s", val.LastUpdated, start)
}
if val.LastUpdated.After(now) {
t.Fatalf("val.LastUpdated %s > now %s", val.LastUpdated, now)
}
}
}
}

expectValueEquals := func(key string, expVal []byte) {
expectKeyValues(map[string][]byte{key: expVal})
}
expectValueEquals := func(key string, expVal []byte) {
expectKeyValues(map[string][]byte{key: expVal})
}

expectKeyNotFound := func(key string) {
var resp serverpb.GetUIDataResponse
url := "uidata?keys=" + key
if err := getAdminJSONProto(s, url, &resp); err != nil {
t.Fatal(err)
expectKeyNotFound := func(key string) {
var resp serverpb.GetUIDataResponse
url := "uidata?keys=" + key
if err := getAdminJSONProtoWithAdminOption(s, url, &resp, isAdmin); err != nil {
t.Fatal(err)
}
if len(resp.KeyValues) != 0 {
t.Fatal("key unexpectedly found")
}
}
if len(resp.KeyValues) != 0 {
t.Fatal("key unexpectedly found")

// Basic tests.
var badResp serverpb.GetUIDataResponse
const errPattern = "400 Bad Request"
if err := getAdminJSONProtoWithAdminOption(s, "uidata", &badResp, isAdmin); !testutils.IsError(err, errPattern) {
t.Fatalf("unexpected error: %v\nexpected: %s", err, errPattern)
}
}

// Basic tests.
var badResp serverpb.GetUIDataResponse
const errPattern = "400 Bad Request"
if err := getAdminJSONProto(s, "uidata", &badResp); !testutils.IsError(err, errPattern) {
t.Fatalf("unexpected error: %v\nexpected: %s", err, errPattern)
}
mustSetUIData(map[string][]byte{"k1": []byte("v1")})
expectValueEquals("k1", []byte("v1"))

mustSetUIData(map[string][]byte{"k1": []byte("v1")})
expectValueEquals("k1", []byte("v1"))
expectKeyNotFound("NON_EXISTENT_KEY")

expectKeyNotFound("NON_EXISTENT_KEY")
mustSetUIData(map[string][]byte{
"k2": []byte("v2"),
"k3": []byte("v3"),
})
expectValueEquals("k2", []byte("v2"))
expectValueEquals("k3", []byte("v3"))
expectKeyValues(map[string][]byte{
"k2": []byte("v2"),
"k3": []byte("v3"),
})

mustSetUIData(map[string][]byte{
"k2": []byte("v2"),
"k3": []byte("v3"),
})
expectValueEquals("k2", []byte("v2"))
expectValueEquals("k3", []byte("v3"))
expectKeyValues(map[string][]byte{
"k2": []byte("v2"),
"k3": []byte("v3"),
})
mustSetUIData(map[string][]byte{"k2": []byte("v2-updated")})
expectKeyValues(map[string][]byte{
"k2": []byte("v2-updated"),
"k3": []byte("v3"),
})

mustSetUIData(map[string][]byte{"k2": []byte("v2-updated")})
expectKeyValues(map[string][]byte{
"k2": []byte("v2-updated"),
"k3": []byte("v3"),
// Write a binary blob with all possible byte values, then verify it.
var buf bytes.Buffer
for i := 0; i < 997; i++ {
buf.WriteByte(byte(i % 256))
}
mustSetUIData(map[string][]byte{"bin": buf.Bytes()})
expectValueEquals("bin", buf.Bytes())
})
}

// Write a binary blob with all possible byte values, then verify it.
var buf bytes.Buffer
for i := 0; i < 997; i++ {
buf.WriteByte(byte(i % 256))
// TestAdminAPIUISeparateData check that separate users have separate customizations.
func TestAdminAPIUISeparateData(t *testing.T) {
defer leaktest.AfterTest(t)()
s, _, _ := serverutils.StartServer(t, base.TestServerArgs{})
defer s.Stopper().Stop(context.TODO())

// Make a setting for an admin user.
if err := postAdminJSONProtoWithAdminOption(s, "uidata",
&serverpb.SetUIDataRequest{KeyValues: map[string][]byte{"k": []byte("v1")}},
&serverpb.SetUIDataResponse{},
true /*isAdmin*/); err != nil {
t.Fatal(err)
}

// Make a setting for a non-admin user.
if err := postAdminJSONProtoWithAdminOption(s, "uidata",
&serverpb.SetUIDataRequest{KeyValues: map[string][]byte{"k": []byte("v2")}},
&serverpb.SetUIDataResponse{},
false /*isAdmin*/); err != nil {
t.Fatal(err)
}

var resp serverpb.GetUIDataResponse
url := "uidata?keys=k"

if err := getAdminJSONProtoWithAdminOption(s, url, &resp, true /* isAdmin */); err != nil {
t.Fatal(err)
}
if len(resp.KeyValues) != 1 || !bytes.Equal(resp.KeyValues["k"].Value, []byte("v1")) {
t.Fatalf("unexpected admin values: %+v", resp.KeyValues)
}
if err := getAdminJSONProtoWithAdminOption(s, url, &resp, false /* isAdmin */); err != nil {
t.Fatal(err)
}
if len(resp.KeyValues) != 1 || !bytes.Equal(resp.KeyValues["k"].Value, []byte("v2")) {
t.Fatalf("unexpected non-admin values: %+v", resp.KeyValues)
}
mustSetUIData(map[string][]byte{"bin": buf.Bytes()})
expectValueEquals("bin", buf.Bytes())
}

func TestClusterAPI(t *testing.T) {
Expand Down
11 changes: 7 additions & 4 deletions pkg/testutils/serverutils/test_server_shim.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,10 +249,13 @@ func GetJSONProtoWithAdminOption(
return httputil.GetJSON(httpClient, ts.AdminURL()+path, response)
}

// PostJSONProto uses the supplied client to POST request to the URL specified by
// the parameters and unmarshals the result into response.
func PostJSONProto(ts TestServerInterface, path string, request, response protoutil.Message) error {
httpClient, err := ts.GetAdminAuthenticatedHTTPClient()
// PostJSONProtoWithAdminOption is like PostJSONProto but the caller
// can customize whether the request is performed with admin
// privilege.
func PostJSONProtoWithAdminOption(
ts TestServerInterface, path string, request, response protoutil.Message, isAdmin bool,
) error {
httpClient, err := ts.GetAuthenticatedHTTPClient(isAdmin)
if err != nil {
return err
}
Expand Down

0 comments on commit 4d24a25

Please sign in to comment.