New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
vbudhram opened this Issue Feb 28, 2018 · 11 comments

Comments

Projects
None yet
4 participants
@vbudhram
Contributor

vbudhram commented Feb 28, 2018

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

Ref: #309 (comment)

@vladikoff

This comment has been minimized.

Show comment
Hide comment
@vladikoff

vladikoff Feb 28, 2018

Member

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.

Member

vladikoff commented Feb 28, 2018

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

This comment has been minimized.

Show comment
Hide comment
@rfk

rfk Feb 28, 2018

Member

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

Member

rfk commented Feb 28, 2018

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

@deeptibaghel

This comment has been minimized.

Show comment
Hide comment
@deeptibaghel

deeptibaghel Mar 7, 2018

Contributor

@vladikoff can I take up this issue ?

Contributor

deeptibaghel commented Mar 7, 2018

@vladikoff can I take up this issue ?

@vladikoff

This comment has been minimized.

Show comment
Hide comment
@vladikoff

vladikoff Mar 7, 2018

Member

@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?

Member

vladikoff commented Mar 7, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@deeptibaghel

deeptibaghel Mar 8, 2018

Contributor

@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.

Contributor

deeptibaghel commented Mar 8, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@vbudhram

vbudhram Mar 8, 2018

Contributor

@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.

Contributor

vbudhram commented Mar 8, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@deeptibaghel

deeptibaghel Mar 8, 2018

Contributor

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 ?

Contributor

deeptibaghel commented Mar 8, 2018

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

This comment has been minimized.

Show comment
Hide comment
@deeptibaghel

deeptibaghel Mar 8, 2018

Contributor

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 ?

Contributor

deeptibaghel commented Mar 8, 2018

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

This comment has been minimized.

Show comment
Hide comment
@vbudhram

vbudhram Mar 8, 2018

Contributor

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')

Contributor

vbudhram commented Mar 8, 2018

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 comment has been minimized.

Show comment
Hide comment
@vladikoff

vladikoff Mar 13, 2018

Member

This was fixed, thanks @deeptibaghel !

Member

vladikoff commented Mar 13, 2018

This was fixed, thanks @deeptibaghel !

@vladikoff vladikoff closed this Mar 13, 2018

@wafflebot wafflebot bot removed the waffle:backlog label Mar 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment