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

Add support for SSO without a connection/confirmation. #10483

Closed
wants to merge 6 commits into from

Conversation

jaswrks
Copy link
Contributor

@jaswrks jaswrks commented Oct 30, 2018

Changes proposed in this Pull Request:

  • Allow site "connections" to be bypassed in favor of just the SSO login action itself whenever the original login URL contains ?connect=false.
    e.g., https://example.com/wp-login.php?connect=false

Testing instructions

  • See D19940-code for example a8c use case.
  • Patch D19940-code and follow test instructions there.

Proposed changelog entry for your changes:

  • No changelog entry necessary.

@jaswrks jaswrks added [Feature] SSO [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Oct 30, 2018
@jaswrks jaswrks requested a review from a team as a code owner October 30, 2018 09:46
@jaswrks jaswrks added [Status] In Progress and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Oct 30, 2018
@Automattic Automattic deleted a comment from jetpackbot Oct 30, 2018
Copy link
Contributor

@ebinnion ebinnion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jetpackbot
Copy link

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: November 6, 2018.
Scheduled code freeze: October 30, 2018

Generated by 🚫 dangerJS

@jaswrks
Copy link
Contributor Author

jaswrks commented Oct 30, 2018

Great. Thanks for the initial review!

Would you write some tests for this please?

  • Unit tests added.

ebinnion
ebinnion previously approved these changes Oct 31, 2018
Copy link
Contributor

@ebinnion ebinnion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a minor comment about the docblock. Besides that, this looks good to me and tests well. 👍

modules/sso/class.jetpack-sso-helpers.php Outdated Show resolved Hide resolved
@jaswrks
Copy link
Contributor Author

jaswrks commented Oct 31, 2018

@ebinnion Should we adjust this line that pulls the current user ID?
To clarify, should we set user_id => 0 when it's a user that was intentionally not connected?

'user_id' => get_current_user_id(),

I ask because I noticed a missing_token error whenever I was testing with a user who was already logged-in and found their way back to wp-login.php?action=jetpack-sso again.

@ebinnion
Copy link
Contributor

Ahh nice catch sir! Yes, we probably do need to change that. We can probably force it user_id => 0 to use the blog token or we can first check to see if the current user has a token.

@jaswrks
Copy link
Contributor Author

jaswrks commented Oct 31, 2018

@ebinnion Got it, thanks for confirming!

Also, does this look like a bug to you?

'wpcom_user_id' => $user_id,

Shouldn't it be user_id => $user_id instead of wpcom_user_id?

@ebinnion
Copy link
Contributor

ebinnion commented Nov 9, 2018

It looks like I did that for a specific reason:

#3756

On XML-RPC server, it looks like we just need to send a user ID in an array and it doesn't necessarily matter what key is used.

@stale
Copy link

stale bot commented Feb 7, 2019

This PR has been marked as stale. This happened because:

  • It has been inactive in the past 3 months.
  • It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

@stale stale bot added the [Status] Stale label Feb 7, 2019
@jeherve
Copy link
Member

jeherve commented Mar 2, 2022

I'll close this PR for now because of the lack of activity on this. We can always reopen in the future if needed, but it will need a rebase, so it may be easier to start a new PR at this point.

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

Successfully merging this pull request may close these issues.

None yet

5 participants