-
Notifications
You must be signed in to change notification settings - Fork 368
[Website] Prevent Query API defaults from overriding login info provided by the Blueprint #2871
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
[Website] Prevent Query API defaults from overriding login info provided by the Blueprint #2871
Conversation
…ded by the Blueprint
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.
Pull Request Overview
This PR prevents the login query parameter from overriding login configurations that are already defined in a blueprint. The change adds logic to check for existing login configurations before applying the query parameter default.
- Adds detection for existing login steps in the blueprint's steps array
- Adds detection for existing login shorthand in the blueprint object
- Only applies the
login=truedefault when neither form of login is already present
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/playground/website/src/lib/state/url/resolve-blueprint-from-url.ts
Outdated
Show resolved
Hide resolved
…gin-step-isnt-provided
|
This means explicitly specifying login=yes won't override the Blueprint value. Let's do it a bit differently and only override if the query parameter was explicitly provided. |
I implemented it in 5834f58, but I'm not sure that this is the best solution. My concern is this scenario. The Blueprints says |
|
| const shouldSetLoginToTrue = () => { | ||
| /** | ||
| * Allow the Query API to explicitly set login | ||
| * if the login query param is provided. | ||
| */ | ||
| if (query.get('login') === 'no') { | ||
| return false; | ||
| } | ||
| if (query.get('login') === 'yes') { | ||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Set login to true by default in the Blueprint | ||
| * only if it doesn't already contain a login step or shorthand. | ||
| * Otherwise, the login provided by the blueprint would be overridden. | ||
| */ | ||
| if ( | ||
| blueprint.steps?.some( | ||
| (step) => | ||
| step && typeof step === 'object' && step?.step === 'login' | ||
| ) | ||
| ) { | ||
| return false; | ||
| } | ||
| if (blueprint.login !== undefined) { | ||
| return false; | ||
| } | ||
| return true; | ||
| }; | ||
| if (shouldSetLoginToTrue()) { | ||
| blueprint.login = true; | ||
| } |
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.
How about this?
| const shouldSetLoginToTrue = () => { | |
| /** | |
| * Allow the Query API to explicitly set login | |
| * if the login query param is provided. | |
| */ | |
| if (query.get('login') === 'no') { | |
| return false; | |
| } | |
| if (query.get('login') === 'yes') { | |
| return true; | |
| } | |
| /** | |
| * Set login to true by default in the Blueprint | |
| * only if it doesn't already contain a login step or shorthand. | |
| * Otherwise, the login provided by the blueprint would be overridden. | |
| */ | |
| if ( | |
| blueprint.steps?.some( | |
| (step) => | |
| step && typeof step === 'object' && step?.step === 'login' | |
| ) | |
| ) { | |
| return false; | |
| } | |
| if (blueprint.login !== undefined) { | |
| return false; | |
| } | |
| return true; | |
| }; | |
| if (shouldSetLoginToTrue()) { | |
| blueprint.login = true; | |
| } | |
| // Override the `login` property if explicitly requested: | |
| if (query.get('login') === 'yes') { | |
| blueprint.login = true; | |
| } else if(query.get('login') === 'no') { | |
| blueprint.login = false; | |
| } |
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.
That's much cleaner, but with it login defaults to false. Do we want to change the default login value?
Now, if you go to http://127.0.0.1:5400/website-server/?php=8.3, the user won't be logged in, while before the change, they would be.
Opening Playground without any query arguments would still login the user because we apply the welcome Blueprint by default.
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.
Ah, good point! Then let's initialize that default Playground-provided Blueprint with login: true – I've just proposed another suggestion above.
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.
Does that mean if I:
- add
"login": falseto my blueprint.json for my plugin SVN; and - have my own "login" in "steps" with specific non-admin user.
Then the resulting Live Preview would have the non-admin user login instead of admin?
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.
Then let's initialize that default Playground-provided Blueprint with login: true
If we do that, opening Playground with /#{"landingPage":"/wp-admin/"} would end up at the login screen because the user wouldn't be logged in by default. While today it logs in the user and loads wp-admin.
@adamziel check out my last commit 0730056. By prepending the default login step instead of appending it to the steps array, we allow Blueprints to override it.
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.
Then the resulting Live Preview would have the non-admin user login instead of admin?
Unfortunately no. If you add login: false to the Blueprint, it would be overridden by the Query API to true.
You can test it with this Playground link.
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.
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.
I updated implementation details to match the new approach and explain why we can have multiple login steps and why it won't cause multiple login attempts.
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.
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.
@bgrgicak this is a pretty cool approach 👍
| // via the hash fragment (#{...}) or via the `blueprint-url` query param. | ||
| return { | ||
| blueprint: { | ||
| plugins: query.getAll('plugin'), |
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.
| login: true, | |
| plugins: query.getAll('plugin'), |
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.
See #2871 (comment)
Motivation for the change, related issues
As reported in #2872 the Playground website will override the
loginstep and shorthand when applying Query API overrides.The default behavior of the Query API is to set the
loginshorthand totrue, but the override doesn't check if the Blueprint already provides aloginstep or shorthand. This behavior results in the user being unable to use theloginstep and shorthand on the Playground website, except if they explicitly set?login=noin the Query API.Implementation details
This PR addresses the issue by prepending the default login step to the steps array instead of appending it to the end. By prepending it, the login steps provided by the user will override the default login step.
The login step only defines the
PLAYGROUND_AUTO_LOGIN_AS_USERconst, we can have multiple, because each step overrides the definition of the previous step. (login step code).On the first page load, a mu-plugin detects the const and logs in the user defined in that const (login code).
Because of that, it doesn't matter how many times we call the login step, login will be executed once.
Testing Instructions (or ideally a Blueprint)
Manual testing
Demo User