-
Notifications
You must be signed in to change notification settings - Fork 864
Auto report unable to decrypt errors via lab option #4325
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
Conversation
73b143d to
f6dc2af
Compare
bmarty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small first remarks.
Weird to expose a ToDeviceService, but why not...
| import javax.inject.Inject | ||
|
|
||
| internal class DefaultToDeviceService @Inject constructor( | ||
| private val sendToDeviceTask: DefaultSendToDeviceTask, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type must be SendToDeviceTask
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| abstract fun bindToDeviceService(deviceService: DefaultToDeviceService): ToDeviceService | ||
|
|
||
| @Binds | ||
| abstract fun bindEventStreamService(deviceService: DefaultEventStreamService): EventStreamService |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be grouped with the other services. Also rename param to service (at 2 places)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| private val listeners = mutableListOf<LiveEventListener>() | ||
|
|
||
| fun addLiveEventListener(listener: LiveEventListener) { | ||
| Timber.v("## VALR: addLiveEventListener") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use logger tag instead of VALR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:) Updated, just a temporary log that I forgot to remove
7323137 to
cfa2f34
Compare
e9cf9e8 to
e56bb1a
Compare
| "device_id" to target.senderDeviceId, | ||
| "user_id" to target.senderUserId, | ||
| "sender_key" to target.senderKey, | ||
| "matching_issue" to reportUrl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
matching_issue is a confusing name for this field, I'd expect that to be a link to a github issue or something; how would you feel about something more like recipient_rageshake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, didn't realize the rageshake server actually does create a github issue and responds with that; I'd had that disabled in my tests, and assumed we were looking at the URL of the submitted rageshake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed matching_issue to recipient_rageshake.
Also align with the Z-UISI name for the issue label
793ac6b to
bbc7e4c
Compare
| android:defaultValue="false" | ||
| android:key="SETTINGS_LABS_USE_RESTRICTED_JOIN_RULE" | ||
| android:summary="@string/labs_use_restricted_join_rule_desc" | ||
| android:title="@string/labs_use_restricted_join_rule" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This removal will need more cleanup in the code, and not really related to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed will restore and creating separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR up here #4889
bmarty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I don't see any issue like that. Performance wise, it should be ok I guess as long as there is not too many listeners on EventStreamService.
Other way of doing that is exposing Flows of event.
Maybe we should at least make them suspend methods? (LiveEventListener)
bbc7e4c to
a047bcb
Compare
Ktlint Results👍 ✅ 👍 |
Fixes #4263
Adds a new lab option to enable auto reporting of fail to decrypt errors. When detected the device will send a rageshake and then send a toDevice message to the other participant to request him to also upload a RS.
The AutoRageShake and UISIs detection part has been put in the app side, and in order to do that I added to new services in the SDK:
@ganfra / @bmarty If you could have a quick look on the SDK part and give some feedbaks
Sample report
https://github.com/matrix-org/element-android-rageshakes/issues/27579