-
Notifications
You must be signed in to change notification settings - Fork 445
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
Provide a setting to use a different REST url during SSR execution #3358
Conversation
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.
Everything works as expected, but still "leaks" the SSR URL and does double work since the transfer state is ignored
@@ -100,6 +101,14 @@ export class ServerInitService extends InitService { | |||
} | |||
|
|||
private saveAppConfigForCSR(): void { | |||
this.transferState.set<AppConfig>(APP_CONFIG_STATE, environment as AppConfig); | |||
if (isNotEmpty(environment.rest.ssrBaseUrl) && environment.rest.baseUrl !== environment.rest.ssrBaseUrl) { | |||
// Avoid to transfer ssrBaseUrl in order to prevent security issues |
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 agree about the potential security concern, good idea.
The state transfer JSON will still list the SSR URL. It's a bit more "hidden" but not secure either.
Hi @atarix83, |
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.
Thanks @atarix83 ! I gave this a code review today, and have some minor requests for improvement. I haven't had a chance to fully test this yet, but I plan to do so next week.
I've improved my implementation trying to address your feedback. Here the main changes:
|
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.
@atarix83 : I was able to test this locally today by using temporary public URLs for the frontend & backend. I ran the frontend & backend on different domains, and ran the frontend in production mode (npm run build:prod; npm run serve:ssr
).
Overall, the basics of this PR (and the backend PR) work well. However, I've found a few bugs in my testing:
- Authentication doesn't work. This might be a side effect of an error I'm seeing on
main
, but here's what I'm trying.- If I use these PRs with the
dspace.server.ssr.url
andrest.ssrBaseUrl
settings disabled, then authentication works fine. However, whenever I authenticate, in the SSR logs I see an "doesn't contain the link profiles" error. This seems harmless, but it appears on every login - If I then enable the
dspace.server.ssr.url
andrest.ssrBaseUrl
settings and point them at http://localhost:8080/, authentication no longer works. After attempting to authenticate, the page hangs and I just see the loading image (on a blank white page). No obvious errors are seen in the logs.
- If I use these PRs with the
- Whenever SSR is triggered, I see a 404 request to an
/undefined
path. This appears in my browser's DevTools "Network" tab.- Enable the
dspace.server.ssr.url
andrest.ssrBaseUrl
settings and point them at http://localhost:8080/ - Go to any Item page.
- Click reload in your browser (to trigger SSR). The
404 /undefined
path error will appear in your DevTools
- Enable the
- Thumbnail images appear to be accessed initially via the private URL.
- Enable the
dspace.server.ssr.url
andrest.ssrBaseUrl
settings and point them at http://localhost:8080/ - Go to an Item page, for an Item that has a thumbnail.
- Click reload in your browser (to trigger SSR)
- Look closely at the
/content
request in your browser's DevTools. It will initially be against the private URL ("http://localhost:8080/"). However, once the page redraws, it will send again to the correct public URL. - If I add the
replaceRestUrl: true
setting to myconfig.prod.yml
and try again, then this behavior will no longer occur. It only occurs ifreplaceRestUrl: false
is set.
- Enable the
Overall, this appears to be working. When this feature is enabled, I do visually notice the flash/redraw of the page when transitioning from SSR to CSR. It's much more severe than when this feature is disabled. However, if there's no way to minimize that flash, then we may just need to warn people that the page flash is more severe when this separate REST URL feature is enabled.
I also have a few minor comments inline below on the code. Mostly it's looking good though.
Hi @atarix83, |
Hi @atarix83, |
# Conflicts: # config/config.example.yml
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.
@atarix83 : I've retested this today, using your separate REST branch (https://github.com/4Science/DSpace/tree/task/main/DURACOM-288_fix_hal_map) and everything seems to be working now!
Login now succeeds! I can no longer reproduce either of the other bugs that I had seen previously. I've also verified that, when I turn off Javascript, it appears all request are going to http://localhost:8080/ instead of the public URL. I've not noticed any obvious bugs after trying out a new submission, search, browse, etc.
So, I think the fixes you've applied to your branch must have solved the authentication errors. If you wanted to move those over to the REST API PR (DSpace/DSpace#9856) then I can give them a code review there. Thanks!
Overall, I think I'm +1 this PR now. But, I want to give it all one last code review & test once all the code is in these PRs.
@tdonohue I pushed changes to REST PR, could you please take a look? thanks |
Hi @atarix83, |
# Conflicts: # src/environments/environment.production.ts # src/environments/environment.test.ts # src/environments/environment.ts
I've added the missing code in the Can i ask how have you set the environment variables during your test? I did using the following commands:
|
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.
👍 Thanks @atarix83 ! This looks good to me now, and it's working for me. I tested it using localxpose.io (to create a dummy public URL), and verified that all functionality works from the User Interface. This includes testing authentication, submission, search, sitemaps, SSR, etc.
I don't have any further feedback. But, I'd still appreciate it if @pnbecker could test this in a "production-like" environment. It's always possible that testing via localxpose.io would miss something that could be caught in a production-like environment.
I have a setup here, when I remove the ssr base url from the configuration I get a 500 as angular is not allowed to access the official url of the rest api, while my browser can access it. When I login, the dsAuth cookie is created. It is valid for 24 hours. When I reload, the cookie is being removed. @atarix83 would it be helpful to gain access to my test installation? Do you have a static IPv4 address? |
Actually in Safari I do see the dsAuthInfo cookie until I refresh. In Chrome and Firefox I never see that cookie. In all three browser after refreshing the page, I'm logged out. |
@pnbecker : @atarix83 just created @atarix83 posted this to Slack:
When you have a chance, could you test this instead using the 8.x version? It might help us temporarily work around the issues you've hit on |
@tdonohue Sorry to jump in. I'm trying to accomplish the same configuration and I found a way to test everything in a "production-like" with Docker. Docker offer some similarities with a "production-like" environment : containers can access each other internally and only what is exposed can be access by the host with It almost work. Seems like
Steps to reproduce
docker-compose -f docker-compose.yml -p d8 up -d docker-compose.yml
What is happeningIn the Chrome DevTool, I can confirm that the rest API uses the public endpoint |
@grenierdev : If it fails immediately (after initial load), that sounds like a possible misconfiguration, or maybe you forgot to rebuild either the frontend or backend images using this PR or DSpace/DSpace#9856 ? Or maybe Docker's network settings are somehow blocking the requests? I'm not able to reproduce that immediate failure in all my testing of these two PRs while using https://localxpose.io/ . When using LocalXPose, I'm able to create a temporary public URL for the That said, I'll admit, I haven't tried setting these params via environment variables (though that would be odd if they only worked if set directly in configs and not via environment variables) or tried this sort of Docker approach. I'll see if I can find time to try out the Docker approach. While I agree it hypothetically should work, I'm a bit worried that Docker networking (which can sometimes be tricky) could be getting in the way here. |
I tested the backport to DSpace 8 of this PR in a production environment. There were two URLs to access the backend: one was accessible only from my browser, the other one from the frontend. The one accessible from my browser filtered the access based on the connecting ip address, the other one was routed via a private IP address space which my browser had no access to. The frontend was able to access the backend only via the private ip. I was able to login, to submit a new Item, to load thumbnails and files with JavaScript switched off, and to load the sitemap. I was not able to search and browse without JavaScript, which is not a surprise after we merged the PR to limit SSR to certain paths and to exclude search and browse from ssr. 👍 I did not review the code, I just tested it. |
@tdonohue I had the impression that the branch I used the branches of both PR in Docker and everything worked. Even with the environment variables. The frontend is accessible on Great work! |
Merging with +2 approvals (and an additional approval from @grenierdev as well, thanks!). Thanks again @atarix83 ! |
@atarix83 : As discussed in today's meeting, this will need to be ported manually to |
References
Add references/links to any related issues or PRs. These may include:
Description
This pull request provide the possibility to use a different DSpace REST url during the SSR
Instructions for Reviewers
List of changes in this PR:
ssrBaseUrl
where to specify a different DSpace REST url to use during SSRssrBaseUrl
property. If this URL was not publicly accessible, it would result in an error. By rendering the thumbnail only in the browser, we avoid this problem.ServerHardRedirectService
. In case the url to redirect contains thessrBaseUrl
it's reinstated with the public base url. This is necessary otherwise download bitstream page doesn't work.Include guidance for how to test or review your PR.
Unfortunately it is difficult to test this PR. The best would be to deploy it in a production like environment and configure the internal base url property.
This might be done on both Angular and REST side, like in the example below where public base url is
https://dspace-rest.org/server
and internal base url ishttp://localhost:8080/server
.ssrBaseUrl
property, e.g :dspace.server.ssr.url
property, e.g. :In order to test it locally you could add an alias hostname for localhost ip to be used as public url
Checklist
main
branch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lint
npm run check-circ-deps
)package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.