Skip to content

Conversation

@povder
Copy link
Contributor

@povder povder commented Dec 21, 2015

This PR is an outcome of this discussion: bblfish@d0800da#commitcomment-15037842

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Other algorithms (e.g. ECDH and DH) also support deriveKey and deriveBits http://www.w3.org/TR/WebCryptoAPI/#algorithm-overview. These two are asymmetric ciphers, not hash algorithms. This makes me think AlgorithmIdentifier is the only correct typing here. (Similar to how you have relaxed verify to verify both HMAC, RSA and AES varients).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking. I'll relax the type here. I'll relax the type in sign method as well.

I think I'll create another pull request in the future introducing MacAlgorithm type for verify and sign. I'll do it in another PR since I think it will need some more discussion.

@povder
Copy link
Contributor Author

povder commented Jan 1, 2016

@sjrd Can you please take a look at this PR?

@mseddon
Copy link
Contributor

mseddon commented Jan 19, 2016

Sorry about the delay- things got a little idle over christmas, I'll review this tomorrow properly. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sjrd do we want this example + deps in the example project?

@mseddon
Copy link
Contributor

mseddon commented Jan 20, 2016

That's all really, the fixes seem good- overall relaxing those signatures solves the underlying problem, and the strengthening of digest to require HashAlgorithmIdentifier is correctly supporting exactly the SHA family.

build.sbt Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dependency is a deal breaker. We cannot afford to have scala-js-dom depend on anything but Scala.js core; otherwise we'll be unable to publish scala-js-dom for new versions of Scala.js until base64 is published, and that will be a nightmare.

@mseddon
Copy link
Contributor

mseddon commented Jan 20, 2016

Alright, suggest we drop commit 6c4db04 and squash the rest, pending any other style issues.

@sjrd
Copy link
Member

sjrd commented Jan 20, 2016

I agree. (I don't have other style issues to report)

@mseddon
Copy link
Contributor

mseddon commented Jan 20, 2016

@povder Post a comment when this is done so we'll notice and we can merge, thanks for the fix!

@mseddon
Copy link
Contributor

mseddon commented Jan 20, 2016

Oh also, if you add "Fix: #196" to the beginning of the commit message, that will also close that issue once merged.

@povder
Copy link
Contributor Author

povder commented Jan 21, 2016

Commit 6c4db04 removed, other commits squashed. Thanks for the review.

@mseddon
Copy link
Contributor

mseddon commented Jan 21, 2016

Thanks!

@mseddon
Copy link
Contributor

mseddon commented Jan 22, 2016

@sjrd okay for a merge?

@sjrd
Copy link
Member

sjrd commented Jan 22, 2016

I suggest removing the changes to Example.scala. This file is only meant to contain the examples in the README. And this example is too complex for the readme.

Maybe we need a different discussion about how we want to go about more complex examples. But in the current state, it should not be included.

@povder
Copy link
Contributor Author

povder commented Jan 22, 2016

Changes to Example.scala removed

@sjrd
Copy link
Member

sjrd commented Jan 22, 2016

LGTM

sjrd added a commit that referenced this pull request Jan 22, 2016
Correct algorithm parameter types in SubtleCrypto interface
@sjrd sjrd merged commit da79949 into scala-js:master Jan 22, 2016
@povder povder deleted the web_crypto_api branch January 22, 2016 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants