Fix: saving logbook token in memory #725
Conversation
Junjiequan
requested review from
nitrosx,
bpedersen2,
martin-trajanovski,
bolmsten and
enthusiastio
src/logbooks/logbooks.service.ts
Outdated
| @@ -132,7 +144,10 @@ export class LogbooksService { | |||
| if (this.logbookEnabled) { | |||
| if (this.username && this.password) { | |||
| try { | |||
| const accessToken = await this.login(this.username, this.password); | |||
| const shouldRenewToken = await this.shouldRenewToken(); | |||
| if (!this.accessToken || shouldRenewToken) { | |||
There was a problem hiding this comment.
Question: Do you think checking for if this.accessToken is falsy could be done in the shouldRenewToken() instead?
There was a problem hiding this comment.
That makes code much cleaner, I have changed them in the new commit. Thanks!
src/logbooks/logbooks.service.ts
Outdated
| this.httpService.get<Logbook[]>(this.baseUrl + "/Logbooks", { | ||
| headers: { Authorization: `Bearer ${accessToken.token}` }, | ||
| }), | ||
| this.httpService.get<Logbook[]>(this.baseUrl + "/Logbooks"), |
There was a problem hiding this comment.
why the token was removed?
Does this mean that I can retrieve the logbook without authentication?
There was a problem hiding this comment.
In the loopback repo we don't use headers to retrieve token. The token used for loopback is created and stored on the first success login to the synapse service, and then will re-use the token until loopback service is restarted.
There was a problem hiding this comment.
Does this mean that there is no authentication between SciCat BE and loopback connector?
If so, is that what we want to do?
There was a problem hiding this comment.
we need to provide synapse credential to the loopback on the first time login, which is saved in scicat backend .env file. then loopback service will generates a token with given credential and stores the token in loopback application scope. Thereafter, we just send request from scicat BE to loopback without further authentication.
One of the main reason we should try to use token exclusively and minimize login attempt is because, synapse service is very sensitive to login Rate limit with same account. If we somehow triggeres login more than 2 times within short period time(less than 1 minute or so), synapse will throw rate limit excedeed error and we will need to wait for 1~5 minutes login restriction untill next login attempt.
src/logbooks/logbooks.service.ts
Outdated
| const shouldRenewToken = await this.shouldRenewToken(); | ||
| if (shouldRenewToken) { | ||
| await this.login(this.username, this.password); | ||
| } | ||
| Logger.log("Fetching logbook " + name, "LogbooksService.findByName"); | ||
| Logger.log(filters, "LogbooksService.findByName"); | ||
| const res = await firstValueFrom( | ||
| this.httpService.get<Logbook>( | ||
| this.baseUrl + `/Logbooks/${name}?filter=${filters}`, |
There was a problem hiding this comment.
Same as my previous comment
src/logbooks/logbooks.service.ts
Outdated
| @@ -141,7 +153,6 @@ export class LogbooksService { | |||
| this.httpService.post<{ event_id: string }>( | |||
| this.baseUrl + `/Logbooks/${name}/message`, | |||
| data, | |||
src/logbooks/logbooks.service.ts
Outdated
| this.httpService.get<Logbook[]>(this.baseUrl + "/Logbooks", { | ||
| headers: { Authorization: `Bearer ${accessToken.token}` }, | ||
| }), | ||
| this.httpService.get<Logbook[]>(this.baseUrl + "/Logbooks"), |
There was a problem hiding this comment.
Does this mean that there is no authentication between SciCat BE and loopback connector?
If so, is that what we want to do?
Junjiequan
deleted the
SWAP-3487-scicat-fix-test-fail-for-the-loopback-recent-pr-a
branch
Description
This PR is correlated with SciCatProject/scichat-loopback#320
In this PR, we're optimizing token handling. Instead of triggering the Synapse login endpoint for every logbook fetch request in scichat-loopback, we now save the token in memory and reuse it until the loopback service restarts. This reduces the need for frequent Synapse login calls and improves efficiency.
Motivation
Before, when the loopback service restarted, access to logbook services from scicat backend was lost. To regain access, it was a must to remove the expired token from MongoDB and generate a new one.
Tests included/Docs Updated?