Closed Bug 1492305 Opened Last year Closed 5 months ago

NSS-implemented OSKeyStore promises does not reject after hitting cancel on master passsword prompt

Categories

(Core :: Security: PSM, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox64 --- wontfix
firefox69 --- fixed

People

(Reporter: timdream, Assigned: kjacobs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [psm-backlog] [psm-wouldtake])

Attachments

(2 files)

Attached patch patchSplinter Review
STR: apply the patch and run the test on Linux and Mac.

Expected:

Mac:

It will trigger OS keychain login twice or trice. You should hit cancel on the last one when the console prints "!!!!!!!!! Hit cancel here when prompted for password". The test should pass.

Linux:

The test should pass on its own since gMockPrompter.promptPassword() will return false?

Actual:

On Linux, we will reach |ok(false, "Should reject");| and result in a test failure.

Note:

Over bug 1486954 I am relying on this method to reject to tell if the user had canceled the login dialog. It does not seem to be the case on Linux; the method still resolves even though I hit cancel.

Can we fix this inconsistency? Thanks. I'll skip a few tests in my patch in the mean time.
Flags: needinfo?(franziskuskiefer)
(Fixing summary since it was my patch that triggers the prompt repeatedly)
Summary: NSS-implemented OSKeyStore prompts master passsword for four times before rejection → NSS-implemented OSKeyStore promises does not reject after hitting cancel on master passsword prompt
I ended up workaround this in bug 1486954 so perhaps this is not that urgent.
Flags: needinfo?(franziskuskiefer)
So there are a couple things here:

1) The build system didn't properly detect Linux and thus didn't use libsecret as it should've.
2) The NSS key store backend didn't return correct errors.
3) The LibSecret keystore unlock didn't work properly and hence returned the wrong error code.

Unfortunately we can't test most of the behaviours :( I'll prepare a fix.
Assignee: nobody → franziskuskiefer
This fixes issues with the NSS and LibSecret keystore not correctly rejecting unlocking of the key store.
It also fixes the build to correctly detect Linux.
Priority: -- → P1
Whiteboard: [psm-assigned]

Seems like this patch just needs to be rebased, that with the Linux libsecret fixes it should just work?

Assignee: franziskuskiefer → nobody
Priority: P1 → P3
Whiteboard: [psm-assigned] → [psm-backlog] [psm-wouldtake]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:franziskus, could you have a look please?

Flags: needinfo?(franziskuskiefer)

No point in ni-ing franziskus. Dana, should we do anything here?

Flags: needinfo?(franziskuskiefer) → needinfo?(dkeeler)
Keywords: stalled

The patch has bit-rotted. I can un-bit-rot and land it if someone tells me it's important. Otherwise, I have more urgent issues to address at the moment.

Flags: needinfo?(dkeeler)

We may look into using OSKeyStore for passwords around the beginning of H2 but we're still making plans.

Assignee: nobody → kjacobs.bugzilla
Status: NEW → ASSIGNED
Attachment #9014364 - Attachment description: Bug 1492305 - fix pkg-config; libsecret unlocking; NSS unlock return values → Bug 1492305 - Fix LibSecret unlocking & NSS return values

Matt, could you please see the linked review? I removed a form autofill workaround for this bug, but would like to make sure this won't have any unintended side effects. Thanks.

Flags: needinfo?(MattN+bmo)

Hi Kevin, I was going to give you a heads up earlier today that I haven't had a chance to look at this yet as I don't have the context in my mind anymore (and therefore just reading the review isn't sufficient), this affects Linux which I've not used recently, and this is used in somewhat dead code (therefore it's not that trivial to just manually test).

I'll try take a look now.

Flags: needinfo?(MattN+bmo)

Not having a pref (or AFAICT any) way to force use of the NSS backend makes this annoying to test now that libsecret support actually works since I don't want to wait for a local build (I'm testing with the try push).

Thanks, Matt.

Keywords: stalledcheckin-needed

Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f3534b56753e
Fix LibSecret unlocking & NSS return values r=keeler,MattN

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.