-
Notifications
You must be signed in to change notification settings - Fork 1
nfc related support #93
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
base: main
Are you sure you want to change the base?
Conversation
Implements NFC rendering support for verifiable credentials following the W3C VC Rendering Methods specification. Provides both static and dynamic rendering modes: - Static mode: Decodes pre-encoded payloads from template/payload fields - Dynamic mode: Extracts credential data using JSON pointer paths - Supports multibase (base58/base64url) and data URI encoding - Compatible with W3C spec renderSuite types (nfc, nfc-static, nfc-dynamic) - Maintains backward compatibility with legacy NfcRenderingTemplate2024 Public API includes supportsNFC() to check NFC capability and renderToNfc() to generate NFC payload bytes from credentials.
Implements test suite covering NFC rendering functionality: - Tests supportsNFC() with various renderSuite configurations - Tests renderToNfc() static rendering with multibase, base64url, and data URIs - Tests renderToNfc() dynamic rendering with renderProperty extraction - Tests EAD credential handling with single/multiple field extraction - Tests legacy NfcRenderingTemplate2024 type compatibility - Tests error handling for invalid credentials and missing parameters - Includes real-world credential tests fetched from external sources Ensures compliance with W3C VC Rendering Methods specification.
Replaces legacy NFC rendering implementation in helper.js with calls to the new standardized nfcRenderer library functions: - Replace toNFCPayload() implementation with renderToNfc() call - Replace hasNFCPayload() implementation with supportsNFC() call - Remove unused base58 and base64url imports - Comment out legacy helper functions: _getNFCRenderingTemplate2024() and _decodeMultibase() - Comment out multibaseDecoders constant (now handled in nfcRenderer.js) This change consolidates NFC rendering logic into a single reusable library while maintaining backward compatibility with existing API. The new implementation supports both W3C spec formats and legacy NfcRenderingTemplate2024 type.
Implements field validation: - TemplateRenderMethod: requires 'template' field only (W3C spec) - NfcRenderingTemplate2024: requires 'payload' field only (legacy) - Rejects credentials with both fields present Updates _renderStatic() and _hasStaticPayload() functions with explicit field checking and clear error messages. Adds comprehensive validation tests and fixes existing tests to use correct fields for their respective render method types.
|
@BigBlueHat cc: @dlongley I have enforced some level of type compliance for |
Replace separate static/dynamic rendering with unified pipeline where template is always required and renderProperty validates credential fields. Update implementation and tests to reflect single rendering flow: filter credential -> decode template -> return bytes. Changes: - Consolidate rendering functions into _decodeTemplateToBytes(). - Add _filterCredential() for renderProperty validation. - Remove obsolete static vs dynamic distinction. - Update all tests to match unified architecture. - Fix test encodings and add comprehensive error coverage.
| // import * as base58 from 'base58-universal'; | ||
| // import * as base64url from 'base64url-universal'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove all the commented stuff here and below so it's easier to understand what would be approved and get merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, just saw the "disclaimer" above.
EDIT: Though, I still think it would be better to remove before approval so it's easier to review and to understand what will get merged.
| if(renderMethod !== null) { | ||
| return true; | ||
| } | ||
| // no NFC render method found | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if(renderMethod !== null) { | |
| return true; | |
| } | |
| // no NFC render method found | |
| return false; | |
| // return whether any NFC render method was found | |
| return renderMethod !== null; |
| export function supportsNFC({credential} = {}) { | ||
| try { | ||
| const renderMethod = _findNFCRenderMethod({credential}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our naming convention is usually camel case (and this will make it consistent with renderToNfc() below):
| export function supportsNFC({credential} = {}) { | |
| try { | |
| const renderMethod = _findNFCRenderMethod({credential}); | |
| export function supportsNfc({credential} = {}) { | |
| try { | |
| const renderMethod = _findNfcRenderMethod({credential}); |
| 'The verifiable credential does not support NFC rendering.' | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linting should be catching this, so we might be missing a rule:
| 'The verifiable credential does not support NFC rendering.' | |
| ); | |
| 'The verifiable credential does not support NFC rendering.'); |
| throw new Error( | ||
| 'NFC rendering requires a template field. ' + | ||
| 'The template should contain the pre-encoded NFC payload.' | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message will get out-of-date if a template becomes something other than a "pre-encoded NFC payload" (and same linting issue as above with dropped lonely parenthesis):
| throw new Error( | |
| 'NFC rendering requires a template field. ' + | |
| 'The template should contain the pre-encoded NFC payload.' | |
| ); | |
| throw new Error('NFC render method is missing the "template" field.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though, on second thought, I think the _findNfcRenderMethod should probably only find valid NFC render methods. Maybe we'd want to console.warn() any invalid methods found, but that might be better than turning on UI elements that won't work anyway ... this is debatable / would love to hear more opinions. We can leave it how it is now (where "inoperable"/"invalid" NFC render methods are still "found") until we decide to change this.
| // validate: should not have both fields | ||
| if(renderMethod.template && renderMethod.payload) { | ||
| throw new Error( | ||
| 'TemplateRenderMethod requires "template". ' + | ||
| 'It should not have both fields.' | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the error text below might be confusing to a reader, i.e, "What two fields?". However, for TemplateRenderMethod, payload isn't specified at all (in the spec) and we don't expect it to be, so it would just be ignored. I had mentioned before this might be an error case, but I think that would have only been for the legacy NFC render (2024) method ... but I don't think it ever used template. In other words, I suspect we don't have to check for the case where both appear at all, it's an "ignore unknown fields" case for both the modern renderer and the legacy one.
| // validate: should not have both fields | |
| if(renderMethod.template && renderMethod.payload) { | |
| throw new Error( | |
| 'TemplateRenderMethod requires "template". ' + | |
| 'It should not have both fields.' | |
| ); | |
| } |
| // validate: should not have both fields | ||
| if(renderMethod.template && renderMethod.payload) { | ||
| throw new Error( | ||
| 'NfcRenderingTemplate2024 should not have both template ' + | ||
| 'and payload fields.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, unless there was ever legitimate use of "template" in an NfcRenderingTemplate2024 renderer, we should treat the presence of both properties as an "ignore unknown field" case. Only if we ever defined "template" and used it in the absence of "payload" should we throw here -- so if we don't have that case, we actually shouldn't throw this error.
| // validate: should not have both fields | |
| if(renderMethod.template && renderMethod.payload) { | |
| throw new Error( | |
| 'NfcRenderingTemplate2024 should not have both template ' + | |
| 'and payload fields.' | |
| ); | |
| } |
|
|
||
| // validate template is a string | ||
| if(typeof encoded !== 'string') { | ||
| throw new Error('Template or payload must be a string.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| throw new Error('Template or payload must be a string.'); | |
| throw new TypeError('Template or payload must be a string.'); |
| function _base64ToBytes({base64String} = {}) { | ||
| // use atob in browser, Buffer in Node | ||
| if(typeof atob !== 'undefined') { | ||
| const binaryString = atob(base64String); | ||
| const bytes = new Uint8Array(binaryString.length); | ||
| for(let i = 0; i < binaryString.length; i++) { | ||
| bytes[i] = binaryString.charCodeAt(i); | ||
| } | ||
| return bytes; | ||
| } | ||
|
|
||
| // Node.js environment | ||
| return Buffer.from(base64String, 'base64'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work -- static analyzers will see Buffer and inject a polyfill into the browser bundle. There are other patterns that solve this that we should use instead. The utilities have to be split and different aliased files loaded. For example:
https://github.com/digitalbazaar/http-digest-header/blob/v2.3.0/lib/util-browser.js
https://github.com/digitalbazaar/http-digest-header/blob/v2.3.0/lib/util.js
Also note the more modern API Uint8Array.toBase64.
| // ======================== | ||
| // JSON pointer utilities | ||
| // ======================== | ||
|
|
||
| /** | ||
| * Resolve a JSON pointer in an object per RFC 6901. | ||
| * | ||
| * @private | ||
| * @param {object} options - Options object. | ||
| * @param {object} options.obj - The object to traverse. | ||
| * @param {string} options.pointer - JSON pointer string. | ||
| * @returns {*} The value at the pointer location or undefined. | ||
| */ | ||
| function _resolveJSONPointer({obj, pointer} = {}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This we really need to get into a shared library (or reuse an existing shared library that doesn't bring in any of its own problems).
Implements the W3C VC Rendering Methods spec for NFC rendering with both static
and dynamic modes. Includes comprehensive tests and maintains backward
compatibility with the legacy NfcRenderingTemplate2024 format.
Disclaimer: I will remove the commented code in helper.js once approved and before merging.