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

Delete create_testuser signal handler #364

Closed
wants to merge 0 commits into from

Conversation

sevein
Copy link
Member

@sevein sevein commented May 31, 2018

This commit removes the signal handler that was responsible for creating the
test superuser. It is still possible to create a superuser from the
command-line interface.

The base fixture file is updated so it includes a new superuser. Previously,
the tests were relying on user created by the create_testuser signal handler.

Connected to #286

@sevein sevein added this to the 0.12.0 milestone May 31, 2018
@sevein sevein self-assigned this May 31, 2018
@sevein sevein requested review from ross-spencer and jrwdunham May 31, 2018 08:18
@qubot qubot force-pushed the dev/issue-286-remove-test-user branch from 997ce2e to 73033dd Compare May 31, 2018 08:21
Copy link
Contributor

@jrwdunham jrwdunham left a comment

Choose a reason for hiding this comment

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

@sevein Can you modify #286 to describe why this is a problem? Is it an issue of not wanting the default behaviour being creation of a superuser with a very guessable password?

Also, I'm wondering if your fixtures solution will still work if we update Django in the future. That is, does the hashed value of the "test" password change if Django uses a different hashing algorithm?

Finally, are you saying that merging this will require making mods in all of our deployment tools (ansible, packages, docker) so that they become responsible for creating this "test" user?

@sevein
Copy link
Member Author

sevein commented Jun 8, 2018

@sevein Can you modify #286 to describe why this is a problem? Is it an issue of not wanting the default behaviour being creation of a superuser with a very guessable password?

Done. I hope it's clearer now.

Also, I'm wondering if your fixtures solution will still work if we update Django in the future. That is, does the hashed value of the "test" password change if Django uses a different hashing algorithm?

I'm using pbkdf2_sha256which is still listed as a supported hasher in Django v2.0. If this becomes a problem in the future I think it's safe to assume that the hasher identifier will raise some kind of exception and the test will break when we perform authentication, i.e. we wouldn't be seeing false positives I believe.

Finally, are you saying that merging this will require making mods in all of our deployment tools (ansible, packages, docker) so that they become responsible for creating this "test" user?

I think that we're going to be solving this differently depending on the distribution type:

@sevein
Copy link
Member Author

sevein commented Jun 8, 2018

@jrwdunham one clarification: I think that artefactual/archivematica-docs#178 could be enough for now when it comes to explaining users how the installation process changes in debs/rpms/ansible without making actual changes in their sources.

Perhaps @mamedin's effort to automate the installation via Ansible (see artefactual-labs/ansible-archivematica-src#162) may need some extra care once this is merged.

IMO this PR is ready to merge.

Copy link
Contributor

@jrwdunham jrwdunham left a comment

Choose a reason for hiding this comment

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

LGTM

@qubot qubot closed this Jun 13, 2018
@qubot qubot force-pushed the dev/issue-286-remove-test-user branch from 73033dd to 2976d25 Compare June 13, 2018 23:17
@sevein
Copy link
Member Author

sevein commented Jun 13, 2018

2976d25

@qubot qubot deleted the dev/issue-286-remove-test-user branch June 13, 2018 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants