Skip to content
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

Add an authentication service to the FFI #820

Merged
merged 7 commits into from
Jul 8, 2022
Merged

Conversation

pixlwave
Copy link
Member

@pixlwave pixlwave commented Jul 6, 2022

This PR adds a basic authentication service to the FFI that abstracts away the client until a login has been completed successfully. Additionally it adds

  • An authentication_issuer property to the SDK client for OIDC detection.

Always happy to learn of any changes that would make this into more idiomatic Rust :)

crates/matrix-sdk/Cargo.toml Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #820 (0dee880) into main (861d899) will decrease coverage by 0.07%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main     #820      +/-   ##
==========================================
- Coverage   77.99%   77.92%   -0.08%     
==========================================
  Files          92       92              
  Lines       13900    13936      +36     
==========================================
+ Hits        10841    10859      +18     
- Misses       3059     3077      +18     
Impacted Files Coverage Δ
crates/matrix-sdk/src/client/mod.rs 84.36% <0.00%> (-0.20%) ⬇️
crates/matrix-sdk/src/client/builder.rs 71.57% <80.00%> (+0.46%) ⬆️
...es/matrix-sdk-common/src/deserialized_responses.rs 66.66% <0.00%> (-20.52%) ⬇️
crates/matrix-sdk/src/room/invited.rs 50.00% <0.00%> (-7.15%) ⬇️
crates/matrix-sdk-base/src/rooms/members.rs 48.27% <0.00%> (-1.73%) ⬇️
crates/matrix-sdk-base/src/client.rs 78.15% <0.00%> (ø)
crates/matrix-sdk-crypto/src/olm/session.rs 100.00% <0.00%> (ø)
crates/matrix-sdk-store-encryption/src/lib.rs 97.58% <0.00%> (ø)
crates/matrix-sdk-crypto/src/olm/account.rs 82.33% <0.00%> (+0.04%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 861d899...0dee880. Read the comment docs.

@pixlwave pixlwave marked this pull request as ready for review July 6, 2022 10:33
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Left a couple of nits, overall looks sane.

crates/matrix-sdk/src/client/mod.rs Show resolved Hide resolved
bindings/matrix-sdk-ffi/src/authentication_service.rs Outdated Show resolved Hide resolved
bindings/matrix-sdk-ffi/src/client.rs Outdated Show resolved Hide resolved
let homeserver_url = client.homeserver();

// Create a new client to setup the store path for the username
let client = Arc::new(ClientBuilder::new())
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, so you create a Client with a fake user to get some metadata about the server. Then you set the correct user here. Am I getting this right?

It might make sense to modify the Client in the bindings to support memory only operation. I guess it isn't that important since the store will only be used once you log in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that's correct, it's totally a workaround 🙈. Jonas and Ben had a conversation around whether the client store should be set at all until after login has succeeded. Amongst other things this would help if the user ID returned by the homeserver differed to what was used to log in.

This all seemed like it was a larger thought than just the bindings to me, so I opted for this for now. However if its something I could address in this PR, would be happy to be given some direction and have a go 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Well we used to have an API where you just pass an optional store path to the Client, similarly to what the FFI Client does. Maybe we should just bring that back, of course only if the sled feature is enabled.

In any case, I'm not sure what we should do, so let's leave bigger changes around this for another time.

@pixlwave pixlwave requested a review from poljar July 7, 2022 16:29
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Looks good, and if it works for you then that's even better.

@poljar poljar merged commit f1c880f into main Jul 8, 2022
@poljar poljar deleted the doug/authentication-service branch July 8, 2022 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants