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

Wrap hash library into a promise #247

Merged
merged 2 commits into from Nov 15, 2017

Conversation

Projects
None yet
4 participants
@tresf
Copy link
Contributor

tresf commented Oct 13, 2017

Closes #209

@dsanders11 testing appreciated. This should address the request to support native WebCrypto SHA256 promises.

This is an educated guess on usage. It hasn't been tested yet with WebCrypto, the machine I'm on doesn't have HTTPS configured to be able to test it.

qz.api.setSha256Promise(function(data) {
   return crypto.subtle.digest('SHA-256', data);
});

@bberenz can you please review the logic for errors or redundancies?

@dsanders11

This comment has been minimized.

Copy link
Contributor

dsanders11 commented Oct 13, 2017

@tresf, tested and it looks like it works just fine.

Your educated guess on usage is missing two things: Converting data to an ArrayBuffer and converting the digest output to hex since that's what QZ Tray expects. Both are covered in the example for digest on MDN.

If you include the two extra functions defined in that example then you simply need to call their sha256 function and you're set.

It's unfortunately still not trivial to create a hex representation in JavaScript, which is what most of that code is dedicated to. I imagine one day that'll finally get spec'd in somewhere and that part will be even simpler.

Still, now both dependencies for qz-tray.js can be replaced with native implementations. 👍

@tresf

This comment has been minimized.

Copy link
Contributor

tresf commented Oct 13, 2017

Thanks! Would you be willing to provide your working snippet for our API? We'll add it to the API overrides section.

@bberenz

This comment has been minimized.

Copy link
Member

bberenz commented Oct 13, 2017

I've reverted your changes to the API, and instead we will just wrap the hashing result in a quick-resolved promise as needed.
This way existing functionality doesn't change at all, there are no new api's to mix up when setting a new hashing method, and any overriding method that is already a promise can work as expected in the existing api.

@tresf

This comment has been minimized.

Copy link
Contributor

tresf commented Oct 13, 2017

@bberenz thanks that looks cleaner. Can we explain this in the documentation too? I don't think the current description explains that this functionality will accept a promise.

@@ -900,8 +909,8 @@ var qz = (function() {
if (data[i].constructor === Object) {
if ((!data[i].format && data[i].type && data[i].type.toUpperCase() !== 'RAW') //unspecified format and not raw -> assume file
|| (data[i].format && (data[i].format.toUpperCase() === 'FILE'
|| (data[i].format.toUpperCase() === 'IMAGE' && !(data[i].data.indexOf("data:image/") === 0 && data[i].data.indexOf(";base64,") !== 0))
|| data[i].format.toUpperCase() === 'XML'))) {
|| (data[i].format.toUpperCase() === 'IMAGE' && !(data[i].data.indexOf("data:image/") === 0 && data[i].data.indexOf(";base64,") !== 0))

This comment has been minimized.

@tresf

tresf Oct 13, 2017

Contributor

This indenting looks inconsistent now...

This comment has been minimized.

@bberenz

bberenz Oct 13, 2017

Member

IntelliJ insisted on it, the indented block is in a sub block of the already subbed block above it which I'm sure is why it happened (it seems every minor update to the IDE comes with alterations to the default code style and it's a bit tiring).
Either you or I can play around with the code-style settings to see if it's possible to change it back.

This comment has been minimized.

@tresf

tresf Oct 13, 2017

Contributor

Sorry, I should have been more specific... the line above it is the problem. Not the ones you touched.

This comment has been minimized.

@tresf

tresf Oct 13, 2017

Contributor
if (foo || // spillover
      || bar
      || bar) {
   // do something

I've never seen it spill twice like that. Are you sure that's the IDE's doing? If so, fine. Just strange. :)

This comment has been minimized.

@tresf

tresf Oct 13, 2017

Contributor

Oh, I see it now. It's because of the grouping ( ). No worries then.

@dsanders11

This comment has been minimized.

Copy link
Contributor

dsanders11 commented Oct 14, 2017

@tresf, it's really just straight from that MDN link. I'll provide the snippet I tested with, but it's out of date now after the changes. The MDN link has a simpler implementation of hex in the second example which is probably more concise but I don't have any time to check if it's a less robust implementation.

function sha256 (str) {
    // We transform the string into an arraybuffer.
    var buffer = new TextEncoder('utf-8').encode(str)
    return crypto.subtle.digest('SHA-256', buffer).then(function (hash) {
        return hex(hash)
    })
}

function hex (buffer) {
    var hexCodes = []
    var view = new DataView(buffer)
    for (var i = 0; i < view.byteLength; i += 4) {
        // Using getUint32 reduces the number of iterations needed (we process 4 bytes each time)
        var value = view.getUint32(i)
        // toString(16) will give the hex representation of the number without padding
        var stringValue = value.toString(16)
        // We use concatenation and slice for padding
        var padding = '00000000'
        var paddedValue = (padding + stringValue).slice(-padding.length)
        hexCodes.push(paddedValue)
    }

    // Join all the hex strings into one
    return hexCodes.join('')
}

qz.api.setSha256Promise(function (data) {
    return sha256(data)
})
@tresf

This comment has been minimized.

Copy link
Contributor

tresf commented Oct 14, 2017

the second example which is probably more concise but I don't have any time to check if it's a less robust implementation.

Thanks. I think I'll just setup SSL locally and see if I can get this all working. Part of me wants to enable this automatically for SSL pages, but I know it'll create support issues for our users with local dev environments, so I think we'll keep it as documentation-only for now.

@tresf tresf added this to the 2.0.5 milestone Oct 25, 2017

@tresf tresf added the enhancement label Oct 25, 2017

@klabarge

This comment has been minimized.

Copy link
Contributor

klabarge commented Nov 15, 2017

I used the below code snippet from @dsanders11 / MDN, (updated qz.api.setSha256Type) and this works in Firefox, Chrome, and Safari on macOS and Windows.

IE and Edge throw an error "'TextEncoder' is undefined"

function sha256 (str) {
    // We transform the string into an arraybuffer.
    var buffer = new TextEncoder('utf-8').encode(str)
    return crypto.subtle.digest('SHA-256', buffer).then(function (hash) {
        return hex(hash)
    })
}

function hex (buffer) {
    var hexCodes = []
    var view = new DataView(buffer)
    for (var i = 0; i < view.byteLength; i += 4) {
        // Using getUint32 reduces the number of iterations needed (we process 4 bytes each time)
        var value = view.getUint32(i)
        // toString(16) will give the hex representation of the number without padding
        var stringValue = value.toString(16)
        // We use concatenation and slice for padding
        var padding = '00000000'
        var paddedValue = (padding + stringValue).slice(-padding.length)
        hexCodes.push(paddedValue)
    }

    // Join all the hex strings into one
    return hexCodes.join('')
}

qz.api.setSha256Type(function (data) {
    return sha256(data)
})

@tresf tresf merged commit b80b7a4 into 2.0 Nov 15, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tresf tresf deleted the hash branch Feb 21, 2018

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