-
Notifications
You must be signed in to change notification settings - Fork 2k
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
logmeinUrl package and usage #52909
logmeinUrl package and usage #52909
Conversation
Test live: https://calypso.live/?branch=add/logmein |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~304 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
packages/logmein/src/index.ts
Outdated
return fullUrl; | ||
} | ||
|
||
url.searchParams.set( 'logmein', '1' ); |
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.
Instead of appending logmein param, would it make sense to directly take a logged in user to the the r-login.wordpress.com endpoint? If the user is in Calypso and logged in, we don't need to take them to wordpress.com/log-in - we can take them to https://r-login.wordpress.com/remote-login.php?action=link&back=https://{MAPPED_DOMAIN.tld}
and save the extra log in screen step:
Appending logmein=1 is okay if we find the user is logged out for some reason. So, I'm thinking:
if ( isLoggedIn ) {
navigate to https://r-login.wordpress.com/remote-login.php?action=link&back=https://{MAPPED_DOMAIN.tld}
} else {
navigate to https://mapped-domain.tld/?logmein=1
}
A scenario in which this may not work is if the user has Calypso open in a tab, logs out of WPCOM in a different tab, and clicks on "Visit site" button on the open Calypso tab. Then they'll still be logged in to the mapped domain even though they're not signed in to WPCOM. Probably there are some checks we can add to handle this.
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 should be fine, I'll see what I can do.
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 tried this here: https://github.com/Automattic/wp-calypso/pull/52909/files#diff-d025c6a21229fe69836222916356ee8a2c395861252a82ff88e97b9facc70a4bR31
But it didn't seem to work for some reason (didn't dig too deep) end up getting js output as plaintext directly from `remote-login.php
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.
Reworked this as a client lib for simplicity and updated code to match up with what @JoshuaGoode is dropping in the demo. Depends on the latest update in D61461-code. Edit: Just noticed I nuked the tests in the process, will add them back tomorrow and amend to include the new selector. (Maybe, if we're still going this way) |
client/lib/logmein/index.ts
Outdated
|
||
// return ( | ||
// 'https://r-login.wordpress.com/remote-login.php?action=link&back=' + | ||
// encodeURIComponent( fullUrl ) |
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 should work, since I have a similar implementation in #53125 and it works well. One thing to note is that the back URL should be https
instead of http
. When I tested this PR by clicking on the mapped domain link on the domain management page, I was taken to the following URL which outputted JS on the screen:
https://r-login.wordpress.com/remote-login.php?action=link&back=http%3A%2F%2Fniranjan792177516.blog
Note the http
in the back URL. When I changed that https
, then the r-login redirect worked well.
Anyhow, with the change in D61461-code to skip the Continue step, is this change still needed?
@@ -398,7 +400,7 @@ export class List extends React.Component { | |||
<div className="list__header-primary-domain-content"> | |||
<ExternalLink | |||
className="list__header-primary-domain-url" | |||
href={ selectedSite.URL } | |||
href={ logmeinUrl( selectedSite.URL, siteUrls ) } |
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.
Would you like to include in this PR the logmeinUrl()'ification of other instances in Calypso that link to the mapped domain URL? We might have to do this in the editor, reader and so on. Or do you think we should take that as a separate PR?
and use it with logmeinUrl to apply logmein params to domain links.
…https, direct flag value
: false | ||
) | ||
.filter( Boolean ); | ||
return result; |
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'm not sure if this is the correct list of restrictions.. do we have an "is simple site" method by chance?
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 don't know the right answer to this, but here's what I found.
We have this selector that returns true if a site is a WPCOM site(atomic or simple) as described the JS Doc:
Whether the site is a WPCOM site ( Atomic or Simple )
Using the logic in that selector, the following should return simple sites:
! isJetpackSite( state, siteId ) || ! isAtomicSite( state, siteId )
Would this be the right check for WPCOM simple sites?
Moving focus to replacement: #53443 |
Changes proposed in this Pull Request
logmeingUrl
method to append the logmein parameter defined in D61461-codeTesting instructions
Apply D61461-code(dependency, since deployed)testing23465324891.home.blog/
)http://calypso.localhost:3000/?flags=oauth,logmein
You should end up the site in a logged in state with cookies against the mapped domain.
For testing in calypso using chrome with third party cookies disabled you'll need to use calypso's oauth mode, e.g.
http://calypso.localhost:3000/domains/manage/testing23465324891.home.blog?flags=oauth,logmein
.Unit tests:
Related to #52741 / #52740 / #52763