fix: remove username matching

[janniks]

scope

EDIT: now removes the username field completely from auth

removes username matching from auth package (application-side)
currently — in applications using @stacks/connect — the @stacks/auth package will "validate" usernames which the wallet returned in the authResponse. as "validation" these usernames are matched to the response of the API's /names endpoint.

the matching is removed from verifyAuthResponse. in addition, the previously used doPublicKeysMatchUsername seems incorrect and is removed completely (only used in verifyAuthResponse previously).

comments

a second PR could investigate removing the username field from auth responses in general, as brought up here (should be coordinated w/ wallet-web).

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/blockstack/stacks-js/5ZoPambcNLVJqV8fmXmFzbPkNZfC
✅ Preview: https://stacks-js-git-remove-username-matching-blockstack.vercel.app

[codecov-commenter]

Codecov Report

Merging #1230 (b25b3c4) into master (265b483) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1230      +/-   ##
==========================================
- Coverage   64.10%   64.00%   -0.10%     
==========================================
  Files         123      123              
  Lines        8619     8574      -45     
  Branches     1830     1818      -12     
==========================================
- Hits         5525     5488      -37     
+ Misses       2986     2978       -8     
  Partials      108      108              

Impacted Files	Coverage Δ
packages/auth/src/index.ts	100.00% <ø> (ø)
packages/keychain/src/identity.ts	98.59% <ø> (-0.02%)	⬇️
packages/wallet-sdk/src/models/account.ts	91.83% <ø> (-0.17%)	⬇️
packages/auth/src/messages.ts	83.33% <100.00%> (+1.14%)	⬆️
packages/auth/src/userSession.ts	53.79% <100.00%> (-0.94%)	⬇️
packages/auth/src/verification.ts	75.60% <100.00%> (-1.90%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 265b483...b25b3c4. Read the comment docs.

[janniks]

cc'ing @kantai and @zone117x for any potential issues with this change that haven't come up yet

[kyranjamie]

Nice improvement @janniks.

This never made sense to me: if the registrar is down, you can't verify an auth response? 🤷🏼

[markmhendrickson]

@janniks Any ETA on when this PR might get released? We're hoping to incorporate it into out test app so we can QA leather-io/extension#2303 again

[kantai]

This is fine to me, so long as you're all clear on what the goal is here: removing application validation of BNS names. Without this validation, applications can no longer trust that the BNS name that the authentication response provides is actually owned by the claimed user.

[markmhendrickson]

Per @kantai's point here, I wonder what documentation may need updating to make this change of behavior clear. And perhaps we should pair this with removing usernames from the auth responses in general, to prevent developers from assuming incorrectly that they've been verified?

@janniks want to audit the docs for such possible updates and make the auth response change as well?

[janniks]

I'm thinking a major release bump and changelog mention. But I'll go through the docs to see what should be updated.

... and make the auth response change as well?

Yes, good point! I'm removing these now and checking how it ties in to the user session data.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
stacks-js	✅ Ready (Inspect)	Visit Preview	Apr 19, 2022 at 0:59AM (UTC)

[janniks]

98bff3f removes the username field completely. since it's just a removal i'm not sure if it warrants a change in the token.version (e.g. see sip-auth, which isn't merged yet →)
cc @kantai @zone117x

[zone117x]

98bff3f removes the username field completely

👍 this seems like a safer approach than just documentation changes.

It seems like a token version bump would make sense with this change -- it would help apps that may want to explicitly check for this rather than having to infer it.

Should this change be brought up in stacksgov/sips#50? Ideally, that SIP would make a similar change or make the username optional.

[markmhendrickson]

Should this change be brought up in stacksgov/sips#50? Ideally, that SIP would make a similar change or make the username optional.

Yep we've been in touch with @friedger about this functional change. @janniks I'd suggest leaving a comment about it on the above SIP PR whenever you have a chance as well, to help with visibility.

[janniks]

✅ great input! will merge and beta-release to test