-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add the missing functions required for the WPCOM OAuth flow. #2145
Add the missing functions required for the WPCOM OAuth flow. #2145
Conversation
Hi @jorgemd24 Thanks for making this progress with the project. I created my APP but in order to test this PR I have some questions.
YOUR_CLIENT_ID
|
/** | ||
* Handles the login action for Authorizing the JSON API | ||
*/ | ||
public function login_form_json_api_authorization() { |
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.
After reading the PR and P2 I found we are copying the functions from https://github.com/Automattic/jetpack/blob/trunk/projects/plugins/jetpack/class.jetpack.php
I think it would be useful to add that reference somewhere. I.e adding "Copied Code" or "Duplicated code" on each duplicated function in the class as well as in the main class PHPDOC. And also adding the link to the original jetpack class.
Otherwise, after this PR is approved, it might be difficult for other developers working on the code to know where this functions come from.
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.
Yep that's right I added a link to the Jetpack functions. See here: 1f7ca4c
src/Integration/JetpackWPCOM.php
Outdated
/** | ||
* If someone logs in to approve API access, store the Access Code in usermeta. | ||
* | ||
* @param string $user_login Unused. | ||
* @param WP_User $user User logged in. | ||
*/ |
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.
This seems to be duplicated.
Hi @jorgemd24 The required functions from https://raw.githubusercontent.com/Automattic/jetpack/trunk/projects/plugins/jetpack/class.jetpack.php are copied correctly, and the translation domains replaced. So that part is ✅ Please help me with the smoke testing. See #2145 (comment) So I can be sure I'm fully testing this PR |
@puntope thanks for your time reviewing this PR!
Once the WPCOM App is created you can get those variables from here: https://developer.wordpress.com/apps/ Your blog/site id can be found in
Yes, you need to fill in all details and the redirect URL can be http://localhost. |
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.
Hi @jorgemd24
Thanks a lot for the deeper explanation in regard the creation of WPCOM APP
I tested it and I was able to get the code in the URL after login in.
Also I tested with wrong params (i.e wrong URL) and I got the error as expected.
so LGTM ✅
Notice this is not accurate, URL needs to be accessible or the next error appears: |
Changes proposed in this Pull Request:
Closes part of #2146
Currently, in order to authorize a WPCOM App from a Jetpack site, the Jetpack plugin is required because the
jetpack-connection
lacks some essential functions necessary for a successful OAuth Flow. The Jetpack team is contemplating the inclusion of these functions in thejetpack-connection
package. However, until that happens, we rely on these functions to enable the connection of any WPCOM App.This PR copies the missing functions from Jetpack to this extension, allowing the complete OAuth flow to be executed without installing the Jetpack plugin. This PR is based on an experimental branch that I created. However, that branch was primarily intended to demonstrate the feasibility of authorizing a WPCOM app by simply copying functions and did not validate the Jetpack signature.
In essence, this PR adds all the necessary functions so that in the event that
jetpack-connection
doesn't incorporate these functions, we can use our version.See more context here: pcTzPl-1SM-p2#comment-2672
Screenshots:
Detailed test instructions:
wp-admin/admin.php?page=connection-test-admin-page
https://public-api.wordpress.com/oauth2/authorize?client_id=YOUR_CLIENT_ID&redirect_uri=YOUR_REDIRECT_URI&response_type=code&blog=YOUR_BLOG_ID&scope=posts
Additional details:
Changelog entry