bcgov / aries-oca-bundles

aries-oca-bundles
Apache License 2.0
8 stars 20 forks source link

feat: removed pii flags from member card and person credential #83

Closed wadeking98 closed 11 months ago

wadeking98 commented 11 months ago

We haven't confirmed with LSBC and IDIM which attributes they want to flag, so we will leave them as unflagged until we confirm. I've also copied the changes to the showcase credentials for consistency.

swcurran commented 11 months ago

PR looks good — just would like to understand the rationale for removing a PII flag from PII attributes.

cvarjao commented 11 months ago

@swcurran, while I agree those elements might be PII, it is not up to the wallet team to define that. That needs to be reviewed and defined by the issuer authority. Those elements were flagged as part of our development and testing cycle. Unless you are saying that you have already checked with those authorities and their needs are already accurately represented.

swcurran commented 11 months ago

Hmmm….

By that logic, the wallet team should not be defining them either, unless you have checked with the authorities.

That said, The Kantera Initiative defines in the Blinded Identity Taxonomy what should be considered PII. When I set the flags on in the original Excel files, I used this list as a guide.

We can merge if you want, I just thing it is pretty strange.

wadeking98 commented 11 months ago

Hmmm….

By that logic, the wallet team should not be defining them either, unless you have checked with the authorities.

That said, The Kantera Initiative defines in the Blinded Identity Taxonomy what should be considered PII. When I set the flags on in the original Excel files, I used this list as a guide.

We can merge if you want, I just thing it is pretty strange.

I think we should merge, best not flag anything in the Member card and Person credentials without the approval of IDIM and LSBC. I agree that these attributes should be flagged and they probably will be flagged later, but I think what @cvarjao is saying is that it's a professional courtesy not to start making assumptions about how IDIM and LSBC want to display their credentials

cvarjao commented 11 months ago

We can hold off for now. I've sent an email to BCSC team, and if they agree with can leave as is

wadeking98 commented 11 months ago

We can hold off for now. I've sent an email to BCSC team, and if they agree with can leave as is

I've improved the excel file layout and a small bug fix as well, if BCSC approves then I'll add the flagged attributes back and then we can merge

amanji commented 11 months ago

I'll defer to comments by @swcurran

One thing to note (and this is a separate bug on its own but I don't believe is actually being used in production) is that this line and possibly other filtering logic using flagged_attributes would need to be updated. Should be a simple fix.