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.

Sign up for GitHub

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

Jump to bottom

VDR: SubjectManager returns DIDs in preferred order #3291

Merged
merged 11 commits into from
Sep 13, 2024
Merged

VDR: SubjectManager returns DIDs in preferred order #3291

merged 11 commits into from
Sep 13, 2024

Conversation

reinkrul
Copy link
Member

@reinkrul reinkrul commented Aug 1, 2024

No description provided.

@reinkrul reinkrul requested review from gerardsn and woutslakhorst and removed request for gerardsn August 1, 2024 08:24
@reinkrul reinkrul mentioned this pull request Aug 1, 2024
Merged
1 task
gerardsn
gerardsn requested changes Aug 1, 2024
View reviewed changes
Copy link
Member

@gerardsn gerardsn left a comment

Choose a reason for hiding this comment

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

The ordering is controlled by a config flag, and it impacts the order of results presented to the user. Should this be in config description or do we not want to make such guarantees

vdr/didsubject/manager.go Outdated Show resolved Hide resolved
vdr/didsubject/manager.go Show resolved Hide resolved
reinkrul and others added 2 commits August 1, 2024 13:12
@reinkrul @gerardsn
Update vdr/didsubject/manager.go
4cac54a 
Co-authored-by: Gerard Snaauw <33763579+gerardsn@users.noreply.github.com>
@reinkrul
allow duplicates
17695e1
@reinkrul reinkrul requested a review from gerardsn August 1, 2024 11:15
gerardsn
gerardsn requested changes Aug 1, 2024
View reviewed changes
vdr/didsubject/manager_test.go Outdated Show resolved Hide resolved
@reinkrul
Copy link
Member Author

reinkrul commented Aug 1, 2024

The ordering is controlled by a config flag, and it impacts the order of results presented to the user. Should this be in config description or do we not want to make such guarantees

CLI passing is (also) documented in https://nuts-node.readthedocs.io/en/latest/pages/deployment/configuration.html

@reinkrul reinkrul requested a review from gerardsn August 1, 2024 12:10
@gerardsn gerardsn mentioned this pull request Aug 1, 2024
Merged
@gerardsn
Copy link
Member

gerardsn commented Aug 1, 2024

last comment is in the wrong PR?

@reinkrul
Copy link
Member Author

reinkrul commented Aug 1, 2024

Oops, comment moved to #3290

woutslakhorst
woutslakhorst requested changes Aug 2, 2024
View reviewed changes
Copy link
Member

@woutslakhorst woutslakhorst left a comment

Choose a reason for hiding this comment

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

This will probably also impact which DID is used to sign a VP? If so change docs for config accordingly.

@gerardsn gerardsn mentioned this pull request Sep 4, 2024
Merged
@woutslakhorst
Copy link
Member

around line 660 of auth/api/iam.go the signingKeyis set for the openid configuration (signingKey = kid). It uses the last key from the list, should become first one.

@reinkrul
Copy link
Member Author

reinkrul commented Sep 4, 2024

I don't think this should matter any more, since any use of DIDs is bound by what the other side accepts. So there's not really a place for "preferred order". Although weird, if a verifier accepts 2 DID methods and the holder has both, it doesn't matter if it presents the first DID initially, and the second DID at some later point?

gerardsn
gerardsn requested changes Sep 6, 2024
View reviewed changes
auth/api/iam/api.go Outdated Show resolved Hide resolved
@reinkrul reinkrul requested a review from gerardsn September 6, 2024 10:04
@reinkrul
Copy link
Member Author

reinkrul commented Sep 9, 2024

@woutslakhorst would you like to re-review?

@reinkrul reinkrul merged commit 97d05fd into master Sep 13, 2024
8 of 9 checks passed
@reinkrul reinkrul deleted the vdr/subjectmanager-stable-order branch September 13, 2024 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gerardsn gerardsn gerardsn approved these changes

@woutslakhorst woutslakhorst woutslakhorst approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@reinkrul @gerardsn @woutslakhorst