Develop - PR 1 #1

atburke · 2021-05-31T13:59:10Z

This part of the implementation should be feature complete and just needs feedback.

russjones · 2021-06-01T20:21:29Z

crypto.go

+// hash are hex-encoded strings.
+func GenHash(password, salt string) string {
+	passwordIn := []byte(password)
+	saltIn, _ := hex.DecodeString(salt)


Don't drop errors here (and everywhere else) return the error to the caller and let the caller make the decision.

russjones · 2021-06-02T01:08:13Z

main.go

+	return &Env{db}, nil
+}
+
+func main() {


I don't think you want to put everything in your main package (main.go and database.go). Your main package (and file) should basically just parse flags and call some library to do the actual work. This will also make it easier for you to write tests because you can target just that functionality.

For example you may want to do something like below.

/pkg/server/server.go /pkg/server/server_test.go /pkg/database/database.go /pkg/database/database_test.go /cmd /cmd/server.go

Then /cmd/server.go which will be your main package would look something like the following:

package main import ( "example.com/pkg/server" ) func main() { var hostport := flag.String("hostport", ":8080", "help message for flag") server, err := server.New(&server.Config{ HostPort: *hostport, }) if err != nil { log.Fatal(err) } if err := server.Start(); err != nil { log.Fatal(err) } }

You don't have to use this exact structure, but having everything in the main package isn't a good idea.

https://github.com/golang-standards/project-layout
I didn't realize that there was a standard(ish) Go project structure. Thanks for introducing it to me!

alex-kovoy

I've reviewed only the front-end part, for the initial PR overall looks good except the error handling part, I left a few comments to address. Once addressed, consider the frontend part approved.

alex-kovoy · 2021-06-02T19:18:19Z

frontend/src/Login.tsx

+    } else {
+      this.setState({ errorMessage: 'Invalid email/password.' });
+    }


lets add a proper error handling, right now all API errors get swallowed by the api.tsx so I suggest to propagate an actual error and display it to the user.

alex-kovoy · 2021-06-02T19:25:26Z

frontend/src/api.tsx

+    return response.status === 200;
+  } catch (error) {
+    return false;
+  }


either return a promise as is and let the caller to handle it, or you can so something like:

return { result, error }

so it could be used like so:

const { result, error } = api.login(xxxx) const { error } = api.login(xxxx) if (error) { /// }

r0mant · 2021-06-04T23:14:28Z

internal/server/server.go

+
+func (env *Env) login(c *gin.Context) {
+	now := time.Now()
+	csrfToken := c.Request.Header.Get("CSRF")


Check that it's not empty?

r0mant · 2021-06-04T23:17:27Z

internal/server/server.go

+func (env *Env) logout(c *gin.Context) {
+	// TODO: move logged in check to its own function
+	now := time.Now()
+	csrfToken := c.Request.Header.Get("CSRF")


Check that it's not empty?

r0mant · 2021-06-04T23:18:22Z

internal/server/server.go

+}
+
+func (env *Env) login(c *gin.Context) {
+	now := time.Now()


Nit: Move this closer to where it's used.

r0mant · 2021-06-04T23:18:34Z

internal/server/server.go

+
+func (env *Env) logout(c *gin.Context) {
+	// TODO: move logged in check to its own function
+	now := time.Now()


Nit: Move this closer to where it's used.

r0mant · 2021-06-04T23:21:28Z

internal/server/server.go

+
+func Run(env *Env) error {
+	go func() {
+		tick := time.NewTicker(time.Minute)


defer tick.Close()

r0mant · 2021-06-04T23:24:59Z

internal/server/server.go

+
+	err = env.db.DeleteSession(sessionToken)
+	if err != nil {
+		log.Printf("Error deleting session: %v\n", err)


Should this return unsuccessful status here if we failed to delete the session?

r0mant · 2021-06-04T23:26:02Z

internal/server/server.go

+}
+
+func (env *Env) logout(c *gin.Context) {
+	// TODO: move logged in check to its own function


Yeah, I think if you move this logic for checking csrf token, retrieving and checking the sesion etc, that would make it a bit cleaner.

r0mant · 2021-06-04T23:27:34Z

internal/database/database.go

+	DeleteExpiredSessions(t time.Time) error
+
+	// Close closes any underlying resources.
+	Close()


Curious why doesn't this return an error? If it did, it'd make it compatible with io.Closer.

r0mant · 2021-06-04T23:29:49Z

internal/database/database.go

+}
+
+func (db *MySqlDatabase) Close() {
+	db.driver.Close()


Can this return an error?

r0mant · 2021-06-04T23:32:29Z

internal/database/database.go

+}
+
+func (db *MySqlDatabase) DeleteExpiredSessions(t time.Time) error {
+	stmt := "DELETE FROM Sessions WHERE expire_idle < ? OR expire_abs < ?"


Wouldn't this always delete any session that's existed for longer than 30 minutes (current expire_idle duration) - even if it's still "active"? I couldn't see expire_idle field being updated anywhere.

expire_idle would be updated when a user makes an authenticated request. For the level 1 task, however, the only authenticated request is logout.

russjones · 2021-06-04T23:12:07Z

internal/server/server.go

+	go func() {
+		tick := time.NewTicker(time.Minute)
+		for {
+			now := <-tick.C
+			log.Println("Clearing expired sessions")
+			err := env.db.DeleteExpiredSessions(now)
+			if err != nil {
+				log.Printf("Error while cleaning up expired sessions: %v\n", err)
+			}
+		}
+	}()


This logic should be internal to your database and not exposed to the caller. So start the goroutine in NewMySqlDatabase.

russjones · 2021-06-04T23:19:14Z

internal/database/database.go

+	return &MySqlDatabase{driver}, nil
+}
+
+func (db *MySqlDatabase) CreatePreAuthSession(csrfToken string, initTime time.Time) (*types.Session, error) {


Instead of passing in time.Now() to each function,. the common Go pattern is to create a fake clock, something like the following:

import ( "github.com/jonboulle/clockwork" ) type MySqlDatabase struct { driver *sql.DB clock clockwork.Clock } func NewMySqlDatabase(username, password, databaseName string, clock clockwork.Clock) (*MySqlDatabase, error) { [...] if clock == nil { clock = clockwork.NewRealClock() } return &MySqlDatabase{ [...] clock: clock, } }

Now the caller does not have to worry about time and you can also fake time in tests.

russjones · 2021-06-04T23:23:18Z

internal/types/types.go

+// Expired checks if the session has expired.
+func (session *Session) Expired(t time.Time) bool {
+	return t.After(session.ExpireIdle) || t.After(session.ExpireAbs)
+}


Three things:

Do you need this function, or can you update your FetchSession query to not fetch expired sessions then let your cleanup loop remove it whenever it's expired.

The way you are setting ExpireIdle and ExpireAbs I think this will always trigger on ExpireIdle because you are not doing any renewal as far as I can tell.

Renewal is not in scope, so just set a maximum session life time for your session. Upon logout just remove it. No need for idle timeouts.

russjones · 2021-06-04T23:30:53Z

internal/server/server.go

+		return
+	}
+
+	if session.Expired(now) {


If you update your query to not return expired sessions, you don't need this function.

russjones · 2021-06-04T23:32:49Z

internal/server/server.go

+	isCorrectPassword, err := crypto.IsCorrectPassword(account, password)
+	if err != nil {
+		log.Println("Salt stored in database is incorrectly encoded")
+		c.AbortWithStatus(http.StatusInternalServerError)
+		return
+	}
+
+	if !isCorrectPassword {
+		log.Println("Bad password")
+		c.AbortWithStatus(http.StatusUnauthorized)
+		return
+	}


I don't think you need to split this out, just consolidate both checks and into crypto.IsCorrectPassword and return an error if either one fails.

atburke added 27 commits May 26, 2021 18:30

Add design doc

d609178

Create barebones Gin server

1a08b4a

Update module to more recent Go version

3e960d4

Add Dockerfile

a92882e

Rearrange Dockerfile

e7d24d2

Add docker-compose and mysql

86930e3

Add React

9abeaa4

Add Makefile

ecb2fbd

Add TLS

8787c65

Update README

bed378b

Add database-related types and interface

cf6a55e

Add MySQL implementation of Database

d163223

Add crypto comparisons

eb8d4fa

Add function to delete expired sessions

f972d95

Add CSRF token injection into index.html

7ce8117

Add login

fc2f7d5

Add logout

27a3fb7

Add api functions

c965c5e

Add frontend components

87b2de2

Add frontend tests

850fab6

Fix frontend compilation issues

83814eb

IT LIVES

8c0a861

Add background task for deleting expired sessions

b4e7e3d

Make login a form again

8617799

Update README

2ee0476

Move genHash to crypto.go

eb45b49

Shorten some long lines

3d5b1da

russjones reviewed Jun 2, 2021

View reviewed changes

alex-kovoy approved these changes Jun 2, 2021

View reviewed changes

Restructure project

244edb5

atburke added 4 commits June 2, 2021 19:58

Adjust some error (non)handling

88b1675

Make frontend errors more explicit

8c6cd10

Fix frontend tests

eb04845

Fix frontend error handling for real

3394f90

r0mant reviewed Jun 4, 2021

View reviewed changes

russjones reviewed Jun 4, 2021

View reviewed changes

atburke added 8 commits June 6, 2021 14:21

Move session validation to its own function

91819eb

Replace time arguments w/ internal clock

857c2df

Get rid of idle expire

bc41603

Make Database an io.Closer

3ce1971

Move session expirer routine to be a db internal

bb69c5f

Revert IsCorrectPassword to only return bool

a99131a

Remove server expire check (db's job)

a1ad3a1

Failed session delete on logout is now a 401

3d97795

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Develop - PR 1 #1

Develop - PR 1 #1

atburke commented May 31, 2021

russjones Jun 1, 2021

russjones Jun 2, 2021 •

edited

Loading

atburke Jun 2, 2021

alex-kovoy left a comment

alex-kovoy Jun 2, 2021

alex-kovoy Jun 2, 2021

r0mant Jun 4, 2021

r0mant Jun 4, 2021

r0mant Jun 4, 2021

r0mant Jun 4, 2021

r0mant Jun 4, 2021

r0mant Jun 4, 2021

r0mant Jun 4, 2021

r0mant Jun 4, 2021

r0mant Jun 4, 2021

r0mant Jun 4, 2021

atburke Jun 5, 2021

russjones Jun 4, 2021

russjones Jun 4, 2021

russjones Jun 4, 2021

russjones Jun 4, 2021

russjones Jun 4, 2021

Develop - PR 1 #1

Are you sure you want to change the base?

Develop - PR 1 #1

Conversation

atburke commented May 31, 2021

Choose a reason for hiding this comment

russjones Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-kovoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russjones Jun 2, 2021 •

edited

Loading