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

PLAT 638 Adjust login wih session and cookies #1180

Merged
merged 47 commits into from
Mar 24, 2023

Conversation

nour-borgi
Copy link
Contributor

In this PR:

  • Added koa-passport
  • Added koa-session
  • Added koa-compose to handle multiple middlewares

Now authentication can be done by either "basic" or "local" types.
"local" means through the UI with username and password, this will create a session and set cookies in the browser.
"basic" means with basic auth either through browser or postman by giving also username and password.

In Mongo, added passport and session tables to store auth info.

Copy link
Collaborator

@marrouchi marrouchi left a comment

Choose a reason for hiding this comment

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

Amazing work @nour-borgi , left a couple of comments !

package.json Outdated Show resolved Hide resolved
src/api/passport.js Outdated Show resolved Hide resolved
src/api/passport.js Outdated Show resolved Hide resolved
src/api/passport.js Outdated Show resolved Hide resolved
src/api/passport.js Outdated Show resolved Hide resolved
src/middleware/sessionStore.js Outdated Show resolved Hide resolved
src/model/passport.js Outdated Show resolved Hide resolved
src/passport.js Show resolved Hide resolved
src/protocols/local.js Outdated Show resolved Hide resolved
src/protocols/local.js Outdated Show resolved Hide resolved
src/middleware/sessionStore.js Outdated Show resolved Hide resolved
src/server.js Outdated Show resolved Hide resolved
src/server.js Outdated Show resolved Hide resolved
marrouchi
marrouchi previously approved these changes Feb 2, 2023
src/middleware/tlsAuthentication.js Outdated Show resolved Hide resolved
src/model/passport.js Outdated Show resolved Hide resolved
Copy link
Member

@rcrichton rcrichton left a comment

Choose a reason for hiding this comment

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

Looks very good to me. A few comments.

config/default.json Show resolved Hide resolved
src/api/authentication.js Show resolved Hide resolved
src/middleware/tlsAuthentication.js Outdated Show resolved Hide resolved
Copy link
Member

@rcrichton rcrichton left a comment

Choose a reason for hiding this comment

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

Looks good on a quick scan, just one comment. I'l try go through and test it and give more feedback when I am able.

src/koaApi.js Outdated Show resolved Hide resolved
src/api/authentication.js Outdated Show resolved Hide resolved
src/api/authentication.js Show resolved Hide resolved
rcrichton
rcrichton previously approved these changes Mar 24, 2023
Copy link
Member

@rcrichton rcrichton left a comment

Choose a reason for hiding this comment

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

LGTM, I've tested this and all appears to work well. Just added a tiny comment.

src/api/authentication.js Outdated Show resolved Hide resolved
…t-fix

Plat 638 adjust login cookies test fix
@codecov
Copy link

codecov bot commented Mar 24, 2023

Codecov Report

Merging #1180 (927a69f) into master (5ece1a1) will increase coverage by 1.14%.
The diff coverage is 95.34%.

❗ Current head 927a69f differs from pull request most recent head ab76407. Consider uploading reports for the commit ab76407 to get more accurate results

@@            Coverage Diff             @@
##           master    #1180      +/-   ##
==========================================
+ Coverage   85.08%   86.23%   +1.14%     
==========================================
  Files          77       84       +7     
  Lines        5310     5716     +406     
==========================================
+ Hits         4518     4929     +411     
+ Misses        792      787       -5     
Impacted Files Coverage Δ
src/server.js 73.17% <69.23%> (+0.01%) ⬆️
src/upgradeDB.js 82.31% <82.75%> (+0.22%) ⬆️
src/api/users.js 92.54% <91.93%> (+9.02%) ⬆️
src/middleware/sessionStore.js 93.75% <93.75%> (ø)
src/passport.js 96.00% <96.00%> (ø)
src/api/authentication.js 95.60% <96.66%> (-0.13%) ⬇️
src/api/transactions.js 90.51% <100.00%> (+0.03%) ⬆️
src/koaApi.js 100.00% <100.00%> (ø)
src/model/index.js 100.00% <100.00%> (ø)
src/model/passport.js 100.00% <100.00%> (ø)
... and 7 more

... and 65 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@nour-borgi nour-borgi merged commit 1a3e390 into master Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants