416 425 merging - Githubissues

TransforMap / data.transformap.co

Here you find a Node.js daemon to serve the public TransforMap web service.

https://data.transformap.co

GNU Affero General Public License v3.0

7 stars 2 forks source link

416 425 merging #54

Closed jum-s closed 6 years ago

jum-s commented 6 years ago

This PR integrates the last features required by 416 user api and 425 oauth It opens endpoint with valid authentication process. As a third party user, i can now access my own information through /user endpoint which fetch user store. It also improve several api service:

obsolete testing routes are deleted (/users/:id, /secretPage)
set cookie is secured
user store views byId update

jum-s commented 6 years ago

should be good to go

almereyda commented 6 years ago

Sorry, I was sick last week. After deploying this branch to staging, a private browser window is immediately presented with a connect.sid cookie for the whole .allmende.io domain before attemping to call /auth. Also the server log contains a warning:

2017-11-11T00:53:43.444683132Z app[web.1]: -----
2017-11-11T00:53:45.551343979Z app[web.1]: Warning: connect.session() MemoryStore is not
2017-11-11T00:53:45.551373090Z app[web.1]: designed for a production environment, as it will leak
2017-11-11T00:53:45.551376178Z app[web.1]: memory, and will not scale past a single process.

almereyda commented 6 years ago

The use of a cookie name of cookie.sid and a domain scope of allmende.io for it breaks again the login at https://hack.allmende.io due to a name and scope collision.

jum-s commented 6 years ago

I dont think refusing this PR the best, nothing is wrong here, the branch works.. not perfectly but it does. To answer your questions:

the cookie name is just a default, doesnt change anything in the behavior.
The cross domain scope has been deleted in the commit 7327bb3, are you talking about the cookie path ? that should be for /data ?
According to https://stackoverflow.com/questions/10760620/using-memorystore-in-production' i dont think storing cookies in a memorystore (aka couchdb) is necessary since the warning (only) about memory leaks is about storing expired cookies of users that dont come back to the platform, which is an acceptable risk for the moment.