-
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
Seacat Auth API refactoring #42
base: main
Are you sure you want to change the base?
Conversation
@@ -43,7 +43,7 @@ function ManageEmailCard(props) { | |||
let redirect_uri = params.get("redirect_uri"); | |||
|
|||
let history = useHistory(); | |||
let SeaCatAuthAPI = props.app.axiosCreate('seacat-auth'); | |||
let SeaCatAccountAPI = props.app.axiosCreate('seacat-auth/account'); |
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.
@byewokko I dont think this is a good way to do this. Here should be just a service name. So only seacat-auth
here should be correct.
This is the correct way:
response = await SeaCatAccountAPI.put("/account/credentials", ...
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.
the idea was to have a separate axios instance for login and auth stuff (seacat-auth/public
) and another one for the account management (seacat-auth/account
). they now even have different authorization and ports on backend. i wanted to make this separation clear in the code:
let SeaCatAccountAPI = props.app.axiosCreate('seacat-auth/account');
let SeaCatPublicAPI = props.app.axiosCreate('seacat-auth/public');
but maybe it is not necessary. what does @ateska think?
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.
As discussed: Franta is right. There is no need for separation, also the proposed changes would make the code harder to search and correlate with the nginx config. So the correct approach is this:
let SeaCatAuthAPI = props.app.axiosCreate('seacat-auth');
...
response = await SeaCatAuthAPI.put("/account/credentials", ...)
Strict dependency on TeskaLabs/seacat-auth/pull/319. UI will not work with older Seacat Admin API versions.
Sync merge with
Breaking changes
/account
/public/login.prologue
changed to/public/login-prologue