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

Use hash instead of encryption for storing passwords refs #234 #235

Merged
4 commits merged into from
Jun 7, 2021

Conversation

glutengo
Copy link
Contributor

@glutengo glutengo commented Jun 2, 2021

Replace password encryption by bcrypt hashing. See #234 for details.

@glutengo
Copy link
Contributor Author

glutengo commented Jun 2, 2021

Server e2e tests fail for oauth setups, I will address these.
For the JWT setups, the Frontend e2e tests fail with the following error, I am not sure if this is related to the changes on this branch:

E/launcher - Error: SessionNotCreatedError: session not created: This version of ChromeDriver only supports Chrome version 91
Current browser version is 90.0.4430.212 with binary path /usr/bin/google-chrome
  (Driver info: chromedriver=91.0.4472.19 (1bf021f248676a0b2ab3ee0561d83a59e424c23e-refs/branch-heads/4472@{#288}),platform=Linux 5.4.0-1047-azure x86_64)
    at Object.checkLegacyResponse (/home/runner/work/generator-jhipster-nodejs/generator-jhipster-nodejs/test-integration/samples/monolith-client-auth-i18n-template-jdl/node_modules/selenium-webdriver/lib/error.js:546:15)
    at parseHttpResponse (/home/runner/work/generator-jhipster-nodejs/generator-jhipster-nodejs/test-integration/samples/monolith-client-auth-i18n-template-jdl/node_modules/selenium-webdriver/lib/http.js:509:13)
    at /home/runner/work/generator-jhipster-nodejs/generator-jhipster-nodejs/test-integration/samples/monolith-client-auth-i18n-template-jdl/node_modules/selenium-webdriver/lib/http.js:441:30
    at processTicksAndRejections (internal/process/task_queues.js:95:5)

@glutengo
Copy link
Contributor Author

glutengo commented Jun 2, 2021

I noticed that the password column exists on the user entity even if the authentication method is oauth. In JHipster, this is removed when oauth is used: https://github.com/jhipster/generator-jhipster/blob/cb8fb188d93eef093ffc303c8a48a26bfce62cc5/generators/server/templates/src/main/java/package/domain/User.java.ejs#L194
I will see if I can remove it.

@glutengo glutengo marked this pull request as draft June 2, 2021 11:50
@glutengo
Copy link
Contributor Author

glutengo commented Jun 2, 2021

I have decided against removing the password in this PR. It seems to big a change and not directly in scope of this change. I will create a new issue to document this deviation from standard JHipster.
I have instead made some adjustments so that bcrypt and the hashing mechanism are only included when the authenticationType is jwt. This should fix the server e2e tests.

@glutengo
Copy link
Contributor Author

glutengo commented Jun 2, 2021

The pipeline now fails on the final step (START APP WITH EVENTUAL CLIENT E2E TESTS). The problem seems to be that the chrome version installed on the github runner via sudo apt install google-chrome-stable (90.0.4430.212-1) according to this) and the chrome webdriver version installed via node node_modules/webdriver-manager/bin/webdriver-manager update --gecko false (91.0.4472.19 according to this) do not match.
I do not know why this is the case, earlier today the pipeline worked fine: https://github.com/jhipster/generator-jhipster-nodejs/runs/2725400014?check_suite_focus=true

@glutengo
Copy link
Contributor Author

glutengo commented Jun 3, 2021

The mentioned Chrome version issue seems to have been a problem with the github action runner, at least I could get past this step in my forked repo: https://github.com/glutengo/generator-jhipster-nodejs/runs/2734972691?check_suite_focus=true

However some of the angular protractor tests (which I did not touch) failed afterwards. This seems a common issue for angular and protractor in combination with chrome (webdriver) in v91: angular/protractor#5519

@glutengo
Copy link
Contributor Author

glutengo commented Jun 3, 2021

As far as I can see, there are two ways to fix the protractor tests:

  1. Revert to chrome webdriver version 90.0.4430.24. This change could be performed by changing this line to
node node_modules/webdriver-manager/bin/webdriver-manager update --gecko false --versions.chrome 90.0.4430.24 && JHI_E2E_HEADLESS=true npm run e2e
  1. Wait for the mentioned protractor issue to be resolved and update the used protractor version.

Either way, the tests are likely to fail on any branch for the current combination of protractor, chrome and chrome webdriver versions. If we decide to proceed with 1) it should also only be a temporary fix until the protractor issue has been resolved

@glutengo glutengo marked this pull request as ready for review June 3, 2021 09:13
@ghost
Copy link

ghost commented Jun 3, 2021

Hi @glutengo,
yes the problem is that the node node_modules/webdriver-manager/bin/webdriver-manager update command update with the last chrome version, while the github action chrome task seems to install not the last 91.x version but the 90.0.
You can update the node command as you advice, because it install always the last version released and we might have problems.

For the password in the entity model, even if it is oauth authentication, for now leave it as it is. Then we can open another issue.

@glutengo
Copy link
Contributor Author

glutengo commented Jun 3, 2021

There is still a problem for apps with mongodb as a database. It seems that the {select: false} on the password of the UserEntity is ignored. The password is still loaded when the users are loaded from the database, despite the annotation. This leads to the password being present when other fields of the user are being adjusted (e.g. the name) because UserEntity.hashPassword() is called and the already-hashed password is hashed again.

It seems that this is a bug in typeorm, I have created a simple reproduction of this here: https://github.com/glutengo/typeorm-mongodb-select-false.

I have reported the issue there and will try to find another solution for removing the password from API responses for nhipster.

@glutengo glutengo force-pushed the feature/issue-234 branch 8 times, most recently from 13c60cf to 056c708 Compare June 4, 2021 13:16
- Drop usage of select:false in Field annotation as this does not work for typeorm with mongodb: typeorm/typeorm#7710
- Explicitly hash the password at distinct places instead: register, changePassword
- Use ClassTransformer with ClassSerializerIntercepter to exclude the password from responses instead
@ghost ghost self-requested a review June 4, 2021 15:15
@glutengo
Copy link
Contributor Author

glutengo commented Jun 7, 2021

Pipeline has finally passed, should be good now

@ghost ghost merged commit 24d3c63 into jhipster:main Jun 7, 2021
@ghost
Copy link

ghost commented Jun 7, 2021

Great!

@ghost ghost linked an issue Jun 7, 2021 that may be closed by this pull request
@glutengo glutengo deleted the feature/issue-234 branch June 9, 2021 21:46
This pull request was closed.
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.

Passwords should be hashed instead of encrypted
1 participant