-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: support iCloud in China #47
Conversation
@dedoussis, would you care to review when you have time? |
Hi @gutenye, thanks for putting together this fix! Have pushed a large change in the extension which affects your PR. Would you be happy with fixing this branch to be compatible with the changes I introduced? If not, I can pick this up and rebase the implementation to what's currently in master. |
Rebased, I'm not able to test it because |
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.
Really appreciate the effort of re-implementing this on top of my recent changes.
Requested some refactoring changes to align this with the existing patterns of the code base.
I'd suggest that we don't read browser storage from within the ICloudClient, and instead pass the setupUrl as a constructor parameter. This will also require changes in the Popup and Options components.
Also, would you mind sharing the build errors you got?
class ICloudClient { | ||
public webservices?: Record<ServiceName, { url: string; status: string }>; | ||
private country: Country |
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.
private country: Country | |
private setupUrl: string |
class ICloudClient { | ||
public webservices?: Record<ServiceName, { url: string; status: string }>; | ||
private country: Country | ||
|
||
constructor(webservices?: ICloudClient['webservices']) { |
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.
constructor(webservices?: ICloudClient['webservices']) { | |
constructor(setupUrl: string, webservices?: ICloudClient['webservices']) { |
@@ -47,6 +56,7 @@ class ICloudClient { | |||
|
|||
public async isAuthenticated(): Promise<boolean> { | |||
try { | |||
this.country = await getBrowserStorageValue<Country>(COUNTRY_KEYS) || '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.
I'd like to keep the ICloudClient
class agnostic of the browser context it runs on. Let's not have it access browser specific APIs (such us the storage one).
@@ -57,7 +67,7 @@ class ICloudClient { | |||
public async validateToken(): Promise<void> { | |||
const { webservices } = (await this.request( | |||
'POST', | |||
`${ICloudClient.setupUrl}/validate` | |||
`${ICloudClient.setupUrl[this.country]}/validate` |
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.
`${ICloudClient.setupUrl[this.country]}/validate` | |
`${ICloudClient.setupUrl}/validate` |
@@ -71,7 +81,7 @@ class ICloudClient { | |||
options: { trust: boolean } = { trust: false } | |||
): Promise<void> { | |||
const { trust } = options; | |||
await this.request('POST', `${ICloudClient.setupUrl}/logout`, { | |||
await this.request('POST', `${ICloudClient.setupUrl[this.country]}/logout`, { |
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.
await this.request('POST', `${ICloudClient.setupUrl[this.country]}/logout`, { | |
await this.request('POST', `${ICloudClient.setupUrl}/logout`, { |
modifiedHeaders.push({ | ||
name: 'Referer', | ||
value: 'https://www.icloud.com/', | ||
value: `https://www.icloud.com${suffix}/`, |
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.
value: `https://www.icloud.com${suffix}/`, | |
value: `https://www.icloud.${tld}/`, |
}); | ||
modifiedHeaders.push({ | ||
name: 'Origin', | ||
value: 'https://www.icloud.com', | ||
value: `https://www.icloud.com${suffix}`, |
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.
value: `https://www.icloud.com${suffix}`, | |
value: `https://www.icloud.${tld}`, |
|
||
constructor(webservices?: ICloudClient['webservices']) { | ||
this.webservices = webservices; | ||
this.country = '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.
this.country = 'default' | |
this.setupUrl = setupUrl |
const country = Object.entries(ICloudClient.setupUrl).find((entries) => url.startsWith(`${entries[1]}/accountLogin`))?.[0]; | ||
await setBrowserStorageValue(COUNTRY_KEYS, country); |
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.
const country = Object.entries(ICloudClient.setupUrl).find((entries) => url.startsWith(`${entries[1]}/accountLogin`))?.[0]; | |
await setBrowserStorageValue(COUNTRY_KEYS, country); | |
const setupUrl = new URL(url).hostname.split("/accountLogin")[0] | |
const client = new ICloudClient(setupUrl); |
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.
Also, instead of storing the setupUrl value here, we can do it as part of performAuthSideEffects
.
@@ -2,6 +2,7 @@ import browser from 'webextension-polyfill'; | |||
|
|||
export const POPUP_STATE_STORAGE_KEYS = ['iCloudHmePopupState']; | |||
export const OPTIONS_STORAGE_KEYS = ['iCloudHmeOptions']; | |||
export const COUNTRY_KEYS = ['country']; |
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.
export const COUNTRY_KEYS = ['country']; | |
export const ICLOUD_CLIENT_STATE = ['iCloudClientState']; |
storing an object of type:
{setupUrl: string}
@dedoussis I got this error on the main branch when run
|
@gutenye I think the issue is related to the node version you're using. Try using node 18. |
Closes #34
Chinese iCloud uses a different domain (icloud.com.cn) instead of (icloud.com), this PR aims to support it.
Test Plan