Skip to content
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.

Sign up for GitHub

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

Improve authorisation #91

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Improve authorisation #91

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6a7db0
improve state handling
mniegrzybowska Sep 19, 2024
9d6b6c2
improve-authorisation
mniegrzybowska Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 25 additions & 11 deletions src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,35 +9,49 @@ import AwardsView from './views/Award/AwardsView.tsx';
import ConsequencesView from './views/ConsequencesView';
import SettingsView from './views/SettingsView';
import Head from './Head';
import { AWARDS_SUB_ROUTES, BASE_ROUTES_NAME, SUB_ROUTES_NAME } from './const/routing.const';
import {
AWARDS_SUB_ROUTES,
BASE_ROUTES_NAME,
SUB_ROUTES_NAME,
} from './const/routing.const';
import Login from './views/Login.tsx';
import AuthProvider from './providers/AuthProvider.tsx';
import AddAward from './views/Award/AddAward.tsx';
import GrantAward from './views/Award/GrantAward.tsx';

function App(): React.ReactNode {
return (
<div className="App">
<Head />
<AuthProvider>
<AuthProvider>
<div className="App">
<Head />

<MainTemplate>
<Routes>
<Route path={BASE_ROUTES_NAME.home} element={<HomeView />} />
<Route path={BASE_ROUTES_NAME.login} element={<Login />} />
<Route path={BASE_ROUTES_NAME.student} element={<StudentView />} />
<Route path={`${BASE_ROUTES_NAME.student}${SUB_ROUTES_NAME.tasks}`} element={<TasksView />} />
<Route path={`${BASE_ROUTES_NAME.student}${SUB_ROUTES_NAME.awards}`} element={<AwardsView />} />

<Route
KGrzeg marked this conversation as resolved.
Show resolved Hide resolved
path={`${BASE_ROUTES_NAME.student}${SUB_ROUTES_NAME.tasks}`}
element={<TasksView />}
/>
<Route
path={`${BASE_ROUTES_NAME.student}${SUB_ROUTES_NAME.awards}`}
element={<AwardsView />}
/>

<Route
path={`${BASE_ROUTES_NAME.student}${SUB_ROUTES_NAME.consequences}`}
element={<ConsequencesView />}
/>
<Route path={`${BASE_ROUTES_NAME.student}${SUB_ROUTES_NAME.info}`} element={<InfoView />} />
<Route
path={`${BASE_ROUTES_NAME.student}${SUB_ROUTES_NAME.info}`}
element={<InfoView />}
/>
<Route
path={`${BASE_ROUTES_NAME.student}${SUB_ROUTES_NAME.settings}`}
element={<SettingsView />}
/>
<Route
<Route
path={`${BASE_ROUTES_NAME.student}${SUB_ROUTES_NAME.awards}${AWARDS_SUB_ROUTES.add}`}
element={<AddAward />}
/>
Expand All @@ -47,8 +61,8 @@ function App(): React.ReactNode {
/>
</Routes>
</MainTemplate>
</AuthProvider>
</div>
</div>
</AuthProvider>
);
}

Expand Down
10 changes: 5 additions & 5 deletions src/components/atoms/Heading/Heading.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,18 @@ import { ThemeTypes } from '../../../theme/appTheme';

interface HeadingPropsInterface {
theme: ThemeTypes;
big?: boolean;
$big?: boolean;
}

export const Heading = styled.h1<HeadingPropsInterface>`
font-size: ${({ theme, big }) =>
big ? theme.fontSize.xl : theme.fontSize.lg};
font-size: ${({ theme, $big }) =>
$big ? theme.fontSize.xl : theme.fontSize.lg};
font-weight: ${({ theme }) => theme.bold};
margin: 0 0 10px;

@media only screen and (max-width: ${({ theme }) => theme.mediaMaxSize.xs}) {
font-size: ${({ theme, big }) =>
big ? theme.fontSize.md : theme.fontSize.sm};
font-size: ${({ theme, $big }) =>
$big ? theme.fontSize.md : theme.fontSize.sm};
}
`;

Expand Down
10 changes: 5 additions & 5 deletions src/components/atoms/Paragraph/Paragraph.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,17 @@ import type { ThemeTypes } from '../../../theme/appTheme';

interface ParagraphInterface {
theme: ThemeTypes;
big?: boolean;
$big?: boolean;
KGrzeg marked this conversation as resolved.
Show resolved Hide resolved
}

const Paragraph = styled.p<ParagraphInterface>`
font-size: ${({ theme, big }) =>
big ? theme.fontSize.md : theme.fontSize.sm};
font-size: ${({ theme, $big }) =>
$big ? theme.fontSize.md : theme.fontSize.sm};
margin: 0 0 10px;

@media only screen and (max-width: ${({ theme }) => theme.mediaMaxSize.xs}) {
font-size: ${({ theme, big }) =>
big ? theme.fontSize.sm : theme.fontSize.xs};
font-size: ${({ theme, $big }) =>
$big ? theme.fontSize.sm : theme.fontSize.xs};
}
`;

Expand Down
2 changes: 1 addition & 1 deletion src/components/modules/StudentCard/StudentCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ const StudentCard = ({ name, studentId, image }: PropTypesStudentCard) => {
return (
<StyledStudentCard to={`/${studentId}`}>
<ProfileImage src={image ? image.toString() : ProfileDefaultImage_SVG} />
<StyledName big={true}>{name}</StyledName>
<StyledName $big={true}>{name}</StyledName>
</StyledStudentCard>
);
};
Expand Down
1 change: 1 addition & 0 deletions src/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const localStorageKey = 'token';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, good idea!

16 changes: 11 additions & 5 deletions src/providers/AuthProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ import {
authenticationReducer,
type AuthenticationReducer,
} from './authenticationReducer.ts';
import { localStorageKey } from '../constants.ts';

export const AuthenticationContext = createContext<Authentication>({
token: "",
token: '',
});
export const TokenDispatchContext = createContext<React.Dispatch<TokenAction>>(
{} as React.Dispatch<TokenAction>
Expand All @@ -18,26 +19,31 @@ export const useAuthenticationData = () => useContext(AuthenticationContext);
export const useTokenDispatch = () => useContext(TokenDispatchContext);

const AuthProvider = ({ children }: { children: React.ReactNode }) => {
const localStorageKey = 'token';
const navigate = useNavigate();

const [authentication, dispatch] = useReducer<AuthenticationReducer>(
authenticationReducer,
{
token: ""
}
{
token: '',
}
);

useEffect(() => {
if (authentication.token) {
axios.defaults.headers.common['Authorization'] =
'Bearer ' + authentication.token;
localStorage.setItem(localStorageKey, authentication.token);
console.log('Have token');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

} else if (localStorage.getItem(localStorageKey)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😎

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I like your suggestion a lot. I will try to implement it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

console.log(localStorage.getItem(localStorageKey), 'token');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially the token shouldn't be exposed in the console for an inquisitive eyes :D

axios.defaults.headers.common['Authorization'] =
'Bearer ' + localStorage.getItem(localStorageKey);

console.log('going to local storage ');
} else {
navigate('/login');

console.log('redirecting');
}
}, [authentication, localStorageKey]);

Expand Down
2 changes: 1 addition & 1 deletion src/providers/authenticationActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,6 @@ export const setToken = (
export const clearToken = (dispatch: React.Dispatch<TokenAction>) => {
dispatch({
type: 'clear',
token: null,
token: '',
});
};
7 changes: 5 additions & 2 deletions src/providers/authenticationReducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { type Authentication, type TokenAction } from './types.ts';
import { localStorageKey } from '../constants.ts';

export type AuthenticationReducer = (
token: Authentication,
Expand All @@ -12,8 +13,10 @@ export const authenticationReducer: AuthenticationReducer = (
switch (action.type) {
case 'set':
return { token: action.token };
case 'clear':
return { token: "" };
case 'clear': {
localStorage.setItem(localStorageKey, '');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not to remove the item from the storage entirely?

return { token: '' };
}
default:
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
throw new Error(`Invalid action type ${action.type}`);
Expand Down
26 changes: 12 additions & 14 deletions src/views/Login.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useState } from 'react';
import React, { useEffect, useState } from 'react';
import { useNavigate } from 'react-router-dom';
import { Heading } from '../components/atoms/Heading/Heading.ts';
import MainBox from '../components/atoms/Sections/MainBox.ts';
Expand All @@ -12,7 +12,7 @@ import { type User } from '../api/Authentication/userTypes.ts';
import Button from '../components/atoms/Buttons/Button.ts';
import useAuthenticateUser from '../api/Authentication/authenticateUser.ts';
import { useTokenDispatch } from '../providers/AuthProvider.tsx';
import { setToken } from '../providers/authenticationActions.ts';
import { clearToken, setToken } from '../providers/authenticationActions.ts';

const Login = () => {
const [formData, setFormData] = useState<User>({
Expand All @@ -25,9 +25,13 @@ const Login = () => {
const { trigger, isMutating, error } = useAuthenticateUser();
const navigate = useNavigate();

const handleSubmit = async (
event: React.MouseEvent<HTMLButtonElement, MouseEvent>
) => {
useEffect(() => {
clearToken(dispatch);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is temporary - its here, because we don't have the refresh token functionality and we never clear the token :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a todo to make it more clear


console.log();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

}, []);

const handleSubmit = async (event: React.FormEvent<HTMLFormElement>) => {
event.preventDefault();
const response = await trigger({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that would be more clean

username: formData.userName,
Expand All @@ -42,35 +46,29 @@ const Login = () => {
<MainBox>
<StyledArticle>
<Heading>Zaloguj się</Heading>
<StyledForm onSubmit={() => handleSubmit}>
<StyledForm onSubmit={handleSubmit}>
<StyledLabel htmlFor="userName">Nazwa użytkownika:</StyledLabel>
<StyledInput
type="text"
name="userName"
id="userName"
value={formData.userName}
onChange={(event) =>
setFormData({ ...formData, userName: event.target.value })
}
required
/>
<StyledLabel htmlFor="password">Hasło:</StyledLabel>
<StyledInput
type="text"
type="password"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this change!

name="password"
id="password"
value={formData.password}
onChange={(event) =>
setFormData({ ...formData, password: event.target.value })
}
required
/>
{error && <div>Błąd logowania. Spróbuj ponownie.</div>}
<Button
type="button"
disabled={isMutating}
onClick={(event) => handleSubmit(event)}
>
<Button type="submit" disabled={isMutating}>
Zaloguj się
</Button>
</StyledForm>
Expand Down