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

Restrict access to My Feeds to logged-in users #760

Closed
Silvyre opened this issue Mar 1, 2020 · 5 comments · Fixed by #933
Closed

Restrict access to My Feeds to logged-in users #760

Silvyre opened this issue Mar 1, 2020 · 5 comments · Fixed by #933
Assignees
Labels
area: front-end area: sso Authentication type: enhancement New feature or request type: security Security concerns

Comments

@Silvyre
Copy link
Contributor

Silvyre commented Mar 1, 2020

Once #748 is landed, anyone on the Internet will be able to add feeds to Telescope. Access to this component should be restricted to logged-in users, ensuring that non-users cannot add unwanted feeds (e.g. spam) to our system.

@Silvyre Silvyre added area: front-end area: sso Authentication type: security Security concerns labels Mar 1, 2020
@Silvyre
Copy link
Contributor Author

Silvyre commented Mar 23, 2020

@humphd If this can be done using graphql-passport functionalities, I'd be happy to take this on for 0.9.

@Silvyre
Copy link
Contributor Author

Silvyre commented Apr 6, 2020

@humphd I don't think this will be making 0.9, but I'm still interested in working on this whenever we can find the time.

@humphd
Copy link
Contributor

humphd commented Apr 6, 2020

There are at least two ways we could go with this.

  1. Have the myfeeds page do a GET to /user/info and if it gets back a 403, we use something like what @raygervais is doing in adds: error page front-end concept #925 to render a "403 Forbidden" page (probably via a redirect). If we get back a 200 from /user/info we go ahead and let the content of the page render. So the render function would have a big if(user) check around all the content, and only render something if there's a user.
  2. We could try and mimic some of what Gatsby does in https://www.gatsbyjs.org/tutorial/authentication-tutorial/. They store user information (e.g., a JWT) in localStorage, and have a service that wraps around it to allow different components to see if the user is logged in or not. We wouldn't want to use localStorage (since it would mean users would seem to be logged in long after they had been timed out on the server), but we could consider using sessionStorage. When the user logs in, we could do window.sessionStorage.setItem('user', JSON.stringify(userInfo)); and store the object we get from /user/info in the session. Now every page (or component) can try and use let user = JSON.parse(window.sessionStorage.getItem('user')) to get that data back again. I'm trying to think of a case where this would get out of sync with the real logged in state. You'd have to make sure to clear it if the user manually logs out; but our session management code in the node server might also accomplish this, I'd have to test.

Doing 1. and hitting /user/info every time you need to determine if the user is logged in is probably the safest way. A user can't really cause any security problems with 2. though.

cc @Grommers00, @c3ho, @manekenpix as well for thoughts.

@Silvyre
Copy link
Contributor Author

Silvyre commented Apr 8, 2020

  1. Have the myfeeds page do a GET to /user/info and if it gets back a 403, we use something like what @raygervais is doing in adds: error page front-end concept #925 to render a "403 Forbidden" page (probably via a redirect). If we get back a 200 from /user/info we go ahead and let the content of the page render.

In #933, I previously had My Feeds redirecting a user to the /auth/login route if it receives a bad response to GET /user/info. I just changed the redirection target to /404 in a9ce067#diff-341423ac4fcd5b3e25e7dd93d2bae8e1R49.

So the render function would have a big if(user) check around all the content, and only render something if there's a user.

Gotcha, also just added in a9ce067#diff-341423ac4fcd5b3e25e7dd93d2bae8e1R127

Since this issue seems like it might get resolved by the aforementioned changes, I went ahead and marked #933 to close this issue upon merge.

@humphd
Copy link
Contributor

humphd commented Apr 8, 2020

This could leverage the 404 page work Ray is doing, and add 403.

Silvyre added a commit that referenced this issue Apr 9, 2020
* add form logic, help modal to My Feeds page

* fetch user feeds into separate form controls

* add delete feed buttons, refactor logic

* address review comments

* remove call to refresh window upon feed deletion

* refactor a useState call for consistency

* redirect non-logged-in users to 404 page without rendering My Feeds page

* add feed deletion logic to DeleteFeedDialogButton

* implement getUserFeeds() stop-gap

* fix DeleteFeedDialogButton props

* correct props destructuring in 3435b76

* remove commented-out proptypes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: front-end area: sso Authentication type: enhancement New feature or request type: security Security concerns
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants