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

FFI: Expose is_verified UserIdentity method and request_user_identity Encryption method. #4142

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pixlwave
Copy link
Member

Nothing exciting here, just exposing another method.

@pixlwave pixlwave requested a review from a team as a code owner October 16, 2024 16:54
@pixlwave pixlwave requested review from bnjbvr and removed request for a team October 16, 2024 16:54
Comment on lines +465 to +504
/// Is the user identity considered to be verified.
///
/// If the identity belongs to another user, our own user identity needs to
/// be verified as well for the identity to be considered to be verified.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should just copy paste the underlying documentation instead of rolling a new one 🤔

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 did but removing the middle section as it seems overly technical from the perspective of an SDK user to me.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see, I was looking at the one inside the crypto crate which seemed just perfect. I'm just worried something will change on one level or another and then the documentation will be out of date.

Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Code looks good, I'm fine with either this code comment or one copied from the SDK. Thanks.

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.83%. Comparing base (08152bd) to head (afc2594).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4142      +/-   ##
==========================================
- Coverage   84.84%   84.83%   -0.01%     
==========================================
  Files         269      269              
  Lines       28793    28788       -5     
==========================================
- Hits        24429    24422       -7     
- Misses       4364     4366       +2     

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

///
/// If the identity belongs to another user, our own user identity needs to
/// be verified as well for the identity to be considered to be verified.
pub fn is_verified(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if this data should be live? Maybe not for now but in the future the app could be more reactive if we had a way to get a live UserIdentity.

Copy link
Member Author

@pixlwave pixlwave Oct 17, 2024

Choose a reason for hiding this comment

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

Yeah, I'd really like this to be a property on RoomMember that would update automatically through the room members stream if I'm honest, but I don't have a clue how to do that as the members live separately to the encryption 🫤

@BillCarsonFr
Copy link
Member

I am not sure of the full context here but if you expose is_verified() you probably want also to expose was_previously_verified() / has_verification_violation().

@pixlwave
Copy link
Member Author

pixlwave commented Oct 17, 2024

I am not sure of the full context here but if you expose is_verified() you probably want also to expose was_previously_verified() / has_verification_violation().

This is being added to show the green badge on the user profile screen (element-hq/element-meta#2569).

I'm unsure now, are you saying that it is possible for is_verified and has_verification_violation to both be true at the same time and we need to think about these together, or just that the other 2 might be useful?

@bmarty
Copy link
Contributor

bmarty commented Oct 17, 2024

(I confirm that the API works as expected on a EXA WIP branch)

@BillCarsonFr
Copy link
Member

I am not sure of the full context here but if you expose is_verified() you probably want also to expose was_previously_verified() / has_verification_violation().

This is being added to show the green badge on the user profile screen (element-hq/element-meta#2569).

I'm unsure now, are you saying that it is possible for is_verified and has_verification_violation to both be true at the same time and we need to think about these together, or just that the other 2 might be useful?

No has_verification_violation means that the user used to be verified, but is not anymore. (so should have a red badge, whereas a user that was never verified and is not should have no badge)

@pixlwave pixlwave changed the title FFI: Expose is_verified UserIdentity method. FFI: Expose is_verified UserIdentity method and request_user_identity Encryption method. Oct 18, 2024
@pixlwave
Copy link
Member Author

For now this is everything we need.

@pixlwave
Copy link
Member Author

@bnjbvr I exposed 1 more method to allow requesting identities for unknown users from the homeserver, otherwise I'm happy to have this merged if you're ok with it 🙂

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.

5 participants