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

Add /users endpoint to API #5490

Closed
eloquence opened this issue Sep 10, 2020 · 2 comments · Fixed by #5506
Closed

Add /users endpoint to API #5490

eloquence opened this issue Sep 10, 2020 · 2 comments · Fixed by #5506
Labels

Comments

@eloquence
Copy link
Member

eloquence commented Sep 10, 2020

In order to reliably update local user databases (see, for example, freedomofpress/securedrop-client#1143), it would be useful for the API to expose a /users endpoint.

Currently, non-admin users do not have the ability to see a list of all users via the Journalist Interface , so we may only want to return the UUID for each user (and potentially a property like is_deleted or is_locked if/when such functionality is implemented, cf. #5467).

@eloquence eloquence added needs/discussion queued up for discussion at future team meeting. Use judiciously. api labels Sep 10, 2020
@eloquence eloquence changed the title Add /users endpoint to API Add /journalists endpoint to API Sep 11, 2020
@sssoleileraaa
Copy link
Contributor

Just want to make a note of what bug this would currently solve. Right now, we learn whether or not a user has been deleted through the replies endpoint. If a user does not have any successful replies on the server and they are deleted, we will not learn whether or not that user has been deleted. From the client, even though the user would not be able to log in, it would look like that user has an active account. Also if that user has any draft replies that never made it to the server, those replies will continue to exist locally and will continue to be attributed to the user, which others will be able to see in the GUI.

@eloquence eloquence changed the title Add /journalists endpoint to API Add /users endpoint to API Sep 16, 2020
@eloquence
Copy link
Member Author

eloquence commented Sep 16, 2020

A few updates on this issue:

  1. We've provisionally agreed that this would be a useful endpoint to add, either in 1.6.0, or after.
  2. We would likely want to return user name, first name, and last name, in addition to the UUID, but not the is_admin flag.
  3. After some back and forth, we agreed it should be called /users, to be consistent with the existing /user endpoint, and because it's more inclusive of users in different roles in an org.
  4. In terms of additional disclosure of user information to non-admins, while non-admins can't currently enumerate all users, they can enumerate many users, since the /replies endpoint discloses the same user data which would be provided by this endpoint, for users who have sent replies.
  5. Once this lands, in a future release, we could consider removing the full user data from the /replies endpoint (returning only the UUID of reply authors). This should only be done once the production release of the SecureDrop Client no longer relies on it.

I've raised my hand for putting together a PR for this issue, and then we can reason a bit more about 1) any additional impact in the context of our threat model, 2) whether or not this should land in 1.6.0.

eloquence added a commit that referenced this issue Sep 17, 2020
@eloquence eloquence mentioned this issue Sep 17, 2020
5 tasks
eloquence added a commit that referenced this issue Sep 21, 2020
eloquence added a commit that referenced this issue Sep 21, 2020
Resolves #5490

Includes the following tests:
- Test that endpoint exists
- Test that endpoint requires authorization
- Test that endpoint enumerates all users
- Test that endpoint returns only expected fields
@eloquence eloquence removed the needs/discussion queued up for discussion at future team meeting. Use judiciously. label Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants