Proposal oauth refactor #501
Conversation
… functionality for authenticate is not complex enough for a Class, and just adds more ceremony
| if (isAuthCallbackRequest) { | ||
| throw await this.handleAuthCallbackRequest(request, shop); | ||
| await this.ensureInstalledOnShop(request); | ||
| await ensureAppIsEmbeddedIfRequired(params, request); |
There was a problem hiding this comment.
I don't see why either this nor ensureSessionTokenSearchParamIfRequired should be in the token acquisition strategy. These are things that need to happen as part of request validation, regardless of how we get the token.
It's also imo confusing for these to live in the respondToOauthRequests function. These have nothing to do with OAuth.
These functions should live in with the rest of the request validation logic (which is currently housed in authenticate.ts.
| const {api, config, logger} = this; | ||
|
|
||
| if (config.isEmbeddedApp) { | ||
| const payload = await validateSessionToken( |
There was a problem hiding this comment.
This step is request validation and shouldn't live in the token acquisition strategy. If the session token is bunk, we should reject the request regardless of the strategy. Otherwise we're creating a security footgun in the future when someone forgets to incorporate this logic into a new strategy they create.
| @@ -125,15 +147,76 @@ export class AuthCodeFlowStrategy< | |||
| } | |||
| } | |||
|
|
|||
| private validateUrlParams(request: Request) { | |||
There was a problem hiding this comment.
Same concern here: this should apply regardless of the token acquisition strategy. I don't see why this should live here.
Co-authored-by: Paulo Margarido <[email protected]> Co-authored-by: Rezaan Syed <[email protected]>
rachel-carvalho
force-pushed
the
proposal-oauth-refactor
branch
from
db7900f to
9e8d99d
Compare
WHAT is this pull request doing?
Type of change
Checklist
yarn changesetto create a draft changelog entry (do NOT update theCHANGELOG.mdfiles manually)