From 4d6a7bfda1761530aea5fa810f91cdd1964a1c0e Mon Sep 17 00:00:00 2001 From: Azareal Date: Mon, 28 May 2018 16:27:12 +1000 Subject: [PATCH] You can now change someone's group again. The background no longer randomly vanishes on Shadow. Fixed the small margins on the paginator on Shadow. Tweaked the padding for the paginator in the Control Panel on Shadow. The footer is no longer 100% wide on Shadow. Fixed a misplaced ':' in Shadow. Added the BlankProfileReply function for tests. Tests now run once more. Made it easier to trace test errors which use recordMustExist and recordMustNotExist. Added tests for profile reply deletion. Added tests for GeneratePassword in addition to the existing ones for BcryptGeneratePassword. Added tests for Auth.Authenticate. Added tests for Auth.CreateSession. Added a contributor convention for highly unstable builds. This can be considered a stable build. --- CONTRIBUTING.md | 2 + common/auth.go | 2 + common/profile_reply.go | 5 ++ common/user.go | 26 +++++----- common/user_store.go | 1 + general_test.go | 2 +- misc_test.go | 86 ++++++++++++++++++++++++++++------ themes/shadow/public/main.css | 22 +++++++-- themes/shadow/public/panel.css | 8 ++++ 9 files changed, 123 insertions(+), 31 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0f59dd8e..1e16e851 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -4,6 +4,8 @@ It's mainly there to deal with any legal issues which come our way and to switch Other uses may arise in the future, e.g. commercial licensing, although that's currently uncertain. +Try to prefix commits which introduce a lot of bugs or otherwise has a large impact on the usability of Gosora with UNSTABLE. + # Coding Standards All code must be unit tested where ever possible with the exception of JavaScript which is untestable with our current technologies, tread with caution there. diff --git a/common/auth.go b/common/auth.go index 7d3c9949..c70be49a 100644 --- a/common/auth.go +++ b/common/auth.go @@ -18,6 +18,7 @@ import ( "golang.org/x/crypto/bcrypt" ) +// TODO: Write more authentication tests var Auth AuthInt const SaltLength int = 32 @@ -83,6 +84,7 @@ func NewDefaultAuth() (*DefaultAuth, error) { } // Authenticate checks if a specific username and password is valid and returns the UID for the corresponding user, if so. Otherwise, a user safe error. +// TODO: Find a better way of handling errors we don't want to reach the user func (auth *DefaultAuth) Authenticate(username string, password string) (uid int, err error) { var realPassword, salt string err = auth.login.QueryRow(username).Scan(&uid, &realPassword, &salt) diff --git a/common/profile_reply.go b/common/profile_reply.go index ab56f463..e397d101 100644 --- a/common/profile_reply.go +++ b/common/profile_reply.go @@ -39,6 +39,11 @@ func init() { }) } +// Mostly for tests, so we don't wind up with out-of-date profile reply initialisation logic there +func BlankProfileReply(id int) *ProfileReply { + return &ProfileReply{ID: id} +} + // TODO: Write tests for this func (reply *ProfileReply) Delete() error { _, err := profileReplyStmts.delete.Exec(reply.ID) diff --git a/common/user.go b/common/user.go index 0112a1a7..71194ffb 100644 --- a/common/user.go +++ b/common/user.go @@ -54,21 +54,23 @@ type User struct { } type UserStmts struct { - activate *sql.Stmt - changeGroup *sql.Stmt - delete *sql.Stmt - setAvatar *sql.Stmt - setUsername *sql.Stmt - updateGroup *sql.Stmt - incrementTopics *sql.Stmt - updateLevel *sql.Stmt + activate *sql.Stmt + changeGroup *sql.Stmt + delete *sql.Stmt + setAvatar *sql.Stmt + setUsername *sql.Stmt + incrementTopics *sql.Stmt + updateLevel *sql.Stmt + + // TODO: Split these into a sub-struct incrementScore *sql.Stmt incrementPosts *sql.Stmt incrementBigposts *sql.Stmt incrementMegaposts *sql.Stmt incrementLiked *sql.Stmt - decrementLiked *sql.Stmt - updateLastIP *sql.Stmt + + decrementLiked *sql.Stmt + updateLastIP *sql.Stmt setPassword *sql.Stmt } @@ -242,9 +244,7 @@ func (user *User) ChangeAvatar(avatar string) (err error) { } func (user *User) ChangeGroup(group int) (err error) { - _, err = userStmts.updateGroup.Exec(group, user.ID) - user.CacheRemove() - return err + return user.bindStmt(userStmts.changeGroup, group) } // ! Only updates the database not the *User for safety reasons diff --git a/common/user_store.go b/common/user_store.go index 6387d077..4de39f94 100644 --- a/common/user_store.go +++ b/common/user_store.go @@ -229,6 +229,7 @@ func (mus *DefaultUserStore) Exists(id int) bool { } // TODO: Change active to a bool? +// TODO: Use unique keys for the usernames func (mus *DefaultUserStore) Create(username string, password string, email string, group int, active bool) (int, error) { // TODO: Strip spaces? diff --git a/general_test.go b/general_test.go index f1b457f5..6519d2be 100644 --- a/general_test.go +++ b/general_test.go @@ -47,7 +47,7 @@ func ResetTables() (err error) { func gloinit() (err error) { common.Dev.DebugMode = false //nogrouplog = true - startTime = time.Now() + common.StartTime = time.Now() err = common.ProcessConfig() if err != nil { diff --git a/misc_test.go b/misc_test.go index a4fd7a28..4ee3961f 100644 --- a/misc_test.go +++ b/misc_test.go @@ -13,16 +13,20 @@ import ( func recordMustExist(t *testing.T, err error, errmsg string, args ...interface{}) { if err == ErrNoRows { + debug.PrintStack() t.Errorf(errmsg, args...) } else if err != nil { + debug.PrintStack() t.Fatal(err) } } func recordMustNotExist(t *testing.T, err error, errmsg string, args ...interface{}) { if err == nil { + debug.PrintStack() t.Errorf(errmsg, args...) } else if err != ErrNoRows { + debug.PrintStack() t.Fatal(err) } } @@ -801,6 +805,11 @@ func TestProfileReplyStore(t *testing.T) { _, err = common.Prstore.Get(1) recordMustNotExist(t, err, "PRID #1 shouldn't exist") + // ? - Commented this one out as strong constraints like this put an unreasonable load on the database, we only want errors if a delete which should succeed fails + //profileReply := common.BlankProfileReply(1) + //err = profileReply.Delete() + //expect(t,err != nil,"You shouldn't be able to delete profile replies which don't exist") + var profileID = 1 prid, err := common.Prstore.Create(profileID, "Haha", 1, "::1") expect(t, err == nil, "Unable to create a profile reply") @@ -814,9 +823,12 @@ func TestProfileReplyStore(t *testing.T) { expect(t, profileReply.CreatedBy == 1, fmt.Sprintf("The profile reply's creator should be 1 not %d", profileReply.CreatedBy)) expect(t, profileReply.IPAddress == "::1", fmt.Sprintf("The profile reply's IP Address should be '::1' not '%s'", profileReply.IPAddress)) - //Get(id int) (*Reply, error) - //Create(profileID int, content string, createdBy int, ipaddress string) (id int, err error) + err = profileReply.Delete() + expectNilErr(t, err) + _, err = common.Prstore.Get(1) + expect(t, err != nil, "PRID #1 shouldn't exist after being deleted") + // TODO: Test profileReply.SetBody() and profileReply.Creator() } func TestSlugs(t *testing.T) { @@ -856,25 +868,71 @@ func TestSlugs(t *testing.T) { func TestAuth(t *testing.T) { // bcrypt likes doing stupid things, so this test will probably fail - var realPassword string - var hashedPassword string - var password string - var salt string - var err error - - /* No extra salt tests, we might not need this extra salt, as bcrypt has it's own? */ - realPassword = "Madame Cassandra's Mystic Orb" + realPassword := "Madame Cassandra's Mystic Orb" t.Logf("Set realPassword to '%s'", realPassword) - t.Log("Hashing the real password") - hashedPassword, err = common.BcryptGeneratePassword(realPassword) + t.Log("Hashing the real password with bcrypt") + hashedPassword, _, err := common.BcryptGeneratePassword(realPassword) if err != nil { t.Error(err) } + passwordTest(t, realPassword, hashedPassword) + // TODO: Peek at the prefix to verify this is a bcrypt hash - password = realPassword + t.Log("Hashing the real password") + hashedPassword2, _, err := common.GeneratePassword(realPassword) + if err != nil { + t.Error(err) + } + passwordTest(t, realPassword, hashedPassword2) + // TODO: Peek at the prefix to verify this is a bcrypt hash + + _, err = common.Auth.Authenticate("None", "password") + errmsg := "Username None shouldn't exist" + if err != nil { + errmsg += "\n" + err.Error() + } + expect(t, err == common.ErrNoUserByName, errmsg) + + uid, err := common.Auth.Authenticate("Admin", "password") + expectNilErr(t, err) + expect(t, uid == 1, fmt.Sprintf("Default admin uid should be 1 not %d", uid)) + + _, err = common.Auth.Authenticate("Sam", "ReallyBadPassword") + errmsg = "Username Sam shouldn't exist" + if err != nil { + errmsg += "\n" + err.Error() + } + expect(t, err == common.ErrNoUserByName, errmsg) + + admin, err := common.Users.Get(1) + expectNilErr(t, err) + // TODO: Move this into the user store tests to provide better coverage? E.g. To see if the installer and the user creator initialise the field differently + expect(t, admin.Session == "", "Admin session should be blank") + + session, err := common.Auth.CreateSession(1) + expectNilErr(t, err) + expect(t, session != "", "Admin session shouldn't be blank") + // TODO: Test the actual length set in the setting in addition to this "too short" test + // TODO: We might be able to push up this minimum requirement + expect(t, len(session) > 10, "Admin session shouldn't be too short") + expect(t, admin.Session != session, "Old session should not match new one") + admin, err = common.Users.Get(1) + expectNilErr(t, err) + expect(t, admin.Session == session, "Sessions should match") + + // TODO: Tests for SessionCheck, GetCookies, and ForceLogout +} + +// TODO: Vary the salts? Keep in mind that some algorithms store the salt in the hash therefore the salt string may be blank +func passwordTest(t *testing.T, realPassword string, hashedPassword string) { + if len(hashedPassword) < 10 { + t.Error("Hash too short") + } + salt := "" + password := realPassword t.Logf("Testing password '%s'", password) t.Logf("Testing salt '%s'", salt) - err = common.CheckPassword(hashedPassword, password, salt) + err := common.CheckPassword(hashedPassword, password, salt) if err == common.ErrMismatchedHashAndPassword { t.Error("The two don't match") } else if err == common.ErrPasswordTooLong { diff --git a/themes/shadow/public/main.css b/themes/shadow/public/main.css index d7552ec1..e93565c3 100644 --- a/themes/shadow/public/main.css +++ b/themes/shadow/public/main.css @@ -35,6 +35,9 @@ body { position: relative; top: -2px; } +#main { + padding-bottom: 5px; +} ul { list-style-type: none; @@ -191,6 +194,9 @@ a { font-size: 12px; } +.colstack { + display: flex; +} .colstack_left, .colstack_right { margin-left: 8px; } @@ -455,6 +461,8 @@ textarea.large { margin-top: 4px; display: flex; flex-direction: row; + margin-left: 8px; + margin-bottom: 8px; } .pageitem { background-color: var(--main-block-color); @@ -559,9 +567,17 @@ input, select, textarea { padding-left: 30px; } +.footBlock { + margin-top: -2px; + display: flex; +} +.footer { + width: 70%; + margin-left: auto; + margin-right: auto; +} #poweredByHolder { background-color: var(--main-block-color); - margin-top: 5px; padding: 10px; font-size: 14px; padding-left: 13px; @@ -1040,12 +1056,12 @@ input[type=checkbox]:checked + label.poll_option_label .sel { text-overflow: ellipsis; } - #back { + #back, .footer { width: calc(100% - 20px); } .opthead, .rowhead, .colstack_head { padding-top: 0px !important; - font-size 15px; + font-size: 15px; } .rowblock:not(.opthead):not(.colstack_head):not(.rowhead) .rowitem { font-size: 14px; diff --git a/themes/shadow/public/panel.css b/themes/shadow/public/panel.css index 0d8d3b17..2aee7b23 100644 --- a/themes/shadow/public/panel.css +++ b/themes/shadow/public/panel.css @@ -95,4 +95,12 @@ } .ct-series-a .ct-bar, .ct-series-a .ct-line, .ct-series-a .ct-point, .ct-series-a .ct-slice-donut { stroke: hsl(359,98%,43%) !important; +} + +.pageset { + margin-left: 0px; + margin-bottom: 0px; +} +.pageitem { + padding: 8px; } \ No newline at end of file