Skip to content

Conversation

oguzkocer
Copy link
Contributor

@oguzkocer oguzkocer commented Jul 1, 2024

This PR implements url discovery for authorization. There are quite a few parts to get this all working:

HeaderMap

We have been postponing the handling of headers for a while now. We attempted to improve it in #132, but then had to revert it in #135 as it conflicted with an ongoing complicated refactor. I've had to address this in this PR as properly parsing the headers is one of the requirements for finding the authorization url.

The PR introduces a new WpNetworkHeaderMap type that wraps http::HeaderMap. It provides ways to construct the type from a Map<String, String> & a Map<String, Vec<String>>. In both cases, the values are split by comma to multiple values, so this should accommodate all consumers. Rust consumers can directly construct this type from HeaderMap.

The Kotlin wrapper and Rust integration tests are both updated to properly parse the response headers.

Note that WpNetworkRequest has not been updated to use this new type yet. Since that'd be an unrelated change - and this PR is complicated enough as is - I've left that to be addresses in a later PR.

Separating Uniffi specific clients

This PR proposes a new approach for splitting Uniffi specific client types from the general Rust type. It introduces a UniffiWpLoginClient wrapper type alongside WpLoginClient. There are a few advantages to this:

  • We can make changes to the Rust type without breaking the exported type.
  • We can re-use the names to create a wrapper type in native clients. This is specifically useful for clients such as WpLoginClient and WpApiClient. I'd like to rename WpRequestBuilder to WpApiClient, but I can't at the moment, because that name is also used in the Kotlin wrapper. However, with this approach, I can do that by separating the exported client as UniffiWpApiClient.
  • We'll have increased flexibility in our Rust types because we won't have to directly deal with the FFI layer.

WpLoginClient

This new type currently only exposes one public function: api_discovery. Here is how that works:

  1. Takes a url string and constructs "attempts" from it. Currently these attempts will include the original string, one with https prefix if the string doesn't start with http, and one with wp-admin.php suffix if the string ends with wp-admin or wp-admin/. The attempts construction is naive and only deals with direct string manipulation.
  2. For every constructed url, it attempts to do a url discovery. It caries out all attempts at the same time, using futures::future::join_all.
  3. Inspects the results of attempts. If there was a successful attempt, it'll return a UrlDiscoverySuccess which contains the api details as well as all other attempts. If all attempts failed, it'll return an error containing information about all the attempts.

Url Discovery Attempt

Url discovery attempts are carried out using a simple state machine. There isn't too much benefit to this in terms of the implementation itself, however, the main goal of this approach is to be able to report detailed data about each attempt back to the consumer which will allow us to improve the url discovery process. The side benefit of this approach is its extensibility. It's very likely that we'll want to handle many more different cases and having a solid approach will give us a way to do that without making a spaghetti out of it.

Here is how it works:

  1. Parses the string url into ParsedUrl, returning an error if it can't parse it.
  2. Fetches the api root by making a HEAD request to it and parsing the headers to find "https://api.w.org/". If the request fails or if it can't find the header, it'll return an error.
  3. Fetches the api details by making a GET request to the found api root (from Step 2) and parses it as WpApiDetails. If the request fails or if it can't parse the response, it'll return an error.

Kotlin Wrapper Changes

  • Adds new WpLoginClient which allows us to keep the WpRequestExecutor internal. It also simplifies the construction of this client, not needing a request executor to be passed to it.
  • Updates the example app to use the api discovery from the login screen. It's a blocking call at the moment - but it's inline with the rest of the example app, and all of those blocking calls can be improved at once when we get a chance.

Unit & Integration Tests

Per usual, the PR adds a lot of new tests:

  • Unit tests for building WpNetworkHeaderMap from single and multi maps
  • Unit tests for constructing attempts
  • Integration tests for finding the correct authorization url for given cases
  • Kotlin integration test for finding the correct authorization url for the given url

To Test

  • make test-server && make dump-mysql && make backup-wp-content-plugins
  • cargo test --test '*' -- --test-threads 1
  • cd native/kotlin && ./gradlew :api:kotlin:integrationTest

@oguzkocer oguzkocer added this to the 0.1 milestone Jul 1, 2024
@oguzkocer oguzkocer marked this pull request as ready for review July 1, 2024 21:07
@oguzkocer oguzkocer requested a review from jkmassel July 1, 2024 21:07
@oguzkocer oguzkocer changed the title Url discovery Authorization Url Discovery Jul 2, 2024
// `test_credentials` file `make test-server` will generate in the root of the repo
// It's the 3rd line in that file
localTestSitePassword = "ncmRlxWjoaGdvMXyoXylhqGX"
localTestSitePassword = "l4dvgr1UjuCKXiqkbdbXn1b5"
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this be consistent between runs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope - the comment above localTestSitePassword explains the situation. I need to take some time to move the current integration tests to the example module and make the test_credentials file available to both the example app and the integration tests. I know how to do address it, but I need to spend time on it. Since this is low priority it's going to remain as such for a little while longer.

modifier = Modifier.fillMaxSize(),
) {
var siteUrl by remember { mutableStateOf("https://loudly-special.jurassic.ninja/") }
var siteUrl by remember { mutableStateOf("boldly-inner.jurassic.ninja") }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just to make it easier to run multiple tests without inputting the url each time. This input field would normally be left empty, but it's useful to have it populated for me during this active development stage.

I think we can add a user populated credentials file - or read it from gradle.properties or something - so each developer can use their own site to populate the field. Similar to the other comment, I know how to address it, but I don't think I should prioritize it.

@jkmassel jkmassel enabled auto-merge (squash) July 2, 2024 21:58
@jkmassel jkmassel merged commit 1791df3 into trunk Jul 2, 2024
@jkmassel jkmassel deleted the login-flow branch July 2, 2024 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants