Skip to content

Feature/VerifyToken#76

Draft
x1m3 wants to merge 7 commits intomainfrom
feature/PID-2501-update-go-iden-3-auth
Draft

Feature/VerifyToken#76
x1m3 wants to merge 7 commits intomainfrom
feature/PID-2501-update-go-iden-3-auth

Conversation

@x1m3
Copy link
Contributor

@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
@x1m3 x1m3 requested review from ilya-korotya and vmidyllic October 2, 2024 15:32
vmidyllic
vmidyllic previously approved these changes Oct 3, 2024
@vmidyllic vmidyllic dismissed their stale review October 3, 2024 14:00

mistake

auth.go Outdated

// 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
Contributor 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
Contributor 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
Contributor 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