The API should look like nsIDOMDOMRequest deleteCa(in DOMString certNickname); Should only delete CAs imported by wifi manager, so we need to find a identifying method in Bug 917102.

I'm going to review this tomorrow (California time).

Update to fit change in bug 917102.

Comment on attachment 8336565 [details] [diff] [review] 0001. IDL Change. Review of attachment 8336565 [details] [diff] [review]: ----------------------------------------------------------------- One non-nit question here. ::: dom/wifi/nsIWifi.idl @@ +196,5 @@ > + * Nickname of imported to be deleted. > + * onsuccess: We have successfully deleted certificate. > + * onerror: We have failed to delete certificate. > + */ > + nsIDOMDOMRequest deleteCa(in DOMString certNickname); This might be a silly question: but I think bsmith told me that nicknames are not necessarily unique. Should we be passing in the cert's fingerprint instead?

Comment on attachment 8336567 [details] [diff] [review] 0003. DOM API implementation. Review of attachment 8336567 [details] [diff] [review]: ----------------------------------------------------------------- Clearing this request until my question about the API is answered.

Comment on attachment 8338292 [details] [diff] [review] 0002. Backend. V2 Review of attachment 8338292 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/wifi/NssUtils.cpp @@ +296,5 @@ > + if (!cert) { > + continue; > + } > + > + srv = SEC_DeletePermCertificate(cert); This means that we might delete two certs for one call here. Is that intended?

Comment on attachment 8338292 [details] [diff] [review] 0002. Backend. V2 Review of attachment 8338292 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/wifi/NssUtils.cpp @@ +289,5 @@ > + SECStatus srv; > + > + CopyUTF16toUTF8(aNickname, userNickname); > + for (int i = 0; i < 2; i++) { > + snprintf(nickname, sizeof(nickname), "%s%s", nicknamePrefix[i], userNickname.get()); You would need to check the return value here, if you continue using snprintf. However, I suggest that instead you sue the much-safer ns[AC]String methods that do correct bounds checking automatically (crash on memory allocation failure). @@ +300,5 @@ > + srv = SEC_DeletePermCertificate(cert); > + > + if (srv != SECSuccess) { > + return NS_ERROR_UNEXPECTED; > + } Please use MapSECStatus. ::: dom/wifi/WifiCaService.cpp @@ +115,5 @@ > NS_DispatchToMainThread(runnable); > } > }; > > +class DeleteCaRunnable : public nsRunnable I suggest replacing this with CryptoTask, which handles NSS shutdown correctly. You need to check for NSS shutdown. Also, this will do file I/O (NSS certificate database access) on the main thread. I suggest that you try using the CryptoTask framework to avoid the main-thread I/O and also avoid having to deal with NSS shutdown. You can find some good examples of how to use CryptoTask in the Gecko source code. (e.g. toolkit/identity, IIRC.) I suggest you try using CryptoTask for the Importing part too.

(In reply to Blake Kaplan (:mrbkap) from comment #6) > Comment on attachment 8336565 [details] [diff] [review] > 0001. IDL Change. > > Review of attachment 8336565 [details] [diff] [review]: > ----------------------------------------------------------------- > > One non-nit question here. > > ::: dom/wifi/nsIWifi.idl > @@ +196,5 @@ > > + * Nickname of imported to be deleted. > > + * onsuccess: We have successfully deleted certificate. > > + * onerror: We have failed to delete certificate. > > + */ > > + nsIDOMDOMRequest deleteCa(in DOMString certNickname); > > This might be a silly question: but I think bsmith told me that nicknames > are not necessarily unique. Should we be passing in the cert's fingerprint > instead? Using nickname instead of hash is because it's easier for user to select from list. And for wifi usage, we have to ensure the nickname is unique for wpa_supplicant to get. So there's a check in |importCa()| to prevent redundant nickname. Because we use nickname directly in Gaia settings and wpa_supplicant config, so we don't have to maintain a database for the mapping.

(In reply to Blake Kaplan (:mrbkap) from comment #8) > Comment on attachment 8338292 [details] [diff] [review] > 0002. Backend. V2 > > Review of attachment 8338292 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/wifi/NssUtils.cpp > @@ +296,5 @@ > > + if (!cert) { > > + continue; > > + } > > + > > + srv = SEC_DeletePermCertificate(cert); > > This means that we might delete two certs for one call here. Is that > intended? Yes, one for server certificate, one for user certificate. In my assumption, these two certificate should only have same nickname because they are imported at once - by pkcs12 format. Since pkcs12 is move out of current scope, next patch will only import/delete server patches.

Change API function name.

Address comment 9, including: 1. Use nsCString instead of c type string. 2. Use MapSECStatus() for returning error code. 3. Use CryptoTask instead of Runnable to do I/O outside of main thread.

1. Changes corresponding to API function name change. 2. Answers for API is in comment 10.

The user story bug this blocks (bug 922930) has a target milestone of future, so this is not a committed feature for 1.3. Non-committed features for 1.3 should not block the release.

(In reply to Chuck Lee [:chucklee] from comment #10) > Using nickname instead of hash is because it's easier for user to select > from list. Sure, but the user isn't the one calling the API function ;-). It isn't a ton more work for the Gaia code to keep a mapping, which would let us have the best of both worlds. That being said... > And for wifi usage, we have to ensure the nickname is unique for > wpa_supplicant to get. > So there's a check in |importCa()| to prevent redundant nickname. I must have missed that. This needs at least a comment somewhere in the IDL. > Because we use nickname directly in Gaia settings and wpa_supplicant config, > so we don't have to maintain a database for the mapping. In general, I'm against implementation details dictating API design -- there hopefully will be other implementations of this API that may not be based on wpa_supplicant and which wouldn't necessarily have this limitation. That being said, having multiple certs with the same nickname seems like Bad News (TM) for the user anyway, so it seems reasonable for me to keep it in the API.

Comment on attachment 8339796 [details] [diff] [review] 0001. IDL Change. V2 Review of attachment 8339796 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/wifi/nsIWifiCertService.idl @@ +41,5 @@ > + * @param certNickname > + * Certificate nickname to delete. > + */ > + void deleteCert(in long id, > + in DOMString certNickname); I think you need to bump the IID here.

Comment on attachment 8339798 [details] [diff] [review] 0002. Backend. V3 Review of attachment 8339798 [details] [diff] [review]: ----------------------------------------------------------------- Nits only from me. ::: dom/wifi/WifiCertService.cpp @@ +79,5 @@ > + mResult.mStatus = 0; > + mResult.mUsageFlag = 0; > + mResult.mNickname = aCertNickname; > + > + MOZ_ASSERT(NS_IsMainThread()); Uber-nit: I'd put this assertion at the top of the function. @@ +91,5 @@ > + MOZ_ASSERT(!NS_IsMainThread()); > + NssUtils nssUtil; > + > + nsresult rv = nssUtil.DeleteCert(mResult.mNickname); > + return rv; Nit: no need for rv.

(In reply to Blake Kaplan (:mrbkap) from comment #17) > Comment on attachment 8339796 [details] [diff] [review] > 0001. IDL Change. V2 > > Review of attachment 8339796 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/wifi/nsIWifiCertService.idl > @@ +41,5 @@ > > + * @param certNickname > > + * Certificate nickname to delete. > > + */ > > + void deleteCert(in long id, > > + in DOMString certNickname); > > I think you need to bump the IID here. id here is used to find the corresponding callback in WifiManager, it can't be removed.

Address comment 17.

Address comment 18.

Rebase.

Rebase.

Rebase.

Rebase.

After discussing PM, it is not an 1.4+ bug. Put it in backlog.

Comment on attachment 8371281 [details] [diff] [review] 0002. Backend. V5 Review of attachment 8371281 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/wifi/NssUtils.cpp @@ +166,5 @@ > + ScopedCERTCertificate cert( > + CERT_FindCertByNickname(CERT_GetDefaultCertDB(), serverCertName.get()) > + ); > + if (!cert) { > + return NS_ERROR_UNEXPECTED; Wouldn't MapSECStatus(SECFailure) be better here? ::: dom/wifi/WifiCertService.cpp @@ +71,5 @@ > > +class DeleteCertTask MOZ_FINAL: public CryptoTask > +{ > +public: > + DeleteCertTask(int32_t aId, const nsAString & aCertNickname) NIT: const nsAString& aCertNickname @@ +89,5 @@ > + { > + MOZ_ASSERT(!NS_IsMainThread()); > + NssUtils nssUtil; > + > + return nssUtil.DeleteCert(mResult.mNickname); Consider just inlining nssUtil.DeleteCert into here. @@ +194,5 @@ > return NS_OK; > } > > +NS_IMETHODIMP > +WifiCertService::DeleteCert(int32_t aId, const nsAString & aCertNickname) NIT: const nsAString& @@ +197,5 @@ > +NS_IMETHODIMP > +WifiCertService::DeleteCert(int32_t aId, const nsAString & aCertNickname) > +{ > + RefPtr<CryptoTask> task = new DeleteCertTask(aId, aCertNickname); > + task->Dispatch("WifiDeleteCert"); NIT: return task->Dispatch("WifiDeleteCert");

Inline nssUtil.DeleteCert() and fix nits per comment 27.

Rebase.

Update to webIDL.

Update to webIDL, use new interface.

Rebase based on bug 917176.

Rebase.

Rebase.

Try result : https://tbpl.mozilla.org/?tree=Try&rev=b586311119d2 This is third bug to check in.

https://hg.mozilla.org/integration/b2g-inbound/rev/6781b96cca1c https://hg.mozilla.org/integration/b2g-inbound/rev/947690de7b62 https://hg.mozilla.org/integration/b2g-inbound/rev/8f803659229a

https://hg.mozilla.org/mozilla-central/rev/6781b96cca1c https://hg.mozilla.org/mozilla-central/rev/947690de7b62 https://hg.mozilla.org/mozilla-central/rev/8f803659229a