177941 - Keychain prompts me twice when doing "allow once"

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

[sairuh (rarely reading bugmail)]

i noticed that when i select "allow once" from the keychain dialog in chimera,
that the dialog appears again after i submit the prefilled username/passwd.

tested on 10.2.1 with 2002.11.01.04 (although i've been seeings this for a while).

0. already have a username/passwd saved in keychain for a site --for example, in
Bugzilla. to test with that example, make sure you're initially *not* logged
into Bugzilla.

1. login to Bugzilla: http://bugzilla.mozilla.org/query.cgi?GoAheadAndLogIn=1

2. the keychain dlg will appear, asking you if you want to deny, allow once or
always allow. select "allow once".

3. the username and passwd fields get prefilled, as expected. now, click the
login button to submit.

results: the keychain dlg (as in step 3) appears *again*. i get logged in fine,
but why does this dlg have to appear redundantly?

Comment 1

[sairuh (rarely reading bugmail)]

in comparison, i only see the keychain dlg once (initially at step 2) if select
"always allow".

Comment 2

[Simon Fraser [no longer active]]

To pink. Do we reallly need to resave the password if it's the same as the one
keychain filled in to start with?

Comment 3

[Benoit]

Once you submit a form, the keychain mechanism checks the keychain to see wether
or not the login/password is stored in the keychain. 

In your case it's obviously already in the keychain... but we don't maintain any
state between the time you load the page (where the values are prefilled) and
the time you submit the form from the same page. So we don't know that we
already got the user/password from the keychain and we need to check it a second
time. 

That's the reason why it access a second time the keychain. I'm not sure this is
something that we can easily avoid... I'll have a closer look at the keychain
service API.

Comment 4

[Mike Pinkerton (not reading bugmail)]

i did some perusing and the problem is i can't see an easy place to stash that
we grabbed the info out of the keychain. there is only one keychain service
instance, and only one keychain form submit observer. there's no link between
the N keychain browser listeners and the single form observer. i guess we could
create a form observer for every browser, but that seems wasteful.

ideas?

Comment 5

[Jens Miltner]

Could the keychain form hold the original values and the keychain form submit
observer compare the original values against the current values and only write
them to keychain if the values changed? 
(I don't really know the internals of the keychain form, so this suggestion may
be way off... )

Comment 6

[Andrew Thompson]

Pink & myself were talking briefly about the idea of a global cache of
URL->id/password. Essentially a hashmap.

I think this would have to have some kind of expiry policy, so the entries only
remained around for a short period of time before automatically being cleaned
up, otherwise it becomes a huge potential security hole. 

30 seconds? 300 seconds? Kinda depends if there's anything else on the page that
needs to be filled out, or whether the form is simply username/password. Seems
likely the common case is a simple form with only 2 fields that gets entirely
filled by the first access to the Keychain. So caching for 30 seconds to give
the user the chance to click [Login] might work.

Comment 7

[Samuel Sidler (old account; do not CC)]

Ping.

Status update? Is this an easy thing to fix? Seems like comment 6 had a good
idea of what to do. How hard would this be to implement?

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This WFMs for all of us in the channel on 10.3 and 10.4 ;)

Comment 9

[Andrew Thompson]

This is most assuredly not fixed, unless it's fixed in a nightly.

OS: 10.3

Camino: 1.0int

Just retried the original steps reported with bugzilla, clicking "Allow Once" and I get prompted twice. Infact, this happens to me almost every day.

If it is fixed in a nightly please let me know which one and I will retest.

Comment 10

[Stuart Morgan]

Attachment: fix

This adds a 2-minute cache of keychain items, so that we don't have to do a second lookup to know if it has changed. I also rearranged the logic when filling multiple forms on a page to prevent redundant lookups there.

(This is for 1.6)

Comment 11

[Mike Pinkerton (not reading bugmail)]

do you really need to bother putting the timer into a dictionary? The run loop will own the timer and since its a one-shot, it'll go away on its own, right? Or am I missing something obvious?

Looks good otherwise.

Comment 12

[Stuart Morgan]

Per IRC, I'm missing an invalidate when storing something in the cache, to prevent reloading a login page from causing the cache not to live as long as it should.

Comment 13

[Stuart Morgan]

Attachment: v2

Added the missing invalidate.

Comment 14

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 264447 [details] [diff] [review]
v2

sr=pink

Comment 15

[Stuart Morgan]

Landed on trunk and MOZILLA_1_8_BRANCH