search

mozilla / persona

Persona is a secure, distributed, and easy to use identification system.
https://login.persona.org
Other
1.83k stars 265 forks source link

Stateless (Goldilocks) API ignores ToS / PP #4142

Closed callahad closed 10 years ago

callahad commented 10 years ago

Easy to revert if we need to, but let's start by keeping the Goldilocks surface area as absolutely minimal as possible. More discussion in the issue below:

Related to #4134

jaredhirsch commented 10 years ago

I gotta be honest, I am not crazy about this change, because it doesn't simplify our code, and it makes our API situation even more complex for RPs (" we'll show your ToS/PP, sometimes ").

I'd prefer either ripping ToS/PP out completely, or leaving it alone. This project has been dormant for months and had accreted a lot of product complexity, and I think we're fine to move fast and start really streamlining.

callahad commented 10 years ago

We can't rip out ToS/PP quite yet, out of respect for existing RPs, but this establishes helps us get there in the future. This patch creates a line in the sand that ToS/PP doesn't exist in Goldilocks, but it does exist in the soon-to-be-deprecated Get and Observer APIs.

We take a negligible bump in complexity for the moment so that we can actually drop ToS/PP once Goldilocks is the only supported API.

callahad commented 10 years ago

@6a68 With updated tests, are you willing to r+ this?

From what I can tell, our options are:

  1. Nuke ToS/PP entirely, surprising RPs and breaking our deprecation contract.
  2. Allow ToS/PP, sticking us with this ill-fated feature for the foreseeable future.
  3. Accept this patch, limiting ToS/PP to deprecated APIs.

This change is literally the addition of 5 functional lines of code; I'm not sure it adds meaningful complexity, and it buys us quite a bit of win in the future.

jaredhirsch commented 10 years ago

Alright, you've convinced me, let's take the gradual approach :-)

Osmose commented 10 years ago

Talked with @callahad, we decided that removing the functionality is a bit too risky to do outright (especially because it'd block me from doing a django-browserid release since it's already on Goldilocks but still supports some Mozilla sites that rely on the arguments), so we're going even more gradual and just warning for the time being.

And given that there's no failure to test, only warnings, this looks r+ to me. :D

callahad commented 10 years ago

Travis is timing out on connecting to MySQL, but the tests pass for me on Linux.

With @Osmose's r+, merging.