webauthn-open-source / fido2-lib

A node.js library for performing FIDO 2.0 / WebAuthn server functionality
https://webauthn.io
MIT License
407 stars 120 forks source link

Move to ESM. Replace Node-specific code. #83

Closed Hexagon closed 2 years ago

Hexagon commented 2 years ago

Overview

PR based on https://github.com/Hexagon/webauthn but ported back to fido2-lib directory structure and code conventions.

Changelog

Fixes

Added

Changed

Removed

Comments

JamesCullum commented 2 years ago

Thanks for the awesome work! Will review asap

JamesCullum commented 2 years ago

Really great changes, thanks! I did a quick review and all looks good.

A few points open:

  1. You changed the algName from "SHA256" to "SHA-256". Will this break something? I don't see this being changed elsewhere.
  2. I would like this to be a major version bump - can you increment the major version in package.json?
  3. Do we need to update documentation?
  4. Code scanning raised line 220 in lib/utils.js - this would only replace the first \r - I assume that you actually want this to be a global replace?

image

Hexagon commented 2 years ago

@JamesCullum Glad to hear it looks ok!

  1. It's changed where it's needed in the code, the background is that node crypto uses algorithm names without dash (https://nodejs.org/api/crypto.html#class-hash) while webcrypto includes the dash (https://nodejs.org/api/webcrypto.html#rsahashedimportparamshash)

  2. Fixed in next commit

  3. The following changes is made to Readme.md

    • Separate examples for require and import
    • Changed CI -> Node CI on badge, to prepare for Deno CI
    • Added --save on npm install command
    • Changed var to const in example implementations
  4. Fixed in next commit. Also added CRLF-line endings to one of the pem fixtures, to make sure it works as intended.

Additional changes:

  1. Replaced npm run test with npm run build in testing pipeline. npm run build run these steps:

    • npm run test
    • npx rollup (to generate dist/main.cjs and test/dist/*.js
    • npm run test:dist
  2. Removed ./test.js

  3. Updated changelog in PR description

JamesCullum commented 2 years ago

Thanks for the awesome work - looking forward to the Deno PR 👍