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

Adds flask admin as a feature. Bumps flask admin to 1.6.0 #551

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

Conversation

oafernandes
Copy link
Collaborator

-Adds flask admin feature
-Bumps flask admin to 1.6.0 and all dependencies
-Issue with password hashing when trying to add a new admin from admin view, password is not hashed and stored as plaintext.

SECRET_KEY=sammy
SECURITY_PASSWORD_SALT=saltedpop
[email protected]
ADMIN_PASSWORD=1234
Copy link
Member

Choose a reason for hiding this comment

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

Can we please add a newline to the end of this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

@@ -19,6 +19,8 @@ RUN apt-get update \
&& pip install poetry \
&& poetry config virtualenvs.create false

RUN poetry lock
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Collaborator Author

@oafernandes oafernandes Jul 8, 2022

Choose a reason for hiding this comment

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

I manually added the dependency versions to the pyproject.toml file. Below link to solution for resolving the "....which doesn't match any versions, version solving failed." error

[Known issue]python-poetry/poetry#1281 (comment)

@@ -37,6 +37,13 @@ def get_sys_exec_root_or_drive():
if not all([algolia_app_id, algolia_api_key]):
print("Application requires 'ALGOLIA_APP_ID' and 'ALGOLIA_API_KEY' for search")

secret_key = os.environ.get('SECRET_KEY', None)
security_password_hash = 'pbkdf2_sha512'
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this an environment variable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

SECURITY_PASSWORD_HASH added to .env

@@ -37,6 +37,13 @@ def get_sys_exec_root_or_drive():
if not all([algolia_app_id, algolia_api_key]):
print("Application requires 'ALGOLIA_APP_ID' and 'ALGOLIA_API_KEY' for search")

secret_key = os.environ.get('SECRET_KEY', None)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
secret_key = os.environ.get('SECRET_KEY', None)
secret_key = os.environ['SECRET_KEY']

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Uses bracket notation instead of .get()

Comment on lines +44 to +45
if not all([secret_key, security_password_salt]):
print('Application requires "SECRET_KEY" and "SECURITY_HASH"')
Copy link
Member

Choose a reason for hiding this comment

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

We can remove this if we use the bracket syntax instead of .get()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Uses bracket notation instead of .get()

Comment on lines +28 to +33
# @event.listens_for(User.password, 'set', retval=True)
# def hash_user_password(target, value, oldvalue, initiator):
# """Encrypts password when new admin created in User View"""
# if value != oldvalue:
# return utils.encrypt_password(value)
# return value
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This listens for when a new admin password is added in the admin view and encrypts with same method as before_first_request(). Removed.



@app.before_first_request
def before_first_request():
Copy link
Member

Choose a reason for hiding this comment

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

Please rename this so we don't accidentally shadow or get confused. Choose a name that is descriptive of what the function is doing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed to add_admin_role

@app.before_first_request
def before_first_request():
""" Adds admin/user roles and default admin account and password if none exists"""
db.create_all()
Copy link
Member

Choose a reason for hiding this comment

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

Is this idempotent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only way to check would be to delete the table from the database I think? May need help with this.

Comment on lines +59 to +60
admin_email = os.environ.get('ADMIN_EMAIL', "[email protected]")
admin_password = os.environ.get('ADMIN_PASSWORD', 'password')
Copy link
Member

Choose a reason for hiding this comment

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

We want this to blow up. Please use bracket syntax here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to bracket from .get()

Comment on lines +52 to +53
@app.before_first_request
def before_first_request():
Copy link
Member

Choose a reason for hiding this comment

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

If the admin user already exists when this function runs, what happens? Are there side effects?

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.

2 participants