[PM-26716] Validate credential exchange request #5994
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
SaintPatrck
requested review from
a team,
brian-livefront and
david-livefront
as code owners
SaintPatrck
force-pushed
the
cxf/app/validate-import-request
branch
2 times, most recently
from
88f5c94 to
1c79133
Compare
| as? SpecialCircumstance.CredentialExchangeExport | ||
|
|
||
| SelectAccountState( | ||
| importRequest = requireNotNull(importRequest?.data), |
There was a problem hiding this comment.
Couldn't this just be specialCircumstanceManager.specialCircumstance as SpecialCircumstance.CredentialExchangeExport
| .onEach(::handleAction) | ||
| .launchIn(viewModelScope) | ||
| sendEvent( | ||
| SelectAccountEvent.ValidateImportRequest( |
There was a problem hiding this comment.
So we send this here because the validator requires an activity context?
There was a problem hiding this comment.
Correct. The only mechanism we currently have to validate the request is by ensuring our activity was invoked by GMS. That requires a call to activity.callingPackage.
| SelectAccountAction.Internal.SelectionDataReceive( | ||
| userState, | ||
| itemRestrictedOrgs, | ||
| personalOwnershipOrgs, |
There was a problem hiding this comment.
Can we add some names to these params?
| */ | ||
| internal class CredentialExchangeImporterImpl( | ||
| private val activity: Context, | ||
| @param:VisibleForTesting |
| * @return `true` if the calling package is GMS, `false` otherwise. | ||
| */ | ||
| override fun validate( | ||
| importCredentialsRequestData: ImportCredentialsRequestData, |
There was a problem hiding this comment.
So we do not actually need the payload to do this check?
Also, if the activity is already open, will the callingPackage value be updated?
There was a problem hiding this comment.
Not at this time. Validations similar to those performed on passkey requests will be performed as the API evolves and makes the information available.
This commit introduces validation for incoming credential exchange requests to ensure they originate from a trusted source (Google Mobile Services). Previously, the app would immediately process any credential import request. This change adds a validation step at the beginning of the flow. If the request is not valid, an error screen is displayed to the user, preventing further processing. Specific changes: - Add `CredentialExchangeRequestValidator` to validate incoming import requests by checking the calling package. - Introduce a `CredentialExchangeRequestValidatorBuilder` and a corresponding DSL for easy instantiation. - Provide the validator via `LocalCredentialExchangeRequestValidator` CompositionLocal. - In `SelectAccountViewModel`, validate the request data upon initialization. If validation fails, transition to an error state. - Add an error state to the `SelectAccountScreen` to handle and display validation failures. - Update `ReviewExportViewModel` to rename `importCredentialsRequest` to `importCredentialsRequestData` for clarity. - Add a new string resource for the import request processing error message.
SaintPatrck
force-pushed
the
cxf/app/validate-import-request
branch
from
1c79133 to
fc61db8
Compare
|
Thanks @david-livefront! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5994 +/- ##
==========================================
+ Coverage 84.57% 84.58% +0.01%
==========================================
Files 720 721 +1
Lines 54678 54716 +38
Branches 7536 7540 +4
==========================================
+ Hits 46242 46283 +41
+ Misses 5802 5793 -9
- Partials 2634 2640 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🎟️ Tracking
PM-26716
📔 Objective
This commit introduces validation for incoming credential exchange requests to ensure they originate from a trusted source (Google Mobile Services).
Previously, the app would immediately process any credential import request. This change adds a validation step at the beginning of the flow. If the request is not valid, an error screen is displayed to the user, preventing further processing.
Specific changes:
CredentialExchangeRequestValidatorto validate incoming import requests by checking the calling package.CredentialExchangeRequestValidatorBuilderand a corresponding DSL for easy instantiation.LocalCredentialExchangeRequestValidatorCompositionLocal.SelectAccountViewModel, validate the request data upon initialization. If validation fails, transition to an error state.SelectAccountScreento handle and display validation failures.ReviewExportViewModelto renameimportCredentialsRequesttoimportCredentialsRequestDatafor clarity.📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes