Add new method for cycling through every login

[xusoo]

To be used from browser extension when autofilling.
Related PR: bitwarden/clients#956

[CLAassistant]

All committers have signed the CLA.

[cscharf]

Okay, didn't catch this before, however.... I've walked through the logic and perhaps I'm missing something, but it would seem that this.lastSortedCiphers is not necessary since you're still getting all decrypted for the URL, then always sorting them, so you're never really "using" the "cache" of prior sorted ciphers and adding a bit of overhead to determine if there were changes between the prior and current list, then update it, but you could just always use the local const variable, ciphers, because that's the only scope both are used.

[cscharf]

I would also assume some invalidation of this approach (or worse a race-condition/thread overstepping) as well when there are multiple tabs open in the browser but only a singleton instance of the cipher service itself.

[xusoo]

this.lastSortedCiphers gets sorted and populated only once (for URL/tab) and it maintains the original sorting order for every keystroke. If I'd remove it and use the local sorted array then you would cycle through the same two items all the time, because that array is always sorted by last used. Imagine 4 ciphers:

[A, B, C, D]

Autofill command pressed

const ciphers = [A, B, C, D]
this.lastSortedCiphers = [A, B, C, D]

Returns B (lastUsedIndex + 1)

B is now the last used

Autofill command pressed

const ciphers = [B, A, C, D] // Changed
this.lastSortedCiphers = [A, B, C, D] // Didn't change because arraysContainSameElements == true

Returns C (lastUsedIndex + 1)

C is now the last used

So far, everything is the same, but if you press again the shortcut:

Autofill command pressed

const ciphers = [C, B, A, D] // Changed
this.lastSortedCiphers = [A, B, C, D] // Didn't change

There. Last used was C, so if we use local ciphers array, next one would be B again, but it should be D.

If you run it again, you will get C, and then B, and then C... 🙂

P.S. Writing this example, I realised that in the first step, you get B, because is the next one. But what if you have 'Auto fill on page load' disabled? Then it should autofill the last used, not the next one. I'll see what I can think of to solve than scenario.

[xusoo]

I would also assume some invalidation of this approach (or worse a race-condition/thread overstepping) as well when there are multiple tabs open in the browser but only a singleton instance of the cipher service itself.

As for this, you are totally right. Although you'd have to use autofill shortcut in two tabs at the same time, wouldn't you? Anyway, I'll see what I can do. Do you have any suggestion?

[cscharf]

Although you'd have to use autofill shortcut in two tabs at the same time, wouldn't you?

yes, assuming so, which seems far-fetched, however I would also assume if you had auto-fill on page load enabled and popped open several tabs at once or in short order, and they loaded at variable times in an interweaving manner... I would think you could get around this by using a hashtable/object based on the URL, however that could lead to memory bloat if there's no routine for cleanup. Perhaps it's not a concern at the moment (until if/when it becomes a concern)

But what if you have 'Auto fill on page load' disabled? Then it should autofill the last used, not the next one. I'll see what I can think of to solve than scenario.

I would assume when auto fill on page load is disabled that is when your changes are appropriate, correct? You would always want to use the "next" one (agree the initial state should return index 0, not 0 + 1),

If auto fill on page load is enabled I believe should be handled in the browser extension's code itself in that scenario when this service is called to activate auto-fill on initial page load (being implementation specific).

[xusoo]

however I would also assume if you had auto-fill on page load enabled and popped open several tabs at once or in short order, and they loaded at variable times in an interweaving manner...

No, because autofill on page load will return earlier:

        if (lastUsed) {
            return lastUsedCipher;
        }

Because it uses getLastUsedForUrl method instead of getNextCipherForUrl.

I would assume when auto fill on page load is disabled that is when your changes are appropriate, correct? You would always want to use the "next" one (agree the initial state should return index 0, not 0 + 1),

If auto fill on page load is enabled I believe should be handled in the browser extension's code itself in that scenario when this service is called to activate auto-fill on initial page load (being implementation specific).

