Remove usage of response intents from retrieve BBQ flow #132
Conversation
Fix for openid#131. In order to avoid the potential for code injection and request forgery attacks from malicious providers, this change deprecates the use of intents as part of the retrieve BBQ flow. Instead, providers now signal the availability of credentials via a simple boolean. Optionally, they can also specify preloaded data that the client should send back to them verbatim, as a replacement for the ability to specify intent extras on returned intents.
Codecov Report
@@ Coverage Diff @@
## master #132 +/- ##
===========================================
+ Coverage 71.85% 72.2% +0.35%
+ Complexity 482 476 -6
===========================================
Files 65 64 -1
Lines 2690 2634 -56
Branches 246 243 -3
===========================================
- Hits 1933 1902 -31
+ Misses 682 661 -21
+ Partials 75 71 -4
Continue to review full report at Codecov.
|
|
There are no tests for the |
| @@ -67,7 +65,7 @@ protected void processCredentialRequest( | |||
|
|
|||
| Intent retrieveIntent = RetrieveActivity.createIntent(applicationContext); | |||
| CredentialRetrieveBbqResponse response = CredentialRetrieveBbqResponse.newBuilder() | |||
| .setRetrieveIntent(ByteString.copyFrom(IntentUtil.toBytes(retrieveIntent))) | |||
There was a problem hiding this comment.
So Intent retrieveIntent = RetrieveActivity.createIntent(applicationContext); is not used anymore?
There was a problem hiding this comment.
I'd ignore the trapdoor code for now, I'm actually going to delete this provider as it's not a good example any more (it treats retrieve() like hint()).
| retrieveIntent.getComponent().getPackageName())) { | ||
| Log.w(LOG_TAG, "Package mismatch between provider and retrieve intent"); | ||
| continue; | ||
| Intent retrieveIntent = new Intent(ProtocolConstants.RETRIEVE_CREDENTIAL_ACTION); |
There was a problem hiding this comment.
We are deprecating retrieve intent, but here it will be just ignored. So we are breaking the compatibility if a provider don't have the action org.openyolo.credential.retrieve, as it was not require before (and it's my case).
I think we should have both, and prefer the action org.openyolo.credential.retrieve if the provider did set getCredentialsAvailable().
There was a problem hiding this comment.
OK, we'll support the old intent for 0.3.0 but please do add support for direct invocation of the retrieve flow ASAP. It's actually required in the spec:
http://openid.net/specs/openyolo-android-ID1.html#retrieving-credentials
There was a problem hiding this comment.
Yes you're right, I've made the change on my side. It will be live in ~ 2 weeks.
| // used. | ||
| // TODO: remove backwards compatibility with retrieve_intent after 0.3.0 | ||
| if (!response.getCredentialsAvailable() | ||
| && !response.getRetrieveIntent().isEmpty()) { |
There was a problem hiding this comment.
&& !response.getRetrieveIntent().isEmpty()
=> && response.getRetrieveIntent().isEmpty(), no?
There was a problem hiding this comment.
Removed as the code has been reordered to fit your request for using the old intent as a fallback.
Fix for #131. In order to avoid the potential for code injection and request forgery attacks
from malicious providers, this change deprecates the use of intents as part of the retrieve
BBQ flow. Instead, providers now signal the availability of credentials via a simple boolean.
Optionally, they can also specify preloaded data that the client should send back to them
verbatim, as a replacement for the ability to specify intent extras on returned intents.