Skip to content
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

Docs: Add guide for enabling Application Passwords on 5.6 #682

Merged
merged 6 commits into from
Jan 13, 2021

Conversation

dinhtungdu
Copy link
Contributor

Description of the Change

This PR adds a new section for Application Passwords and 5.6 debug guide. This also updates the auth page URL to make the Auth Wizard work on 5.6.

Verification Process

  • Update a distributor development site without HTTPS to 5.6.
  • Notice the Application Passwords section disappears on the user edit page.
  • Add a new external connection, don't see the wizard.
  • Follow the guide in this PR to enable Application Passwords.
  • See the Application Passwords section on the user edit page.
  • Add a new external connection, see the wizard working.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

Changelog Entry

@dinhtungdu dinhtungdu added this to the 1.6.2 milestone Dec 12, 2020
@dinhtungdu dinhtungdu requested a review from helen December 12, 2020 07:45
@dinhtungdu dinhtungdu self-assigned this Dec 12, 2020
dkotter
dkotter previously approved these changes Dec 17, 2020
@dkotter
Copy link
Collaborator

dkotter commented Dec 17, 2020

Code looks fine here. Tested on both WP 5.5 and 5.6 and on 5.5, everything worked great. I did run into one issue on 5.6 though. If you try and use the wizard to create a connection to another site that isn't on 5.6 yet, you'll end up getting sent to a 404 page (because the authorize-application.php page won't exist unless you're on 5.6).

Not sure if this is something we want to try and fix here, checking what WordPress version the external site is running and using the admin.php?page=auth_app page if it's not on 5.6? Or if we want to leave this alone and just add a note in the wizard page, mentioning how the external site needs to be on 5.6 (similar to the note we already have in there about how both sites need to be running Distributor 1.6.0 or greater)?

@dkotter dkotter self-requested a review December 17, 2020 23:00
@dkotter dkotter dismissed their stale review December 17, 2020 23:01

Discussion needed

@dinhtungdu
Copy link
Contributor Author

@dkotter Nice catch! I missed that important point. The Application Password endpoint should be based on the remote site, not the origin site. I pushed a solution to fix that.

There is still a case that hasn't been covered: the remote site is updated to WP 5.6 but Distributor isn't. We can't get the WP version or the core Application Passwords status through REST API. A dev note in the readme is the only thing we can do IMO.

@dinhtungdu
Copy link
Contributor Author

Here is what happens if Application Passwords is not available on the remote site:
image

@jeffpaul
Copy link
Member

@dinhtungdu does that minor docs change help call out what we're requiring or should we be more verbose/explicit?

@dkotter dkotter mentioned this pull request Jan 13, 2021
@dkotter dkotter merged commit 315d64f into develop Jan 13, 2021
@dkotter dkotter deleted the docs/enable-app-pwd-56 branch January 13, 2021 16:32
@TimothyBJacobs
Copy link

Just wanted to mention that you can check whether the target site supports app passwords by making a request to the REST API Index ( /wp-json ) and looking for application-passwords in the authentication section. See rest_add_application_passwords_to_index for details. That should also be the preferred way to get the URL to redirect the user to instead of hardcoding authorize-application.php.

@jeffpaul
Copy link
Member

@dkotter is there perhaps an update we should make in how Distributor is leveraging App Pwds based on @TimothyBJacobs note above?

@dkotter
Copy link
Collaborator

dkotter commented Feb 19, 2021

Yeah, that seems like a better approach. Probably worth opening an issue to get things updated to follow best practices

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants