Improve authorisation #91
Conversation
| ); | ||
|
|
||
| useEffect(() => { | ||
| if (authentication.token) { | ||
| axios.defaults.headers.common['Authorization'] = | ||
| 'Bearer ' + authentication.token; | ||
| localStorage.setItem(localStorageKey, authentication.token); | ||
| console.log('Have token'); |
There was a problem hiding this comment.
Please remove all the console.log calls from the production code. If you like to keep them, it's better to define some utlitity log method that only print messages in dev mode.
| } else if (localStorage.getItem(localStorageKey)) { | ||
| console.log(localStorage.getItem(localStorageKey), 'token'); |
There was a problem hiding this comment.
Especially the token shouldn't be exposed in the console for an inquisitive eyes :D
| @@ -0,0 +1 @@ | |||
| export const localStorageKey = 'token'; | |||
There was a problem hiding this comment.
If this key is used for authentication only, please define more specified variable name for the variable, like authLocalStoragekey. I think the token value is okay.
There was a problem hiding this comment.
yes, good idea!
| console.log(); | ||
| }, []); | ||
|
|
||
| const handleSubmit = async (event: React.FormEvent<HTMLFormElement>) => { | ||
| event.preventDefault(); | ||
| const response = await trigger({ |
There was a problem hiding this comment.
I think all the authentication process should be moved out of the Login.tsx. In the view there should be method like loginWithCredentials(username, password) that do all the job, so:
- make an request
- save the token
- maybe even redirect the user? But that can be also handled separately
Can we move it to the AuthProvider?
There was a problem hiding this comment.
yes, that would be more clean
| case 'clear': | ||
| return { token: "" }; | ||
| case 'clear': { | ||
| localStorage.setItem(localStorageKey, ''); |
There was a problem hiding this comment.
Why not to remove the item from the storage entirely?
| ); | ||
|
|
||
| useEffect(() => { | ||
| if (authentication.token) { | ||
| axios.defaults.headers.common['Authorization'] = | ||
| 'Bearer ' + authentication.token; | ||
| localStorage.setItem(localStorageKey, authentication.token); | ||
| console.log('Have token'); | ||
| } else if (localStorage.getItem(localStorageKey)) { |
There was a problem hiding this comment.
I believe we shouldn't access the localStorage directly. The data from localStorage should be copied into the reducer's state once the page loads, and then you should only use the state.
You can add an action like refreshFromStore to the authentiaction.Actions.ts, that reads localStorage and set the token if it's defined. Then in the App.tsx you can call that action a the root level to make sure it runs on page refreshes.
The approach should be:
- If I want to login, run an action on reducer that obtains a token and saves it in reducer's state and in localstorage,
- If you want to logout, run an action on reducer that clears the state and localstorage,
- If you refresh the page, call an action on reducer that loads data from localStorage an save it in state, or do nothing if storage is empty,
- If you want to access the data (check if user is logged), then only access the state, never localStorage, as it's already in sync because of rules above.
This way you have a single point where localStorage is mutated.
There was a problem hiding this comment.
But hey, if this is too much, merge it as this. It's much better than it was before.
Please don't feel pressured by this. 😎
There was a problem hiding this comment.
yes, I like your suggestion a lot. I will try to implement it :)
There was a problem hiding this comment.
But hey, if this is too much, merge it as this. It's much better than it was before.
Please don't feel pressured by this. 😎
I am not, nice to have someone to read the code. I was starting to feeling lonely ;) Thanks a lot for the comments :)
| event: React.MouseEvent<HTMLButtonElement, MouseEvent> | ||
| ) => { | ||
| useEffect(() => { | ||
| clearToken(dispatch); |
There was a problem hiding this comment.
I don't think we would like to logout an user whenever he accesses the Login page. This method might be called when users clicks button like "logout".
The better approach is to redirect the authenticated user to the app, if he reaches the Login. view.
There was a problem hiding this comment.
this is temporary - its here, because we don't have the refresh token functionality and we never clear the token :)
There was a problem hiding this comment.
I will add a todo to make it more clear
| onChange={(event) => | ||
| setFormData({ ...formData, userName: event.target.value }) | ||
| } | ||
| required | ||
| /> | ||
| <StyledLabel htmlFor="password">Hasło:</StyledLabel> | ||
| <StyledInput | ||
| type="text" | ||
| type="password" |
|
Well done! Finally the authentication will work as supposed^^ |
No description provided.