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

Fix Panic in CLI Convert #24

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix Panic in CLI Convert #24

wants to merge 1 commit into from

Conversation

apardods
Copy link

The CLI convert function does not add a certificate to the TicketBAI client. This makes sense as it does not send it to TicketBAI and a certificate is not needed. However, when creating the client it attempts to open a gateway connection with no certificate with leads to a panic.

I have added a nil check for the certificate so that if an invoice with no certificate is sent to the client, it doesn't try to open a gateway to the TicketBAI service.

Copy link
Contributor

@samlown samlown left a comment

Choose a reason for hiding this comment

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

Are you sure this is the correct approach?

@@ -123,6 +123,10 @@ func New(software *Software, zone l10n.Code, opts ...Option) (*Client, error) {
opt(c)
}

if c.cert == nil {
return c, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it not raise an error instead?

Copy link
Author

Choose a reason for hiding this comment

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

It depends on whether a certificate should be mandatory to convert a TicketBAI document. With these changes, if a user wants to convert a document with without a certificate it can (with empty signature and fingerprint).

If a user attempts to send a document from the CLI without a certificate an error is raised when attempting to load the certificate. If a user attempts to fingerprint the document it will do so with no problem, but if it attempts to sign it returns {"error":"signing: cannot sign without a certificate"}.

The way I see if we have a Convert() function we want to make accessible (it even has its CLI command), it should not require a certificate, especially because the result is not signed.

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.

2 participants