From afd48a380e504ba2f5aceb5a5c76204b0cff93f6 Mon Sep 17 00:00:00 2001 From: Wessie Date: Thu, 25 Apr 2024 16:35:30 +0100 Subject: [PATCH] website/admin: fix more issues with the profile pages storage/mariadb: fix UserStorage.Get looking up from the djs table, not the users table website/middleware: add RequestURL to shared input --- storage/mariadb/user.go | 32 ++++++------ website/admin/profiles.go | 62 ++++++++++++++++++---- website/admin/profiles_test.go | 94 ++++++++++++++++++++++++++++++++++ website/middleware/input.go | 26 +++++----- 4 files changed, 176 insertions(+), 38 deletions(-) diff --git a/storage/mariadb/user.go b/storage/mariadb/user.go index 387e38c2..002cf11b 100644 --- a/storage/mariadb/user.go +++ b/storage/mariadb/user.go @@ -413,27 +413,27 @@ func (us UserStorage) All() ([]radio.User, error) { users.created_at AS created_at, IFNULL(users.user, '') AS username, (SELECT group_concat(permission) FROM permissions WHERE user_id=users.id) AS userpermissions, - djs.id AS 'dj.id', - djs.regex AS 'dj.regex', - djs.djname AS 'dj.name', - - djs.djtext AS 'dj.text', - djs.djimage AS 'dj.image', - - djs.visible AS 'dj.visible', - djs.priority AS 'dj.priority', - djs.role AS 'dj.role', - - djs.css AS 'dj.css', - djs.djcolor AS 'dj.color', + IFNULL(djs.id, 0) AS 'dj.id', + IFNULL(djs.regex, '') AS 'dj.regex', + IFNULL(djs.djname, '') AS 'dj.name', + + IFNULL(djs.djtext, '') AS 'dj.text', + IFNULL(djs.djimage, '') AS 'dj.image', + + IFNULL(djs.visible, 0) AS 'dj.visible', + IFNULL(djs.priority, 0) AS 'dj.priority', + IFNULL(djs.role, '') AS 'dj.role', + + IFNULL(djs.css, '') AS 'dj.css', + IFNULL(djs.djcolor, '') AS 'dj.color', IFNULL(themes.id, 0) AS 'dj.theme.id', IFNULL(themes.name, 'default') AS 'dj.theme.name', IFNULL(themes.display_name, 'default') AS 'dj.theme.displayname', IFNULL(themes.author, 'unknown') AS 'dj.theme.author' FROM - djs - JOIN - users ON djs.id = users.djid + users + LEFT JOIN + djs ON djs.id = users.djid LEFT JOIN themes ON djs.theme_id = themes.id; ` diff --git a/website/admin/profiles.go b/website/admin/profiles.go index 7a490301..e848a201 100644 --- a/website/admin/profiles.go +++ b/website/admin/profiles.go @@ -75,6 +75,29 @@ func (ProfileForm) TemplateName() string { return "form_profile" } +const profileFormAction = "/admin/profile" + +func (ProfileForm) FormAction() template.HTMLAttr { + return profileFormAction +} + +func (ProfileForm) FormType() template.HTMLAttr { + return `enctype="multipart/form-data"` +} + +func (f *ProfileForm) AddDJProfileURL() template.URL { + uri, err := url.Parse(profileFormAction) + if err != nil { + return "" + } + + query := uri.Query() + query.Set("username", f.Username) + query.Set("new", profileNewDJ) + uri.RawQuery = query.Encode() + return template.URL(uri.String()) +} + // GetProfile is mounted under /admin/profile and shows the currently logged // in users profile func (s *State) GetProfile(w http.ResponseWriter, r *http.Request) { @@ -275,6 +298,10 @@ func (s *State) postNewProfileUser(r *http.Request, username string) (*ProfileFo return nil, errors.E(op, err) } + // NewProfileForm will have emptied our UserPermissions because the new user + // form doesn't send any, so just overwrite it here + form.UserPermissions = user.UserPermissions + // generate a password for the user newPassword, err := postProfilePassword(form.PasswordChangeForm, true) if err != nil { @@ -316,14 +343,15 @@ func (s *State) postNewProfileDJ(r *http.Request, username string) (*ProfileForm if err != nil { return form, errors.E(op, err) } - form.User.DJ.ID = djid + dj.ID = djid // apply the id + form.User.DJ = dj // then add it to the form we're returning return form, nil } func (s *State) postNewProfile(r *http.Request) (*ProfileForm, error) { const op errors.Op = "website/admin.postNewProfile" - newMode := r.FormValue("new") // what we're making a new thing off + newMode := r.FormValue("new") // what we're making a new thing of username := r.FormValue("username") // the user we're making a new thing for switch newMode { @@ -447,7 +475,7 @@ func NewProfileForm(user radio.User, r *http.Request) (*ProfileForm, error) { // initial check to see if we're actually editing the expected user if values.Get("username") != user.Username { - return nil, errors.E(op, errors.AccessDenied) + return nil, errors.E(op, errors.AccessDenied, "username does not match") } form := newProfileForm(user, r) @@ -473,16 +501,30 @@ func newProfileForm(user radio.User, r *http.Request) ProfileForm { } func (pf *ProfileForm) Update(form url.Values) { - pf.Username = form.Get("username") - pf.IP = form.Get("ip") - pf.Email = form.Get("email") - pf.DJ.Visible = form.Get("dj.visible") != "" - pf.DJ.Name = form.Get("dj.name") + if form.Has("username") { + pf.Username = form.Get("username") + } + if form.Has("ip") { + pf.IP = form.Get("ip") + } + if form.Has("email") { + pf.Email = form.Get("email") + } + if form.Has("dj.visible") { + pf.DJ.Visible = form.Get("dj.visible") != "" + } + if form.Has("dj.name") { + pf.DJ.Name = form.Get("dj.name") + } if prio, err := strconv.Atoi(form.Get("dj.priority")); err == nil { pf.DJ.Priority = prio } - pf.DJ.Regex = form.Get("dj.regex") - pf.DJ.Theme.Name = form.Get("dj.theme.name") + if form.Has("dj.regex") { + pf.DJ.Regex = form.Get("dj.regex") + } + if form.Has("dj.theme.name") { + pf.DJ.Theme.Name = form.Get("dj.theme.name") + } // password handling pf.PasswordChangeForm.Current = form.Get("password.current") diff --git a/website/admin/profiles_test.go b/website/admin/profiles_test.go index dda295a0..37a9a548 100644 --- a/website/admin/profiles_test.go +++ b/website/admin/profiles_test.go @@ -2,8 +2,10 @@ package admin import ( "context" + "maps" "net/http" "net/http/httptest" + "reflect" "strings" "testing" @@ -12,6 +14,10 @@ import ( "github.com/R-a-dio/valkyrie/errors" "github.com/R-a-dio/valkyrie/mocks" "github.com/R-a-dio/valkyrie/website/middleware" + "github.com/leanovate/gopter" + "github.com/leanovate/gopter/arbitrary" + "github.com/leanovate/gopter/gen" + "github.com/leanovate/gopter/prop" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -37,6 +43,9 @@ type profileTest struct { CreateRet radio.UserID CreateErr error + CreateDJRet radio.DJID + CreateDJErr error + GetRet radio.User GetErr error @@ -80,6 +89,9 @@ var profileTests = []profileTest{ Form: ProfileForm{ User: radio.User{ Username: "newuser", + UserPermissions: radio.UserPermissions{ + radio.PermActive: struct{}{}, + }, }, PasswordChangeForm: ProfilePasswordChangeForm{ New: "hackme", @@ -107,6 +119,26 @@ var profileTests = []profileTest{ ExpectedForm: nil, Error: errors.E(errors.AccessDenied), }, + { + Name: "NewDJProfileCreation", + Path: "/admin/profile?new=" + profileNewDJ, + User: *adminUser, + Form: ProfileForm{ + User: *profileTestUser, + }, + ExpectedForm: &ProfileForm{ + User: mutateUser(*profileTestUser, func(u radio.User) radio.User { + u.DJ = newProfileDJ(u.Username) + u.DJ.ID = 70 // should be same as CreateDJRet + return u + }), + }, + TxFunc: mocks.CommitTx, + GetRet: *profileTestUser, + GetErr: nil, + CreateDJRet: 70, + CreateDJErr: nil, + }, { // permissions update executed by an admin, should work Name: "UpdatePermissionsAsAdmin", @@ -124,6 +156,7 @@ var profileTests = []profileTest{ ExpectedForm: profileSameAsInput, TxFunc: mocks.CommitTx, GetRet: *profileTestUser, + GetErr: nil, }, { // permissions update by the user themselves, should not work, @@ -147,6 +180,7 @@ var profileTests = []profileTest{ }, TxFunc: mocks.CommitTx, GetRet: *profileTestUser, + GetErr: nil, }, { // users should be able to update their own password assuming they know @@ -167,6 +201,7 @@ var profileTests = []profileTest{ ExpectedPassword: "donthackme", TxFunc: mocks.CommitTx, GetRet: *profileTestUser, + GetErr: nil, }, { // should only be able to change passwords if New and Repeated match @@ -186,6 +221,7 @@ var profileTests = []profileTest{ ExpectedPassword: "hackme", TxFunc: mocks.CommitTx, GetRet: *profileTestUser, + GetErr: nil, Error: errors.E(errors.InvalidForm), }, { @@ -206,6 +242,7 @@ var profileTests = []profileTest{ ExpectedPassword: "hackme", TxFunc: mocks.CommitTx, GetRet: *profileTestUser, + GetErr: nil, Error: errors.E(errors.AccessDenied), }, { @@ -225,6 +262,7 @@ var profileTests = []profileTest{ ExpectedPassword: "hackme", TxFunc: mocks.CommitTx, GetRet: *profileTestUser, + GetErr: nil, Error: errors.E(errors.InvalidForm), }, { @@ -244,6 +282,7 @@ var profileTests = []profileTest{ ExpectedPassword: "donthackme", TxFunc: mocks.CommitTx, GetRet: *profileTestUser, + GetErr: nil, }, } @@ -293,6 +332,10 @@ func TestPostProfile(t *testing.T) { assert.Equal(t, test.ExpectedForm.Username, user.Username) return test.UpdateRet, test.UpdateErr }, + CreateDJFunc: func(user radio.User, dJ radio.DJ) (radio.DJID, error) { + assert.Equal(t, test.ExpectedForm.Username, user.Username) + return test.CreateDJRet, test.CreateDJErr + }, } } @@ -350,4 +393,55 @@ func checkForm(t *testing.T, test profileTest, got *ProfileForm) { assert.NoError(t, got.User.ComparePassword(test.ExpectedPassword)) assert.Equal(t, expected.Username, got.Username) + assert.Equal(t, expected.UserPermissions, got.UserPermissions) + assert.Equal(t, expected.DJ, got.DJ) +} + +func TestProfileFormRoundTrip(t *testing.T) { + a := arbitrary.DefaultArbitraries() + p := gopter.NewProperties(nil) + + profileUserGen := gen.Struct(reflect.TypeOf(radio.User{}), map[string]gopter.Gen{ + "Username": gen.AnyString(), + "Email": gen.AnyString(), + "IP": gen.AnyString(), + "UserPermissions": genForType[radio.UserPermissions](a), + "DJ": gen.Struct(reflect.TypeOf(radio.DJ{}), map[string]gopter.Gen{ + "ID": genForType[radio.DJID](a).SuchThat(func(v radio.DJID) bool { + return v != 0 + }), + "Visible": gen.Bool(), + "Name": gen.AnyString(), + "Priority": gen.Int(), + "Regex": gen.AnyString(), + }), + }) + + p.Property("form should roundtrip", prop.ForAll( + func(u radio.User) bool { + in := ProfileForm{ + User: u, + } + var out ProfileForm + + out.Update(in.ToValues()) + // null the djid, since we don't actually roundtrip it but it is required + // by the ToValues to be set + in.DJ.ID = 0 + + return in.Username == out.Username && + in.Email == out.Email && + in.IP == out.IP && + in.DJ == out.DJ && + in.PasswordChangeForm.Current == out.PasswordChangeForm.Current && + in.PasswordChangeForm.New == out.PasswordChangeForm.New && + in.PasswordChangeForm.Repeated == out.PasswordChangeForm.Repeated && + maps.Equal(in.UserPermissions, out.newPermissions) + }, profileUserGen, + )) + p.TestingRun(t, gopter.ConsoleReporter(false)) +} + +func genForType[T any](a *arbitrary.Arbitraries) gopter.Gen { + return a.GenForType(reflect.TypeFor[T]()) } diff --git a/website/middleware/input.go b/website/middleware/input.go index 375f390f..5a4c1064 100644 --- a/website/middleware/input.go +++ b/website/middleware/input.go @@ -23,12 +23,13 @@ func InputMiddleware(cfg config.Config, status *util.Value[radio.Status]) func(h user := UserFromContext(ctx) input := Input{ - Now: time.Now(), - IsHTMX: util.IsHTMX(r), - IsUser: user != nil, - User: user, - StreamURL: PublicStreamURL, - Status: status.Latest(), + Now: time.Now(), + IsHTMX: util.IsHTMX(r), + IsUser: user != nil, + User: user, + StreamURL: PublicStreamURL, + RequestURL: template.URL(r.URL.String()), + Status: status.Latest(), } ctx = context.WithValue(ctx, inputKey{}, input) @@ -53,12 +54,13 @@ func InputFromContext(ctx context.Context) Input { // Input is a bunch of data that should be accessable to the base template type Input struct { - Now time.Time - IsUser bool - IsHTMX bool - User *radio.User - Status radio.Status - StreamURL template.URL + Now time.Time + IsUser bool + IsHTMX bool + User *radio.User + Status radio.Status + StreamURL template.URL + RequestURL template.URL } func (Input) TemplateName() string {