Skip to content
This repository has been archived by the owner on Sep 10, 2024. It is now read-only.

Set password admin API #3021

Merged
merged 3 commits into from
Aug 7, 2024
Merged

Set password admin API #3021

merged 3 commits into from
Aug 7, 2024

Conversation

sandhose
Copy link
Member

@sandhose sandhose commented Jul 29, 2024

This adds an API to set the password of a user

Copy link

cloudflare-workers-and-pages bot commented Jul 29, 2024

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

Latest commit: 2ade8c2
Status: ✅  Deploy successful!
Preview URL: https://c00bf310.matrix-authentication-service-docs.pages.dev
Branch Preview URL: https://quenting-admin-api-set-passw.matrix-authentication-service-docs.pages.dev

View logs

@sandhose sandhose force-pushed the quenting/admin-api/set-password branch 2 times, most recently from 4d6732b to b2dbcb0 Compare July 29, 2024 11:50
@sandhose sandhose changed the base branch from main to quenting/admin-api/temp-merge-base July 29, 2024 11:51
@sandhose sandhose added the A-Admin-API Related to the admin API label Jul 29, 2024
@sandhose sandhose force-pushed the quenting/admin-api/set-password branch from b2dbcb0 to 6dd674b Compare July 30, 2024 09:50
@sandhose sandhose force-pushed the quenting/admin-api/temp-merge-base branch from f382846 to cc10495 Compare July 30, 2024 15:48
@sandhose sandhose force-pushed the quenting/admin-api/set-password branch 2 times, most recently from d8406c5 to df12feb Compare August 1, 2024 14:31
@sandhose sandhose changed the base branch from quenting/admin-api/temp-merge-base to main August 1, 2024 14:32
@sandhose sandhose force-pushed the quenting/admin-api/set-password branch from df12feb to fa35020 Compare August 1, 2024 14:39
@sandhose sandhose marked this pull request as ready for review August 1, 2024 14:40
@sandhose sandhose requested a review from reivilibre August 1, 2024 14:40
@sandhose sandhose mentioned this pull request Aug 2, 2024
13 tasks
.lookup(*id)
.await?
.ok_or(RouteError::NotFound(*id))?;

Copy link
Contributor

Choose a reason for hiding this comment

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

is it intended that we don't apply any complexity limits on this API?

also, it may be worth checking the password isn't empty either. I feel like that is dodgy enough to protect against

Copy link
Member Author

Choose a reason for hiding this comment

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

I was assuming that because this is the admin API, it should just accept it? Synapse does that (and I don't think it checks for empty passwords either)

But I'd be happy to make it check here and have a skip_password_check parameter or something similar

Copy link
Member Author

Choose a reason for hiding this comment

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

I ended up checking the complexity and adding the flag

@sandhose sandhose force-pushed the quenting/admin-api/set-password branch from fa35020 to 4dbb5eb Compare August 6, 2024 13:39
@sandhose sandhose requested a review from reivilibre August 6, 2024 15:09
@sandhose sandhose force-pushed the quenting/admin-api/set-password branch from 0be3cb8 to 9551f55 Compare August 6, 2024 15:10
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.

other than one minor thing SGTM thanks :)

Comment on lines +112 to +115
.is_password_complex_enough(&params.password)
.unwrap_or(false)
{
return Err(RouteError::PasswordTooWeak);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will give a PasswordTooWeak error code if the password manager is disabled, which could be a bit misleading, should we have an error code just for it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh you're right! Did that in 87482b4

@sandhose sandhose force-pushed the quenting/admin-api/set-password branch from 87482b4 to aec9653 Compare August 7, 2024 11:00
@sandhose sandhose force-pushed the quenting/admin-api/set-password branch from aec9653 to 2ade8c2 Compare August 7, 2024 12:51
@sandhose sandhose merged commit c61a52a into main Aug 7, 2024
16 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Admin-API Related to the admin API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants