Update GoogleAuth.ts to account for self-hosted instances #1633
Conversation
Always use the APP_HOME_URL env variable (if set) to create the Auth Redirect url.
| const homeUrl = process.env.APP_HOME_URL; | ||
| // if homeUrl is localhost - (in dev environment) - use the development url | ||
| if (homeUrl && new URL(homeUrl).hostname === "localhost") { | ||
| if (homeUrl) { |
There was a problem hiding this comment.
What do you think about adding a dedicated variable for this redirect? Something like:
| if (homeUrl) { | |
| if (process.env.GRIST_GOOGLE_IMPORT_REDIRECT_URL) { | |
| return process.env.GRIST_GOOGLE_IMPORT_REDIRECT_URL; | |
| } | |
| if (homeUrl && new URL(homeUrl).hostname === "localhost") { |
and leaving the code as it is? It will be much simpler to manage this integration this way, rather then constructing those URL by hand.
There was a problem hiding this comment.
@berhalak Do correct me if i'm wrong, but:
in the case of GRIST_GOOGLE_IMPORT_REDIRECT_URL, user will need to know what to fill in, whereas,
in the case of just the APP_HOME_URL, no intervention is really required.
There was a problem hiding this comment.
Yes, but:
- user already needs to know what to put in
APP_HOME_URL, - this code that checks localhost is needed, I think, for some tests to work properly,
- this change will probably break some existing installation that relies on this format
With new env variable, this change will be backward compatible, and would require minimal tests change.
There was a problem hiding this comment.
With new env variable, this change will be backward compatible, and would require minimal tests change.
No denying that.
But, in that case, the user will also need to take care of the authHandlerPath in
if (homeUrl && new URL(homeUrl).hostname === "localhost") {
return `${homeUrl}${authHandlerPath}`;
}
when formulating the GRIST_GOOGLE_IMPORT_REDIRECT_URL, and that will need to be properly documented as well.
My suggestion was to get only the instance URL (which happens to be equal to the APP_HOME_URL value) from the user, and append the authHandlerPath to that subsequently.
There was a problem hiding this comment.
Yes, we would need to put the format of that URL in the property description in the README.md.
There was a problem hiding this comment.
The rest of the code in getFullAuthEndpointUrl() does look very specific to the Grist Labs SaaS in its handling of domains. Perhaps, after the local dev case, there could be another case that kicks in for simple (and common) single-domain instances of Grist?
3 participants
Context
#1549 (comment)
Proposed solution
Always use the
APP_HOME_URLenv variable (if set) to create the Auth Redirect url.Related issues
#1549 (comment)
Has this been tested?
Screenshots / Screencasts