Skip to content
This repository has been archived by the owner on May 10, 2019. It is now read-only.

Internal APIs changed in dev (was: The assertion object is not serializable (on development)) #1395

Closed
st3fan opened this issue Apr 5, 2012 · 9 comments

Comments

@st3fan
Copy link

st3fan commented Apr 5, 2012

I'm using the following code to call BrowserID and then pass the assertion to our own backend for verification:

function doBrowserId() {
  navigator.id.getVerifiedEmail(function(assertion) {
    if (assertion) {
      $.ajax({
        url: "/browserid/verify",
        data: { 'assertion': assertion },
        dataType: "json",
        success: function(data, status, xhr) {
           // Not important what happens here
        },
        error: function(xhr, status, error) {
           // Not important what happens here
        }
      });
    } else {
        // Not important what happens here
    }
  });
}

This works ok in production but fails on development. The assertion is serialized as "[Object object]" instead of a proper JSON encoded dictionary. Since this is pretty much based on sample code that many people have in production, I wonder if this should break like this.

@lloyd
Copy link
Contributor

lloyd commented Apr 5, 2012

@st3fan - steps to reproduce?

@st3fan
Copy link
Author

st3fan commented Apr 5, 2012

Not sure how to document that. Login to browserid & serialize assertion?

@benadida
Copy link
Contributor

benadida commented Apr 5, 2012

@st3fan you're doing this on dev.myfavoritebeer.org?

@lloyd
Copy link
Contributor

lloyd commented Apr 6, 2012

this is a regression in our internal APIs. It won't affect a lot of people, but the people it will affect are important (looking at you, @st3fan). I think it's easy to work around this, and don't think we should needlessly break contracts. Here's the conclusion I reached on IRC in discussing with @st3fan, with the context preceeding it: http://irclog.gr/#show/irc.mozilla.org/identity/73222

given this, assigning to self and giving it 5 stars.

I repeat, dev specific, internal api specific.

@lloyd
Copy link
Contributor

lloyd commented Apr 9, 2012

so I'm unhappy with our testing of this feature. It seems like the only tests we have don't actually execute the code, and they don't catch this regression...

@benadida
Copy link
Contributor

benadida commented Apr 9, 2012

@lloyd yeah, we haven't given the internal API enough love, agreed. We should, especially as we're about to add more.

@lloyd
Copy link
Contributor

lloyd commented Apr 9, 2012

The only way I can think of improving the situation is to simulate native embedding with a test specific iframe that mimics how pancake (and other native code) will use our internal API, then to script that with something like selenium...

For the purposes of this bug I'm going to deploy a blind fix on a branch/ephemeral VM and let @st3fan test it...

lloyd added a commit that referenced this issue Apr 9, 2012
@lloyd
Copy link
Contributor

lloyd commented Apr 9, 2012

@benadida - @callahad reports that there are people interested in this feature. so that's a +1 to formalizing it, documenting it, providing and example, and making sure we have tests that validate our API isn't inadvertently broken.

context: http://irclog.gr/#show/irc.mozilla.org/identity/73775

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants
@benadida @st3fan @lloyd and others