refactor: restructure consent model for updated api #85
Conversation
| import { NotFoundError, RevokedConsentModificationError } from '../errors' | ||
| import Knex from 'knex' | ||
| import { NotFoundError } from '../errors' | ||
| import Knex from 'knex'; | ||
|
|
||
| /* | ||
| * Interface for Consent resource type | ||
| */ | ||
| export interface Consent { |
There was a problem hiding this comment.
So the Consent model was built around the idea of asking the auth-service for a challenge, storing a partial active consent and then later it would have a credential later be verified and updated on the Consent object.
Our API has changed to where the DFSP derives the challenge, PISP user signs and then the DFSP sends the Consent creation request alongside a credential to be verified.
So my thinking is
- that the
credential*fields should no longer be nullable since they are available on creation of theConsentobject - all credentials in
Consentobjects are verified since theregisterConsentflow verifies before storing, bringing into question ifcredentialStatusis still needed Consentobjects never need to be updated in the traditional sense. The only "updating" that happens is the revoking of theConsent. So I've removed all update logic that fell into the old logic of "createConsentobject now, verify/update credential later"
The DB schema ditates a lot and I could use a second opinion @lewisdaly , as well as on any TODO you see as well.
Other than those refactors. I've added two fields that we should need later in for assertion and removed fields that I don't think necessary anymore.
src/model/consent/consent.ts
Outdated
| credentialPayload?: string; | ||
| credentialChallenge?: string; | ||
| // NOTE: not sure what purpose credentialId serves. | ||
| credentialId: string; |
There was a problem hiding this comment.
I think this was the keyHandleId of the credential that is used to lookup the credential on the user's device.
There was a problem hiding this comment.
We can remove it, as it would be covered in credentialPayload.id
There was a problem hiding this comment.
credentialPayload is just the public key correct? Where is credentialPayload.id coming from?
| // credential on receiving them | ||
| credentialStatus: string; | ||
| // assuming this is the public key of the pair | ||
| credentialPayload: string; |
There was a problem hiding this comment.
I think this should be type string | FIDOPublicKeyCredentialAttestation, depending on the credentialType
There was a problem hiding this comment.
Consent interface is 1:1 with the db are you suggesting we store all the fields from FIDOPublicKeyCredentialAttestation?
Looking at https://www.npmjs.com/package/fido2-lib I'm just assuming we need to store the public key from await f2l.attestationResult for FIDO at least
There was a problem hiding this comment.
I saw some old code that pointed me to the assumption that credentialPayload is a public key. Can't find it atm...
There was a problem hiding this comment.
Yeah, you're right. The f2l.attestationResult should do the trick
src/model/consent/consent.ts
Outdated
| participantId?: string; | ||
| // participant DFSP that requested a Consent resource be made | ||
| participantId: string; | ||
| // status of the consent | ||
| status: string; |
There was a problem hiding this comment.
Should be of type ConsentStatus
There was a problem hiding this comment.
Hmm auth-service used ACTIVE/REVOKED but since we are re-doing this might as well make it the same as the api currently i.e VERIFIED/REVOKED
kleyow
force-pushed
the
refactor/consent
branch
from
2472197
to
f5bcd7b
Compare
|
I believe you are commenting under the assumption that the I think I'm going to rename the interface to something else. |
|
Think we might need to talk this over voice chat. @lewisdaly |
|
Yeah I'm pretty lost here. Let's talk it over tonight before we dive into the thirdparty-scheme-adapter stuff |
No description provided.