task(settings): Fix type safety on data.integration #18671
Conversation
dschom
force-pushed
the
settings-make-data-integration-typesafe
branch
3 times, most recently
from
d68b434 to
4d5a565
Compare
dschom
force-pushed
the
settings-make-data-integration-typesafe
branch
10 times, most recently
from
21feaab to
50ca9d5
Compare
|
|
||
| await expect(page).toHaveURL(/settings/); | ||
| await expect(page).toHaveURL(/signin/); | ||
| await expect(signin.badRequestHeading).toBeVisible(); |
There was a problem hiding this comment.
Just confirming — are we intentionally changing this to throw an error, versus:
-
The previous behavior where the invalid param was ignored, or
-
Stripping out the invalid param?
If so, the test description should probably be updated to reflect that expectation more clearly.
Some additional testing observations:
-
This param check currently doesn't trigger an error until the
confirm signup_codepage in the signup flow. Should we consider validating earlier — e.g., at the entry point like email-first? Ideally, we'd catch invalidredirectTovalues as early as possible and either error immediately or sanitize them. -
In the previous test case, adding
redirect_to=https://evil.com/during the signup flow triggers a banner error after code submission on theconfirm_signup_code page. That suggests a difference in validation layers — it looks like the param passed general validation but failedwebRedirectCheck. + outcome is different depending on the flow:- during signin the invalid param is ignored but the user can continue
- during signup there is an error message and the user cannot continue
There was a problem hiding this comment.
This param check currently doesn't trigger an error until the confirm signup_code page in the signup flow. Should we consider validating earlier — e.g., at the entry point like email-first? Ideally, we'd catch invalid redirectTo values as early as possible and either error immediately or sanitize them.
Yes, I think we should shift catching / checking required parameters to the 'entry points' as we have done with email first and a couple other pages. I'm not 100% sure this approach is always viable, but I think we should at least strive to support it. IMO we should be able to determine whats required, because after all if we can't specify and validate the query parameters, how would an RP know what they need to pass us?
I'm not sure if we should do these shifts in this PR. They seem like good follow up tasks though.
In the previous test case, adding redirect_to=https://evil.com/ during the signup flow triggers a banner error after code submission on the confirm_signup_code page
Yep, I think we should probably beef up the validation on redirect_to. Let me look into this.
There was a problem hiding this comment.
Agree to this being a follow-up/clean-up task.
| @@ -114,16 +114,6 @@ describe('bind-decorator', function () { | |||
| }).toThrow(); | |||
| }); | |||
|
|
|||
| it('throws on access of a invalid state', () => { | |||
There was a problem hiding this comment.
What is the new expectation on access of an invalid state?
There was a problem hiding this comment.
Looking at how the code ended up being used, I think throwing on access was causing problems, and there were artifacts in the code of trying to work around this. i.e. There were usages of getData on the data model directly instead of going through a property with @bind()... Also validating on every read is kind of heavy handed and might cause a lot of overhead... hindsight being 20/20.
I think we are generally making a shift towards validating early and failing hard / fast strategically at the entry points of our application. This is at odds with initial approach which was more optimistic about data being passed correctly, and a bit more laissez-faire about throwing errors on data access.
The new approach feels more rigid to me, but probably results in a better user experience in the end. Therefore, as long as we can be consistent validating at the start of the flow, I'm think the more pragmatic way to handle validation is:
- Validate after a model is first created. ie Create the new model instance bound to URL or whatever underlying data source, and then call
model.validate()immediately after instantiation. This would be the case regardless of if we are initializing integration data model, or query parameter models. Doing this insures the initial state of the model is valid. - Validation will still run when data is set on the model. Writing data happens much less frequently, and doing this allows us to be confident the model state is valid. The only caveat is if someone goes behind the models back and manipulates the data source directly.
There was a problem hiding this comment.
Thanks for the explanation, that sounds good to me. I was wondering if we needed to replace the test with a new expectation but sounds like no 👍
There was a problem hiding this comment.
One thing to keep an eye on now is our entry point validation. With these changes it becomes more important that we get those entry point validations right. In addition to not checking on read, the integration model validation no longer has 'required' fields. This is in response to the comment in FXA-8696 that states this expectation:
We should be able to redefine the validation criteria (for example changing from optional to required) on a per-page basis without encountering an uncaught error.
packages/fxa-settings/src/pages/Signup/ConfirmSignupCode/index.test.tsx
Outdated
Show resolved
Hide resolved
packages/fxa-settings/src/pages/Signup/ConfirmSignupCode/mocks.tsx
Outdated
Show resolved
Hide resolved
| @bind(T.snakeCase) | ||
| redirectTo: string | undefined; | ||
|
|
||
| // TODO: Double check context. This might be sync data! |
There was a problem hiding this comment.
Context is required for sync (at least, for oauth_desktop_v1). If signing in to sync and context is omitted, I see this popup:
There was a problem hiding this comment.
Should we add this logic to query param model?
| @IsOptional() | ||
| @IsIn(['offline', 'online']) | ||
| @bind(T.snakeCase) | ||
| accessType: string | undefined; |
There was a problem hiding this comment.
If this is omitted for sync sign in, browser sign in is not complete:
Sync provides a response_type param as well, though we don't seem to use it in fxa-settings.
There was a problem hiding this comment.
I guess same question again, should we add it to our entry point query parameter validation?
There was a problem hiding this comment.
Concerning response_type, is that an oversight in settings? Seems like auth-client could take it as option here...
https://github.com/mozilla/fxa/blob/main/packages/fxa-settings/src/lib/oauth/hooks.tsx#L130
https://github.com/mozilla/fxa/blob/main/packages/fxa-auth-client/lib/client.ts#L2005
There was a problem hiding this comment.
Probably a good idea to validate on entrypoint in case we're provided with nonsense/invalid values for either of these params? Not sure if we should error of fallback to the defaults though.
response_type: Joi.string()
.valid('code')
.default('code')
.description(DESCRIPTION.responseType),
responseType:
"Determines the format of the response.
Since we only support the authorization-code grant flow,
the only permitted value is 'code'.",
accessType:
'If specified, a value of `offline` will cause the connecting client
to be granted a refresh token alongside its access token.',
access_type: Joi.string()
.valid('offline', 'online')
.default('online')
.description(DESCRIPTION.accessType),
| @@ -131,13 +130,15 @@ export const App = ({ | |||
| let isValidSession = false; | |||
|
|
|||
| // Request and update account data/state to match the browser state. | |||
| // If there is a user actively signed into the browser, | |||
| // When we are accessing FxA from the browser menu or the user is going through | |||
There was a problem hiding this comment.
Was this comment from a merge conflict resolution? Currently there is no isWebChannelIntegration flag used here
| @@ -180,16 +180,22 @@ export function tryAgainError() { | |||
| */ | |||
| export function useFinishOAuthFlowHandler( | |||
| authClient: AuthClient, | |||
| integration: Pick<Integration, 'clientInfo' | 'type' | 'wantsKeys' | 'data'> | |||
| // TODO: Make this type safe by requiring an OAuthIntegration be provided. | |||
There was a problem hiding this comment.
Is this a follow-up todo?
dschom
force-pushed
the
settings-make-data-integration-typesafe
branch
3 times, most recently
from
cbc8058 to
fa138e6
Compare
Because: - We want to make sure integration.data is typesafe This Commit: - Adds a generic type, TData, so integration.data is now strongly typed. - Cleans up messy class inheritance. We no longer have the 'base-integration' class. - Cleans up mocks in mocks.tsx and makes sure they semantically aligned with the mocking function's name. - Moves integration data to a single file - Removes unused classes - channel-info - client-info - Makes all data model fields optional. Required fields are now controlled by query param models that are checked on specific entrypoint pages.
dschom
force-pushed
the
settings-make-data-integration-typesafe
branch
from
fa138e6 to
91ae90c
Compare
Because
This pull request
TData, so child classes can specify the data they haveIssue that this pull request solves
Closes: FXA-8696
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
It's important that integration data is strongly typed. Without this it's pretty easy for mistakes to occur. It could also explain why don't have a consistent access layer. Note, this came up while working on FXA-11297, and I'm still not entirely sure what the best approach here is.