Allow users to create multiple API tokens #697

Nemo157 · 2017-05-01T14:03:33Z

Fixes #688

Summary of changes:

Database:
- Add new api_tokens table that stores the named tokens, their creation date and last used date
API:
- Remove singular api_token field from User, it is still in the database for easy rollback, but should be removed from there as well soon.
- Remove existing /me/reset_token endpoint.
- Update user middleware to search incoming tokens via the api_tokens table, updating the last used date as it does so, and to record whether the current request was authenticated via an API token or a session cookie
  - This also changes this search to function via diesel where it was previously using pg
- Add new set of endpoints at /me/tokens allowing creating, listing and revoking API tokens
  - Listing tokens doesn't return the tokens value, that's only available once in the response from creating the token.
- Tests
Frontend:
- Remove the special support for an api_token parameter coming back from the /me endpoint
- Add new api-token model
  - Has a special adapter as the endpoints are namespaced under /me/tokens instead of the default /api/v1/api_tokens
  - Has a special serializer to set the key used when creating a new API token
- Update /me/index controller and template
  - Render a sorted list of all API tokens in the data store instead of the single token
  - Remove button to reset the token
  - Add button to start creating a new API token
- Add new api-token-row component for rendering the API tokens in
  the list on /me/index
  - Has three major states:
    - New token that has yet to be saved to the API, shows a text box to enter a name and button to save the token
    - Newly saved token, shows the normal token data + the actual token value and details on how to save it with cargo
    - Normal token, shows the name, creation date and last used date along with a button to revoke
  - In any of those states can have an error message shown below the token data if creating or revoking the token failed
- Some small general CSS changes:
  - A small variant for buttons that reduces the padding
  - Support for disabled state on buttons making them a little lighter than the specified color

old description

WIP for #688, still has a bit of work to go, but it's currently working and stylish enough that feedback would be useful.

I took inspiration from GitHub's "Personal Access Tokens" around showing the token value only once along with the last usage of the token.

TODOs

Screenshots

The settings page

Creating a new token

Showing the newly created token

Nemo157 · 2017-05-01T14:21:52Z

This is my first time using Ember.js, so if I've completely misunderstood how some parts of this should be structured just give me a pointer to how it should be structured and I'll fix it up. The big thing that seems the most likely to be non-idiomatic to me is manually fiddling around with the api_tokens array, but since this is mostly a read-only website I couldn't find any example of how that should be handled.

carols10cents

Just started a review but gtg. I'll look at more later!

carols10cents · 2017-05-02T19:36:09Z

migrations/20170428154714_multiple_api_tokens/up.sql

+CREATE TABLE api_tokens (
+  id SERIAL PRIMARY KEY,
+  user_id integer NOT NULL REFERENCES users(id),
+  token character varying DEFAULT random_string(32) NOT NULL UNIQUE,


😰 oh... oh my... there's a possibility in the current code that two users could have the same api_token.... probably pretty remote but it's still there......... good thing this code fixes it!

I suppose if random_string creates a duplicate that creating a new token would just fail? And the user would try again? Probably fine.

The random_string function is using a 62 character dictionary, giving a 32 character string ~190 bits of randomness. That makes it more collision resistant than a UUID, and UUIDs are already collision resistant enough that you're recommended not to ever bother checking for a collision.

Using an approximation to the birthday problem the chance of having two colliding tokens in the first 10,000 users is ~2.2×10^-50, if every person on Earth were to create two accounts the chance of any two colliding is ~5×10^-38.

But yeah, if random_string somehow does create a duplicate it would raise a constraint violation, which would presumably be returned as an error in token::new, which I assume conduit or some sort of middleware would transform to a 500 error response. Once I re-add the client-side error handling that should then be shown to user somehow, even if it's just a generic please try again message.

This is super niche, but can we use text instead of varchar here? They're identical under the hood, but are considered different types and specifically have conversion issues if you were to use array_agg or similar.

carols10cents · 2017-05-02T21:10:22Z

The screenshots look awesome!!!

Some thoughts:

Put one of the existing api tokens back in when reverting the SQL migration (not really sure if this is worth doing)

I don't think this is worth doing, no. Honestly, the only time I use down migrations are in development. To make production a bit less risky, though, could you remove the line in the up migration that drops the api_token column from user? That way, if we decide to roll back this code when deploying, we don't actually need to roll back the database as well. Then in a separate, future deploy, we can remove that column once we're sure it's no longer being used.

Make the name required (need something to set as the name of the original token)

I think empty string, that you have here, is fine.

carols10cents · 2017-05-03T19:16:31Z

Limit length of body read during POSTs (potential DOS)

This sounds like a good idea.... and something that I don't think we're doing anywhere right now, oops....

Along these lines, wdyt about putting a limit on the number of tokens one user can have? Something high by reasonable standards (100? 500? 1000?) but not unlimited?

carols10cents · 2017-05-03T19:26:04Z

What's the reason to require a token name? We're going to have tokens with empty string names in the database from the current tokens anyway, and if people don't want to name their tokens that's their choice.

Nemo157 · 2017-05-03T19:31:39Z

This sounds like a good idea.... and something that I don't think we're doing anywhere right now, oops....

I think it is actually checked everywhere right now, krate and uploader seem to be the only modules that read the request body, and they both obey app.config.max_upload_size which appears to be 10MB.

Actually, looking at those krate seems to assume there will be a Content-Length header sent, which is probably a valid assumption here for a POST from a browser; so that will be super easy to implement.

Along these lines, wdyt about putting a limit on the number of tokens one user can have? Something high by reasonable standards (100? 500? 1000?) but not unlimited?

Sure, that should be easy to add.

What's the reason to require a token name? We're going to have tokens with empty string names in the database from the current tokens anyway, and if people don't want to name their tokens that's their choice.

Since the actual token value is only returned during creation the only distinguishing factor for unnamed tokens is the date they were created. I guess it's not strictly necessary, but it seems better UX to force the user to add something there so they can distinguish them later. GitHub even goes so far as to disallow having duplicate names for its tokens.

Nemo157 · 2017-05-03T21:32:59Z

I've been thinking about

a small permissions model so that API tokens don't have permission to create a new API token

what's your opinion on adding something really simple like

enum AuthenticationMode {
    SessionCookie,
    ApiToken,
}

impl<'a> RequestUser for Request + 'a {
    fn auth_mode(&self) -> CargoResult<AuthenticationMode> {
        self.extensions().find::<AuthenticationMode>().chain_error(|| Unauthorized)
    }
}

and setting it during the user::middleware processing?

It probably wouldn't scale for any other permissions, but if API tokens ever get limited permissions those could be reused for denying API tokens to create new tokens and this could be dropped.

Nemo157 · 2017-05-15T07:12:31Z

I'm pretty happy with where this is now, I'll squash everything that's there now down to a single commit tonight (~10 hours from now); I don't think there's anything really useful in the history. Then I'll take another look at the two TODOs on the diesel code for the user lookup by api token, but the rest of it is done other than review feedback.

carols10cents · 2017-06-06T15:14:13Z

Hi @Nemo157, sorry this has taken me so long to look at but I'll be reviewing it in the next few days hopefully!

Nemo157 · 2017-06-14T17:18:48Z

I just rebased this onto master, there was only one significant change which was that the new UserShowResponse ended up identical to the MeResponse in the tests so I merged them. We can just consider /me to be an alias of /users/<my user id>.

carols10cents · 2017-06-15T00:08:01Z

Thank you and I'm sorry this has taken me so long!!! I'm trying this out right now :)

Fixes rust-lang#688 Summary of changes: * Database: * Add new `api_tokens` table that stores the named tokens, their creation date and last used date * API: * Remove singular `api_token` field from `User`, it is still in the database for easy rollback, but should be removed from there as well soon. * Remove existing `/me/reset_token` endpoint. * Update user middleware to search incoming tokens via the `api_tokens` table, updating the last used date as it does so, and to record whether the current request was authenticated via an API token or a session cookie * This also changes this search to function via `diesel` where it was previously using `pg` * Add new set of endpoints at `/me/tokens` allowing creating, listing and revoking API tokens * Listing tokens doesn't return the tokens value, that's only available once in the response from creating the token. * Tests * Frontend: * Remove the special support for an `api_token` parameter coming back from the `/me` endpoint * Add new `api-token` model * Has a special `adapter` as the endpoints are namespaced under `/me/tokens` instead of the default `/api/v1/api_tokens` * Has a special `serializer` to set the key used when creating a new API token * Update `/me/index` controller and template * Render a sorted list of all API tokens in the data store instead of the single token * Remove button to reset the token * Add button to start creating a new API token * Add new `api-token-row` component for rendering the API tokens in the list on `/me/index` * Has three major states: * New token that has yet to be saved to the API, shows a text box to enter a name and button to save the token * Newly saved token, shows the normal token data + the actual token value and details on how to save it with cargo * Normal token, shows the name, creation date and last used date along with a button to revoke * In any of those states can have an error message shown below the token data if creating or revoking the token failed * Some small general CSS changes: * A small variant for buttons that reduces the padding * Support for disabled state on buttons making them a little lighter than the specified color

carols10cents · 2017-06-15T13:17:50Z

Ok 2 small things:

Could we make it so that hitting enter while in the token name field submits the form?
Could we add a label for the migrated tokens? It looks a bit empty without one, and since we don't allow creation of tokens without labels, the migrated ones sort of imply that you can create a token without a label... maybe like Initial token or something?

Everything seems to be working great with the permissons and revoking the tokens!

carols10cents · 2017-06-15T13:59:06Z

Also we added rustfmt, so I rebased and ran it on your changes, which should get the build passing again :)

Nemo157 · 2017-06-15T18:06:31Z

Cool, made those changes and re-ran the migration to check it worked.

carols10cents · 2017-06-18T16:26:12Z

Hmmmmm ok one more thing: I see you have autofocus on the input field for a new token's name, but when I click the new token button, I'm not getting the cursor automatically focused in the name field, in either Firefox 54.0 or Chrome 59.0.3071.104 on macOS. Is it working for you?

autofocus on injected elements doesn't work in all browsers

Nemo157 · 2017-06-18T16:50:42Z

That is odd, it autofocuses for me with Safari 9.1.2 (I know, I can't update) but doesn't with Chrome 51.0.2704.10 or 59.0.3071.104 on macOS. By the sounds of turbolinks/turbolinks-classic#365 (and its fix) Firefox (and now Chrome) don't autofocus on injected elements. Added manual focussing to support browsers that don't autofocus.

sgrif · 2017-06-18T19:56:25Z

src/schema.rs

@@ -132,7 +143,6 @@ table! {
        id -> Int4,
        email -> Nullable<Varchar>,


This made me realize that we're using varchar all over the place. 😓

sgrif · 2017-06-18T19:58:32Z

src/token.rs

+
+    /// Converts this `ApiToken` model into an `EncodableApiToken` for JSON
+    /// serialization.
+    pub fn encodable(self) -> EncodableApiToken {


Is this method separate from encodable_with_token for security reasons? What do you think about separating them into separate types to make future mistakes less likely, and further clarify intent?

Sort of for security reasons. The HTTP API is following the common practice of only returning the token value once when creating the token. I'm pretty ambivalent about separating this into two types or not; if there were a way to specify one as a superset of the other + an extra field then it would definitely be the better way to go, but since it currently requires manual synchronisation if a field is added I see downsides to both approaches.

Do you think fields are likely to be added to this type frequently enough that we need to be concerned about synchronization?

No, I'll separate them out now so we can at least see what that looks like.

sgrif · 2017-06-18T19:58:57Z

src/token.rs

+    }
+}
+
+struct BadRequest<T: CargoError>(T);


Should this be in a file focused on HTTP responses?

Also -- reading further down, this is only ever passed human. Should it just take a String instead until there's evidence that it needs to take something else?

It's a bit of a hack to support the Ember frontend. My preference would be to drop this and just have human return errors with the status code BadRequest, but cargo does not handle that well. I could move this to util::errors and add a comment about what it's intended use is.

sgrif · 2017-06-18T19:59:39Z

src/token.rs

+
+/// Handles the `GET /me/tokens` route.
+pub fn list(req: &mut Request) -> CargoResult<Response> {
+    let tokens = ApiToken::find_for_user(&*req.db_conn()?, req.user()?.id)?


What do you think about pulling these out into local variables? I personally find the condensed punctuation hard to read

sgrif · 2017-06-18T19:59:59Z

src/token.rs

+    /// The incoming serialization format for the `ApiToken` model.
+    #[derive(RustcDecodable, RustcEncodable)]
+    struct NewApiToken {
+        pub name: String,


Nit: Does pub do anything here?

Nope, not sure where that came from...

sgrif · 2017-06-18T20:03:49Z

src/user/mod.rs

-            .map(|r| Model::from_row(&r))
-            .chain_error(|| NotFound)
+    pub fn find_by_api_token(conn: &PgConnection, token_: &str) -> CargoResult<User> {
+        // TODO: This should be doable as a single query, but I can't figure


What's the SQL that you would write to do this as a single query? I don't see any way that you could do this in one query.

UPDATE api_tokens SET last_used_at = now() FROM users WHERE api_tokens.token = $1 AND users.id = api_tokens.user_id RETURNING users.*

Hm intersting. I didn't realize you could set FROM on an update like that. I'll look into providing an API. I think we can ✂️ this comment though, the answer is "you can't"

sgrif · 2017-06-18T20:05:00Z

src/user/mod.rs

@@ -419,54 +391,3 @@ pub fn updates(req: &mut Request) -> CargoResult<Response> {
        meta: Meta { more: more },
    }))
 }
-
-#[cfg(test)]


Does this PR make these tests invalid? It doesn't seem like the behavior they're testing has changed, and I'm not sure where these tests moved to

Nope, I can readd and update them. They just seemed very low value tests with the tokens pulled out to be a separate resource.

Looking at the commit which added them they seem to be testing something very specific which is still relevant here

Actually, I believe the intent of the first test is covered under create_token_multiple_have_different_values in src/test/token. The second one I can try and come up with an equivalent with the new data structure.

…tokens

carols10cents · 2017-07-01T23:31:49Z

I'm really really really sorry this has taken me so long. I'm feeling ready to get this merged in-- just testing it out one last time ❤️

carols10cents · 2017-07-02T00:16:18Z

Merged and deployed to prod, it seems to be doing well!! I'm so excited about this change, thank you so much for your patience!!!

carols10cents reviewed May 2, 2017

View reviewed changes

Nemo157 mentioned this pull request May 4, 2017

HTTP error codes are barely used #712

Closed

Nemo157 force-pushed the multi-token branch 2 times, most recently from 55c5582 to b4c8190 Compare May 5, 2017 20:36

Nemo157 force-pushed the multi-token branch from 411bcac to 1ad6af6 Compare May 14, 2017 10:38

Nemo157 force-pushed the multi-token branch 2 times, most recently from f29e2a4 to e8381df Compare May 15, 2017 17:56

Nemo157 changed the title ~~WIP: Multiple API tokens~~ Allow users to create multiple API tokens May 15, 2017

carols10cents added the S-waiting-on-review label Jun 6, 2017

carols10cents mentioned this pull request Jun 6, 2017

[NOT TO MERGE] Chapter 14 from nostarch rust-lang/book#751

Closed

Nemo157 force-pushed the multi-token branch from e8381df to 20d85b0 Compare June 14, 2017 17:17

carols10cents force-pushed the multi-token branch from 20d85b0 to e5e1b33 Compare June 15, 2017 00:09

Run rustfmt

b450a36

Nemo157 added 2 commits June 15, 2017 19:50

Provide a name of API token for existing users

7c94506

Fix action run when pressing enter on API token name

4910828

carols10cents added S-waiting-on-author and removed S-waiting-on-review labels Jun 18, 2017

Manually focus on input element

1ede604

autofocus on injected elements doesn't work in all browsers

sgrif reviewed Jun 18, 2017

View reviewed changes

Nemo157 and others added 11 commits June 18, 2017 22:43

Separate encodable api tokens with/without the token value

6d8f7c0

Move bad_request into util::errors and clarify what it's intended for

0d3b038

Minor review changes

24430e5

Remove TODO, not possible with diesel's current API

5610dac

Add test that two users creating tokens get different values

09af226

Re-add test that updating user's github login doesn't invalidate api …

f64e109

…tokens

Merge remote-tracking branch 'upstream/master' into multi-token

080a34a

cargo fmt

3124ddf

Fix unneeded pass-by-value

fbbf538

Merge remote-tracking branch 'upstream/master' into multi-token

21eddca

Merge remote-tracking branch 'upstream/master' into multi-token

66e36f9

carols10cents merged commit 086249c into rust-lang:master Jul 1, 2017

Turbo87 removed the S-waiting-on-author label Dec 3, 2017

Nemo157 deleted the multi-token branch July 30, 2023 11:18

		@@ -132,7 +143,6 @@ table! {
		id -> Int4,
		email -> Nullable<Varchar>,

Allow users to create multiple API tokens #697

Allow users to create multiple API tokens #697

Conversation

Nemo157 commented May 1, 2017 • edited Loading

Nemo157 commented May 1, 2017

carols10cents left a 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

carols10cents commented May 2, 2017

carols10cents commented May 3, 2017

carols10cents commented May 3, 2017

Nemo157 commented May 3, 2017

Nemo157 commented May 3, 2017

Nemo157 commented May 15, 2017

carols10cents commented Jun 6, 2017

Nemo157 commented Jun 14, 2017

carols10cents commented Jun 15, 2017

carols10cents commented Jun 15, 2017

carols10cents commented Jun 15, 2017

Nemo157 commented Jun 15, 2017

carols10cents commented Jun 18, 2017

Nemo157 commented Jun 18, 2017 • 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

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

carols10cents commented Jul 1, 2017

carols10cents commented Jul 2, 2017

Nemo157 commented May 1, 2017 •

edited

Loading

Nemo157 commented Jun 18, 2017 •

edited

Loading