-
Notifications
You must be signed in to change notification settings - Fork 56
Add storage for Personal Access Tokens #5106
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
Changes from 9 commits
1519de2
87950bb
b54a657
6dfa0e3
0619f83
b6d8cdb
6aa483a
9f78061
72d3ea8
277e8e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// Copyright 2025 New Vector Ltd. | ||
// | ||
// SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial | ||
// Please see LICENSE files in the repository root for full details. | ||
|
||
pub mod session; | ||
|
||
use chrono::{DateTime, Utc}; | ||
use ulid::Ulid; | ||
|
||
#[derive(Debug, Clone, PartialEq, Eq)] | ||
pub struct PersonalAccessToken { | ||
pub id: Ulid, | ||
pub session_id: Ulid, | ||
pub created_at: DateTime<Utc>, | ||
pub expires_at: Option<DateTime<Utc>>, | ||
pub revoked_at: Option<DateTime<Utc>>, | ||
} | ||
|
||
impl PersonalAccessToken { | ||
#[must_use] | ||
pub fn is_valid(&self, now: DateTime<Utc>) -> bool { | ||
if self.revoked_at.is_some() { | ||
return false; | ||
} | ||
if let Some(expires_at) = self.expires_at { | ||
expires_at > now | ||
} else { | ||
true | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,132 @@ | ||
// Copyright 2025 New Vector Ltd. | ||
// | ||
// SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial | ||
// Please see LICENSE files in the repository root for full details. | ||
|
||
use std::net::IpAddr; | ||
|
||
use chrono::{DateTime, Utc}; | ||
use oauth2_types::scope::Scope; | ||
use serde::Serialize; | ||
use ulid::Ulid; | ||
|
||
use crate::{Client, InvalidTransitionError, User}; | ||
|
||
#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize)] | ||
pub enum SessionState { | ||
#[default] | ||
Valid, | ||
Revoked { | ||
revoked_at: DateTime<Utc>, | ||
}, | ||
} | ||
|
||
impl SessionState { | ||
/// Returns `true` if the session state is [`Valid`]. | ||
/// | ||
/// [`Valid`]: SessionState::Valid | ||
#[must_use] | ||
pub fn is_valid(&self) -> bool { | ||
matches!(self, Self::Valid) | ||
} | ||
|
||
/// Returns `true` if the session state is [`Revoked`]. | ||
/// | ||
/// [`Revoked`]: SessionState::Revoked | ||
#[must_use] | ||
pub fn is_revoked(&self) -> bool { | ||
matches!(self, Self::Revoked { .. }) | ||
} | ||
|
||
/// Transitions the session state to [`Revoked`]. | ||
/// | ||
/// # Parameters | ||
/// | ||
/// * `revoked_at` - The time at which the session was revoked. | ||
/// | ||
/// # Errors | ||
/// | ||
/// Returns an error if the session state is already [`Revoked`]. | ||
/// | ||
/// [`Revoked`]: SessionState::Revoked | ||
pub fn revoke(self, revoked_at: DateTime<Utc>) -> Result<Self, InvalidTransitionError> { | ||
match self { | ||
Self::Valid => Ok(Self::Revoked { revoked_at }), | ||
Self::Revoked { .. } => Err(InvalidTransitionError), | ||
} | ||
} | ||
|
||
/// Returns the time the session was revoked, if any | ||
/// | ||
/// Returns `None` if the session is still [`Valid`]. | ||
/// | ||
/// [`Valid`]: SessionState::Valid | ||
#[must_use] | ||
pub fn revoked_at(&self) -> Option<DateTime<Utc>> { | ||
match self { | ||
Self::Valid => None, | ||
Self::Revoked { revoked_at } => Some(*revoked_at), | ||
} | ||
} | ||
} | ||
|
||
#[derive(Debug, Clone, PartialEq, Eq, Serialize)] | ||
pub struct PersonalSession { | ||
pub id: Ulid, | ||
pub state: SessionState, | ||
pub owner: PersonalSessionOwner, | ||
pub actor_user_id: Ulid, | ||
pub human_name: String, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if this should be optional… but I guess most tools force you to have a name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was my thought really; other tools make you put in a name and it seems self-sabotaging to not have a label for your PATs, so it seems like a good idea to just have one for simplicity. |
||
/// The scope for the session, identical to OAuth 2 sessions. | ||
/// May or may not include a device scope | ||
/// (personal sessions can be deviceless). | ||
pub scope: Scope, | ||
pub created_at: DateTime<Utc>, | ||
pub last_active_at: Option<DateTime<Utc>>, | ||
pub last_active_ip: Option<IpAddr>, | ||
} | ||
|
||
#[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize)] | ||
pub enum PersonalSessionOwner { | ||
/// The personal session is owned by the user with the given `user_id`. | ||
User(Ulid), | ||
/// The personal session is owned by the OAuth 2 Client with the given | ||
/// `oauth2_client_id`. | ||
OAuth2Client(Ulid), | ||
} | ||
|
||
impl<'a> From<&'a User> for PersonalSessionOwner { | ||
fn from(value: &'a User) -> Self { | ||
PersonalSessionOwner::User(value.id) | ||
} | ||
} | ||
|
||
impl<'a> From<&'a Client> for PersonalSessionOwner { | ||
fn from(value: &'a Client) -> Self { | ||
PersonalSessionOwner::OAuth2Client(value.id) | ||
} | ||
} | ||
|
||
impl std::ops::Deref for PersonalSession { | ||
type Target = SessionState; | ||
|
||
fn deref(&self) -> &Self::Target { | ||
&self.state | ||
} | ||
} | ||
|
||
impl PersonalSession { | ||
/// Marks the session as revoked. | ||
/// | ||
/// # Parameters | ||
/// | ||
/// * `revoked_at` - The time at which the session was finished. | ||
/// | ||
/// # Errors | ||
/// | ||
/// Returns an error if the session is already finished. | ||
pub fn finish(mut self, revoked_at: DateTime<Utc>) -> Result<Self, InvalidTransitionError> { | ||
self.state = self.state.revoke(revoked_at)?; | ||
Ok(self) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -625,6 +625,11 @@ pub(crate) async fn post( | |
device_id: session.device.map(Device::into), | ||
} | ||
} | ||
|
||
TokenType::PersonalAccessToken => { | ||
// TODO | ||
return Err(RouteError::UnknownToken(TokenType::PersonalAccessToken)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A few things I have in mind: this needs to check that both the actor and the owner are still active. Deactivating (not locking) users should revoke all their tokens Validating those tokens should also be done for requests to the GraphQL endpoint (optional maybe, we want to deprecate 'admin' access to the GraphQL API) and for the admin API access There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noted down. Given our plans with GraphQL I'm tempted to say that we just don't accept PATs there; cuts off one more opportunity for people to get locked into a deprecated API. |
||
} | ||
}; | ||
|
||
repo.save().await?; | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm now thinking that potentially you could have no actor (like you don't need one when talking to the MAS admin API) but then the main reason for you to use PATs (and not just the client credentials grant) is if you need to access Synapse and MAS, so probably not worth supporting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also thought the same really. Seems like extra kerfuffle for no apparent benefit; could be relaxed later if it turns out to be useful though