refactor(oauth): Introduce `AccountManagementUrlBuilder` by zecakeh · Pull Request #4831 · matrix-org/matrix-rust-sdk

zecakeh · 2025-03-22T15:24:12Z

It allows to reuse the URL for different actions more easily than having to call OAuth::account_management_url every time for a different action.

AccountManagementUrlBuilder::build_or_ignore_action() also allows to ignore errors when serializing actions to always have a URL to use (although I'm not sure how to get errors there).

codecov · 2025-03-22T15:38:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.52%. Comparing base (aa07108) to head (13fd7d0).
Report is 29 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4831      +/-   ##
==========================================
+ Coverage   86.50%   86.52%   +0.01%     
==========================================
  Files         297      297              
  Lines       34600    34607       +7     
==========================================
+ Hits        29931    29942      +11     
+ Misses       4669     4665       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

It allows to reuse the URL for different actions more easily than having to call `OAuth::account_management_url` every time for a different action. It also adds a method with fallback if we want to ignore action serialization errors, to always present a URL. Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

zecakeh · 2025-03-24T13:24:21Z

Rebased on main to solve conflicts.

poljar

Thanks, I left some small nits.

Could you clarify what you mean by:

AccountManagementUrlBuilder::build_or_ignore_action() also allows to ignore errors when serializing actions to always have a URL to use (although I'm not sure how to get errors there).

Do you mean that the serialization should be infallible?

poljar · 2025-03-26T09:20:21Z

+        match url_builder.build() {
+            Ok(url) => Ok(Some(url.to_string())),
+            Err(e) => {
+                tracing::error!("Failed to build account management URL: {e}");


Let's import error!() if it's used in multiple places.

Changed. It was already imported 😛.

poljar · 2025-03-26T09:30:39Z

+/// Builder for the URL for accessing the account management capabilities, as
+/// defined in [MSC4191].
 ///
-/// # Arguments
+/// This type can be instantiated with [`OAuth::account_management_url()`] and
+/// [`OAuth::fetch_account_management_url()`].
 ///
-/// * `account_management_uri` - The URL to access the issuer's account
-///   management capabilities.
-///
-/// * `action` - The action that the user wishes to take.
-///
-/// # Returns
-///
-/// A URL to be opened in a web browser where the end-user will be able to
-/// access the account management capabilities of the issuer.
-///
-/// # Errors
-///
-/// Returns an error if serializing the action fails.
+/// [`AccountManagementUrlBuilder::build()`] and
+/// [`AccountManagementUrlBuilder::build_or_ignore_action()`] return a URL to be
+/// opened in a web browser where the end-user will be able to access the
+/// account management capabilities of the issuer.


Can we get an example here or on the method that returns this struct? There's now enough indirection for this to warrant some examples.

I added an example.

zecakeh · 2025-03-26T10:05:06Z

Could you clarify what you mean by:

AccountManagementUrlBuilder::build_or_ignore_action() also allows to ignore errors when serializing actions to always have a URL to use (although I'm not sure how to get errors there).

Do you mean that the serialization should be infallible?

It means that I am not sure when serialization in serde_html_form would fail, I couldn't find anything in the docs about it.

From what I understand, it should not be possible for it to fail though. The only variable input is the DeviceId which can be any string, and according to the x-www-form-urlencoded serializing rules, it should be possible to serialize any scalar value string. According to the Rust docs, chars are Unicode scalar values, so there shouldn't be a problem there.

poljar · 2025-03-26T10:11:13Z

Could you clarify what you mean by:

AccountManagementUrlBuilder::build_or_ignore_action() also allows to ignore errors when serializing actions to always have a URL to use (although I'm not sure how to get errors there).

Do you mean that the serialization should be infallible?

It means that I am not sure when serialization in serde_html_form would fail, I couldn't find anything in the docs about it.

From what I understand, it should not be possible for it to fail though. The only variable input is the DeviceId which can be any string, and according to the x-www-form-urlencoded serializing rules, it should be possible to serialize any scalar value string. According to the Rust docs, chars are Unicode scalar values, so there shouldn't be a problem there.

Ah ok, yeah it seems like it should™ be infallible to me as well. But let us be cautious here.

zecakeh · 2025-03-26T12:58:20Z

Actually, there is a way to avoid the error if we don't use serde_html_form at all, but instead use Url::query_pairs_mut and append the key-value pairs manually.

It's already imported. Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

zecakeh · 2025-03-26T13:43:22Z

I got rid of potential errors by doing as I said.

poljar · 2025-03-26T14:25:32Z

I got rid of potential errors by doing as I said.

Ah neat.

poljar

Nice, looks good.

zecakeh requested a review from a team as a code owner March 22, 2025 15:24

zecakeh requested review from poljar and removed request for a team March 22, 2025 15:24

zecakeh force-pushed the account-management-url-builder branch from c9257d6 to 23f5d6f Compare March 23, 2025 10:56

zecakeh force-pushed the account-management-url-builder branch from 23f5d6f to 46da41e Compare March 24, 2025 13:24

poljar requested changes Mar 26, 2025

View reviewed changes

zecakeh added 2 commits March 26, 2025 14:04

Don't qualify error! macro

c854ba6

It's already imported. Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

Don't use serde to avoid returning an error

13fd7d0

Signed-off-by: Kévin Commaille <zecakeh@tedomum.fr>

zecakeh requested a review from poljar March 26, 2025 13:43

poljar approved these changes Mar 26, 2025

View reviewed changes

poljar merged commit e0e9c06 into matrix-org:main Mar 26, 2025
42 checks passed

zecakeh deleted the account-management-url-builder branch March 26, 2025 16:59

Conversation

zecakeh commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zecakeh commented Mar 24, 2025

Uh oh!

poljar left a comment

Choose a reason for hiding this comment

Uh oh!

poljar Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

zecakeh Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

poljar Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

zecakeh Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

zecakeh commented Mar 26, 2025

Uh oh!

poljar commented Mar 26, 2025

Uh oh!

zecakeh commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zecakeh commented Mar 26, 2025

Uh oh!

poljar commented Mar 26, 2025

Uh oh!

poljar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zecakeh commented Mar 22, 2025 •

edited

Loading

codecov Bot commented Mar 22, 2025 •

edited

Loading

zecakeh commented Mar 26, 2025 •

edited

Loading