-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: create use case for getting e2ei certificate (WPB-3214) #2129
Conversation
…te_usecase # Conflicts: # logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/UserSessionScope.kt
Codecov Report
@@ Coverage Diff @@
## develop #2129 +/- ##
=============================================
+ Coverage 57.88% 57.90% +0.01%
Complexity 21 21
=============================================
Files 1059 1064 +5
Lines 40190 40258 +68
Branches 3722 3729 +7
=============================================
+ Hits 23265 23311 +46
- Misses 15314 15336 +22
Partials 1611 1611
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ❌ 2 Total Test Services: 1 Failed, 0 with New Flaky, 1 Passed Test Services
❌ Failed Tests (1)
|
...mmonMain/kotlin/com/wire/kalium/logic/feature/e2ei/usecase/DownloadE2eiCertificateUseCase.kt
Outdated
Show resolved
Hide resolved
logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/e2ei/E2eiCertificate.kt
Show resolved
Hide resolved
...ommonJvmAndroid/kotlin/com/wire/kalium/logic/feature/e2ei/decodePemCertificate.JvmAndroid.kt
Outdated
Show resolved
Hide resolved
enum class CertificateStatus { | ||
REVOKED, | ||
EXPIRED, | ||
VALID |
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.
suggestion: either here as 'Valid(until: Date)' or in the certificate details the expiresAt... to the properties. we need it atleast for the current client's renewal button.
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'd say let's add it when we need it
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, please add tests for the decoder and the usecase.
Datadog ReportAll test runs ✅ 2 Total Test Services: 0 Failed, 0 with New Flaky, 2 Passed Test Services
|
fun getE2eiCertificate(clientId: ClientId): Either<E2EIFailure, String> | ||
} | ||
|
||
class E2eiCertificateRepositoryImpl : E2eiCertificateRepository { |
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.
suggestion: instead of having a new repository, you an move this function to the E2EIRepository we have at the moment.
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.
Actually E2EIRepository is already doing too much, I think it's better to have a newer one than having a God class.
package com.wire.kalium.logic.feature.e2ei | ||
|
||
actual interface CertificateStatusChecker { | ||
actual fun status(notAfterTimestamp: Long): CertificateStatus |
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.
question: why we need to pass the time? how is not the current system time?
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.
We are getting current time in CertificateStatusChecker implementation and we compare it with this passed one to know if it's expired or not
class PemCertificateDecoderTest { | ||
|
||
@Test | ||
fun givenAValidCertificate_whenDecodingIt_thenReturnCertificateObject() { |
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.
question: can you add an actual comparison here on the data you expect? based on the PEM string you passed.
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.
Looking good 👍, added some other questions and suggestions.
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764
The PR Description
What's new in this PR?
Description
In this PR I created
bouncy-castle
dependency for JVMDependencies (Optional)
If there are some other pull requests related to this one (e.g. new releases of frameworks), specify them here.
Needs releases with:
Testing
Test Coverage (Optional)
How to Test
Briefly describe how this change was tested and if applicable the exact steps taken to verify that it works as expected.
Notes (Optional)
Specify here any other facts that you think are important for this issue.
Attachments (Optional)
Attachments like images, videos, etc. (drag and drop in the text box)
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764
.