Improve Account issuer/name validation and trim values.

This adds the checking for duplicate issuer/name to Add Account, and
changes the error handling to indicate that the Name is incorrect
instead of both Issuer and Name. It also trims both issuer name name to
avoit leading/trailing whitespace.
This commit is contained in:
Dain Nilsson 2022-09-05 15:02:36 +02:00
parent e9f8c434d8
commit 07f9bab181
No known key found for this signature in database
GPG Key ID: F04367096FBA95E8
3 changed files with 53 additions and 35 deletions

View File

@ -31,9 +31,15 @@ enum _QrScanState { none, scanning, success, failed }
class OathAddAccountPage extends ConsumerStatefulWidget { class OathAddAccountPage extends ConsumerStatefulWidget {
final DevicePath devicePath; final DevicePath devicePath;
final OathState state; final OathState state;
final List<OathCredential>? credentials;
final bool openQrScanner; final bool openQrScanner;
const OathAddAccountPage(this.devicePath, this.state, const OathAddAccountPage(
{super.key, required this.openQrScanner}); this.devicePath,
this.state, {
super.key,
required this.openQrScanner,
required this.credentials,
});
@override @override
ConsumerState<ConsumerStatefulWidget> createState() => ConsumerState<ConsumerStatefulWidget> createState() =>
@ -102,8 +108,8 @@ class _OathAddAccountPageState extends ConsumerState<OathAddAccountPage> {
_loadCredentialData(CredentialData data) { _loadCredentialData(CredentialData data) {
setState(() { setState(() {
_issuerController.text = data.issuer ?? ''; _issuerController.text = data.issuer?.trim() ?? '';
_accountController.text = data.name; _accountController.text = data.name.trim();
_secretController.text = data.secret; _secretController.text = data.secret;
_oathType = data.oathType; _oathType = data.oathType;
_hashAlgorithm = data.hashAlgorithm; _hashAlgorithm = data.hashAlgorithm;
@ -134,16 +140,26 @@ class _OathAddAccountPageState extends ConsumerState<OathAddAccountPage> {
final remaining = getRemainingKeySpace( final remaining = getRemainingKeySpace(
oathType: _oathType, oathType: _oathType,
period: period, period: period,
issuer: _issuerController.text, issuer: _issuerController.text.trim(),
name: _accountController.text, name: _accountController.text.trim(),
); );
final issuerRemaining = remaining.first; final issuerRemaining = remaining.first;
final nameRemaining = remaining.second; final nameRemaining = remaining.second;
final secret = _secretController.text.replaceAll(' ', ''); final secret = _secretController.text.replaceAll(' ', '');
final secretLengthValid = secret.length * 5 % 8 < 5; final secretLengthValid = secret.length * 5 % 8 < 5;
final isValid = _accountController.text.isNotEmpty &&
// is this credentials name/issuer pair different from all other?
final isUnique = widget.credentials
?.where((element) =>
element.name == _accountController.text.trim() &&
(element.issuer ?? '') == _issuerController.text.trim())
.isEmpty ??
false;
final isValid = _accountController.text.trim().isNotEmpty &&
secret.isNotEmpty && secret.isNotEmpty &&
isUnique &&
issuerRemaining >= -1 && issuerRemaining >= -1 &&
nameRemaining >= 0 && nameRemaining >= 0 &&
period > 0; period > 0;
@ -152,11 +168,11 @@ class _OathAddAccountPageState extends ConsumerState<OathAddAccountPage> {
void submit() async { void submit() async {
if (secretLengthValid) { if (secretLengthValid) {
final issuer = _issuerController.text; final issuer = _issuerController.text.trim();
final cred = CredentialData( final cred = CredentialData(
issuer: issuer.isEmpty ? null : issuer, issuer: issuer.isEmpty ? null : issuer,
name: _accountController.text, name: _accountController.text.trim(),
secret: secret, secret: secret,
oathType: _oathType, oathType: _oathType,
hashAlgorithm: _hashAlgorithm, hashAlgorithm: _hashAlgorithm,
@ -230,9 +246,9 @@ class _OathAddAccountPageState extends ConsumerState<OathAddAccountPage> {
enabled: issuerRemaining > 0, enabled: issuerRemaining > 0,
maxLength: max(issuerRemaining, 1), maxLength: max(issuerRemaining, 1),
inputFormatters: [limitBytesLength(issuerRemaining)], inputFormatters: [limitBytesLength(issuerRemaining)],
buildCounter: buildByteCounterFor(_issuerController.text), buildCounter: buildByteCounterFor(_issuerController.text.trim()),
decoration: InputDecoration( decoration: const InputDecoration(
border: const OutlineInputBorder(), border: OutlineInputBorder(),
labelText: AppLocalizations.of(context)!.oath_issuer_optional, labelText: AppLocalizations.of(context)!.oath_issuer_optional,
helperText: '', // Prevents dialog resizing when enabled = false helperText: '', // Prevents dialog resizing when enabled = false
prefixIcon: const Icon(Icons.business_outlined), prefixIcon: const Icon(Icons.business_outlined),
@ -251,13 +267,16 @@ class _OathAddAccountPageState extends ConsumerState<OathAddAccountPage> {
key: const Key('name'), key: const Key('name'),
controller: _accountController, controller: _accountController,
maxLength: max(nameRemaining, 1), maxLength: max(nameRemaining, 1),
buildCounter: buildByteCounterFor(_accountController.text), buildCounter: buildByteCounterFor(_accountController.text.trim()),
inputFormatters: [limitBytesLength(nameRemaining)], inputFormatters: [limitBytesLength(nameRemaining)],
decoration: InputDecoration( decoration: InputDecoration(
border: const OutlineInputBorder(), border: const OutlineInputBorder(),
prefixIcon: const Icon(Icons.person_outline),
labelText: AppLocalizations.of(context)!.oath_account_name, labelText: AppLocalizations.of(context)!.oath_account_name,
helperText: '', // Prevents dialog resizing when enabled = false helperText: '', // Prevents dialog resizing when enabled = false
prefixIcon: const Icon(Icons.person_outline), errorText: isUnique
? null
: 'This name already exists for the Issuer', // TODO
), ),
textInputAction: TextInputAction.next, textInputAction: TextInputAction.next,
onChanged: (value) { onChanged: (value) {

View File

@ -124,8 +124,7 @@ class _UnlockedViewState extends ConsumerState<_UnlockedView> {
header: 'No accounts', header: 'No accounts',
keyActions: _buildActions( keyActions: _buildActions(
context, context,
used: 0, credentials: null,
capacity: widget.oathState.version.isAtLeast(4) ? 32 : null,
), ),
); );
} }
@ -176,16 +175,19 @@ class _UnlockedViewState extends ConsumerState<_UnlockedView> {
), ),
keyActions: _buildActions( keyActions: _buildActions(
context, context,
used: credentials?.length ?? 0, credentials: credentials,
capacity: widget.oathState.version.isAtLeast(4) ? 32 : null,
), ),
child: AccountList(widget.devicePath, widget.oathState), child: AccountList(widget.devicePath, widget.oathState),
), ),
); );
} }
List<PopupMenuEntry> _buildActions(BuildContext context, List<PopupMenuEntry> _buildActions(
{required int used, int? capacity}) { BuildContext context, {
required List<OathCredential>? credentials,
}) {
final used = credentials?.length ?? 0;
final capacity = widget.oathState.version.isAtLeast(4) ? 32 : null;
return [ return [
buildMenuItem( buildMenuItem(
title: const Text('Add account'), title: const Text('Add account'),
@ -198,6 +200,7 @@ class _UnlockedViewState extends ConsumerState<_UnlockedView> {
builder: (context) => OathAddAccountPage( builder: (context) => OathAddAccountPage(
widget.devicePath, widget.devicePath,
widget.oathState, widget.oathState,
credentials: credentials,
openQrScanner: Platform.isAndroid, openQrScanner: Platform.isAndroid,
), ),
); );

View File

@ -35,8 +35,8 @@ class _RenameAccountDialogState extends ConsumerState<RenameAccountDialog> {
@override @override
void initState() { void initState() {
super.initState(); super.initState();
_issuer = widget.credential.issuer ?? ''; _issuer = widget.credential.issuer?.trim() ?? '';
_account = widget.credential.name; _account = widget.credential.name.trim();
} }
@override @override
@ -69,10 +69,8 @@ class _RenameAccountDialogState extends ConsumerState<RenameAccountDialog> {
final isValidFormat = _account.isNotEmpty; final isValidFormat = _account.isNotEmpty;
// are the name/issuer values different from original // are the name/issuer values different from original
final didChange = (widget.credential.issuer != null final didChange = (widget.credential.issuer ?? '') != _issuer ||
? _issuer != widget.credential.issuer widget.credential.name != _account;
: _issuer != '') ||
_account != widget.credential.name;
// can we rename with the new values // can we rename with the new values
final isValid = isUnique && isValidFormat; final isValid = isUnique && isValidFormat;
@ -126,13 +124,11 @@ class _RenameAccountDialogState extends ConsumerState<RenameAccountDialog> {
maxLength: issuerRemaining > 0 ? issuerRemaining : null, maxLength: issuerRemaining > 0 ? issuerRemaining : null,
buildCounter: buildByteCounterFor(_issuer), buildCounter: buildByteCounterFor(_issuer),
inputFormatters: [limitBytesLength(issuerRemaining)], inputFormatters: [limitBytesLength(issuerRemaining)],
decoration: InputDecoration( decoration: const InputDecoration(
border: const OutlineInputBorder(), border: OutlineInputBorder(),
labelText: 'Issuer (optional)', labelText: 'Issuer (optional)',
helperText: '', helperText: '', // Prevents dialog resizing when enabled = false
// Prevents dialog resizing when enabled = false prefixIcon: Icon(Icons.business_outlined),
errorText: isUnique ? null : ' ', // make the decoration red
prefixIcon: const Icon(Icons.business_outlined),
), ),
textInputAction: TextInputAction.next, textInputAction: TextInputAction.next,
onChanged: (value) { onChanged: (value) {
@ -153,9 +149,9 @@ class _RenameAccountDialogState extends ConsumerState<RenameAccountDialog> {
// Prevents dialog resizing when enabled = false // Prevents dialog resizing when enabled = false
errorText: !isValidFormat errorText: !isValidFormat
? 'Your account must have a name' ? 'Your account must have a name'
: isUnique : !isUnique
? null ? 'This name already exists for the Issuer'
: 'Same account already exists on the YubiKey', : null,
prefixIcon: const Icon(Icons.people_alt_outlined), prefixIcon: const Icon(Icons.people_alt_outlined),
), ),
textInputAction: TextInputAction.done, textInputAction: TextInputAction.done,