Use email buffer for DEL `/email/:email` route

[vbudhram]

This was missed in original implemenation, but we should be using an emailBuffer for this route.

Ref: #309 (comment)

[vladikoff]

Also https://twitter.com/trott/status/968561198819024896

The plan for now is to have a runtime-deprecation warning for Buffer constructor in @nodejs 10.0.0. 😱

Switch to Buffer.from()/Buffer.alloc()/Buffer.allocUnsafe() now.

[rfk]

IIRC the auth-server has some linting rules to make sure we don't use new Buffer(), maybe we need to copy them everywhere.

[vladikoff]

https://github.com/mozilla/eslint-plugin-fxa/blob/master/lib/rules/no-new-buffer.js

[deeptibaghel]

@vladikoff can I take up this issue ?

[vladikoff]

@deeptibaghel sure!

See this file https://github.com/mozilla/fxa-auth-db-mysql/blob/master/db-server/index.js#L95-L113
There is inconsistency, 3/4 methods use Buffer

Move all Buffer(req.params.email, 'hex') calls from that file into the methods they are calling and do a Buffer.from(email, 'hex') in the methods.

Watch out there is a mem.js and mysql.js that are different database drivers, you might have make function changes in different places.
Make sure to start MySQL (default port 3306) locally for tests to pass locally when you run npm test

@vbudhram sounds about right?

[deeptibaghel]

@vladikoff i am having an issue :
If I move the buffer line inside function , it doesn't work with hex encoding

Original function
var GET_SECONDARY_EMAIL = 'CALL getSecondaryEmail_1(?)'
MySql.prototype.getSecondaryEmail = function (emailBuffer) {
return this.readFirstResult(GET_SECONDARY_EMAIL, [emailBuffer.toString('utf8')])
}
New function
var GET_SECONDARY_EMAIL = 'CALL getSecondaryEmail_1(?)'
MySql.prototype.getSecondaryEmail = function (email) {
email = Buffer.from(email, 'hex')
return this.readFirstResult(GET_SECONDARY_EMAIL, [email.toString('utf8')])
}

It fails for the db test "should get secondary emails"
passed email : 29943977415458534@bar.com
converted email : )�9wATXS

But if i change it to utf8 it works,
email = Buffer.from(email, 'utf8')

i have to change another test for this though, ie remove conversion to hex :
add account, add email, get secondary email, get emails, delete email (47ms)
// Attempt to get a specific secondary email
return client.getThen('/email/' + emailToHex(thirdEmailRecord.email))

If we just convert the Buffer to new format in api calls without moving to methods, it will work as it is.
Please suggest.

[vbudhram]

@deeptibaghel

Your update is almost there, by removing the Buffer(req.params.email, 'hex'), the function will now be getting an emailBuffer.

The test needs to be updated to convert the email.

      it('should get secondary email', () => {
        return db.getSecondaryEmail(emailToHex(secondEmail.email))
          .then((result) => {
            ...
          })
      })

You can reuse the emailHex from here. Any references to db.getSecondaryEmail should be updated to pass a buffer.

The same logic would have to be done for the other endpoints as well.

[deeptibaghel]

Thanks @vbudhram , how do i reuse emailHex from remote.js as right now it is exporting an unnamed function. Can i rewrite the same function again in db_tests ?

[deeptibaghel]

Also there are more usage of Buffer function the old way like in emailToHex of
fxa-auth-db-mysql/db-server/test/backend/remote.js, should i change them all to new format ?

[vbudhram]

Can i rewrite the same function again in db_tests?

I think that should be fine. It is a fairly tiny function, maybe if it was a little larger or more overlap in the files then they could be pulled into a common helper file.

Also there are more usage of Buffer function the old way like in emailToHex of
fxa-auth-db-mysql/db-server/test/backend/remote.js, should i change them all to new format ?

Sure, you can update references to use Buffer.from(email, 'hex')

[vladikoff]

This was fixed, thanks @deeptibaghel !