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

Feature/VerifyToken #76

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

x1m3
Copy link

@x1m3 x1m3 commented Oct 2, 2024

New version in #78

To remove.

@x1m3 x1m3 added the help wanted Extra attention is needed label Oct 2, 2024
vmidyllic
vmidyllic previously approved these changes Oct 3, 2024
auth.go Outdated
) (*protocol.AuthorizationResponseMessage, error) {

// VerifyToken performs verification of jws/jwz token using the registered packers in package manager
func (v *Verifier) VerifyToken(token string) (*protocol.AuthorizationResponseMessage, error) {
msg, _, err := v.packageManager.Unpack([]byte(token))
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to pass options as optional params also, so we can verify 'accept delay for proof transition'

Copy link
Author

@x1m3 x1m3 Oct 3, 2024

Choose a reason for hiding this comment

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

Sorry. I'm not understanding. Can you clarify? Are you proposing changing signature to this?

func (v *Verifier) VerifyToken(token string, opts ...pubsignals.VerifyOpt) (*protocol.AuthorizationResponseMessage, error)

Then.. What should we do with this opts params? The v.packageManager.Unpack([]byte(token)) doesn't accept optional parameters. Should I stop using Unpack and verify token in a more low level way?

Copy link
Contributor

Choose a reason for hiding this comment

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

The v.packageManager.Unpack([]byte(token)) doesn't accept optional parameters. This is a valid point.
We need to think about providing options to unpack (or setuping packers with this option as a workaroud).

Copy link
Author

Choose a reason for hiding this comment

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

Params added.

@x1m3 x1m3 marked this pull request as draft October 10, 2024 11:12
@x1m3
Copy link
Author

x1m3 commented Oct 10, 2024

@vmidyllic @ilya-korotya This is still a draft to discuss. I left some comments in my own code as questions.

First of all is that this other PR iden3/iden3comm#58 must be accepted before merging this.

Another issue is that I failed trying to test it.

@x1m3 x1m3 mentioned this pull request Oct 17, 2024
@x1m3 x1m3 added invalid This doesn't seem right and removed help wanted Extra attention is needed labels Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants