
Golang and MySQL login tutorial - lllorddino
https://dinosaurscode.xyz/go/2016/06/19/golang-mysql-authentication/
======
taspeotis

        hashPassword, _ := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost)
    
        _, err = db.Exec("INSERT INTO users(usrname, password) VALUES(?, ?)", username, password)
    

I'm not a Go expert. Doesn't this:

1) ignore an error, if any? And then

2) ignore hashPassword and just insert the non-hashed password into the
database?

It looks like if someone follows along from home they'll get a syntax error
from "usrname" but let's say they fixed that...

The code at the bottom of the article doesn't seem to suffer from the latter
issue, but still suffers from the former.

~~~
afghanPower
Seems like the author went a little too fast here. Should definitely insert
the hashed password into the db.

edit: Otherwise, great read! Hoping for more.

~~~
stygiansonic
Yeah, it seems like a typo. The "Signup Page" example doesn't use
`hashPassword` but in the "entire source code" example at the bottom, it does
properly use `hashedPassword` in the statement. (But still ignores any error
from `bcrypt.GenerateFromPassword`)

------
jtruk
panic() in a web serving route (e.g. signupPage here) is bad practice - an
error will bring down your web server. Panics should be reserved for errors
that invalidate your system, e.g. missing view assets or db connection on
system startup.

Much better to http.Redirect/5xx.

~~~
lllorddino
Fixed.

------
fideloper
I'm not familiar with how the DB classes work (and I guess what counts as a
function "ending"). Since `db.Close()` is deferred, is there a mysql
connection open for as long as the web server is open in this case?

The main() function doesn't "end" until the web listener is stopped, correct?

~~~
stygiansonic
Yes, that's correct. `http.ListenAndServe()` doesn't return until the web
listener is stopped.

This is actually the intended usage of `sql.Open()`:
[https://golang.org/pkg/database/sql/#Open](https://golang.org/pkg/database/sql/#Open)
(it maintains its own connection pool and is safe for concurrent use)

------
andmarios
Whilst I am always grateful for new tutorials, I think this is a special case
where the author should have gone the extra mile and talk about salting and
maybe even about hashing the password client side.

Security related tutorials should not skip steps, even if only to mention the
more advanced of them.

~~~
rimantas
There is no skipped step, bcrypt does salting itself.

~~~
andmarios
Ah yes! Thank you for the correction. :)

------
insertnickname

        var db *sql.DB
        var err error
    

Why declare this error globally?

~~~
falcolas
The DB: because there's an internal goroutine-safe connection pool, so you
want it to either be global, or exist in the server singleton (i.e. global).

The err: because they use it so frequently, easier to define it once and
simply assign to it multiple times than try and track if it's already been
defined per block of code.

Personally, I prefer to use distinct error variables for each error which can
be thrown, but either method is idiomatic.

~~~
hellcow
It's bad practice to define an error globally. Let's say you create a
goroutine which uses that same err variable (such as an http handler)--now you
have a race condition, as that memory is being modified across threads.

~~~
falcolas
True, and looking at the code, it doesn't make sense to define it as a global,
since each potential goroutine (i.e. the HTTP handler functions) is re-
defining 'err' whenever it's being used.

------
avinassh
why not use an ORM?

~~~
arien
It's a good practice to learn the inner works before adding layers that may
simplify (but obscure) functionality.

