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

Implemented helper for password reset #1043

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Conversation

s3inlc
Copy link
Member

@s3inlc s3inlc commented Feb 21, 2024

Helper added for non-authenticated requests for a reset of a users password.

This function requires emails to be sent, therefore the email docker configuration is included as well (which for example would also be used by email notifications).

As a simple sendmail utility, ssmtp is used whose configuration is now included in docker-compose and the example env as well.

@s3inlc s3inlc requested a review from zyronix February 21, 2024 10:56
@zyronix zyronix linked an issue Mar 27, 2024 that may be closed by this pull request
@jessevz jessevz self-requested a review November 13, 2024 14:18
@@ -25,6 +25,15 @@ ENV HASHTOPOLIS_LOG_PATH=${HASHTOPOLIS_PATH}/log
ENV HASHTOPOLIS_CONFIG_PATH=${HASHTOPOLIS_PATH}/config
ENV HASHTOPOLIS_BINARIES_PATH=${HASHTOPOLIS_PATH}/binaries

ENV HASHTOPOLIS_SSMTP_ENABLE=0
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is a little bit confusing to put all the environment variabels here instead out of the .env file. since the env.example file now also have these environment variables but from there they are never used.

Personally I would say an even better solution would be to instead of using environment variables, is to create a ssmtp.conf template here and let the user fill that in. Then in the docker file it can be transferred like "COPY ssmtp-template.conf /etc/ssmtp/ssmtp.conf". That way the user has more flexibility in setting up the ssmtp configuration and is not limited to the few environment variables.

@@ -32,6 +32,7 @@ public function getFormFields(): array {
public function actionPost($data): array|null {
UserUtils::userForgotPassword($data[User::USERNAME], $data[User::EMAIL]);

# TODO: Check how to handle custom return messages that are not object, probably we want that to be in some kind of standardized form.
Copy link
Contributor

Choose a reason for hiding this comment

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

We now have a standardized way so this comment can be removed

@@ -10,6 +10,9 @@ for path in ${paths[@]}; do
fi
done

echo "Running required root setups."
sudo -E /usr/local/bin/second-level-docker-entry.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

For me this will give the following errors:

sudo: a terminal is required to read the password; either use the -S option to read from standard input or configure an askpass helper
sudo: a password is required

I think the reason for this error is because the user that executes entrypoint script is vscode and not www-data

@@ -77,6 +87,11 @@ COPY --from=preprocess /HEA[D] ${HASHTOPOLIS_DOCUMENT_ROOT}/../.git/
COPY composer.json ${HASHTOPOLIS_DOCUMENT_ROOT}/../
RUN composer install --working-dir=${HASHTOPOLIS_DOCUMENT_ROOT}/..

RUN echo "www-data ALL=NOPASSWD:SETENV: /usr/local/bin/second-level-docker-entry.sh" >> /etc/sudoers.d/10_docker
Copy link
Contributor

Choose a reason for hiding this comment

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

Like mentioned, I think www-data here should be replaced by vscode. But im not 100% sure

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.

[BUG] Notification inside Docker -- SendMail
2 participants