upon first time signup, don't ask the user if this is their computer - i... #1410
Conversation
ghost
assigned 
|
hey @shane-tomlinson, can you review this one? |
| @@ -19,7 +19,7 @@ exports.process = function(req, res) { | |||
| if (wsapi.isAuthed(req, 'assertion')) { | |||
| db.userOwnsEmail(req.session.userid, email, function(err, owned) { | |||
| if (err) wsapi.databaseDown(res, err); | |||
| else if (owned) res.json({ status: 'complete' }); | |||
| else if (owned) res.json({ status: 'complete', userid: req.session.userid }); | |||
There was a problem hiding this comment.
is this the first time we're exposing userid to the client, or is this the userid we're already exposing to the client? Is the only other place it exists in the encrypted cookie?
There was a problem hiding this comment.
we're exposing the userid outside of our servers for the first time with the API change. train-2012.03.28 did not expose it, train-2012.04.11 will expose it in a number of different API calls. This userid is the same that is in the encrypted cookie, which is the same that is in the database.
There was a problem hiding this comment.
and is that necessary? No easy workarounds?
There was a problem hiding this comment.
the motivation was to be able to keep track of a user's response to "is this your computer". If a user logs into different sites using different emails, they should only have to answer the question once per device, not once per email address.
challenging myself a bit, we could instead keep a collection of emails and update that based on the server's response to list_emails. I'd have to think through this a bit to see how reliable it would be.
How strongly opposed are you to exposing a user's own id to him?
There was a problem hiding this comment.
we're sure we're only exposing this to the user themselves? In that case I'm r+ for this, and I'll add an issue to investigate whether this is problematic.
There was a problem hiding this comment.
I'm actually not worried about the privacy leak (although it's a good point to bring up), I'm worried about the security implications of exposing internal identifiers. That can bite you at some point. However, if we're only exposing the user_id to that specific user, it's not nearly the same risk, so I'm good to go.
There was a problem hiding this comment.
Opened a new issue to explore this: GH-1421
|
Outside of the comments I have already made:
In the long run we are going to want to remove the notion of a userid from network.js and put that into user.js. In /dialog/resources/state.js, we should change
to use an encapsulation in user.js so that we can keep this logic out of site of the main client code. These are out of the scope of this pull request though. With the changes indicated to network.js, I give an r+. |
…y between a string and function which would always be false
…serid in response
|
13:12 < stomlinson> lloyd: with the change to network.js, I give my r+ to 1410 |
...ssue #1406 - issue #1405