Readjustement of the introduction image and first version of casbin authorization mechanism #201

Ghita2002 · 2022-11-01T13:35:26Z

No description provided.

coveralls · 2022-11-01T13:39:03Z

Pull Request Test Coverage Report for Build 3517567483

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-7.2%) to 56.825%

Files with Coverage Reduction	New Missed Lines	%
services/dkg/pedersen/controller/action.go	4	48.97%

Totals
Change from base Build 3320078377:	-7.2%
Covered Lines:	3272
Relevant Lines:	5758

💛 - Coveralls

pierluca

Nice progress !
I just made a quick pass on the codebase, this is not a full in-depth review and I'll let @emduc and @nkcr give their own feedback.

There are a couple of remarks that apply throughout. Let me know if you have questions 😄

web/backend/model.conf

pierluca · 2022-11-02T16:30:06Z

web/backend/src/Server.ts

+// function isAuthorized(roles: string[], req: express.Request): boolean {
+// return true;
+// if (!req.session || !req.session.userid) {
+// return false;
+// }

-  const { role } = req.session;
+// const { role } = req.session;

-  return roles.includes(role as string);
-}
+// return roles.includes(role as string);
+// }


feel free to delete code that is not relevant anymore. Commented out code is generally frowned upon 😄

pierluca · 2022-11-02T16:31:27Z

web/backend/src/Server.ts

-    usersDB.getRange(opts).map(({ key, value }) => ({ id: '0', sciper: key, role: value }))
-  );
-  res.json(users);
+app.get('/api/user_rights', async (req, res) => {


are you sure that express supports async handlers natively?

It's my mistake, I forgot to delete it. I was just trying some things while testing.

pierluca · 2022-11-02T16:34:04Z

web/backend/src/Server.ts

-  );
-  res.json(users);
+app.get('/api/user_rights', async (req, res) => {
+  e.then((enforcer) =>


if you have an async function, you don't need to do .then and .catch, you can just use await where you would use a then, and try/catch instead of .catch.

see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await

pierluca · 2022-11-02T16:34:29Z

web/backend/src/Server.ts

+      .catch((erreur) => console.log('error', erreur))
+  ).catch((erreur1) => console.log('error', erreur1));


avoid french words in the codebase 😁
also, you can reuse the same variable name, as it's scoped just to that function.

Thanks for the view, it is fixed.

- Updates the initialisation of Casbin so that we start only when the enforcer is ready - Uses enforceSync, which makes the checking mechanism was easier - Adds a utility function isAuthorized

Uses enforceSync to check authorizations

…voting into d-voting_frontend_ghita

pierluca · 2022-11-15T14:10:50Z

web/backend/src/Server.ts

+Promise.all([enforcerLoading])
+  .then((res) => {
+    [enf] = res;


Do you need a Promise.all if you're looking at just one promise resolving?
Why can't you do the following ?

Suggested change

Promise.all([enforcerLoading])

.then((res) => {

[enf] = res;

enforcerLoading

.then((enf) => {

I may be missing something here, so do let me know if I'm wrong 😁

pierluca · 2022-11-15T14:11:56Z

web/backend/src/Server.ts

+let enf: Enforcer;
+
+const enforcerLoading = newEnforcer('model.conf', 'policy.csv');
+const e = newEnforcer('model.conf', 'policy.csv');


this looks unused

pierluca · 2022-11-15T14:12:27Z

web/backend/src/Server.ts

@@ -26,6 +26,27 @@ const app = express();

 app.use(morgan('tiny'));

+let enf: Enforcer;
+
+const enforcerLoading = newEnforcer('model.conf', 'policy.csv');


do you need this to be in the global scope, or could you put it (along with the promise resolution below) in an initialization function ?

web/backend/src/Server.ts

pierluca · 2022-11-16T08:50:36Z

web/backend/src/Server.ts

-  }
-
-  next();
+  e.then((enforcer) => {


is there are a reason not to use isAuthorized here?

I just forget it. It is fixed !

pierluca · 2022-11-16T08:52:43Z

web/backend/src/Server.ts

+  e.then((enforcer) => {
+    enforcer
+      .enforce(req.session.userid, 'election', 'create')
+      .then((isOk) => {
+        if (isOk) {
+          next();
+        } else {
+          res.status(400).send('Unauthorized - only admins and operators allowed');
+        }
+      })
+      .catch((error) => console.log('error', error));
+  }).catch((error1) => console.log('error', error1));


As per our discussion today, this should work:

Suggested change

e.then((enforcer) => {

enforcer

.enforce(req.session.userid, 'election', 'create')

.then((isOk) => {

if (isOk) {

next();

} else {

res.status(400).send('Unauthorized - only admins and operators allowed');

}

})

.catch((error) => console.log('error', error));

}).catch((error1) => console.log('error', error1));

e.then((enforcer) => {

return enforcer

.enforce(req.session.userid, 'election', 'create')

.then((isOk) => {

if (isOk) {

next();

} else {

res.status(400).send('Unauthorized - only admins and operators allowed');

}

})

}).catch((error) => console.log('error', error));

No need to do a double-catch 😁
And since you're directly returning a value, you could also do this (notice the absence of {} and return):

Suggested change

e.then((enforcer) => {

enforcer

.enforce(req.session.userid, 'election', 'create')

.then((isOk) => {

if (isOk) {

next();

} else {

res.status(400).send('Unauthorized - only admins and operators allowed');

}

})

.catch((error) => console.log('error', error));

}).catch((error1) => console.log('error', error1));

e.then((enforcer) =>

enforcer

.enforce(req.session.userid, 'election', 'create')

.then((isOk) => {

if (isOk) {

next();

} else {

res.status(400).send('Unauthorized - only admins and operators allowed');

}

})).catch((error) => console.log('error', error));

I modify that using the proposition of Noemien.

pierluca · 2022-11-16T08:53:07Z

web/backend/src/Server.ts

+// const port = process.env.PORT || 5000;
+// app.listen(port);

-console.log(`App is listening on port ${port}`);
+// console.log(`App is listening on port ${port}`);


feel free to delete code, no need to comment it out 😁

pierluca · 2022-11-22T10:27:10Z

web/frontend/src/layout/NavBar.tsx

@@ -177,7 +181,7 @@ const LeftSideNavBar = ({ authCtx, t }) => (
          className={'text-black text-lg hover:text-indigo-700'}>
          {t('navBarStatus')}
        </NavLink>
-        {authCtx.role === 'admin' && authCtx.isLogged && (
+        {authCtx.isLogged && authCtx.authorization.get('roles').indexOf('list') > -1 && (


you could define and use the constants on the front-end as well.

pierluca · 2022-11-22T10:27:19Z

web/frontend/src/layout/NavBar.tsx

+      authCtx.authorization.has('election') &&
+      authCtx.authorization.get('election').indexOf('create') > -1 && (


pierluca · 2022-11-22T10:27:29Z

web/frontend/src/layout/NavBar.tsx

+                authCtx.authorization.has('election') &&
+                authCtx.authorization.get('election').indexOf('create') > -1 && (


pierluca · 2022-11-22T10:27:35Z

web/frontend/src/layout/NavBar.tsx

@@ -67,7 +67,7 @@ const MobileMenu = ({ authCtx, handleLogout, fctx, t }) => (
                    </Popover.Button>
                  </NavLink>
                }
-                {authCtx.isLogged && (authCtx.role === 'admin' || authCtx.role === 'operator') && (
+                {authCtx.isLogged && authCtx.authorization.get('roles').indexOf('add') > -1 && (


pierluca · 2022-11-22T10:42:11Z

web/backend/src/Server.ts

+    for (let i = 0; i < list.length; i += 1) {
+      for (let j = 1; j < list[0].length; j += +2) {
+        console.log(list[i][j]);
+        if (m.has(list[i][j])) {
+          m.get(list[i][j])?.push(list[i][j + 1]);
+        } else {
+          m.set(list[i][j], [list[i][j + 1]]);
+        }
+      }
+    }


this kind of complex operations probably deserves its own function, that you can write a short test for.

outside the scope of this PR: would it make sense to memoize this for performance reasons?

pierluca · 2022-11-22T11:12:30Z

web/backend/src/Server.ts

+      for (let j = 1; j < list[0].length; j += +2) {
+        console.log(list[i][j]);
+        if (m.has(list[i][j])) {
+          m.get(list[i][j])?.push(list[i][j + 1]);


do you really need the ? if you already have called m.has ?
conversely, why not just do m.get and do the push if the result is not null ?

Even if I check m.has I still have the error (Object is possibly 'undefined') This why I used ?. However it can be a good way to test the result of m.get. I was just wondering how it is better than using ?

pierluca · 2022-11-22T11:14:02Z

web/frontend/src/layout/NavBar.tsx

+    {authCtx.isLogged &&
+      authCtx.authorization.has('election') &&
+      authCtx.authorization.get('election').indexOf('create') > -1 && (


I was thinking that here (and elsewhere) you're mixing up the details of the implementation (authorization is a map, permissions an array) and the actual concern (verifying access).

Could you add a helper function that will deal with these details ?
So that here (and elsewhere) you can just call something like hasAuthorization(authCtx, ROLES, LIST) ?
This will make it easier to refactor (if needed, in the future) the actual implementation details.

Thanks for this review. It is quite a good idea. I implement it !

nkcr · 2022-11-28T09:32:34Z

web/backend/src/Server.ts

@@ -154,49 +185,58 @@ app.post('/api/logout', (req, res) => {
 // As the user is logged on the app via this express but must also be logged in
 // the react. This endpoint serves to send to the client (actually to react)
 // the information of the current user.


The comment is for the statement bellow, not the setMapAuthorization function, which must also have its own comment describing what it does.

This function is hard to understand without any explanation. You should at least describe what contains list: string[][]).

I added some comments and while doing it I noticed that the iteration on j isn't necessary.

nkcr · 2022-11-28T09:34:31Z

web/backend/src/Server.ts

+function setMapAuthorization(list: string[][]) {
+  const m = new Map<String, Array<String>>();
+  for (let i = 0; i < list.length; i += 1) {
+    for (let j = 1; j < list[0].length; j += +2) {


you should explain (1) what is contained in list[0], and (2) why you are doing += 2.

I delete that.

nkcr · 2022-11-28T09:34:51Z

web/backend/src/Server.ts

+  const m = new Map<String, Array<String>>();
+  for (let i = 0; i < list.length; i += 1) {
+    for (let j = 1; j < list[0].length; j += +2) {
+      console.log(list[i][j]);


you probably used it for debugging. To be removed.

nkcr · 2022-11-28T09:41:18Z

web/backend/src/Server.ts

+app.get('/api/personal_info', (req, res) => {
+  let m = new Map<String, Array<String>>();
+  enf.getFilteredPolicy(0, String(req.session.userid)).then((list) => {
+    m = setMapAuthorization(list);


If the user is not logged you can avoid calling setMapAuthorization and return an empty map.

nkcr · 2022-11-28T09:50:00Z

web/backend/src/Server.ts


 const MemoryStore = createMemoryStore(session);
-
+const ROLES = 'roles';


This is way too generic. You should prefix the variable name to indicate how the variable is used:

SUBJECT_ROLES = 'roles'; SUBJECT_PROXIES = 'proxies'; ... ACTION_LIST = 'list'; ACTION_REMOVE = 'remove'; ...

nkcr · 2022-11-28T09:51:12Z

web/frontend/src/layout/NavBar.tsx

@@ -24,6 +24,17 @@ import { Popover, Transition } from '@headlessui/react';
 import { LoginIcon, LogoutIcon, MenuIcon, XIcon } from '@heroicons/react/outline';
 import { PlusIcon } from '@heroicons/react/solid';

+const ELECTION = 'election';


You should prefix the variable name:

SUBJECT_ELECTION = ... ... ACTION_CREATE = ...

nkcr · 2022-11-28T09:53:00Z

web/frontend/src/layout/NavBar.tsx

+
+function hasAuthorization(authCtx, subject: string, action: string): boolean {
+  return (
+    authCtx.authorization.has(subject) && authCtx.authorization.get(subject).indexOf(action) > -1


Suggested change

authCtx.authorization.has(subject) && authCtx.authorization.get(subject).indexOf(action) > -1

authCtx.authorization.has(subject) && authCtx.authorization.get(subject).indexOf(action) !== -1

pierluca

Not absolutely perfect, but sooooo much better, great job ! 👍

I've some minor remarks, but at this point it looks nearly mergeable to me :)

pierluca · 2022-11-28T13:53:56Z

web/backend/src/Server.ts

+// list[0] contains the policies so list[i][0] is the sciper
+// list[i][1] is the object and list[i][2] is the action
+function setMapAuthorization(list: string[][]) {
+  const m = new Map<String, Array<String>>();


do you want to use String the object or just string the plain data type?

When searching which one use, I found that it is better to avoid bugs to use string

pierluca · 2022-11-28T13:55:17Z

web/backend/src/Server.ts

+// an object to the action authorized
+// list[0] contains the policies so list[i][0] is the sciper
+// list[i][1] is the object and list[i][2] is the action
+function setMapAuthorization(list: string[][]) {


in typescript it's good to explicitly indicate the return type.

Suggested change

function setMapAuthorization(list: string[][]) {

function setMapAuthorization(list: string[][]): Map<string, Array<string>> {

pierluca · 2022-11-28T13:55:38Z

web/backend/src/Server.ts

@@ -207,7 +215,7 @@ app.get('/api/personal_info', (req, res) => {
        firstname: req.session.firstname,
        role: req.session.role,
        islogged: true,
-        authorization: Object.fromEntries(m),
+        authorization: Object.fromEntries(setMapAuthorization(list)),


much cleaner :)

pierluca · 2022-11-28T13:55:53Z

web/backend/src/Server.ts

+  console.log(m);
+  return m;
+}
+
 // As the user is logged on the app via this express but must also be logged in
 // the 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) => {
  const m = new Map<String, Array<String>>();


do you still need this variable?

I decide to delete it

nkcr · 2022-11-28T13:54:32Z

web/backend/src/Server.ts

+function setMapAuthorization(list: string[][]) {
+  const m = new Map<String, Array<String>>();
+  for (let i = 0; i < list.length; i += 1) {
+    if (m.has(list[i][1])) {


The comment is already a great improvement. 👍

Now, the next step is self-explanatory code.. ;)

You could do:

const [subject, action] = list[i][1], list[i][2]; if (m.has(subject)) { ...

Thanks for the review ! Done

nkcr · 2022-11-28T13:55:10Z

web/backend/src/Server.ts

+  const m = new Map<String, Array<String>>();
+  for (let i = 0; i < list.length; i += 1) {
+    if (m.has(list[i][1])) {
+      m.get(list[i][1])?.push(list[i][2]);


I'm not sure I understand the ?. Is it to prevent adding multiple times the same action ?

It's necessary, however I check that m.has(list[i][1], to make sure that m.get(list[i][1] is not undefined.

nkcr · 2022-11-28T13:57:53Z

web/backend/src/Server.ts

+        firstname: '',
+        role: '',
+        islogged: false,
+        authorization: Object.fromEntries(m),


I think you can just use authorization: new Map<String, Array<String>>(),
or even just authorization: {},

sonarqubecloud · 2022-11-28T16:19:03Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
9 Code Smells

0.0% Coverage
0.0% Duplication

nkcr

Good to go 🚀

Readjustement of the introduction image

d0e75e7

Ghita2002 requested a review from a team as a code owner November 1, 2022 13:35

Adding Casbin for the authorization mechanism

4d938f8

Ghita2002 marked this pull request as draft November 1, 2022 13:44

Ghita2002 requested a review from nkcr November 1, 2022 13:44

Ghita2002 self-assigned this Nov 1, 2022

Ghita2002 added the web frontend label Nov 1, 2022

Ghita2002 added 5 commits November 1, 2022 14:53

Some refactoring

18ddcf4

Some refactoring

6704510

add some authorizations

2032ebe

Adding last sciper for authorization

8bd2661

trying fixing the bug

812635c

Ghita2002 marked this pull request as ready for review November 2, 2022 14:09

pierluca reviewed Nov 2, 2022

View reviewed changes

Replace french words, add comments

9eef052

Ghita2002 changed the title ~~Readjustement of the introduction image~~ Readjustement of the introduction image and first version of casbin authorization mechanism Nov 7, 2022

Uses enforceSync to check authorizations

ce7eff5

- Updates the initialisation of Casbin so that we start only when the enforcer is ready - Uses enforceSync, which makes the checking mechanism was easier - Adds a utility function isAuthorized

nkcr added the web backend label Nov 7, 2022

Ghita2002 and others added 4 commits November 13, 2022 14:11

Merge pull request #204 from dedis/casbin-async

bd59c49

Uses enforceSync to check authorizations

Add enforceSync to check authorizations

51e23b7

Merge branch 'd-voting_frontend_ghita' of https://github.com/dedis/d-…

3dc4943

…voting into d-voting_frontend_ghita

Correct some lint errors

f262a4f

pierluca reviewed Nov 15, 2022

View reviewed changes

pierluca reviewed Nov 16, 2022

View reviewed changes

nkcr requested a review from pierluca November 21, 2022 08:11

Ghita2002 added 3 commits November 21, 2022 20:36

Answering reviewers, adding auth in personal_info

821891a

Fix some lint erros

cfd6ba5

Fix eslint no-plusplus

e9a99e9

pierluca reviewed Nov 22, 2022

View reviewed changes

Replacing strings with constant and modularizing

694e98b

Ghita2002 requested a review from pierluca November 23, 2022 16:39

nkcr suggested changes Nov 28, 2022

View reviewed changes

Ghita2002 added 2 commits November 28, 2022 14:34

Add some comments + changes requests the reviewers

28c0ce2

Fix the eslint error

810e3dd

Ghita2002 requested a review from nkcr November 28, 2022 13:48

pierluca reviewed Nov 28, 2022

View reviewed changes

nkcr reviewed Nov 28, 2022

View reviewed changes

Some changes answering the comments

2478fd1

Ghita2002 requested review from nkcr and pierluca November 28, 2022 16:20

nkcr approved these changes Nov 28, 2022

View reviewed changes

Ghita2002 merged commit 0f99cbf into main Nov 28, 2022

Ghita2002 deleted the d-voting_frontend_ghita branch November 28, 2022 18:10

		.catch((erreur) => console.log('error', erreur))
		).catch((erreur1) => console.log('error', erreur1));

		authCtx.authorization.has('election') &&
		authCtx.authorization.get('election').indexOf('create') > -1 && (


		const MemoryStore = createMemoryStore(session);

		const ROLES = 'roles';

	authCtx.authorization.has(subject) && authCtx.authorization.get(subject).indexOf(action) > -1
	authCtx.authorization.has(subject) && authCtx.authorization.get(subject).indexOf(action) !== -1

	function setMapAuthorization(list: string[][]) {
	function setMapAuthorization(list: string[][]): Map<string, Array<string>> {

Readjustement of the introduction image and first version of casbin authorization mechanism #201

Readjustement of the introduction image and first version of casbin authorization mechanism #201

Conversation

Ghita2002 commented Nov 1, 2022

coveralls commented Nov 1, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3517567483

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

pierluca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierluca left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 28, 2022

nkcr left a comment

Choose a reason for hiding this comment

coveralls commented Nov 1, 2022 •

edited

Loading

pierluca left a comment •

edited

Loading