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

Auth: replace DID with subject in internal API endpoints #3277

Merged
merged 31 commits into from
Aug 30, 2024

Conversation

reinkrul
Copy link
Member

@reinkrul reinkrul commented Jul 29, 2024

TODO:

Requires:

@reinkrul reinkrul force-pushed the auth/internal-api-subject branch from 556795a to 1c733aa Compare July 31, 2024 04:39
@woutslakhorst
Copy link
Member

the random ordering might also impact the test results (random failures)

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.

Shouldn't you pass down the subject to the RFC021 request? The PD should be held against all wallets.

@reinkrul
Copy link
Member Author

reinkrul commented Aug 1, 2024

Shouldn't you pass down the subject to the RFC021 request? The PD should be held against all wallets.

Can you describe how that works? What if the Authorization Server doesn't support did:nuts (because of configured didmethods or Presentation Definition). Does the client still try to present a credential issued to did:nuts?

@woutslakhorst
Copy link
Member

Shouldn't you pass down the subject to the RFC021 request? The PD should be held against all wallets.

Can you describe how that works? What if the Authorization Server doesn't support did:nuts (because of configured didmethods or Presentation Definition). Does the client still try to present a credential issued to did:nuts?

metadata doesn't support it (yet) so it's a governance thing. Before requesting a VC issued to a certain did method in PEX, all members should have updated.

@reinkrul
Copy link
Member Author

reinkrul commented Aug 3, 2024

Shouldn't you pass down the subject to the RFC021 request? The PD should be held against all wallets.

Yes, but the PEX implementation (Submission Builder) and VP verification are all fixed to 1 DID now. Changing it to support multiple DIDs is going to take a lot of work, so best to do that later, and do the API changes first.

Nothing breaks now, worst case credentials aren't found if the "didmethods" config order is off, but there aren't actual use cases that support those at this moment (e.g. KIK-v, eOverdracht or ANW starting to use did:web)

@reinkrul reinkrul requested a review from woutslakhorst August 3, 2024 05:32
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.

I think PD fulfillment needs to happen at a lower level. If we put it here now I think it will be a lot harder to fix later on

auth/api/iam/api.go Show resolved Hide resolved
auth/api/iam/api.go Outdated Show resolved Hide resolved
auth/api/iam/api.go Outdated Show resolved Hide resolved
auth/api/iam/api.go Outdated Show resolved Hide resolved
auth/api/iam/openid4vci.go Outdated Show resolved Hide resolved
auth/api/iam/user_test.go Outdated Show resolved Hide resolved
auth/client/iam/openid4vp.go Outdated Show resolved Hide resolved
auth/api/iam/api.go Show resolved Hide resolved
@woutslakhorst
Copy link
Member

@reinkrul ping me when other PRs have been merged.

@reinkrul reinkrul requested a review from gerardsn August 27, 2024 17:40
@reinkrul
Copy link
Member Author

@woutslakhorst ready for review

auth/api/iam/api.go Show resolved Hide resolved
auth/api/iam/metadata.go Outdated Show resolved Hide resolved
@reinkrul reinkrul merged commit c1c5329 into master Aug 30, 2024
8 of 9 checks passed
@reinkrul reinkrul deleted the auth/internal-api-subject branch August 30, 2024 07:50
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