-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix: save synapse token in memory instead of mongoDB #320
fix: save synapse token in memory instead of mongoDB #320
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.
Very nice cleanup 💯 👍
It makes sense, but just have a question about one test.
@@ -28,15 +20,5 @@ describe("UserController (acceptance)", () => { | |||
.send(credentials) | |||
.expect(401); | |||
}); | |||
|
|||
it("should resolve in a jwt token when logging in with the correct credentials", async () => { |
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.
Question. Why was this test removed?
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.
After refactoring JWT validation is no longer needed so is for the JWT test.
Within this PR synapse credential will be directly passed to loopback from SciCat BE, then loopback will use the credential to login Synapse server and save generated token in the memory.
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 do not understand why the authentication has been removed?
Does that mean that some URLs are open and provide information to unauthenticated users?
} while (true); | ||
} | ||
|
||
@authenticate("jwt") | ||
@get("/Logbooks/{name}", { |
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.
Why are we removing authentication?
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.
Users whomever are able to see logbook tab and able to click into it should be able to see logbook messages, we already have that permission logic somewhere in the scicat BE and scicat FE. So I figured that JWT auth step is redundant.
As loopback service will be running in a SSH protected VM I don't see open URL would cause any harm to our side.
But one alternative way I think we could do is save synapse token in the scicat BE after first login, then trigger synapse service using token received from header in loopback.
console.error(err); | ||
} | ||
} | ||
break; | ||
} while (true); | ||
} | ||
|
||
@authenticate("jwt") | ||
@post("/Logbooks/{name}/message", { |
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.
Why no authentication
Description
This PR aims to save synapse token in the memory rather than in mongoDB.
Motivation
Currently loopback saves token in the mongoDB which requires extra user credential verification layers to get the token and does not update token on application restart.
We exclusively use synapse for fetching scichat room messages where user credential verification is unnecessary step and it increases application complexity.
Also we don't want to manually update token everytime when the application restarts, so it is better to save the token in the memory and automatically generate new token on every restart.
Fixes:
Changes:
Tests included/Docs Updated?