QR Code stricter validation (EXPOSUREAPP-2570) #1127
Conversation
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/SubmissionConstants.kt
Outdated
Show resolved
Hide resolved
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/SubmissionService.kt
Outdated
Show resolved
Hide resolved
Corona-Warn-App/src/test/java/de/rki/coronawarnapp/service/submission/SubmissionServiceTest.kt
Outdated
Show resolved
Hide resolved
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/SubmissionConstants.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
The regexp should check for https://localhost?GUID (with a valid GUID being 3D6D08-3567F3F2-4DCF-43A3-8737-4CD1F87D6FDA). Anything else should be rejected. Please add corresponding unit tests to cover scenarios such as
- https://localhost/?3D6D08-3567F3F2-4DCF-43A3-8737-4CD1F87D6FDA -> pass
- https://localhost/%20?3D6D08-3567F3F2-4DCF-43A3-8737-4CD1F87D6FDA -> not pass
- https://some-host.com/?3D6D08-3567F3F2-4DCF-43A3-8737-4CD1F87D6FDA -> not pass
- https://localhost/?3567F3F2-4DCF-43A3-8737-4CD1F87D6FDA -> not pass
- https://localhost/?4CD1F87D6FDA -> not pass
anything else (e.g. existing test cases) should fail
These tests have been added. Existing test cases were failing, they're amended as well |
There was a problem hiding this comment.
Please check whether all the scanresult validation could be made part of a neat little data class, i.e.
data class ScanResult(val rawResult: String) {
val isValid by lazy {
// do checks here.
}
val guid: String? by lazy {
if(isValid) ... else null
}
companion object {
// related const stuff
}
}
fun validateAndStoreTestGUID(rawResult: String) {
val scanResult = ScanResult(rawResult)
if (scanResult.isValid) {
SubmissionService.storeTestGUID(scanResult.guid)
_scanStatus.value = Event(ScanStatus.SUCCESS)
} else {
_scanStatus.value = Event(ScanStatus.INVALID)
}
}
mlenkeit
left a comment
There was a problem hiding this comment.
LGTM from a requirement's perspective 👍 But I think it's worth having a fellow Engineer reviewing the code, too.
@d4rken Please check out the new logic. I have tried to make it easier and keep it simple at the same time. |
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/ScanResult.kt
Outdated
Show resolved
Hide resolved
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/ScanResult.kt
Outdated
Show resolved
Hide resolved
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/ScanResult.kt
Outdated
Show resolved
Hide resolved
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/ScanResult.kt
Outdated
Show resolved
Hide resolved
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/ScanResult.kt
Outdated
Show resolved
Hide resolved
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/service/submission/SubmissionService.kt
Outdated
Show resolved
Hide resolved
1, Data object for ScanResults 2. new Test for ScanResults 3. logic simplified
|
Kudos, SonarCloud Quality Gate passed!
|
Description
This PR implements the Stricter QR code validations
Earlier only the presence of GUID was checked, now the URL format will be checked as well