Add /users endpoint #5506

eloquence · 2020-09-17T00:52:13Z

Resolves #5490

Description

Adds the /users endpoint as described in the issue and subsequent comments, enumerating non-sensitive information for all users.

Documentation PR dependent on this one:
freedomofpress/securedrop-docs#3

Status

Ready for review.

Test plan

Spin up a development environment from this PR.

Observe that you see this endpoint listed on http://localhost:8081/api/v1
Observe that you cannot access this endpoint at http://localhost:8081/api/v1/users without an authentication token
Observe that you can access the list of users when providing a valid authentication token as part of the request (see example below)
Observe that the list of users only includes the expected fields (UUID, username, first name, last name)

Token usage example (curl)

$ curl -X POST -H "Content-Type: application/json" \ 
  -d '{"username":"journalist", "passphrase":"correct horse battery staple profanity oil chewy", "one_time_code": "123456"}' \ 
  'http://localhost:8081/api/v1/token'

$ curl -H "Authorization: Token TOKEN_HERE" http://localhost:8081/api/v1/users

Checklist

Linting (make lint) and tests (make test) pass in the development container (tested for API tests)

eloquence · 2020-09-17T00:59:46Z

securedrop/models.py

+           fields are excluded."""
+
+        try:
+            last_login = self.last_access.isoformat() + 'Z'


When I first attempted to request a to_json representation of all users in the dev env, this raised an exception due to the last_access attribute never having been set. While not required for the bare representation, it seemed sensible to add a check here.

[nit/feel free to ignore] you could set last_login within the bare clause where bare is false since that's the only place it's used

[suggestion] instead of "bare" you could call it "all_details" or "all_info"

i think it makes sense to also add an "all_details" check that defaults to false for the "/user" endpoint. otherwise, we'll have some admins with is_admin set and some admins without is_admin set depending on who has logged into the workstation. looking at the table directly as an admin could be misleading. also, we don't do anything with this information in the client and should probably wait to include it once we finish our client admin stories.

you could set last_login within the bare clause where bare is false since that's the only place it's used

That makes sense; we'll still have to do the try check to avoid errors on newly created users, but I'll move it.

instead of "bare" you could call it "all_details" or "all_info"

I like all_info, will use that. I was trying to avoid suggesting that it's a full representation, since both representations exclude credentials.

also add an "all_details" check that defaults to false for the "/user" endpoint

Could you clarify? This PR doesn't modify the behavior of the /user endpoint (it currently already includes the is_admin Boolean), do you think that it should?

Made the other two changes in aa0a591

Could you clarify? This PR doesn't modify the behavior of the /user endpoint (it currently already includes the is_admin Boolean), do you think that it should?

Oh, I see, I forgot that we don't store the is_admin result from the /user endpoint so we don't have to worry about the situation I brought up where some admin acounts will have is_admin set and some will not. Okay, ignore previous comment.

sssoleileraaa · 2020-09-21T21:43:24Z

docs/development/journalist_api.rst

+      {
+        "first_name": "Nellie",
+        "last_name": "Bly",
+        "username": "nbly",


sssoleileraaa · 2020-09-21T21:46:24Z

docs/development/journalist_api.rst

+      {
+        "first_name": "Daniel",
+        "last_name": "Ellsberg",
+        "username": "dellsberg",


a-ha! now i get the whole "dellsberg" reference in the test data

sssoleileraaa · 2020-09-21T21:56:39Z

securedrop/models.py

+           fields are excluded."""
+
+        try:
+            last_login = self.last_access.isoformat() + 'Z'


[nit/feel free to ignore] you could set last_login within the bare clause where bare is false since that's the only place it's used

[suggestion] instead of "bare" you could call it "all_details" or "all_info"

sssoleileraaa · 2020-09-21T22:00:43Z

securedrop/models.py

