WIP: Implement sessions database table
#3307
Conversation
There is no need for us to convert the header value to a `String` here when `ApiToken::find_by_api_token()` only needs a `&str`
This allows us to give more precise error messages and enables further simplification
Turbo87
added
C-enhancement ✨
| user_id INTEGER NOT NULL | ||
| CONSTRAINT sessions_users_id_fk | ||
| REFERENCES users | ||
| ON UPDATE CASCADE ON DELETE CASCADE, |
There was a problem hiding this comment.
You'll probably want to delete stale sessions eventually. In a big database, deleting old rows can be relatively expensive. For this, you might want to use partitioning (https://www.postgresql.org/docs/10/ddl-partitioning.html), which lets you drop old, stale partitions cheaply while still treating the partitions as a single table. However, to use partitioning you need to have no foreign keys on the partitioned table. Suggestion: Make user_id not a foreign key. We don't expect that user_ids change, so we don't need ON UPDATE CASCADE. And if the users entry is deleted, we can simply treat the session as invalid. So we don't need ON DELETE CASCADE.
| // try to revoke the session in the database, but explicitly | ||
| // ignore failures | ||
| let _result: Result<_, Box<dyn AppError>> = req | ||
| .db_conn() | ||
| .map_err(Into::into) | ||
| .and_then(|conn| Session::revoke_by_token(&conn, &token).map_err(Into::into)); |
There was a problem hiding this comment.
Perhaps log or increment a stat in this case? Or have a set of expected errors (like timeout) and pass through unexpected errors (like permission denied or malformed SQL).
| // If the database is in read only mode, we can't update these fields. | ||
| // Try updating in a new transaction, if that fails, fall back to reading | ||
| conn.transaction(|| { | ||
| diesel::update(sessions) | ||
| .set(( | ||
| sessions::last_used_at.eq(diesel::dsl::now), | ||
| sessions::last_ip_address.eq(IpNetwork::from(ip_address)), | ||
| sessions::last_user_agent.eq(user_agent), | ||
| )) | ||
| .get_result(conn) | ||
| .optional() | ||
| }) | ||
| .or_else(|_| sessions.first(conn).optional()) |
There was a problem hiding this comment.
It's probably a good idea to do the update only if:
- The IP address has changed, or
- The User-Agent has changed, or
- The last_used_at has increased by more than 15 minutes.
This will save you from having to do a DB write on every cookie-authenticated request.
Simplify `authenticate_user()` function The function was a bit hard to read and difficult to extend (see #3307). This PR simplifies it a little bit by returning early if a suitable authentication method was found. r? `@jtgeibel`
|
☔ The latest upstream changes (presumably #3328) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Filed a request to the foundation for the additional data collection. |
|
closing this for now due to all the conflicts and the foundation not having given us an answer yet wether we can do this or not... 🤷♂️ |
As discussed in the team meeting last week, this PR implements a
sessionstable in the database to address #2630.This is still work-in-progress, but feel free to comment if you have any concerns.
/cc @jsha @jtgeibel