Defer user authentication until requested by endpoint #2077

jtgeibel · 2019-12-28T22:49:33Z

Many endpoints do not require user authentication, and by moving the
logic out of the middleware layer these endpoints now avoid obtaining a
database connection from the pool and 1 or 2 queries.

Due to lifetime issues a small middlware component remains. Further
details are described in the middleware's documentation.

r? @carols10cents

Turbo87

nice work!

Turbo87 · 2019-12-30T08:25:39Z

src/middleware/current_user.rs

-                req.mut_extensions().insert(api_token);
-            }
+        if let Some(id) = req.session().get("user_id").and_then(|s| s.parse().ok()) {
+            req.mut_extensions().insert(TrustedUserId(id));


out of curiosity, why is it not possible to put this into the authenticate call too?

(in response to tobias) I think this req.session() is the call that needs the mutable reference to the request

I think this req.session() is the call that needs the mutable reference to the request

Yes, that's exactly the problem I ran into. session() would probably be better called session_mut(), since it calls mut_extensions() and find_mut() internally to return a &mut HashMap<_, _>.

carols10cents

This is nice!!! I just have a few questions :)

carols10cents · 2020-01-03T15:47:23Z

src/middleware/current_user.rs

+//! Due to lifetimes it is not possible to call `mut_extensions()` while a reference obtained from
+//! `extensions()` is still live.  The call to `conduit_cookie::RequestSession::session` needs
+//! mutable access to the request and its extensions, and there is no read-only alternative in
+//! `conduit_cookie` to access the session cookie.  This means that it is not possible to access


Would our code be better if there was a read-only alternative to get the session cookie? It looks like the write access is used for caching the results of decoding the cookie, if we don't care about that, would that be a better alternative? Or is this PR acceptable/better? Just making sure that enhancing conduit-cookie isn't what we actually want :)

Would our code be better if there was a read-only alternative to get the session cookie?

It would be nice to support that upstream in conduit-cookie. We only use the session cookie here and during the login oauth workflow, and in both cases we don't need a database connection at that particular moment so it hasn't been a problem, yet. Although I don't really see us adding more complex cookie logic anytime soon.

It looks like the write access is used for caching the results of decoding the cookie.

As I understand it, the middleware layer from conduit-cookie decodes the cookie before the request is processed by the endpoint, and then the middleware adds the (possibly updated) cookie back into every response. So the &mut HashMap<_, _> returned here is what provides the API for mutating the session cookie.

There are some changes (especially with respect to bumping old deps, and more especially wrt our very old dependency on semver) that I would like make to the conduit-* ecosystem. Last I looked we (crates.io) are the only public users of these crates, so it would be awesome if we could make some improvements and release new versions of these crates. Longer term, it would be nice if we could add async/await support in our middleware stack.

For now, I think its fine to document the workaround. This also provides an easy way for the test harness to simulate cookie authenticated requests, although it would certainly be nice to construct real cookies in the test harness instead.

src/controllers/util.rs

carols10cents · 2020-01-03T17:53:15Z

src/middleware/current_user.rs

-                req.mut_extensions().insert(api_token);
-            }
+        if let Some(id) = req.session().get("user_id").and_then(|s| s.parse().ok()) {
+            req.mut_extensions().insert(TrustedUserId(id));


(in response to tobias) I think this req.session() is the call that needs the mutable reference to the request

carols10cents · 2020-01-03T18:00:22Z

src/controllers/krate/follow.rs

-        user_id: user.id,
-        crate_id,
-    })
+    Ok(Follow { user_id, crate_id })


I like that this abstraction gets rid of a User lookup in cases like this spot when it's not needed 👍

carols10cents · 2020-01-03T18:02:04Z

src/controllers/krate/publish.rs


    let conn = app.diesel_database.get()?;
+    let ids = req.authenticate(&conn)?;
+    let user = ids.find_user(&conn)?;


LOVE that this is out of parse_new_headers now!!!!

carols10cents · 2020-01-03T18:15:18Z

src/controllers/version.rs

 use crate::models::{Crate, CrateVersions, Version};
 use crate::schema::versions;