+           fields are excluded."""
+
+        try:
+            last_login = self.last_access.isoformat() + 'Z'


i think it makes sense to also add an "all_details" check that defaults to false for the "/user" endpoint. otherwise, we'll have some admins with is_admin set and some admins without is_admin set depending on who has logged into the workstation. looking at the table directly as an admin could be misleading. also, we don't do anything with this information in the client and should probably wait to include it once we finish our client admin stories.

sssoleileraaa

alright this looks good to me. I think it would be good followup to add the all_info check to the "/user" endpoint eventually for consistency. plus it's easy to get confused about how we're adding is_admin to the response body yet never using or storing it.

looks like lint is failing, so once that's fixed I'll approve

Dismissing review at @creviera's request, needs a rebase and likely another round of linting

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

sssoleileraaa

lgtm

Observe that you see this endpoint listed on http://localhost:8081/api/v1
Observe that you cannot access this endpoint at http://localhost:8081/api/v1/users without an authentication token
Observe that you can access the list of users when providing a valid authentication token as part of the request (see example below)
Observe that the list of users only includes the expected fields (UUID, username, first name, last name)

conorsch

Looks great! @creviera already reviewed, but I'll add further confirmation:

Observe that you see this endpoint listed on http://localhost:8081/api/v1
Observe that you cannot access this endpoint at http://localhost:8081/api/v1/users without an authentication token
Observe that you can access the list of users when providing a valid authentication token as part of the request
Observe that the list of users only includes the expected fields (UUID, username, first name, last name) - The the first/last fields were present, although empty, which I'd expect given the create-dev-data logic.

Not blocking, but let's look for an opportunity to test the "last_login" field later on.

conorsch · 2020-09-23T22:44:03Z

Proceeding with merge. There's a bit more review required for #5505. Once both #5505 & #5506 are in, we'll rebase and review #5513.

eloquence commented Sep 17, 2020

View reviewed changes

eloquence marked this pull request as ready for review September 18, 2020 01:03

eloquence requested review from emkll, kushaldas, redshiftzero and zenmonkeykstop as code owners September 18, 2020 01:03

sssoleileraaa reviewed Sep 21, 2020

View reviewed changes

sssoleileraaa previously approved these changes Sep 21, 2020

View reviewed changes

sssoleileraaa self-requested a review September 21, 2020 22:37

eloquence force-pushed the 5490-users-api branch from aa0a591 to a37fdae Compare September 21, 2020 22:53

eloquence mentioned this pull request Sep 21, 2020

Add documentation for /users endpoint freedomofpress/securedrop-docs#3

Merged

Add /users endpoint

135a148

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 force-pushed the 5490-users-api branch from a37fdae to 135a148 Compare September 21, 2020 23:58

sssoleileraaa approved these changes Sep 22, 2020

View reviewed changes

eloquence mentioned this pull request Sep 23, 2020

Add support for /users endpoint freedomofpress/securedrop-sdk#131

Closed

conorsch self-requested a review September 23, 2020 22:33

conorsch approved these changes Sep 23, 2020

View reviewed changes

conorsch merged commit 56200ba into develop Sep 23, 2020

emkll mentioned this pull request Sep 28, 2020

Release SecureDrop 1.6.0 #5501

Closed

22 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add /users endpoint #5506

Add /users endpoint #5506

eloquence commented Sep 17, 2020 •

edited

Loading

eloquence Sep 17, 2020 •

edited

Loading

sssoleileraaa Sep 21, 2020

sssoleileraaa Sep 21, 2020

eloquence Sep 21, 2020

eloquence Sep 21, 2020

eloquence Sep 21, 2020

sssoleileraaa Sep 21, 2020

sssoleileraaa Sep 21, 2020

sssoleileraaa Sep 21, 2020

sssoleileraaa Sep 21, 2020

sssoleileraaa Sep 21, 2020

sssoleileraaa left a comment •

edited

Loading

sssoleileraaa left a comment

conorsch left a comment

conorsch commented Sep 23, 2020

Add /users endpoint #5506

Add /users endpoint #5506

Conversation

eloquence commented Sep 17, 2020 • edited Loading

Description

Status

Test plan

Token usage example (curl)

Checklist

eloquence Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sssoleileraaa left a comment • edited Loading

Choose a reason for hiding this comment

sssoleileraaa left a comment

Choose a reason for hiding this comment

conorsch left a comment

Choose a reason for hiding this comment

conorsch commented Sep 23, 2020

eloquence commented Sep 17, 2020 •

edited

Loading

eloquence Sep 17, 2020 •

edited

Loading

sssoleileraaa left a comment •

edited

Loading