Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary password field from add account via plaid step #4944

Merged
merged 6 commits into from
Sep 1, 2021
Merged

Remove unnecessary password field from add account via plaid step #4944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1b71b2f
remove password input, test flow
Julesssss Aug 31, 2021
40c76b3
clear up removed input field
Julesssss Aug 31, 2021
f7f3cdb
remove now unused string
Julesssss Aug 31, 2021
e6c61df
remove now unused import
Julesssss Aug 31, 2021
8a9e650
remove unused translated string
Julesssss Aug 31, 2021
5fd5d61
remove password from AddPlaidBankAccount
Julesssss Sep 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 2 additions & 19 deletions src/components/AddPlaidBankAccount.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,11 @@ import {
} from '../libs/actions/BankAccounts';
import ONYXKEYS from '../ONYXKEYS';
import styles from '../styles/styles';
import canFocusInputOnScreenFocus from '../libs/canFocusInputOnScreenFocus';
import compose from '../libs/compose';
import withLocalize, {withLocalizePropTypes} from './withLocalize';
import Button from './Button';
import ExpensiPicker from './ExpensiPicker';
import Text from './Text';
import ExpensiTextInput from './ExpensiTextInput';

const propTypes = {
...withLocalizePropTypes,
Expand Down Expand Up @@ -93,7 +91,6 @@ class AddPlaidBankAccount extends React.Component {

this.state = {
selectedIndex: undefined,
password: '',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we are still referencing this on line 116

account, password: this.state.password, plaidLinkToken: this.props.plaidLinkToken,

I honestly can't remember, but is it safe to just remove this / is nothing expecting the password?
What are we expecting on the API side here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot, I thought I had removed this 😕

Yes, I believe it is safe to remove this. The password param is sent at each step, but it is only required at step: CompanyStep. I'll remove the param and add some additional test steps to prove this is the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice that we don't send password for the 'Connect manually' step either:

App/src/pages/ReimbursementAccount/BankAccountStep.js

Lines 79 to 91 in 993bc0b

addManualAccount() {
setupWithdrawalAccount({
acceptTerms: this.state.hasAcceptedTerms,
accountNumber: this.state.accountNumber,
routingNumber: this.state.routingNumber,
setupType: CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL,
// Note: These are hardcoded as we're not supporting AU bank accounts for the free plan
country: CONST.COUNTRY.US,
currency: CONST.CURRENCY.USD,
fieldsType: CONST.BANK_ACCOUNT.FIELDS_TYPE.LOCAL,
});
}

Copy link
Contributor Author

@Julesssss Julesssss Sep 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make this even clearer, I could raise a PR in Web-Secure which would only pass password to the API here if parameters.password is not undefined (or only for those steps which require it).

However, as all it does is prevent an undefined param from being sent in the API, I'm not sure how valuable this is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I am mostly curious here if this was an oversight or if the Web-Secure API was ever expecting this parameter? Probably we just went off of mockups here and assumed the password is required.

I think it's necessary when calling BankAccount_Create (for the PBA flow) but perhaps is not required for the VBA flow. I'm not too sure. I do want to point out that SetupWithdrawalAccount does accept a password parameter and validates it under certain conditions.

https://github.com/Expensify/Auth/blob/3ed205bafa8c39de56a77183732d3d6d0ce4429c/auth/command/SetupWithdrawalAccount.cpp#L130-L133

But unsure if that's something we need to worry about here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, that Auth line did make me question this too... But as my testing raised no issues I think it can be explained as an oversight or change.

isCreatingAccount: false,
institution: {},
};
Expand All @@ -116,7 +113,7 @@ class AddPlaidBankAccount extends React.Component {
selectAccount() {
const account = this.getAccounts()[this.state.selectedIndex];
this.props.onSubmit({
account, password: this.state.password, plaidLinkToken: this.props.plaidLinkToken,
account, plaidLinkToken: this.props.plaidLinkToken,
});
this.setState({isCreatingAccount: true});
}
Expand Down Expand Up @@ -174,28 +171,14 @@ class AddPlaidBankAccount extends React.Component {
value={this.state.selectedIndex}
/>
</View>
{!_.isUndefined(this.state.selectedIndex) && (
<View style={[styles.mb5]}>
<ExpensiTextInput
label={this.props.translate('addPersonalBankAccountPage.enterPassword')}
secureTextEntry
value={this.state.password}
autoCompleteType="password"
textContentType="password"
autoCapitalize="none"
autoFocus={canFocusInputOnScreenFocus()}
onChangeText={text => this.setState({password: text})}
/>
</View>
)}
</View>
<View style={[styles.m5]}>
<Button
success
text={this.props.translate('common.saveAndContinue')}
isLoading={this.state.isCreatingAccount}
onPress={this.selectAccount}
isDisabled={_.isUndefined(this.state.selectedIndex) || !this.state.password}
isDisabled={_.isUndefined(this.state.selectedIndex)}
/>
</View>
</>
Expand Down
1 change: 0 additions & 1 deletion src/languages/en.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,6 @@ export default {
},
},
addPersonalBankAccountPage: {
enterPassword: 'Enter Expensify password',
alreadyAdded: 'This account has already been added.',
chooseAccountLabel: 'Account',
},
Expand Down
1 change: 0 additions & 1 deletion src/languages/es.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,6 @@ export default {
},
},
addPersonalBankAccountPage: {
enterPassword: 'Escribe tu contraseña de Expensify',
alreadyAdded: 'Esta cuenta ya ha sido agregada.',
chooseAccountLabel: 'Cuenta',
},
Expand Down
2 changes: 0 additions & 2 deletions src/pages/ReimbursementAccount/BankAccountStep.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@ class BankAccountStep extends React.Component {

/**
* @param {Object} params
* @param {String} params.password
* @param {Object} params.account
* @param {String} params.account.bankName
* @param {Boolean} params.account.isSavings
Expand All @@ -108,7 +107,6 @@ class BankAccountStep extends React.Component {
setupType: CONST.BANK_ACCOUNT.SETUP_TYPE.PLAID,

// Params passed via the Plaid callback when an account is selected
password: params.password,
plaidAccessToken: params.plaidLinkToken,
accountNumber: params.account.accountNumber,
routingNumber: params.account.routingNumber,
Expand Down