-fn version_and_crate(req: &mut dyn Request) -> AppResult<(Version, Crate)> {
+fn version_and_crate(req: &dyn Request) -> AppResult<(DieselPooledConn<'_>, Version, Crate)> {


This function is a little strange now that it returns a connection too, but 🤷‍♀ it's not exactly the cleanest abstraction to begin with anyway given that half the time we don't even use the returned crate. Always more to do.

This function is a little strange now that it returns a connection too

I would be cleaner to obtain a connection and pass it to this function, but it just ~~isn't~~ wasn't possible because of the lifetimes here. As I write this, I now see that I eventually dropped the mutness of the &dyn Request, so it should now be possible to pass in a &PgConnection as well, at least I think.

Always more to do.

Yup!

bors · 2020-01-04T01:58:54Z

☔ The latest upstream changes (presumably #2072) made this pull request unmergeable. Please resolve the merge conflicts.

jtgeibel · 2020-01-04T03:16:26Z

I've rebased to resolve merge conflicts, and added a 2nd commit that addresses the review comments.

bors · 2020-01-06T02:39:01Z

☔ The latest upstream changes (presumably #2032) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2020-01-15T18:11:42Z

☔ The latest upstream changes (presumably #1955) made this pull request unmergeable. Please resolve the merge conflicts.

Many endpoints do not require user authentication, and by moving the logic out of the middleware layer these endpoints now avoid obtaining a database connection from the pool and 1 or 2 queries. Due to lifetime issues a small middlware component remains. Further details are described in the middleware's documentation.

jtgeibel · 2020-01-16T03:59:53Z

Rebased to resolve merge conflicts. I believe all review comments have been addressed. I'd like to deploy to staging tonight, and possibly production tomorrow.

@bors r=carols10cents

bors · 2020-01-16T03:59:54Z

📌 Commit c06e7c2 has been approved by carols10cents

bors · 2020-01-16T04:00:02Z

⌛ Testing commit c06e7c2 with merge 63d45b6...

@carols10cents

Defer user authentication until requested by endpoint Many endpoints do not require user authentication, and by moving the logic out of the middleware layer these endpoints now avoid obtaining a database connection from the pool and 1 or 2 queries. Due to lifetime issues a small middlware component remains. Further details are described in the middleware's documentation. r? @carols10cents

bors · 2020-01-16T04:08:00Z

☀️ Test successful - checks-travis
Approved by: carols10cents
Pushing 63d45b6 to master...

Fixes rust-lang#2166. In rust-lang#2077 (d0b9bcc), we changed the type of the authentication data stored in the extensions from `User` to `TrustedUserId`, but we missed this spot :( We're allowed to insert anything we want into the extensions, but if we use a different type than what we use when looking up data in the extensions, it won't find what we inserted.

@jtgeibel

Fix auth Set the TrustedUserId in the extensions when logging in Fixes #2166. In #2077 (d0b9bcc), we changed the type of the authentication data stored in the extensions from `User` to `TrustedUserId`, but we missed this spot :( We're allowed to insert anything we want into the extensions, but if we use a different type than what we use when looking up data in the extensions, it won't find what we inserted. I checked all the other locations of `mut_extensions` and `extensions` and I think they're good, but overall this system feels a bit loosey-goosey to me. Not sure how to improve it, though. r? @jtgeibel

rust-highfive assigned carols10cents Dec 28, 2019

rust-highfive added the S-waiting-on-review label Dec 28, 2019

Turbo87 approved these changes Dec 30, 2019

View reviewed changes

carols10cents requested changes Jan 3, 2020

View reviewed changes

jtgeibel force-pushed the defer-user-auth branch 2 times, most recently from 6458a59 to d13d22a Compare January 4, 2020 03:14

jtgeibel force-pushed the defer-user-auth branch from d13d22a to b3281f0 Compare January 6, 2020 03:08

jtgeibel added 2 commits January 15, 2020 22:52

Address PR review comments

c06e7c2

jtgeibel force-pushed the defer-user-auth branch from b3281f0 to c06e7c2 Compare January 16, 2020 03:54

bors merged commit c06e7c2 into rust-lang:master Jan 16, 2020

bors mentioned this pull request Jan 16, 2020

Use the read-only replica for some queries in production #2073

Merged

jtgeibel deleted the defer-user-auth branch January 24, 2020 02:09

carols10cents mentioned this pull request Jan 27, 2020

Fix auth #2167

Merged

jsha mentioned this pull request Jul 21, 2020

Remove current_user middleware in favor accessing cookie in authenticate #2651

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Defer user authentication until requested by endpoint #2077

Defer user authentication until requested by endpoint #2077

jtgeibel commented Dec 28, 2019

Turbo87 left a comment

Turbo87 Dec 30, 2019

carols10cents Jan 3, 2020 •

edited

Loading

jtgeibel Jan 4, 2020

carols10cents left a comment

carols10cents Jan 3, 2020

jtgeibel Jan 4, 2020

carols10cents Jan 3, 2020 •

edited

Loading

carols10cents Jan 3, 2020

carols10cents Jan 3, 2020

carols10cents Jan 3, 2020

jtgeibel Jan 4, 2020

bors commented Jan 4, 2020

jtgeibel commented Jan 4, 2020

bors commented Jan 6, 2020

bors commented Jan 15, 2020

jtgeibel commented Jan 16, 2020

bors commented Jan 16, 2020

bors commented Jan 16, 2020

bors commented Jan 16, 2020

Defer user authentication until requested by endpoint #2077

Defer user authentication until requested by endpoint #2077

Conversation

jtgeibel commented Dec 28, 2019

Turbo87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carols10cents Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

carols10cents Jan 3, 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

bors commented Jan 4, 2020

jtgeibel commented Jan 4, 2020

bors commented Jan 6, 2020

bors commented Jan 15, 2020

jtgeibel commented Jan 16, 2020

bors commented Jan 16, 2020

bors commented Jan 16, 2020

bors commented Jan 16, 2020

carols10cents Jan 3, 2020 •

edited

Loading

carols10cents Jan 3, 2020 •

edited

Loading