Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Description • 12 years ago

Needed to decode b64-encoded assertions

	Ben Adida [:benadida]
	Comment 1 • 12 years ago

Comment on attachment 647239 [details] [diff] [review]
expose b64 decode; unit tests

looks good. Non-blocking question: why new UUIDs?

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 2 • 12 years ago

(In reply to Ben Adida [:benadida] from comment #1)
> Comment on attachment 647239 [details] [diff] [review]
> expose b64 decode; unit tests
> 
> looks good. Non-blocking question: why new UUIDs?

Thanks for the review, Ben!

I guess new UUIDs aren't necessary for the other functions, true.  Perhaps I ought to change those back to make subsequent merges less likely to be painful?

	Ben Adida [:benadida]
	Comment 3 • 12 years ago

(In reply to Jed Parsons [:jparsons] from comment #2)
> I guess new UUIDs aren't necessary for the other functions, true.  Perhaps I
> ought to change those back to make subsequent merges less likely to be
> painful?

Nah, it's okay, like I said, non-blocking :)

	Jason Smith [:jsmith]
	Comment 4 • 12 years ago

Does this bug need to be checked in?

Looks like an old bug - does this still block?

	Alex Keybl [:akeybl]
	Comment 5 • 12 years ago

This bug has been called out as likely having risk to non-B2G platforms. Given that, marking as P1, and moving into the C2 milestone. We should prioritize this landing to mozilla-beta as soon as possible, to prevent late-breaking regressions to other platforms.

	Jason Smith [:jsmith]
	Comment 6 • 12 years ago

Spoke with Jed about this - this is no longer needed for v1 of basecamp, so removing the basecamp flag.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 7 • 11 years ago

It turns out we have need for this now in Bug 878941.

I've updated this patch so that it still applies.  Ben, can I ask you for a fresh review?

	Ben Adida [:benadida]
	Comment 8 • 11 years ago

Comment on attachment 766149 [details] [diff] [review]
Add and expose Base64UrlDecode in IdentityCryptoService

Review of attachment 766149 [details] [diff] [review]:
-----------------------------------------------------------------

looks good to me, may want a quick look at this from bsmith.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 9 • 11 years ago

Comment on attachment 766149 [details] [diff] [review]
Add and expose Base64UrlDecode in IdentityCryptoService

Review of attachment 766149 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/identity/IdentityCryptoService.cpp
@@ +261,5 @@
>    return Base64UrlEncodeImpl(utf8Input, result);
>  }
>  
> +NS_IMETHODIMP
> +IdentityCryptoService::Base64UrlDecode(const nsACString & base64Input, 

trailing whitespace.

::: toolkit/identity/nsIIdentityCryptoService.idl
@@ +44,5 @@
>    void generateKeyPair(in AUTF8String algorithm,
>                         in nsIIdentityKeyGenCallback callback);
>  
>    ACString base64UrlEncode(in AUTF8String toEncode);
> +  ACString base64UrlDecode(in AUTF8String toDecode);

You must change the UUID of an interface whenever you add/remove/modify things within it. The reason for this rule has to do with the possibility of something (e.g. a native code extension) providing its own implementation of the interface, and the UUID mechanism is designed to help such extensions. (Sadly, [builtinclass] doesn't stop extensions from doing that.)

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 10 • 11 years ago

Thanks for the feedback, Brian.  I've changed the uuid for that interface.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 11 • 11 years ago

... and trailing whitespace removed

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 12 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=3a044aee53ac

	Masatoshi Kimura [:emk]
	Comment 13 • 11 years ago

Why atob/btoa is insufficient?

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/534fd5ea4363

	Masatoshi Kimura [:emk]
	Comment 15 • 11 years ago

Can anyone answer comment #13?

	Ed Morley [:emorley]
	Comment 16 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/534fd5ea4363

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 17 • 10 years ago

(In reply to Masatoshi Kimura [:emk] from comment #15)
> Can anyone answer comment #13?

I think you're correct that we can and should use atob and btoa as the core of these functions.