Add route to Discovery for verifying JWT C-445 #3099

nicoback2 · 2022-05-16T14:55:33Z

Description

Add route to Discovery:
-Accepts JWT from read-only OAuth flow
-Gives success response with decoded JWT payload if the JWT is valid
-Otherwise returns error (404 or 400)

Tests

-Tested by calling the endpoint in browser console with JWTs generated from /oauth/auth client page

How will this change be monitored? Are there sufficient logs?

jowlee · 2022-05-16T15:00:07Z

discovery-provider/src/api/v1/models/users.py

+decoded_user_token = ns.model(
+    "decoded_user_token",
+    {
+        "userId": fields.String(required=True),


Generally, the convention used in discovery is to use snake_case for api responses vs camelCase, but not sure if there's some standard for camelcase in oauth flows.

Yeah totally - I originally snake cased everything but then realized it would be confusing to change the key/property names from camel case (which is what the original JWT payload has) to snake case. And yeah camelCase seems to be the standard for JSON https://stackoverflow.com/questions/5543490/json-naming-convention-snake-case-camelcase-or-pascalcase

jowlee · 2022-05-16T15:02:09Z

discovery-provider/src/api/v1/users.py

+        if not raw_token:
+            abort_bad_request_param("token", ns)


I don't think you need this since the verify_token_parser adds token as required but would test first

jowlee · 2022-05-16T15:03:56Z

discovery-provider/src/api/v1/users.py

+
+        # 2. Decode the signature
+        try:
+            signature = base64.urlsafe_b64decode(token_parts[2] + "==")


Is there a reason to add the "==" here instead of including it from the client?

Yeah this is just padding to make sure we don't run into any decoding issues, since this function throws if the length is not divisible by 4. Padding is optional for urlsafe-base64 so I left it out in the JWT which seems to be convention

jowlee · 2022-05-16T15:04:51Z

discovery-provider/src/api/v1/users.py

+        if not len(token_parts) == 3:
+            abort_bad_request_param("token", ns)


Is there a reason it's a single url param vs three params for signature, header, and payload? or is this the convention?

yeah I thought it'd be simpler for the client to just send the whole jwt (which is returned from oauth redirect) instead of splitting it up. This way, they don't necessarily even have to know how to deal with the jwt or what a jwt is.

jowlee · 2022-05-16T15:09:22Z

discovery-provider/src/queries/get_user_with_wallet.py

+logger = logging.getLogger(__name__)
+
+
+def get_user_with_wallet(wallet):


could add types if you wanted ie
def get_user_with_wallet(wallet: str) -> int

jowlee · 2022-05-16T15:11:41Z

discovery-provider/src/api/v1/users.py

+        except Exception:
+            ns.abort(404, "The JWT signature is invalid.")
+        if not wallet:
+            ns.abort(404, "The JWT signature is invalid.")


nit, the same error message is used for all the ns.aborts, could be more specific for each to differentiate when it errored

[e804aa1] [C-2248, C-2373] Use playlistUpdates, remove legacyNotifications (#3094) Dylan Jeffers [824933e] [C-2366] Improve web notification selection performance (#3103) Dylan Jeffers [4b8edef] [PLAT-696] Add trending-playlists/underground notifications (#3089) Dylan Jeffers [1f9cf3e] [C-2275] Fix android drawer offsets (#3095) Dylan Jeffers [fc14c82] [PAY-1063][PAY-1085][PAY-1086] Update UI for inaccessible gated tracks from favorites and history pages (#3100) Saliou Diallo [b0441f5] [C-2365] Update play buttons on web and mobile to show resume when track is current (#3101) Kyle Shanks [453910f] [C-2378] Add upload v2 feature flag (#3099) Sebastian Klingler [962a6df] [C-2337] Remove reachability mobile web (#3090) Raymond Jacobson [4ad5cd2] Fix visible collectibles for upload popup (#3093) Saliou Diallo [c143078] Fix feature flag bug (#3092) Saliou Diallo [44435b5] Fix upload prompt modal learn more url (#3091) Saliou Diallo [c9024ad] Use chat.messagesStatus instead of selector (#3087) Reed [38d43c4] [C-2369] Fix issue where notification poll can break app on signout (#3088) Dylan Jeffers [90122d9] [PAY-923] DMs: Add desktop entrypoints (#3083) Marcus Pasell [00f27e8] [PAY-907] Mobile chat reactions (#3020) Reed [4678b89] DMs: Fix broken typecheck on main (#3086) Marcus Pasell [756ade4] [PAY-1000][PAY-1084][PAY-1096][PAY-1097][PAY-1098] - More gated content fixes (#3085) Saliou Diallo [820aa9d] Fix upload and repost probers tests and lint (#3076) Sebastian Klingler [345607e] [C-2320] Fix profile socials alignment (#3079) Dylan Jeffers [569199c] Fix prod build timeout (#3084) Sebastian Klingler [12f6c22] Remove ports for local dev (#3082) Theo Ilie [1940618] Fix broken Main build due to typeerror (#3080) Marcus Pasell [eb8d47e] [PAY-1082] DMs: Dedupe sent messages (#3066) Marcus Pasell [50a11c3] Update SDK to 2.0.3-beta.0 (#3078) Marcus Pasell [c420fbb] Clean up NPM package lock (#3077) Marcus Pasell [35d1124] [C-2327] Add playlist updates slice (#3063) Dylan Jeffers [59862ad] [C-2344] Update the web playbar scrubber to respect the playback speed of podcasts (#3075) Kyle Shanks [ffeb0d3] [C-2349] Default download on wifi only to false (#3074) Andrew Mendelsohn [cafae41] [C-2325] Fix playlist table date-added column (#3073) Dylan Jeffers [384a510] [PAY-927] DMs: Empty messages state (#3068) Marcus Pasell [1132f83] Update @jup-ag/core to 2.0.0-beta.9 (#3072) Marcus Pasell [49c0ebf] [PAY-1072] Change "Download App" icon on Settings Page (#3067) Marcus Pasell [928dcaf] [PAY-1056] - More gated content updates and fixes (#3070) Saliou Diallo [1e1f769] [C-2345] Move PlaybackRate drawer to common drawers map (#3071) Kyle Shanks [f5d1251] Fix web-dist CI steps (#3069) Sebastian Klingler [5f89800] Fix heavy rotation playlist on client (#3056) sabrina-kiam [c0191e2] [C-2316] Add remote config for all oauth verification (#3052) Raymond Jacobson [40f5627] [PAY-1074][PAY-1075][PAY-1076][PAY-1080] - Update availability settings states + more QA fixes (#3059) Saliou Diallo [5be60ac] [C-2339] Update podcast control updates to also work for audiobooks (#3065) Kyle Shanks [163ebf5] [C-2297] Add fallback flag to podcast feature (#3064) Sebastian Klingler [f206391] [PAY-904] - Add gated content upload prompt (#3057) Saliou Diallo [1afc4e5] [C-1344] Move probers to monorepo and make tests pass (#3061) Sebastian Klingler [e198279] Remove random line (#3062) Saliou Diallo [24a001b] Add playback position logic for mobile (#3051) Kyle Shanks [d210124] [PAY-1070] Update TabSlider/SegmentedControl slider size on resize (#3044) Marcus Pasell

nicoback2 requested a review from jowlee May 16, 2022 14:55

pull-request-size bot added the size/L label May 16, 2022

nicoback2 assigned sliptype May 16, 2022

nicoback2 requested a review from rickyrombo May 16, 2022 14:56

nicoback2 changed the title ~~Add route to Discovery for verifying JWT~~ Add route to Discovery for verifying JWT C-445 May 16, 2022

jowlee approved these changes May 16, 2022

View reviewed changes

nicoback added 5 commits May 16, 2022 13:13

add verify token route to discovery

0d72a20

pr review

f2fc5be

pr review

1431597

type fix

215cd87

type fix again

332719e

nicoback2 force-pushed the nkang--discovery-verify-token branch from 595e9e9 to 332719e Compare May 16, 2022 17:13

nicoback2 merged commit a9d2ac7 into master May 16, 2022

nicoback2 deleted the nkang--discovery-verify-token branch May 16, 2022 17:46

sliptype added a commit that referenced this pull request Sep 10, 2023

[C-2378] Add upload v2 feature flag (#3099)

f276c19

AudiusProject deleted a comment from linear bot Sep 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add route to Discovery for verifying JWT C-445 #3099

Add route to Discovery for verifying JWT C-445 #3099

nicoback2 commented May 16, 2022

jowlee May 16, 2022

nicoback2 May 16, 2022

jowlee May 16, 2022

jowlee May 16, 2022

nicoback2 May 16, 2022

jowlee May 16, 2022

nicoback2 May 16, 2022

jowlee May 16, 2022

nicoback2 May 16, 2022

jowlee May 16, 2022

		if not len(token_parts) == 3:
		abort_bad_request_param("token", ns)

		logger = logging.getLogger(__name__)


		def get_user_with_wallet(wallet):

Add route to Discovery for verifying JWT C-445 #3099

Add route to Discovery for verifying JWT C-445 #3099

Conversation

nicoback2 commented May 16, 2022

Description

Tests

How will this change be monitored? Are there sufficient logs?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment