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

Refactor: makes session management a bit more readable #304

Merged
merged 9 commits into from
Aug 23, 2023
Merged

Refactor: makes session management a bit more readable #304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6e0b53a
refactor: renames store to sessionStore
Aug 17, 2023
a722623
refactor: renames server port to serveOnPort for clarity
Aug 17, 2023
021e5a7
refactor: returns 401 instead of 200 for non authenticated users
Aug 17, 2023
9e58824
refactor: renames session variables to be camelCase
Aug 17, 2023
acb41ca
refactor: handles 401 error, and new response schema
Aug 17, 2023
54a6880
refactor: moves error handling outside of function
Aug 22, 2023
5131771
refactor: change if-statement to switch case
Aug 22, 2023
a9225ee
refactor: removes break in default case in switch case
Aug 22, 2023
01c4478
refactor: use promise.catch instead of try/catch
Aug 22, 2023
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
90 changes: 42 additions & 48 deletions web/backend/src/Server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,11 @@ const PERMISSIONS = {
},
};

// store is used to store the session
const store = new MemoryStore({
const sessionStore = new MemoryStore({
checkPeriod: 86400000, // prune expired entries every 24h
});

// Keeps an in-memory mapping between a SCIPER (userid) and its opened session
// Keeps an in-memory mapping between a SCIPER (userId) and its opened session
// IDs. Needed to invalidate the sessions of a user when its role changes. The
// value is a set of sessions IDs.
const sciper2sess = new Map<number, Set<string>>();
Expand Down Expand Up @@ -65,13 +64,13 @@ async function initEnforcer() {
return newEnforcer('src/model.conf', dbAdapter);
}

const port = process.env.PORT || 5000;
const serveOnPort = process.env.PORT || 5000;
Promise.all([initEnforcer()])
.then((createdEnforcer) => {
[authEnforcer] = createdEnforcer;
console.log(`🛡 Casbin authorization service loaded`);
app.listen(port);
console.log(`🚀 App is listening on port ${port}`);
app.listen(serveOnPort);
console.log(`🚀 App is listening on port ${serveOnPort}`);
})
.catch((err) => {
console.error('❌ failed to start:', err);
Expand All @@ -82,10 +81,11 @@ function isAuthorized(sciper: number | undefined, subject: string, action: strin
}

declare module 'express-session' {
// This overrides express-session
export interface SessionData {
userid: number;
firstname: string;
lastname: string;
userId: number;
firstName: string;
lastName: string;
}
}

Expand All @@ -100,7 +100,7 @@ app.use(
saveUninitialized: true,
cookie: { maxAge: oneDay },
resave: false,
store: store,
store: sessionStore,
})
);

Expand All @@ -113,7 +113,7 @@ app.use(express.urlencoded({ extended: true }));
// app.use((req, res, next) => {
// const begin = req.url.split('?')[0];
// let role = 'everyone';
// if (req.session.userid && req.session.role) {
// if (req.session.userId && req.session.role) {
// role = req.session.role;
// }

Expand Down Expand Up @@ -166,11 +166,11 @@ app.get('/api/control_key', (req, res) => {
const lastname = response.data.split('\nname=')[1].split('\n')[0];
const firstname = response.data.split('\nfirstname=')[1].split('\n')[0];

req.session.userid = parseInt(sciper, 10);
req.session.lastname = lastname;
req.session.firstname = firstname;
req.session.userId = parseInt(sciper, 10);
req.session.lastName = lastname;
req.session.firstName = firstname;

const sciperSessions = sciper2sess.get(req.session.userid) || new Set<string>();
const sciperSessions = sciper2sess.get(req.session.userId) || new Set<string>();
sciperSessions.add(req.sessionID);
sciper2sess.set(sciper, sciperSessions);

Expand All @@ -184,17 +184,17 @@ app.get('/api/control_key', (req, res) => {

// This endpoint serves to log out from the app by clearing the session.
app.post('/api/logout', (req, res) => {
if (req.session.userid === undefined) {
if (req.session.userId === undefined) {
res.status(400).send('not logged in');
}

const { userid } = req.session;
const { userId } = req.session;

req.session.destroy(() => {
const a = sciper2sess.get(userid as number);
const a = sciper2sess.get(userId as number);
if (a !== undefined) {
a.delete(req.sessionID);
sciper2sess.set(userid as number, a);
sciper2sess.set(userId as number, a);
}
res.redirect('/');
});
Expand Down Expand Up @@ -224,24 +224,18 @@ function setMapAuthorization(list: string[][]): Map<String, Array<String>> {
// be logged into react. This endpoint serves to send to the client (actually to react)
// the information of the current user.
app.get('/api/personal_info', (req, res) => {
authEnforcer.getFilteredPolicy(0, String(req.session.userid)).then((AuthRights) => {
authEnforcer.getFilteredPolicy(0, String(req.session.userId)).then((AuthRights) => {
res.set('Access-Control-Allow-Origin', '*');
if (req.session.userid) {
if (req.session.userId) {
res.json({
sciper: req.session.userid,
lastname: req.session.lastname,
firstname: req.session.firstname,
islogged: true,
sciper: req.session.userId,
lastName: req.session.lastName,
firstName: req.session.firstName,
isLoggedIn: true,
authorization: Object.fromEntries(setMapAuthorization(AuthRights)),
});
} else {
res.json({
sciper: 0,
lastname: '',
firstname: '',
islogged: false,
authorization: {},
});
res.status(401).send();
}
});
});
Expand All @@ -252,7 +246,7 @@ app.get('/api/personal_info', (req, res) => {
// This call allows a user that is admin to get the list of the people that have
// a special role (not a voter).
app.get('/api/user_rights', (req, res) => {
if (!isAuthorized(req.session.userid, PERMISSIONS.SUBJECTS.ROLES, PERMISSIONS.ACTIONS.LIST)) {
if (!isAuthorized(req.session.userId, PERMISSIONS.SUBJECTS.ROLES, PERMISSIONS.ACTIONS.LIST)) {
res.status(400).send('Unauthorized - only admins allowed');
return;
}
Expand All @@ -266,7 +260,7 @@ app.get('/api/user_rights', (req, res) => {

// This call (only for admins) allow an admin to add a role to a voter.
app.post('/api/add_role', (req, res, next) => {
if (!isAuthorized(req.session.userid, PERMISSIONS.SUBJECTS.ROLES, PERMISSIONS.ACTIONS.ADD)) {
if (!isAuthorized(req.session.userId, PERMISSIONS.SUBJECTS.ROLES, PERMISSIONS.ACTIONS.ADD)) {
res.status(400).send('Unauthorized - only admins allowed');
return;
}
Expand All @@ -286,7 +280,7 @@ app.post('/api/add_role', (req, res, next) => {
// This call (only for admins) allow an admin to remove a role to a user.

app.post('/api/remove_role', (req, res, next) => {
if (!isAuthorized(req.session.userid, PERMISSIONS.SUBJECTS.ROLES, PERMISSIONS.ACTIONS.REMOVE)) {
if (!isAuthorized(req.session.userId, PERMISSIONS.SUBJECTS.ROLES, PERMISSIONS.ACTIONS.REMOVE)) {
res.status(400).send('Unauthorized - only admins allowed');
return;
}
Expand All @@ -302,7 +296,7 @@ app.post('/api/remove_role', (req, res, next) => {
// ---
const proxiesDB = lmdb.open<string, string>({ path: `${process.env.DB_PATH}proxies` });
app.post('/api/proxies', (req, res) => {
if (!isAuthorized(req.session.userid, PERMISSIONS.SUBJECTS.PROXIES, PERMISSIONS.ACTIONS.POST)) {
if (!isAuthorized(req.session.userId, PERMISSIONS.SUBJECTS.PROXIES, PERMISSIONS.ACTIONS.POST)) {
res.status(400).send('Unauthorized - only admins and operators allowed');
return;
}
Expand All @@ -317,7 +311,7 @@ app.post('/api/proxies', (req, res) => {
});

app.put('/api/proxies/:nodeAddr', (req, res) => {
if (!isAuthorized(req.session.userid, PERMISSIONS.SUBJECTS.PROXIES, PERMISSIONS.ACTIONS.PUT)) {
if (!isAuthorized(req.session.userId, PERMISSIONS.SUBJECTS.PROXIES, PERMISSIONS.ACTIONS.PUT)) {
res.status(400).send('Unauthorized - only admins and operators allowed');
return;
}
Expand Down Expand Up @@ -354,7 +348,7 @@ app.put('/api/proxies/:nodeAddr', (req, res) => {
});

app.delete('/api/proxies/:nodeAddr', (req, res) => {
if (!isAuthorized(req.session.userid, PERMISSIONS.SUBJECTS.PROXIES, PERMISSIONS.ACTIONS.DELETE)) {
if (!isAuthorized(req.session.userId, PERMISSIONS.SUBJECTS.PROXIES, PERMISSIONS.ACTIONS.DELETE)) {
res.status(400).send('Unauthorized - only admins and operators allowed');
return;
}
Expand Down Expand Up @@ -507,13 +501,13 @@ function sendToDela(dataStr: string, req: express.Request, res: express.Response
// Secure /api/evoting to admins and operators
app.put('/api/evoting/authorizations', (req, res) => {
if (
!isAuthorized(req.session.userid, PERMISSIONS.SUBJECTS.ELECTION, PERMISSIONS.ACTIONS.CREATE)
!isAuthorized(req.session.userId, PERMISSIONS.SUBJECTS.ELECTION, PERMISSIONS.ACTIONS.CREATE)
) {
res.status(400).send('Unauthorized');
return;
}
const { FormID } = req.body;
authEnforcer.addPolicy(String(req.session.userid), FormID, PERMISSIONS.ACTIONS.OWN);
authEnforcer.addPolicy(String(req.session.userId), FormID, PERMISSIONS.ACTIONS.OWN);
});

// https://stackoverflow.com/a/1349426
Expand All @@ -528,7 +522,7 @@ function makeid(length: number) {
}
app.put('/api/evoting/forms/:formID', (req, res, next) => {
const { formID } = req.params;
if (!isAuthorized(req.session.userid, formID, PERMISSIONS.ACTIONS.OWN)) {
if (!isAuthorized(req.session.userId, formID, PERMISSIONS.ACTIONS.OWN)) {
res.status(400).send('Unauthorized');
return;
}
Expand All @@ -537,7 +531,7 @@ app.put('/api/evoting/forms/:formID', (req, res, next) => {

app.post('/api/evoting/services/dkg/actors', (req, res, next) => {
const { FormID } = req.body;
if (!isAuthorized(req.session.userid, FormID, PERMISSIONS.ACTIONS.OWN)) {
if (!isAuthorized(req.session.userId, FormID, PERMISSIONS.ACTIONS.OWN)) {
res.status(400).send('Unauthorized');
return;
}
Expand All @@ -548,23 +542,23 @@ app.post('/api/evoting/services/dkg/actors', (req, res, next) => {
});
app.use('/api/evoting/services/dkg/actors/:formID', (req, res, next) => {
const { formID } = req.params;
if (!isAuthorized(req.session.userid, formID, PERMISSIONS.ACTIONS.OWN)) {
if (!isAuthorized(req.session.userId, formID, PERMISSIONS.ACTIONS.OWN)) {
res.status(400).send('Unauthorized');
return;
}
next();
});
app.use('/api/evoting/services/shuffle/:formID', (req, res, next) => {
const { formID } = req.params;
if (!isAuthorized(req.session.userid, formID, PERMISSIONS.ACTIONS.OWN)) {
if (!isAuthorized(req.session.userId, formID, PERMISSIONS.ACTIONS.OWN)) {
res.status(400).send('Unauthorized');
return;
}
next();
});
app.delete('/api/evoting/forms/:formID', (req, res) => {
const { formID } = req.params;
if (!isAuthorized(req.session.userid, formID, PERMISSIONS.ACTIONS.OWN)) {
if (!isAuthorized(req.session.userId, formID, PERMISSIONS.ACTIONS.OWN)) {
res.status(400).send('Unauthorized');
return;
}
Expand Down Expand Up @@ -604,7 +598,7 @@ app.delete('/api/evoting/forms/:formID', (req, res) => {
.status(500)
.send(`failed to proxy request: ${req.method} ${uri} - ${error.message} - ${resp}`);
});
authEnforcer.removePolicy(String(req.session.userid), formID, PERMISSIONS.ACTIONS.OWN);
authEnforcer.removePolicy(String(req.session.userId), formID, PERMISSIONS.ACTIONS.OWN);
});

// This API call is used redirect all the calls for DELA to the DELAs nodes.
Expand All @@ -613,7 +607,7 @@ app.delete('/api/evoting/forms/:formID', (req, res) => {
// DELA node To make this work, React has to redirect to this backend all the
// request that needs to go the DELA nodes
app.use('/api/evoting/*', (req, res) => {
if (!req.session.userid) {
if (!req.session.userId) {
res.status(400).send('Unauthorized');
return;
}
Expand All @@ -627,7 +621,7 @@ app.use('/api/evoting/*', (req, res) => {
// only needed to allow users to cast multiple ballots, where only the last
// ballot is taken into account. To preserve anonymity the web-backend could
// translate UserIDs to another random ID.
// bodyData.UserID = req.session.userid.toString();
// bodyData.UserID = req.session.userId.toString();
bodyData.UserID = makeid(10);
}

Expand Down
78 changes: 43 additions & 35 deletions web/frontend/src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import * as endpoints from 'components/utils/Endpoints';

const flashTimeout = 4000;

// By default we load the mock messages when not in production. This is handy
// By default, we load the mock messages when not in production. This is handy
// because it removes the need to have a backend server.
if (process.env.NODE_ENV !== 'production' && process.env.REACT_APP_NOMOCK !== 'on') {
const { dvotingserver } = require('./mocks/dvotingserver');
Expand All @@ -22,8 +22,8 @@ if (process.env.NODE_ENV !== 'production' && process.env.REACT_APP_NOMOCK !== 'o
const arr = new Map<String, Array<String>>();
const defaultAuth = {
isLogged: false,
firstname: '',
lastname: '',
firstName: '',
lastName: '',
authorization: arr,
isAllowed: (subject: string, action: string) => false,
};
Expand All @@ -36,8 +36,8 @@ export const AuthContext = createContext<AuthState>(defaultAuth);

export interface AuthState {
isLogged: boolean;
firstname: string;
lastname: string;
firstName: string;
lastName: string;
authorization: Map<String, Array<String>>;
isAllowed: (subject: string, action: string) => boolean;
}
Expand Down Expand Up @@ -224,39 +224,47 @@ const AppContainer = () => {
};

async function fetchData() {
try {
const res = await fetch(ENDPOINT_PERSONAL_INFO, req);

if (res.status !== 200) {
const txt = await res.text();
throw new Error(`unexpected status: ${res.status} - ${txt}`);
const response = await fetch(ENDPOINT_PERSONAL_INFO, req);
let result;
switch (response.status) {
case 200: {
result = await response.json();
break;
}
case 401: {
result = {
isLoggedIn: false,
firstName: '',
lastName: '',
authorization: {},
};
break;
}
default: {
const txt = await response.text();
Copy link
Contributor

Choose a reason for hiding this comment

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

if the response did not return a 200, doesn't this always throw ?

Copy link
Contributor Author

@lanterno lanterno Aug 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

if it's neither 200 nor 401

Copy link
Contributor

@pierluca pierluca Aug 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

what I meant is: isn't response.text() going to throw before your throw new Error(), when there's no HTTP body ? (i.e. no error information other than the HTTP status sent from the backend)

I'm not sure about how the .text() works in the Fetch API, but according to the documentation and the standard it seems it reads the body. Does it succeed on an empty body ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pierluca It succeeds. I tested it.

My test: I restarted the backend, and refreshed the UI causing the backend to return 504. the frontend then displayed the intended message.

throw new Error(`Unexpected status: ${response.status} - ${txt}`);
}

const result = await res.json();
setAuth({
isLogged: result.islogged,
firstname: result.firstname,
lastname: result.lastname,
authorization: result.islogged ? new Map(Object.entries(result.authorization)) : arr,
isAllowed: function (subject: string, action: string) {
return (
this.authorization.has(subject) &&
this.authorization.get(subject).indexOf(action) !== -1
);
},
});

// wait for the default proxy to be set
await setDefaultProxy();

setContent(<App />);
} catch (e: any) {
setContent(<Failed>{e.toString()}</Failed>);
console.log('error:', e);
}
setAuth({
isLogged: result.isLoggedIn,
firstName: result.firstName,
lastName: result.lastName,
authorization: result.isLoggedIn ? new Map(Object.entries(result.authorization)) : arr,
isAllowed: function (subject: string, action: string) {
return (
this.authorization.has(subject) &&
this.authorization.get(subject).indexOf(action) !== -1
);
},
});
// wait for the default proxy to be set
await setDefaultProxy();
setContent(<App />);
}

fetchData();
fetchData().catch((e) => {
setContent(<Failed>{e.toString()}</Failed>);
Copy link
Contributor

Choose a reason for hiding this comment

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

also, be careful. Here you're changing the content of the page, I assume.
You shouldn't do that in an async context without verifying if the component is still loaded or not.
You might want to have a read
https://devtrium.com/posts/async-functions-useeffect

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 believe that is correct, but it already existed like this, so I wouldn't want to change it in this PR.

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 still I'm gonna take the advice for when I can apply am improvement confidently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, I'm just reviewing some of this code for the first time - so I'm pointing out issues as I see them. That doesn't mean I expect all the fixes in the same PR :)

console.log('error:', e);
});
}, []);

return (
Expand Down
Loading