Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement client playlists #17

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

coditva
Copy link
Member

@coditva coditva commented Feb 19, 2017

Addresses #5
Do not merge. Not complete. Will keep adding commits as things are done.
What works:

  • Users can login. Signup is not yet tested.

What I want to know is,

  • How are you restricting access to who can load songs in the db. Only admin can do that right? But I am unable to find where the check for admin is.
  • How should I store playlists per user. (Make a separate field in the db? We can do this. The playlists can even have option to set their visibility. ie users can share their playlists like spotify)

@0xRampey
Copy link
Member

Just append /admin at the end of your URL to access the admin panel.
A DB entry would be a good idea if Sharing playlists was a login-only feature. Then every registered user would have his own DB record of playlists.
Is that what you had in mind?

@coditva
Copy link
Member Author

coditva commented Feb 19, 2017

Arey I know about the '/admin' thing. What I'm asking is, Is there a check in the code such that, say, a command is run only by an admin...
Yes, it'll be basically one db for all 'user-playlists', and each user will have an array field for storing the playlist IDs...

@SebastinSanty
Copy link
Member

Actually the socket is open for everyone (we need to close it to avoid breach). We have only created a check in the front-end template.

@SebastinSanty SebastinSanty changed the title Implement client playlists [WIP] Implement client playlists Feb 19, 2017
@coditva
Copy link
Member Author

coditva commented Feb 19, 2017

@SebastinSanty, Will do that too...

@SebastinSanty
Copy link
Member

@0xRampey
Copy link
Member

@SebastinSanty Hard scan is registered to one of the socket events in the backend right?
@utkarshme Why don't you include your middleware functions inside the socket.on listener for all admin events?

@SebastinSanty
Copy link
Member

@prampey Yes, it is

@0xRampey
Copy link
Member

I think it pertains to socket.on('scan_update' in the socket.js file.

@0xRampey 0xRampey changed the title [WIP] Implement client playlists Implement client playlists Feb 24, 2017
@0xRampey 0xRampey added the WIP label Feb 24, 2017
@coditva
Copy link
Member Author

coditva commented Feb 24, 2017

Isn't socket.js public? Also, I am unable to figure out a way to get user session data (ie username or id) in the socket events in the routes/index.js file... :/

@SebastinSanty
Copy link
Member

SebastinSanty commented Feb 26, 2017

@utkarshme Yeah, it is public and that's the downside. This can be exploited only if the enduser makes a socket-client and connects (rare case). After our initial release, we can create an authenticated socket connection, to avoid external interference

This? https://github.com/OSDLabs/Encore/blob/master/routes/index.js#L110-L132

@0xRampey
Copy link
Member

Even though it's public, you can use the middleware functions to ensure access to only an admin or registered user right?

@mukkachaitanya
Copy link
Member

I got two ways to solve this issue :

  1. Use neDBSessionStore in the sessionOpts and access user details in the socket calls. (Ref)
  2. Use passport.socketio to get the current user details in socket calls. (needs a little setup I think, but good option)

So based on above we can have two ways to store/get playlists:

  1. Store playlists for each user on ther server and access using either of the above methods.
  2. Store playlists on users system. ( Ignore this in fact)

Also we might be able to tackle the admin problems with this.

So yeah if you people agree with me on this then we can continue with either of the ideas or improve on them.

@coditva
Copy link
Member Author

coditva commented Feb 26, 2017

@SebastinSanty, that works for a function that takes req as an argument (ie a middleware), but it fails for socket... I think passport.socketio, like @mukkachaitanya suggested, should work...

@SebastinSanty
Copy link
Member

Oh yes, I agree. I didn't notice the socket events you mentioned.

@mukkachaitanya
Copy link
Member

mukkachaitanya commented Feb 27, 2017

I think neDBSessionStore would be better in long run as we will be able to recommend songs and even implement public/share playlists better this way (I think so!).
Though I am not against passport.socketio, as at first glance even I thought this would be best.

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

Successfully merging this pull request may close these issues.

4 participants