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.

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 files structure and do some little fixes #6

Merged
merged 9 commits into from
Aug 10, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
50 changes: 36 additions & 14 deletions app.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,32 @@
*/
import express from 'express';
import logger from 'morgan';
import session from 'express-session';

import userInfosHelper from './helpers/userInfo';
import idHintHelper from './helpers/idHintToken';
import logoutHelper from './helpers/logout';
import authorizationHelper from './helpers/authorization';
import utilsHelper from './helpers/utils';
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use destructured import to avoid too generic naming like "utilsHelper":

import { getAuthorizationUrl } from './helpers/utils';

import getAccessTokenHelper from './helpers/accessToken';
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use destructured import to avoid useless variables like "getAccessTokenHelper":

import { getAccessToken } from './helpers/utils';


const app = express();
const port = process.env.PORT || '3000';
Copy link
Collaborator

Choose a reason for hiding this comment

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


/**
* session config
* @type {{secret: string, cookie: {}, saveUninitialized: boolean}}
*/
const sess = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

sess => session

secret: 'demo secret', // put your own secret
cookie: {},
saveUninitialized: true,
};

/**
* session config for production
*/
if (app.get('env') === 'production') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

are you sure it's needed ? This project intend to be the simpliest as possible. We must keep in mind that PHPers must understand this code

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think that simple enough even me a PHPer can understand that piece of code!!

app.set('trust proxy', 1); // trust first proxy
sess.cookie.secure = true; // serve secure cookies
}

if (process.env.NODE_ENV !== 'test') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have added comments in https://github.com/sandybemonkey/fd_mock/blob/use-csv-database/src/app.js, it could be a good thing to have the same comments in both files

app.use(logger('dev'));
}
Expand All @@ -22,7 +38,7 @@ app.set('view engine', 'ejs');
app.set('port', port);

app.use(express.static('public'));

app.use(session(sess));

/**
* Routes
Expand All @@ -31,34 +47,40 @@ app.get('/', (req, res) => {
res.render('pages/index');
});

/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can use line comment syntax instead of group syntax comment for line comment:

// Init authorization and login process

* Init authorization and login process
*/
app.get('/login', (req, res) => {
res.redirect(authorizationHelper.getAuth());
res.redirect(utilsHelper.getAuthorizationUrl());
});

/**
* Getting the access token required to get the user data
*/
app.get('/callback', (req, res) => {
// check if the mandatory Authorization code is there.
if (!req.query.code) {
res.sendStatus(400);
}
getAccessTokenHelper.getAccessToken(res, req.query.code);
getAccessTokenHelper.getAccessToken(res, req);
});

app.get('/profile', (req, res) => {
/**
* Getting the user informations by calling helpers/userInfo.
* @type {{}}
*/
const user = userInfosHelper.sendUserInfo();
const user = req.session.userInfo;
res.render('pages/profile', { user });
});

/**
* Init logout process
*/
app.get('/logout', (req, res) => {
res.redirect(logoutHelper.logout());
res.redirect(utilsHelper.getLogoutUrl(req));
});

app.get('/end', (req, res) => {
// resetting the id token hint.
idHintHelper.resetHintToken();
req.session.id_token = null;
req.session.userInfo = null;
res.render('pages/end');
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

please add a message in the console when the server start (see https://github.com/sandybemonkey/fd_mock/blob/use-csv-database/src/app.js#L33)

Expand Down
20 changes: 10 additions & 10 deletions helpers/accessToken.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,28 @@
import axios from 'axios';
import querystring from 'querystring';
import getUserHelper from './user';
Copy link
Collaborator

Choose a reason for hiding this comment

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

use destructured import

import idHintToken from './idHintToken';
import config from '../config/config.json';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please destructure this import


const sendTokenToFD = false;
const tokenUrl = config.TOKEN_URL;
const redirectUrl = config.REDIRECT_URL;
const clientId = config.CLIENT_SECRET;
const secretKey = config.SECRET_KEY;

/**
* Init FranceConnect authentication login process.
* Make every http call to the different API endpoints.
* @see @link{ https://partenaires.franceconnect.gouv.fr/fcp/fournisseur-service# }
Copy link
Collaborator

Choose a reason for hiding this comment

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

plz mention this link in only one comment in this file

*/
exports.getAccessToken = async (res, queryCode) => {
const tokenUrl = config.TOKEN_URL;
const redirectUrl = config.REDIRECT_URL;
const clientId = config.CLIENT_SECRET;
const secretKey = config.SECRET_KEY;

exports.getAccessToken = async (res, req) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I made a comment about using the export keyword instead of this #5 (comment) . Did you see it ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

function with req & res as parameters = controller (or middleware)

=> plz move this file out of the helpers folder

// Set request params.
const url = tokenUrl;
const body = {
grant_type: 'authorization_code',
redirect_uri: redirectUrl,
client_id: clientId,
client_secret: secretKey,
code: queryCode,
code: req.query.code,
};
const headerConfig = {
headers: {
Expand All @@ -42,9 +40,11 @@ exports.getAccessToken = async (res, queryCode) => {
await axios.post(url, querystring.stringify(body), headerConfig)
Copy link
Collaborator

Choose a reason for hiding this comment

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

querystring.stringify => To my opinion this is typically the kind of library we should avoid. A simple reduce in native javascript can do the job.

Copy link
Collaborator

Choose a reason for hiding this comment

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

you should use the params option and remove this lib.

See axios/axios#739 (comment).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Try with params but use querystring as the documention said.

.then(response => response.data)
.then((tokenData) => {
idHintToken.setHintToken(tokenData.id_token);
req.accessToken = tokenData.access_token;
req.session.id_token = tokenData.id_token;
/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

plz do not use jsdoc inside functions

* Use to send the access token to an data provider.
* @return an object of data from the provider.
Copy link
Collaborator

Choose a reason for hiding this comment

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

an object of data ?

* @see @link{ https://partenaires.franceconnect.gouv.fr/fcp/fournisseur-service# }
* @see @link{ https://partenaires.franceconnect.gouv.fr/fcp/fournisseur-donnees }
* @see @link{ https://github.com/france-connect/data-providers-examples }
Expand All @@ -64,7 +64,7 @@ exports.getAccessToken = async (res, queryCode) => {
/**
* Make a call to the France Connect API endpoint to get user data.
*/
getUserHelper.getUser(tokenData.access_token, res);
getUserHelper.getUser(req, res);
})
.catch(err => res.send(err.message));
};
9 changes: 0 additions & 9 deletions helpers/authorization.js

This file was deleted.

29 changes: 0 additions & 29 deletions helpers/idHintToken.js

This file was deleted.

13 changes: 0 additions & 13 deletions helpers/logout.js

This file was deleted.

10 changes: 5 additions & 5 deletions helpers/user.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,21 @@
*/

import axios from 'axios';
import userInfosHelper from './userInfo';
import config from '../config/config.json';

exports.getUser = async (token, res) => {
if (!token) res.sendStatus(401);
exports.getUser = async (req, res) => {
if (!req.accessToken) res.status(401).send('Access token is required');
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should return here, to stop the execution of the function here

// Set request header
const headerConfig = {
headers: {
Authorization: `Bearer ${token}`,
Authorization: `Bearer ${req.accessToken}`,
},
};
await axios.get(config.USERINFO_URL, headerConfig)
.then((response) => {
// Helper to set userInfo value available to the profile page.
userInfosHelper.getUserInfo(response.data);
req.session.userInfo = response.data;

res.redirect('profile');
})
.catch(err => res.send(err.message));
Expand Down
26 changes: 0 additions & 26 deletions helpers/userInfo.js

This file was deleted.

18 changes: 18 additions & 0 deletions helpers/utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* Format the url use in the redirection call
* to the France Connect Authorization and logout API endpoint.
* @see @link{ https://partenaires.franceconnect.gouv.fr/fcp/fournisseur-service# }
*/
import config from '../config/config.json';

exports.getAuthorizationUrl = () => `${config.AUTHORIZATION_URL}?response_type=code`
+ `&client_id=${config.CLIENT_SECRET}&redirect_uri=${config.REDIRECT_URL}`
+ `&scope=${config.SCOPE}&state=${config.STATE}&nonce=${config.NONCE}`;


/**
* Format the url 's that is used in a redirect call to France Connect logout API endpoint
* @returns {string}
*/
exports.getLogoutUrl = req => `${config.LOGOUT_URL}?id_token_hint=${req.session.id_token}`
+ `&state=${config.STATE}&post_logout_redirect_uri=${config.LOGOUT_REDIRECT_URL}`;
Loading