Skip to content
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

Javascript NPM packaging #239

Merged
merged 29 commits into from
May 3, 2022
Merged

Javascript NPM packaging #239

merged 29 commits into from
May 3, 2022

Conversation

micwallace
Copy link
Collaborator

Hey @jot2re & @oleggrib,

This branch adds stuff for publishing the JS as a typescript library on NPM (https://www.npmjs.com/package/@tokenscript/attestation)

It also:

  • Moves postMesssage integration logic out of Authenticator to a separate IntegrationExample class for usage in the old demos.
  • Adds old schema versions, that are currently used by TN demos to a folder tn-compat. When the getUseTicket function is called there is an optional argument to use the old versions. These can be removed once we have updated TN ticket issuing to use the new schema.
  • Ensure ASN parsing errors are included in thrown exceptions in getUseTicket.

This doesn't necessarily need to be merged into main, we could keep a separate branch for JS library publishing and I'll keep it up to date with main. Let me know what you think.

Cheers,
Michael

@jot2re
Copy link
Collaborator

jot2re commented Mar 29, 2022

Hi Michael, thanks a lot!
I think it would make sense to have it all merged in main to keep it all in place, and simply remove the code for handling tickets when we can.
But @oleggrib is the one who is most aware of what is best to do regarding TS and JS code :)

@jot2re jot2re requested a review from oleggrib March 29, 2022 13:46
@jot2re
Copy link
Collaborator

jot2re commented Apr 4, 2022

@oleggrib can you check and merge this into main if you think it is fine?

- Hybrid ES6 & commonjs build.
- Add various functions to remove dependencies from
  projects using the library.
@micwallace
Copy link
Collaborator Author

Hey @oleggrib the library is now integrated with TN & attestation.id, and I have confirmed all existing tests are passing.

Let me know if you want me to resolve the merge conflict from main.

Copy link
Collaborator

@jot2re jot2re left a comment

Choose a reason for hiding this comment

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

I have a hard time judging the TS/JS stuff, but ran a test and things still seem to work on my end.

src/main/javascript/crypto/package.json Outdated Show resolved Hide resolved
- Added proof validation function for TN.
- Change getUseTicket function to static for consistency.
- Fix type signature for logging functions.
Update version to be in sync with jar major and minor version.
@micwallace
Copy link
Collaborator Author

Hey @oleggrib & @jot2re

As discussed here I've had to make the package into a commonjs version rather than es6 or hybrid to make it work with both TN and attestation.id.

In the new version I have also added a proof validation function to Authenticator.js to validate the proof in TN. Can you have a look and see if it's okay?

@jot2re
Copy link
Collaborator

jot2re commented Apr 15, 2022

Thanks a lot @micwallace ! I think we need @oleggrib to judge it

@micwallace
Copy link
Collaborator Author

Hey @oleggrib & @jot2re

Issues with the attestation packaging have been ironed out and it's working well as a commonjs module in attestation.id and TN for all platforms in use (webpack, UMD bundle, react, next.js). Please review the changes at your earliest convenience.

Copy link
Collaborator

@oleggrib oleggrib left a comment

Choose a reason for hiding this comment

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

Can you check my comments and make some updates/comments so we can move this repo to the /authenticator and avoid duplication.
for testing - we can put /attestation and /authenticator next each to other and update path to read JAVA-builds to test JS version.
@micwallace , @jot2re , ok?

notAfter GeneralizedTime,
notAfterInt INTEGER OPTIONAL -- NotAfter time since epoch in milliseconds
notAfterInt INTEGER OPTIONAL -- NotAfter time since epoch in seconds
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jot2re , I think we have to make all 4 dates required. because SmartContract require integers

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, they are definitely needed for something running on the smart contract. However, optional for the any web2 usage. Because if we make them all required then the schema is not backward compatible with old attestations (furthermore GeneralizedTime becomes totally superfluous and should just be removed).
However, as we talked about loosing backward compatibility is not really an issue since we have only issued 1 hour valid attestations for awhile, and I think it is fair to deprecated those few "unlimited time" old attestations we issued.
So ideally I would just want to remove GeneralizedTime and deprecate the initial attestation, but I think that should be cleared with Weiwu. This would also require many code changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The logic in all the code bases (including smart contract), is that notAfterInt is optional but required to work on smart contracts, and when new attestations are issued notAfterInt and notBeforeInt will always be set unless explicitly requested to not be included.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, correct, but do we use option to force old legasy format somewhere? As I know - we dont use it and always add int values.
even more - libs updated to always read Int and GeneralizedTime both(if exists) and compare them, so in real usage we always use new format only @jot2re

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, true. I do think the best would simply be to get rid of the GeneralizedTime field. But I don't think we should do that before we get a good and integrated CI/CD setup.
I can change the backend code to always enforce that notBeforeInt and notAfterInt are included, but in that case we should make a separate issue, since this is not related to NPM packaging ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, lets leave it for separate issue

Copy link
Collaborator

Choose a reason for hiding this comment

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

Added a new issue #249

} else {
ticket = Ticket.fromBase64(base64ticket,{"6": KeyPair.publicFromBase64(base64senderPublicKey)});
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Better remove hardcoded conferenceID "6" and receive keysArray as argument, like here
https://github.com/TokenScript/attestation/blob/ticket_validator/src/main/javascript/crypto/src/Authenticator.ts#L682-L710

Copy link
Collaborator

Choose a reason for hiding this comment

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

this code can be updated after merge with main, because its related to the Ticket

@@ -163,7 +81,7 @@ export class Authenticator {
logger(DEBUGLEVEL.MEDIUM,'ticked valid (signature OK)');
} catch (e) {
logger(DEBUGLEVEL.MEDIUM,'getUseTicket: ticket validation failed',e);
throw new Error("getUseTicket: ticked validation failed");
throw new Error("getUseTicket: ticked validation failed: " + e.message);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need some way to set DEBUGLEVEL, because it can't be set in config.ts, because its will be ready NPM package.
lets add some variable to the .env , so code can read and use it

Copy link
Collaborator

Choose a reason for hiding this comment

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

added commit, where I moved debug variable to the process.env.DISPLAY_DEBUG_LEVEL or window.DISPLAY_DEBUG_LEVEL so we can change debug level even for UMD builds

@AsnProp({ type: AsnPropTypes.Utf8String }) public devconId: string;
@AsnProp({ type: AsnPropTypes.Integer }) public ticketId: number;
@AsnProp({ type: AsnPropTypes.Integer }) public ticketClass: number;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you check main branch for code?
https://github.com/TokenScript/attestation/blob/main/src/main/javascript/crypto/src/asn1/shemas/SignedDevconTicket.ts#L10-L17
its single object , compatible with old and new ticket ASN schema
and Ticket.ts updated to be compatable with ID as integer/string and old and new ticket formats

Copy link
Collaborator

Choose a reason for hiding this comment

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

we can merge this branch with main and stay Tickets.ts from main branch. it can read both Ticket formats

@@ -64,6 +65,13 @@ export class Attestation {
if (decodedAttestationObj.validity){
me.notValidBefore = decodedAttestationObj.validity.notBefore.generalizedTime.getTime();
me.notValidAfter = decodedAttestationObj.validity.notAfter.generalizedTime.getTime();
// TODO validate time when it will be updated in Java code
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be updated in the java code, so I think the code fragment can be enabled.



public getUrlEncoding() {
// TODO implement
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe throw an exception if it happens to be called? It will make debugging easier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

usually throw an error is good option, but in this case I prefer to use Ticket reader from main branch, so no need to update duplicated code. we can remove tn-compat/Ticket.ts because schema is not compatable with legacy ticket. main branch already can work with both ticket types.


attachPostMessageListener(listener: Function){
if (window.addEventListener) {
window.addEventListener("message", (e) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

To avoid potential cross origin attacks, it should be validated that the origin of the message is as expected, ie. something like the following should be added

if (event.origin !== "http://example.org") // Compliant
    return;

Copy link
Collaborator

Choose a reason for hiding this comment

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

this part of code just add event listener , so we have to check event.origin in the callback function. this part of code doesnt have access to the event object

@micwallace micwallace merged commit e783cc9 into main May 3, 2022
@jot2re jot2re deleted the feature/js-packaging branch November 14, 2022 15:10
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