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

Delete unused client information from account data (PSG-871) #7754

Merged
merged 6 commits into from
Dec 13, 2022

Conversation

onurays
Copy link
Contributor

@onurays onurays commented Dec 9, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

With new session manager implementation we started to store information of user devices in account data. If user logouts / delete a session we need to delete the corresponding client information to keep the account data as clean as possible. Backend has now a new endpoint to be able to delete account data events. This PR implements this feature.

Motivation and context

Implement MSC-3391

Screenshots / GIFs

Tests

  • Enable client info recording from Labs settings
  • Login from another client
  • Debug or use Postman to see the new session is recorded: account_data/io.element.matrix_client_information.DEVICE
  • Logout from this client
  • Check account data to see this record is deleted from account data

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@ElementBot
Copy link

Warnings
⚠️

Please add a changelog. See instructions here

Generated by 🚫 dangerJS against 22cce30

@ElementBot
Copy link

Warnings
⚠️

Please add a changelog. See instructions here

Generated by 🚫 dangerJS against 7a667b5

@ElementBot
Copy link

Warnings
⚠️

Please add a changelog. See instructions here

Generated by 🚫 dangerJS against 85a6c8c

@onurays onurays marked this pull request as ready for review December 9, 2022 16:56
@onurays onurays requested review from a team and mnaturel and removed request for a team December 9, 2022 16:56
)
}

internal class DefaultDeleteUserAccountDataTask @Inject constructor(
Copy link
Contributor

Choose a reason for hiding this comment

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

We could add unit tests on this class.

return realmSessionProvider.withRealm { realm ->
realm
.where(UserAccountDataEntity::class.java)
.contains(UserAccountDataEntityFields.TYPE, type)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use beginsWith instead of contains if we want to reflect the method name.

import org.matrix.android.sdk.api.session.crypto.model.DeviceInfo
import javax.inject.Inject

class DeleteUnusedClientInformationUseCase @Inject constructor(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should put this class in the package core.session.clientinfo with the other classes about MatrixClientInfo.

* limitations under the License.
*/

package im.vector.app.features.settings.devices.v2
Copy link
Contributor

Choose a reason for hiding this comment

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

This test class should also be moved to core.session.clientinfo package.

private val activeSessionHolder: ActiveSessionHolder,
) {

suspend fun execute(deviceInfoList: List<DeviceInfo>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should wrap the implementation using a runCatching and return a Result to avoid having the parent coroutine scope to be cancelled in case of network error (e.g. delete api not supported)?

@@ -102,6 +105,9 @@ class UnknownDeviceDetectorSharedViewModel @AssistedInject constructor(
) { cryptoList, infoList, pInfo ->
// Timber.v("## Detector trigger ${cryptoList.map { "${it.deviceId} ${it.trustLevel}" }}")
// Timber.v("## Detector trigger canCrossSign ${pInfo.get().selfSigned != null}")

deleteUnusedClientInformation(infoList)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we should add a unit test on this ViewModel to cover this use case.

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.

I have added some small comments.
Can you also fill up the PR description and add a changelog entry?

Base automatically changed from feature/ons/remove_account_data to develop December 12, 2022 16:31
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.

I didn't test it but LGTM! Thanks for the updates!

@sonarcloud
Copy link

sonarcloud bot commented Dec 12, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

82.8% 82.8% Coverage
0.0% 0.0% Duplication

@onurays onurays merged commit 250bd9c into develop Dec 13, 2022
@onurays onurays deleted the feature/ons/remove_client_information_account_data branch December 13, 2022 08:10
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.

3 participants