Skip to content

[KnownUsernameField] Engine update (make the system more flexible)#1011

Merged
cscharf merged 5 commits intobitwarden:masterfrom
contribucious:KnownUsernameField--engine-update--flexibility
Aug 6, 2020
Merged

[KnownUsernameField] Engine update (make the system more flexible)#1011
cscharf merged 5 commits intobitwarden:masterfrom
contribucious:KnownUsernameField--engine-update--flexibility

Conversation

@contribucious
Copy link

@contribucious contribucious commented Jul 14, 2020

CONTEXT: This is an update of this new system (system allowing "user ID" field detection — i.e. email/username/phone/whatever — without "password" field using the accessibility service).

UPDATED: Engine.
↪️ Prerequisite for my next PR which concerns the entries.


 
This:

  • allows to have multiple pairs "path / username view ID" per domain;

  • adds more advanced path support:

    • in its case-sensitive version: startswith:string, contains:string, endswith:string and
    • in its case-insensitive version: istartswith:string, icontains:string, iendswith:string.
       

💡 About using StringComparison.Ordinal and StringComparison.OrdinalIgnoreCase, this is recommended for a variety of reasons including better performance. See the page Best Practices for Using Strings in .NET from Microsoft documentation.
 
 

@contribucious
Copy link
Author

When you have some time @mportune-bw, could you take a look at this slight update of your system?

My next PR is already ready @ local side (big update of entries).
This one being only a required preamble. 👍

Thanks!

@cscharf cscharf requested review from mpbw2 and removed request for mpbw2 July 27, 2020 19:57
@cscharf
Copy link

cscharf commented Jul 27, 2020

@contribucious , is there a chance you can refactor a little bit using Tuples or actual types vs. multidimensional arrays (string[,]) for Access Options in the KnownUsernameField class? That way it can be explicit on how those dimensions are to be defined and used (code as documentation). It seems as if the only the first value of each dimension of the array is being used, explicitly, which feels more like a Tuple, Dictionary, etc.

@contribucious
Copy link
Author

contribucious commented Jul 30, 2020

Hello @cscharf,

Sorry for the delay.

You are right. Is it better now?
 

Note: Compiles successfully and is functional in practice (APK installed and tested).

@contribucious contribucious requested a review from cscharf July 30, 2020 09:30
@contribucious
Copy link
Author

@cscharf Is last week's change OK for you?

Copy link

@cscharf cscharf left a comment

Choose a reason for hiding this comment

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

Just one question/open comment and then yes, we should be good.

@contribucious contribucious requested a review from cscharf August 6, 2020 16:25
Copy link

@cscharf cscharf left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@contribucious
Copy link
Author

You're welcome. 👍

@cscharf cscharf merged commit 22570e0 into bitwarden:master Aug 6, 2020
@contribucious contribucious deleted the KnownUsernameField--engine-update--flexibility branch August 6, 2020 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants