A major problem with this line of code
// Translate empty callback url to oob (out of band).
if (empty($values['callback_url'])) {
$values['callback_url'] = 'oob';
}
// Add scheme if missing, and if the callback_url isn't out of band.
else if ($values['callback_url'] != 'oob' && preg_match('/^http:\/\/|https:\/\//', $values['callback_url']) === 0) {
//TODO: What about custom callback url:s used by eg iphone-apps? We should allow them - right?
$values['callback_url'] = 'http://' . $values['callback_url'];
}
// Remove trailing slash
$values['callback_url'] = rtrim($values['callback_url'], '/');
Is that I cannot set a custom callback, just like the comment states.
The added patch removes the code where it automatically adds http if it doesnt exist. Ive also added a description so it states this to the user.
The only way around this is to export the Context to code and change the callback in that code, which is wonky, especially if I want it in the database.
Ive also updated the trailing slash code to only do this in cases where its not // so that I can have a callback of just iphoneappname:// Which is all iOS requires to get the notification that my request tokens are authorized.
Patch inc.
Comment | File | Size | Author |
---|---|---|---|
#10 | callback_optional-1809068-10.patch | 692 bytes | Deciphered |
#6 | 1809068-allow-custom-callbacks-2.patch | 2.01 KB | MrMaksimize |
#1 | 1809068-allow-custom-callbacks.patch | 1.42 KB | kylebrowning |
Comments
Comment #1
kylebrowning CreditAttribution: kylebrowning commentedComment #2
kylebrowning CreditAttribution: kylebrowning commentedComment #3
MrMaksimize CreditAttribution: MrMaksimize commentedHi Kyle,
Was testing this out today, and looks like that patch unfortunately is not the end of the story.... Or it may be.
In oauth_common.pages.inc inside the oauth_common_form_authorize_submit
The parse_url function returns FALSE if you have entered iphoneapp:// as the callback url. It also happens in the same file in oauth_common_form_authorize():
However it seems to work fine with iphoneapp://some-path because it's able to parse the url.
I'm not sure if this is really desired functionality - I could see it being a best practice in the app to have the oauth callback at a specific path rather than the app protocol root.
In that case - I would vote for updating the ['description'] on the callback form element to be updated.
However, if the oauth should be allowed to call back to a global app namespace, I think some regex is required to account for that and not fire parse_url in those scenarios.
Setting this back to needs work - let me know what you think :)
Comment #4
MrMaksimize CreditAttribution: MrMaksimize commentedAlso - saw that this patch was agains the d7 version of the module - and testing with drupal_parse_url (introduced in d7 - http://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_p...) it seems to work :)
Comment #5
kylebrowning CreditAttribution: kylebrowning commentedOauth should be allowed to set whatever URL it wants as a callback, theres no requirement in the spec that states it needs to be http://
So I suppose I will work on some regex unless you are fine with switching it to use drupal_parse_url.
Its strange that in my testing this code works flawlessly and my callbacks are called even those parse_url returns false.
What version of PHP are you using?
Comment #6
MrMaksimize CreditAttribution: MrMaksimize commentedGenerated a patch that validates the form submission to have a proper url
Comment #7
kylebrowning CreditAttribution: kylebrowning commentedJust tested this and it looks good to me.
Comment #8
juampynr CreditAttribution: juampynr commentedRemoved trailing white space, wrapped a string with t() and committed. Thanks!
http://drupalcode.org/project/oauth.git/commitdiff/5cb2e79
Comment #9
kylebrowning CreditAttribution: kylebrowning commentedCallback is not a required field but it appears that if you hit submit without filling in it, it complains its not a valid http schema.
:/
Comment #10
Deciphered CreditAttribution: Deciphered commentedYeah... that's just silly. If it's not required, don't validate it when it's empty.
Patch attached.
Comment #11
kylebrowning CreditAttribution: kylebrowning commentedComment #12
juampynr CreditAttribution: juampynr commentedCommitted. Thanks!