I just remembered why I needed that this.lastUsedCipher in the previous PR 🤣 So if it was filled, it's because an autofill already happened (either on page load or shortcut triggered), so you would get next cipher in line (sorted by date). If not, you will get last used (position 0).

That's what happens when you pick up a PR from a year ago 😄

[cscharf]

haha, for sure, good catch and glad you caught that 😉

[kspearrin]

Sorry if I missed some previous conversation, but does this account for always using the lastUsed unless I am pressing the command multiple times in succession? For example, given login [A,B] for a website, I may always want login A (my last used) to autofill when I visit a website and press the command. I only want it to cycle to B if I press the command a second time within a short amount of time.

I don't see where the code in either project is handling this. It seems like you'll always be advancing to the next item in the array every time you perform autofill via command.

[kspearrin]

This should be cleared up when the app is locked, just like _decryptedCipherCache, right? See clearCache()

[xusoo]

Sorry if I missed some previous conversation, but does this account for always using the lastUsed unless I am pressing the command multiple times in succession? For example, given login [A,B] for a website, I may always want login A (my last used) to autofill when I visit a website and press the command.

But do you mean that pressing multiple times won't change last used login? Now if A is last used and gets auto-filled on page load, if I press the shortcut, it will fill B and therefore become the last used. So next time you go to this same site it will auto-fill B, not A. Is this not what we want?

The problem now is that if you have "auto-fill on page load" disabled (or it didn't autofill automatically for some reason), and then you press the shortcut, it should load the last used not the next one, so that's the bug I'm trying to solve now.

I only want it to cycle to B if I press the command a second time within a short amount of time.

Is this really needed? If time is set to 2 seconds, what would happen if I press the shortcut again after 5 seconds? Will it fill up the same login I already have on screen? Doesn't it make more sense to just continue cycling no matter how much time has passed?

[xusoo]

This is what I have now, on autofill.service.ts:

    private lastFilledUrl: string;

    constructor(private cipherService: CipherService, private userService: UserService,
        private totpService: TotpService, private eventService: EventService) {

        // Here I need an event triggered on page load, to reset lastFilledUrl to null
    }

    // At the end of doAutoFill() function
    if (didAutofill) {
        this.lastFilledUrl = tab.url;
        ...
    }

        if (fromCommand && this.lastFilledUrl === tab.url) {
            cipher = await this.cipherService.getNextCipherForUrl(tab.url);
        } else {
            cipher = await this.cipherService.getLastUsedForUrl(tab.url);
        }

So, about that event, what would be the easiest way?

I see that the way it handles it right now, the extension, is sending an event from a content script, so that's what I did in autofiller.ts (but I think it should be in a separate content script):

document.addEventListener('DOMContentLoaded', (event) => {
    ...
    sendPlatformMessage({ command: 'pageLoad' });
    ...
});

And then in autofill.service.ts constructor:

BrowserApi.messageListener('runtime.background', async (msg: any) => {
    if (msg.command === 'pageLoad') {
        this.lastFilledUrl = null;
    }
});

Is this approach valid? Is there a better way?

Another option is to use chrome.tabs.onUpdated like I had before and find an equivalent event in Safari.

[kspearrin]

I guess one scenario I have some up with in my head is, what happens if:

1. I log into a website using command.
2. Log out of the website.
3. Log into website again using command.

Does it use the same login both times? On on step 3, does it cycle to a new login?

[xusoo]

Assumming that either on login or on logout, it refreshes the page, then it will use the same login both times.

[kspearrin]

Assumming that either on login or on logout, it refreshes the page, then it will use the same login both times.

We can't assume that. Many single page apps may never reload. The bitwarden web vault, for example.

[xusoo]

We can't assume that. Many single page apps may never reload. The bitwarden web vault, for example.

Fair enough, but there is no easy way to detect page changes without reload apparently, is there? The idea would be to combine DOMContentLoaded event along with others that enable you to listen for these URL changes. But usually, these URL changes are done with history.pushState and apparently there is no native event for that (only for popState). And if you use a hash strategy, the hashchange event doesn't seem too reliable either: it doesn't fire in Bitwarden Web Vault for instance.

One possibility would be to listen to form submit event (you already do for the notification bar, so it would be just sending a message from there) and when it happens reset this.lastFilledUrl (or whatever variable we end up using), so next time you trigger the shortcut (after login and logout in your example) it will start from last used one again instead of trying next.

BrowserApi.messageListener('runtime.background', async (msg: any) => {
    if (msg.command === 'pageLoad' || msg.command === 'formSubmitted') {
        this.lastFilledUrl = null;
    }
});

Then is your suggestion of using a timer of course. That would be also a possibility. It has its drawbacks as well, but indeed it's cleaner and easier to put in place:

this.lastFilledUrl = tab.url;

clearTimeout(this.lastFilledUrlTimer);
this.lastFilledUrlTimer = setTimeout(() => {
    this.lastFilledUrl = null;
}, 5000);

The drawbacks are that if you set a timeout too small you could end up always pressing the command twice just to proceed to the next login. And if you set it too high then maybe the scenario you've described could still be happening (I know it's a bit far fetched because it'd had to be very high for you to login and logout and that still the timer hadn't triggered yet, but I don't know, I'm not a fan of timers).

But again, it would be very easy and simple to do. It's fine by me if we go with this solution too.

I have another quite crazy idea, probably with too many flaws and too complicated, but since we are brainstorming, here it is:
Would it be possible to check if, username field we are about to fill, already contains the same username? If it does, then go for the next cipher. Otherwise, fill last used.

Seems easy on paper, but looking at the code, it will be quite hard to implement, too many variables and edge cases to take into account (like for instance multiple username fields), and also more error-prone because the change will spread across other methods like generateFillScript.

The advantages would be not needing neither a class variable nor event listeners/timers, and you will always predict what the user wants because seems obvious I guess that if, the username is already filled, then I want the next one, not the same I already have in the field.

I don't know, let me know what do you think :)

[cscharf]

@xusoo , I also like the timer approach. I'm generally also not a fan of timers, however in the JS world they can be useful for specific use-cases. Take for instance an "auto-search / auto-suggest" box that searches as you type. If you have a very large data set AND search with each keystroke, you'll be killing the process, slowing things down and backup the call stack (bad), and instead a cleaner user experience can be achieved by using timers to "group" keystrokes so if you're still waiting a few milliseconds between you'll get the next, reset + wait, etc. until the user is reasonably done typing (not a long pause, just enough for an average typing speed and not to overload the search function). In this instance described, if I type "foo", which as a developer comes off very fast, I'm only running 1 search vs. 3.

That same approach can be taken here using a hashtable + timer method. Keeping a hashtable of URLs with object values that store the [] of lastSortedCiphers and perhaps a counter for current index and date when last command used. When the command runs, if null/missing, fill it/set it, set the index to 0 and return the 0th index; also set a timer where the callback method sets that key's value to null if last accessed/used date >= 4.5 seconds ago for say, 5 second timeout.

When the command is run again, and you can access that value in the hashtable by that URL, add 1 to the index, set the new last accessed time for that value and create a new timer/setTimeout for another 5 seconds (each and every time the command is used for that URL), and keep that rotation until that timer callback is successful in setting the value to null.

IMO, this type of pattern should in theory keep memory footprint relatively low(ish), support multiple threads on different tabs acting with similar timing, as well as support the idea that:

1. As a user when I first go to a page I can reasonably expect that I'll get auto-fill of the last used cipher
2. As a user when I log out and back into the same site I can reasonably expect that I'll get auto-fill of the last used cipher
3. As a user when I first auto-fill on a page, and then subsequently invoke the auto-fill command within a reasonably short time-period (5 seconds or less between commands) I will be able to cycle through my logins for that URL

[xusoo]

PR updated using a timer and a hashtable by URL.

[kspearrin]

This seems ok to me, but can you please move the SortedCiphersCache class to src/model/domain?