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: split code into modules #343

Merged
merged 21 commits into from
Sep 20, 2023

Conversation

lanterno
Copy link
Contributor

@lanterno lanterno commented Sep 6, 2023

Splits the one-file project into different modules for different purposes.

I think this is a good first iteration, and I will keep trying to improve the structure as I go.

You will also see that I refactored the authentication part a little bit as I moved it to a separate module authManager.ts.

I believe all the module names will still evolve in the future, so no need to think about it too much right now.

@lanterno lanterno requested a review from PascalinDe September 6, 2023 08:09
@lanterno lanterno requested a review from a team as a code owner September 6, 2023 08:09

res
.status(500)
.send(`failed to proxy request: ${req.method} ${uri} - ${error.message} - ${resp}`);

Check failure

Code scanning / SonarCloud

Endpoints should not be vulnerable to reflected cross-site scripting (XSS) attacks

<!--SONAR_ISSUE_KEY:AYppjV0pjKAggFKLfO_K-->Change this code to not reflect user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=dedis_d-voting&issues=AYppjV0pjKAggFKLfO_K&open=AYppjV0pjKAggFKLfO_K&pullRequest=343">SonarCloud</a></p>
// we strip the `/api` part: /api/form/xxx => /form/xxx
const uri = process.env.DELA_NODE_URL + xss(req.url.slice(4));

axios({

Check warning

Code scanning / SonarCloud

Server-side requests should not be vulnerable to forging attacks

<!--SONAR_ISSUE_KEY:AYppjV0pjKAggFKLfO_I-->Change this code to not construct the URL from user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=dedis_d-voting&issues=AYppjV0pjKAggFKLfO_I&open=AYppjV0pjKAggFKLfO_I&pullRequest=343">SonarCloud</a></p>

console.log('sending payload:', JSON.stringify(payload), 'to', uri);

axios({

Check warning

Code scanning / SonarCloud

Server-side requests should not be vulnerable to forging attacks

<!--SONAR_ISSUE_KEY:AYppjV0pjKAggFKLfO_J-->Change this code to not construct the URL from user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=dedis_d-voting&issues=AYppjV0pjKAggFKLfO_J&open=AYppjV0pjKAggFKLfO_J&pullRequest=343">SonarCloud</a></p>
@lanterno lanterno force-pushed the refactor/moves-auth-code-to-different-module branch from 8c9fabf to f8fa598 Compare September 6, 2023 09:00
@lanterno lanterno requested a review from PascalinDe September 7, 2023 01:01
@ineiti
Copy link
Member

ineiti commented Sep 7, 2023

Regarding the SonarCloud complaints: is this from moved code, or from code you wrote? If it's the latter, it should be fixed. For the former, we'll ask Pierluca to override the block.

@lanterno
Copy link
Contributor Author

lanterno commented Sep 7, 2023

Regarding the SonarCloud complaints: is this from moved code, or from code you wrote? If it's the latter, it should be fixed. For the former, we'll ask Pierluca to override the block.

@ineiti All of it from moved code.

PascalinDe
PascalinDe previously approved these changes Sep 7, 2023
@ineiti
Copy link
Member

ineiti commented Sep 7, 2023

Regarding the SonarCloud complaints: is this from moved code, or from code you wrote? If it's the latter, it should be fixed. For the former, we'll ask Pierluca to override the block.

@ineiti All of it from moved code.

@pierluca I propose to override the complaints then.

@lanterno lanterno force-pushed the refactor/moves-auth-code-to-different-module branch from 7be6ee6 to 8750939 Compare September 13, 2023 17:53
@coveralls
Copy link

coveralls commented Sep 13, 2023

Pull Request Test Coverage Report for Build 6235084693

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 57.391%

Totals Coverage Status
Change from base Build 6197509007: 0.0%
Covered Lines: 3269
Relevant Lines: 5696

💛 - Coveralls

@ineiti
Copy link
Member

ineiti commented Sep 14, 2023

@lanterno good to go now.

Copy link
Contributor

@pierluca pierluca left a comment

Choose a reason for hiding this comment

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

A couple minor points, but this is so much better. Thank youuuu !!!

@lanterno
Copy link
Contributor Author

@pierluca Thanks for the review.. I applied most of your comments. Check them out.. This one, I might not have understood it very well.

@pierluca
Copy link
Contributor

I also just checked something, documentation-wise:

https://github.com/expressjs/session

Note Since version 1.5.0, the cookie-parser middleware no longer needs to be used for this module to work. This module now directly reads and writes cookies on req/res. Using cookie-parser may result in issues if the secret is not the same between this module and cookie-parser.

I haven't checked the versions but it's something to keep an eye on.

@lanterno lanterno force-pushed the refactor/moves-auth-code-to-different-module branch from d84b62d to feaf710 Compare September 19, 2023 11:13
@lanterno
Copy link
Contributor Author

-- update --
Rebased over main

Ahmed Elghareeb added 2 commits September 19, 2023 13:43
New versions of expressJs don't need this anymore
Notice that the http method was also switches from POST to GET. Yes, that works the same. GET is obviously better for us.
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability E 3 Vulnerabilities
Security Hotspot E 2 Security Hotspots
Code Smell A 6 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@lanterno
Copy link
Contributor Author

lanterno commented Sep 19, 2023

@ineiti @pierluca @PascalinDe Can I get another review here please? I think it's ready.

btw @pierluca , regarding your comment:

I also just checked something, documentation-wise:

https://github.com/expressjs/session

Note Since version 1.5.0, the cookie-parser middleware no longer needs to be used for this module to work. This module now directly reads and writes cookies on req/res. Using cookie-parser may result in issues if the secret is not the same between this module and cookie-parser.

I haven't checked the versions but it's something to keep an eye on.

I removed the middleware and it still works! 👍

Copy link
Member

@ineiti ineiti left a comment

Choose a reason for hiding this comment

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

Let's get this in.

Copy link
Contributor

@PascalinDe PascalinDe left a comment

Choose a reason for hiding this comment

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

OK for me as well

@PascalinDe
Copy link
Contributor

heya, managed to set up integration test environment on this head

I cannot shuffle an election, I get the following error message:

docker-compose-dela-worker-1-1  | 2023-09-20T13:44:50Z INF d-voting/services/shuffle/neff/handler.go:85 > Starting the neff shuffle protocol ...
docker-compose-dela-worker-1-1  | 2023-09-20T13:44:50Z DBG pkg/mod/go.dedis.ch/dela@v0.0.0-20230907131212-0d61b081b4af/core/txn/signed/mod.go:332 > manager synchronized nonce=0
docker-compose-dela-worker-1-1  | 2023-09-20T13:44:51Z INF pkg/mod/go.dedis.ch/dela@v0.0.0-20230907131212-0d61b081b4af/core/ordering/cosipbft/pbft/mod.go:390 > finalize round with leader: 0
docker-compose-dela-worker-1-1  | 2023-09-20T13:44:51Z INF pkg/mod/go.dedis.ch/dela@v0.0.0-20230907131212-0d61b081b4af/core/ordering/cosipbft/mod.go:387 > block event addr=dela-worker-1:2000 index=16 root=d569eee5
docker-compose-dela-worker-1-1  | 2023-09-20T13:44:51Z DBG pkg/mod/go.dedis.ch/dela@v0.0.0-20230907131212-0d61b081b4af/core/ordering/cosipbft/blocksync/default.go:230 > received synchronization message addr=dela-worker-1:2000 index=16
docker-compose-dela-worker-1-1  | 2023-09-20T13:44:51Z WRN pkg/mod/go.dedis.ch/dela@v0.0.0-20230907131212-0d61b081b4af/core/ordering/cosipbft/pbft/mod.go:639 > transaction not accepted addr=dela-worker-1:2000 reason="failed to shuffle ballots: wrong shuffle round: expected round '1', transaction is for round '0'"

but the interface proceeds to the next step (decrypting), which of course fails as well, since the ballots have not been shuffled

I will try to reproduce on the main branch just to make sure although I haven't seen this error before

Copy link
Contributor

@PascalinDe PascalinDe left a comment

Choose a reason for hiding this comment

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

on-going integration tests

Copy link
Contributor

@PascalinDe PascalinDe left a comment

Choose a reason for hiding this comment

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

the good news is that I can reproduce on the main's head, so it's not this PR that introduced the error

the bad news is that this means we have a regression on the main branch... I'll open a ticket and look into it: #361

Copy link
Contributor

@pierluca pierluca left a comment

Choose a reason for hiding this comment

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

All lights are green for me !

@pierluca pierluca merged commit ca86384 into main Sep 20, 2023
@pierluca pierluca deleted the refactor/moves-auth-code-to-different-module branch September 20, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants