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

Migration to bcrypt #158

Open
8 tasks
UlisesGascon opened this issue Aug 6, 2019 · 14 comments
Open
8 tasks

Migration to bcrypt #158

UlisesGascon opened this issue Aug 6, 2019 · 14 comments

Comments

@UlisesGascon
Copy link
Collaborator

UlisesGascon commented Aug 6, 2019

Context

Tasks

  • Remove dependency bcrypt-nodejs in package.json
  • Add dependency bcrypt in package.json
  • Migrate file app/data/user-dao.js to bcrypt
  • Validate the instalation with the local test
  • Add and submit the changes in package-lock.json
  • Add the primary dependency list to the readme.md
  • Check that the npm tasks are working as expected
  • Update the readme.md with the extra relevant info (if needed)

Assignation

  • This tasks is open for assignation, just claim it (as reply to this) and submit your PR ;-)

Important

@PeterWunderlich
Copy link

PeterWunderlich commented Sep 23, 2019

Hi all!
As i want to get more into FOSS I stumbled upon this project again and saw this issue. I would claim it for now and would be glad to provide the PR by the end of this week, if it's okay? :)

If so, i'll do the migration from bcrypt-nodejs to bcrypt.

@ckarande
Copy link
Member

@PeterWunderlich thank you for your interest in contributing to the project. Yes, you are very welcome to make the PR. I have assigned it to you 👍

@UlisesGascon
Copy link
Collaborator Author

Thank you, @PeterWunderlich! :-)

@PeterWunderlich
Copy link

Hey @ckarande & @UlisesGascon,
the migration is finished from my part but i have 2 final questions regarding the tasks:

  1. Add the primary dependency list to the readme.md: Shall i add a list of the dependencies (devDependencies exlucded) with a short explanation to the README.md? I would add the list before the "How to Setup Your Copy of NodeGoat" chapter.
  2. I've run into a few problems on my Windows machine executing the npm scripts, as "NODE_ENV" is not a known command for my OS. Usually i use cross-env for convenience. Do you want me to add this, do you have any other suggestions or did I use something wrong? Maybe this could become a new issue? :)

As I'm finished i would create a PR heading to the "OWASP:release-1.5" branch or do you want me to merge elsewhere?

Thanks for your help. :)

@UlisesGascon
Copy link
Collaborator Author

Hi @PeterWunderlich!

  1. Don't worry about the README.md dependencies. I will include an script that will autodocument that part for us.

  2. Feel free to add cross-env to add support for windows users.

  3. PR to OWASP:release-1.5 is perfect 👍

Thanks a lot for your support!

@jantaylor
Copy link

It looks like this task is already assigned and has not been implemented. Would you like me to take it on instead?

@PeterWunderlich PeterWunderlich removed their assignment Jun 4, 2021
@PeterWunderlich
Copy link

@jantaylor sure, feel free to take over.

@jantaylor
Copy link

@PeterWunderlich I've submitted the PR if you want to look it over and give your review.
@UlisesGascon, the PR is ready for your review as well.

@snowkluster
Copy link

Hi, I would like to work on this issue if the above PR has not been approved yet.

All that need to be done is to migrate from the deprecated bcrypt-nodejs to bcrypt and to validate the installation with the local tests.

@lirantal
Copy link
Collaborator

lirantal commented Mar 6, 2023

I've pinged Jan in the original PR. Let's see if he is able to follow-up first.

@snowkluster
Copy link

I've pinged Jan in the original PR. Let's see if he is able to follow-up first.

Ok

@snowkluster
Copy link

Is it alright if I open a PR as I think, I have fixed this issue as well as issue #271 and ran "npm run precommit" and fixed all those errors.

@snowkluster
Copy link

Are there any updates about the PR

@jantaylor
Copy link

jantaylor commented Mar 7, 2023

@snow-kluster I haven't gotten any indication that the maintainers wanted to merge my PR #237.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants