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

Feature/bca/verif resist no age #6328

Merged
merged 5 commits into from
Jun 28, 2022
Merged

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented Jun 16, 2022

Type of change

  • Bugfix

Content

Under certain condition, incoming verification are displayed in the UI without Accept/Decline button.
image

Making it impossible to continue the verification.

This PR fixes the case when a verification request is ignored because deemed too in the past or too in the future.
This computation is based on ageLocalTs, that is computed from the unsigned.age. The processor and the timeline display had different test, the timeline was accepting null age and not processor.

To fix it, we now ensure that ageLocalTs is set even if no unsigned.age in the synced event.

I noticed that it was also possible to open the bottom sheet from typing on the verification tile and not on one of the button. I modified the bottom sheet to show accept/decline buttons when the request is not yet ready.
image

Motivation and context

Screenshots / GIFs

Tests

Tested against polyjuice for regressions.

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@BillCarsonFr BillCarsonFr force-pushed the feature/bca/verif_resist_no_age branch 2 times, most recently from 111869d to 6cc1fce Compare June 16, 2022 15:40
@BillCarsonFr BillCarsonFr requested review from a team, mnaturel and fedrunov and removed request for a team June 27, 2022 08:21
@BillCarsonFr BillCarsonFr force-pushed the feature/bca/verif_resist_no_age branch from d858ed7 to 9929d6a Compare June 27, 2022 08:23
@BillCarsonFr
Copy link
Member Author

related to matrix-org/synapse#8429

@BillCarsonFr BillCarsonFr added Z-NextRelease For issues and PRs which should be included in the NextRelease. A-E2EE-SAS-Verification labels Jun 27, 2022
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Some remarks, since this is "NextRelease", I have made a review. Did not test the code though.

android:layout_height="wrap_content"
android:gravity="center_horizontal"
android:orientation="horizontal"
tools:visibility="visible">
Copy link
Member

Choose a reason for hiding this comment

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

tools:visibility can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@mnaturel mnaturel left a comment

Choose a reason for hiding this comment

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

Good for me though I didn't correctly understand how to test and if is possible to test. I tried triggering some session verifications but I have not the same UI with simple "Accept"/"Decline" bottom sheet. So I am not sure this is the correct tests.

Not related, but I simply noticed for a while the top bar notification to trigger a verification is not displayed just after the first sign in. You need to restart the app to see it, I don't know if it was an intended change.

@sonarcloud
Copy link

sonarcloud bot commented Jun 28, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

25.4% 25.4% Coverage
0.0% 0.0% Duplication

android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginEnd="16dp"
tools:text="@string/action_decline" />
Copy link
Member

Choose a reason for hiding this comment

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

OK to use tools but in this case, I think that positiveText and destructiveText should not be nullable. (see https://github.com/vector-im/element-android/pull/6328/files#diff-e03d15165cd3cc55c597e31b0494b0088b8ef56da1edb7468b870973d2948265R35)

This is not a blocker though.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

@BillCarsonFr BillCarsonFr merged commit a2aa047 into develop Jun 28, 2022
@BillCarsonFr BillCarsonFr deleted the feature/bca/verif_resist_no_age branch June 28, 2022 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-E2EE-SAS-Verification Z-NextRelease For issues and PRs which should be included in the NextRelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants