Skip to content

Conversation

sandhose
Copy link
Member

This adds 3 endpoints to finish sessions, one for each kind of session.

Copy link

cloudflare-workers-and-pages bot commented Sep 30, 2025

Deploying matrix-authentication-service-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7e3df55
Status: ✅  Deploy successful!
Preview URL: https://09cdcbe5.matrix-authentication-service-docs.pages.dev
Branch Preview URL: https://quenting-admin-api-sessions.matrix-authentication-service-docs.pages.dev

View logs

@sandhose sandhose added the A-Admin-API Related to the admin API label Sep 30, 2025
@sandhose sandhose force-pushed the quenting/admin-api/sessions-finish branch from 1f72136 to b856c88 Compare September 30, 2025 14:50
@sandhose sandhose requested a review from reivilibre October 7, 2025 15:31
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine overall

repo.save().await?;

Ok(Json(SingleResponse::new(
CompatSession::from((session, sso_login)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slight eyebrow raise at CompatSession::from((CompatSession, Option<CompatSsoLogin>)) but I guess it's OK and matches what we do in other places

// If the session has a user associated with it, schedule a job to sync devices
if let Some(user_id) = session.user_id {
tracing::info!(user.id = %user_id, "Scheduling device sync job for user");
let job = SyncDevicesJob::new_for_id(user_id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weirdly inconsistent with how it's done for CompatSessions; why use the db-loaded User in one place and the ID in the other?

I think just using the user_id should win, though; should be no need to verify the user exists in the DB given the FK constraint should do that for us

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good q, dunno why I did that on the compat sessions. Fixed that in 7e3df55

@sandhose sandhose enabled auto-merge October 8, 2025 08:24
@sandhose sandhose merged commit cd39513 into main Oct 8, 2025
20 checks passed
@sandhose sandhose deleted the quenting/admin-api/sessions-finish branch October 8, 2025 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Admin-API Related to the admin API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants