I am interested in working on this .

@bhuvana Did you already complete the tasks in ticket #100 and #103? I suggest starting there first so we make sure your development environment is working successfully. Once you accomplish those starter tasks, we can find something new for you to work on. :smiley:

Hello @jflory7 , I would like to work on this issue

Hello @jflory7 If this issue is not taken yet I would like to give it a go :grinning:

@bhuvana @saidat @anasustic It's awesome that all of you want to help with this ticket! :tada:

A first step would be to reproduce this problem and finding a cause behind it.

Since there are a few folks interested, let's spend some time debugging first. Can anyone reproduce the bug? And more importantly, any ideas on why this is happening? I suggest checking out the Django docs about the Django Admin Site.

If someone can figure out why this bug is happening, share your findings in a comment. I'll assign the ticket once someone knows why this bug is happening and maybe an idea of how to fix it.

@jflory7 The issue is because, the superuser we are making is not being synced with the db. When I accessed the shell and set is_superuser and is_staff to True for the current user after querying it from the User model and saving this user instance, I was successfully able to access the Django Admin Site.

A solution for this can be to auto populate the User model with a super user using this.

Hope this helps! :)

To reproduce the problem we run docker-compose exec web ./manage.py createsuperuser and then logon to http://localhost:8000/drunken-octo-lama/ using the credentials we just created. After that we get an error msg: Please enter the correct user name and password for a staff account. Note that both fields may be case-sensitive.

@anasustic Thanks for reproducing. Do you want to go ahead and take this ticket on? @shraddhaag suggested some next steps for what a fix might look like for this ticket.

@jflory7 Thanks. I will give it a go :grinning:

@jflory7 The issue is because, the superuser we are making is not being synced with the db. When I accessed the shell and set is_superuser and is_staff to True for the current user after querying it from the User model and saving this user instance, I was successfully able to access the Django Admin Site.
A solution for this can be to auto populate the User model with a super user using this.
Hope this helps! :)

Hi @jflory7 , I was wondering if the authenticated user (signed in user) has always the id:1? I am asking this since I would like to auto populate the User model and set is_superuser and is_staff to True like shraddhaag suggested for the authenticated user using fixtures .Thanks :)

Hi @jflory7 , I was wondering if the authenticated user (signed in user) has always the id:1?

Hey @anasustic, I haven't looked closely at our authentication code yet. Can you point me to where you are looking in our code for context?

Hi @jflory7, I believe it is in the auth.py where the user is created.
I could then maybe just set user.is_superuser=True and user.is_staff=True here before saving the user?

Hi @jflory7 , I was wondering if the authenticated user (signed in user) has always the id:1?

Hi @anasustic! Since in the development environment there is only one user being registered [i.e only the one that you login with], that username is saved in the db with id=1.

I could then maybe just set user.is_superuser=True and user.is_staff=True here before saving the user?

Setting is_superuser and is_staff to True here, means for every user that logs in, the user will be registered as admin. This isn't a good practice as the is_superuser and is_staff shouldn't be set for all users, as then all users will be able to access the Django Admin Site.

I am asking this since I would like to auto populate the User model and set is_superuser and is_staff to True like shraddhaag suggested for the authenticated user using fixtures .

On second thoughts, I think we should only create a local superuser in development environment. Since auto populating the User model is not ideal in the production setup. The creation of the superuser in the production can be handled at the time of setting up the production server in ticket #111.
I think the instructions in documentation are enough for creating a superuser. Once created, the db container now will store it and it will be accessible throughout the development procedure.

@jflory7 I'd love to hear your thoughts on this.

Hi @shraddhaag, I agree with you that setting is_superuser=True and is_staff=True for all Users in production would not be a good idea. But then again in development the user must be authenticated in order to login to Django Admin in addition to having is_superuser=True and is_staff=True. If I create a superuser in the shell I will not be able to signon to Django Admin unless this was the user I signed on in the app.

But then again in development the user must be authenticated in order to login to Django Admin in addition to having is_superuser=True and is_staff=True. If I create a superuser in the shell I will not be able to signon to Django Admin unless this was the user I signed on in the app.

@anasustic The user that you're using to send emails, ie the current logged in user, will be able to login in on the Django Admin Site and perform the necessary superuser actions, after is_superuser and is_staff are set to True once. The setting up of superuser becomes a one off in the development environment as a container for db is also made now, so all our changes are saved here to the User model fields for future use.

I see. Thanks for the clarification @anasustic and @shraddhaag.

I think the best way to handle superusers with staff privileges should be a list of usernames in a configuration file. Since we use FAS for authentication, we can expect to know someone's FAS username. Specifying a group of admin users in a config file allows multiple people, both developers and sysadmins, to quickly access an admin panel without sharing a single account.

This is valuable because FHP needs different stakeholders to have administrative access; for example, FHP includes functionality to blacklist an email, and we may want to convert that to blacklisting a FAS account. We can specify a group of people in Fedora Infrastructure team to have admin panel access for administrative tasks. Additionally, this makes the application easier to audit when looking at what user performed which action.

Does this make sense?

Hi, Thanks @jflory7 and @shraddhaag for sharing your thoughts. It makes perfect sense to use a group of admin users. I would need some guidance for implementing that :smile:

I think the best way to handle superusers with staff privileges should be a list of usernames in a configuration file. Since we use FAS for authentication, we can expect to know someone's FAS username. Specifying a group of admin users in a config file allows multiple people, both developers and sysadmins, to quickly access an admin panel without sharing a single account.

This sounds like a very realistic approach. Thank you for pointing us in the right direction :smile:

Hi, Thanks @jflory7 and @shraddhaag for sharing your thoughts. It makes perfect sense to use a group of admin users. I would need some guidance for implementing that 😄

@anasustic What we can do here is, in messaging/auth.py create a list (for this scope asuming admin_list) of users that should be given admin privileges. At the time of creation of their accounts, [i.e when they first login to the site], check the username against admin_list and if found, set is_superuser and is_staff to True.

Now when a user [one that is in the admin_list] logs into the site and then access the Django Admin Site at localhost:8000/drunken-octo-lama/ (on another note, we should probably rename this to localhost:8000/admin/) they will have access to the Django Admin Site and be able to perform Admin actions. I verified this implementation at my end.

I also dug a little deeper and found the root cause to the error Please enter the correct username and password for a staff account. Note that both fields may be case-sensitive.
Django Admin Site by default is configured to authenticate users against ModelBackend which needs to be configured in AUTHENTICATION_BACKENDS. Since in our project we have configured happinesspackets.messaging.auth.OIDC, the Django Admin Site is not able to login users created with createsuperuser command. To rectify this we simple need to add django.contrib.auth.backends.ModelBackend and then we will be able to make superusers with createsuperuser command.

@jflory7 Should we incorporate this ability in the Django Admin Site too, or does the admin_list covers our entire use case?

@anasustic What we can do here is, in messaging/auth.py create a list (for this scope asuming admin_list) of users that should be given admin privileges. At the time of creation of their accounts, [i.e when they first login to the site], check the username against admin_list and if found, set is_superuser and is_staff to True.
Now when a user [one that is in the admin_list] logs into the site and then access the Django Admin Site at localhost:8000/drunken-octo-lama/ (on another note, we should probably rename this to localhost:8000/admin/) they will have access to the Django Admin Site and be able to perform Admin actions. I verified this implementation at my end.

Thanks so much @shraddhaag for elaborating the use case for this :open_hands: I will give it a try. What is a good practice for the location of the admin_list.txt in the file system?

I also dug a little deeper and found the root cause to the error Please enter the correct username and password for a staff account. Note that both fields may be case-sensitive.
Django Admin Site by default is configured to authenticate users against ModelBackend which needs to be configured in AUTHENTICATION_BACKENDS. Since in our project we have configured happinesspackets.messaging.auth.OIDC, the Django Admin Site is not able to login users created with createsuperuser command. To rectify this we simple need to add django.contrib.auth.backends.ModelBackend and then we will be able to make superusers with createsuperuser command.

Wow awesome troubleshooting @shraddhaag :thumbsup: The documentation you pointed to says that the order matters so django.contrib.auth.backends.ModelBackend should probably be the second in the list something like AUTHENTICATION_BACKENDS = ( 'happinesspackets.messaging.auth.OIDC','django.contrib.auth.backends.ModelBackend', )

Could adding django.contrib.auth.backends.ModelBackend pose a security issue in production?

@shraddhaag wrote…
@jflory7 Should we incorporate this ability in the Django Admin Site too, or does the admin_list covers our entire use case?

@anasustic wrote…
Could adding django.contrib.auth.backends.ModelBackend pose a security issue in production?

Thanks for researching and linking out to the docs @shraddhaag. I better understand this now.

Since our application never works with usernames and passwords for authentication, I am not sure adding a new authentication back-end is the best implementation. I prefer a way to update the database after a user logged in whose username matches the admin list. For example, here is a line (with some trimming) from the auth_user table in the FHP database:

 id | username | is_superuser | is_staff 
----+----------+--------------+----------
  1 | jflory7  | f            | f

See what happens when I run this SQL command inside the postgres Docker container:

# psql -U postgres -d postgres -c \
       "UPDATE auth_user \
       SET is_superuser = 't', is_staff = 't' \
       WHERE username = 'jflory7'"
UPDATE 1
# psql -U postgres -d postgres -c \
       "SELECT id,username,is_superuser,is_staff \
       FROM auth_user \
       WHERE username = 'jflory7'"
 id | username | is_superuser | is_staff 
----+----------+--------------+----------
  1 | jflory7  | t            | t

After this change, I could log into the admin portal.

So, my preferred way of doing this is to add a post-condition to authentication. Every time a user authenticates, check if they are listed as an admin user in the application config. If yes, set the booleans to true in the database for these two fields.

Like @shraddhaag already suggested, we can do this in messaging/auth.py. We could add a check there when creating a user. But I'm also not clear on the best way to update admin privileges on a user whose account already exists. I need to dig into our authentication code more to see how this is working today.

I feel like this is the most simple way of solving this, but I am curious to hear what both of you think, or if this explanation makes sense.

@anasustic wrote…
What is a good practice for the location of the admin_list.txt in the file system?

This is also a good question.

Currently, many app settings are set in settings/base.py. I want an easy-to-read YAML file with my configuration settings. These settings would include many options currently in base.py and should also include things like FAS account credentials for authentication.

However, I think that task is a new ticket. I will try to file a new one soon for this.

In the meanwhile, I suggest we use the existing ADMINS list for this (we need to add FAS usernames to the data as well). We can check if someone's username is specified there and set them as an admin user if it's a match after they authenticate.

This is a lot to throw out all at once but I hope this is useful and makes sense! Please let me know if anything is unclear and I will do my best to clarify or deconstruct it further. :smile:

Since our application never works with usernames and passwords for authentication, I am not sure adding a new authentication back-end is the best implementation.

Oh right! That makes sense. I was unsure about the need to incorporate createsuperuser. Thank you for clearing it up! :)

But I'm also not clear on the best way to update admin privileges on a user whose account already exists. I need to dig into our authentication code more to see how this is working today.

I was able to figure out how to update admin permissions on a user whose account already exists. It can done by overriding the update_user method of mozilla-django-oidc. We can check at the time of login, if the user logging in is in the admin_list and if yes, whether is_superuser is set or not. If not, then we can update the permissions is_superuser and is_staff to True.

In the meanwhile, I suggest we use the existing ADMINS list for this (we need to add FAS usernames to the data as well). We can check if someone's username is specified there and set them as an admin user if it's a match after they authenticate.

I was trying to make this change when I realised the ADMINS is a list of all the people who get code error notifications as specified here. I'm not sure whether changing it's syntax would be ideal. As an alternative we could define a Python List in auth.py itself. Or should I look into other methods to get this implementation done?

I was able to figure out how to update admin permissions on a user whose account already exists. It can done by overriding the update_user method of mozilla-django-oidc. We can check at the time of login, if the user logging in is in the admin_list and if yes, whether is_superuser is set or not. If not, then we can update the permissions is_superuser and is_staff to True.

This looks viable. :thumbsup: I think this is the right approach.

I was trying to make this change when I realised the ADMINS is a list of all the people who get code error notifications as specified here. I'm not sure whether changing it's syntax would be ideal. As an alternative we could define a Python List in auth.py itself. Or should I look into other methods to get this implementation done?

Thanks for pointing this out! I didn't see it used elsewhere in the code, but I didn't realize it used internally by Django. I agree then.

For users with admin capabilities, I think this should be defined in a YAML configuration file. It can be a list of FAS usernames. To get the list of admin usernames, the code should read in the list from the config file. (For more background on why this way, read the Config section of the 12 Factor App manifesto).

For the users currently in ADMINS, this should also be specified in the config file and read into Django. This allows us to use Ansible templating later when deploying the application (e.g. #111).

Hi @shraddhaag and @jflory7 . Thanks for all the great explanations :raised_hands:
@shraddhaag It appears you have part of this change already in the works. Is there anything I could help with?

For users with admin capabilities, I think this should be defined in a YAML configuration file. It can be a list of FAS usernames. To get the list of admin usernames, the code should read in the list from the config file. (For more background on why this way, read the Config section of the 12 Factor App manifesto).
For the users currently in ADMINS, this should also be specified in the config file and read into Django. This allows us to use Ansible templating later when deploying the application (e.g. #111).

For both of the above I have made a config.yml file from which ADMINS (in settings/base.py) and admin_list (in messaging/auth.py) is read.

I have also added the configurations in messaging/auth.py to set admin permissions for new and preexisting admin users.

I was wondering if I need to add the configurations to disable admin permissions for a use case when a particular admin user is removed from config.yml.

If the changes seem relevant, should I send a PR for review?

For both of the above I have made a config.yml file from which ADMINS (in settings/base.py) and admin_list (in messaging/auth.py) is read.

I have also added the configurations in messaging/auth.py to set admin permissions for new and preexisting admin users.

Awesome @shraddhaag! :thumbsup: This sounds like what I had in mind.

I was wondering if I need to add the configurations to disable admin permissions for a use case when a particular admin user is removed from config.yml.

Yes. When a user is removed from the config.yml, they should not have access to admin capabilities once the app is restarted.

If the changes seem relevant, should I send a PR for review?

Yep! Feel free to send one when you have a chance.

Hi @shraddhaag and @jflory7 . Thanks for all the great explanations 🙌

@shraddhaag It appears you have part of this change already in the works. Is there anything I could help with?

@anasustic I think this ticket became something bigger. Originally, it was about making the existing admin interface visible and discoverable. One thing we have not talked about yet is improving the admin user interface. If you have ideas on how we can improve the admin page, we can brainstorm on this. :smiley:

@jflory7 I've opened a PR regarding the same. Would love to hear some feedback whenever you have time :)

Hi @jflory7 ,

Here are some of my ideas how to improve the Admin interface:

For the Messages list view, we could display the sender_name, receipient_name, status and identifier (rather than the default __str__() text which is the identifier).
We could also use a list filter for message status and maybe add sender_approved_public and receipient_approved_public to the filter too.
Maybe we could also section the detailed message view in sections for the sender, the receiver, the message, the status and the identifier and organize some Boolean fields horizontally.

The BlacklistedEmail has only three fields and currently lists by email address. Maybe we can leave this one as is.

Would love to hear your insights when you have a moment :grinning:

@jflory7 I've opened a PR regarding the same. Would love to hear some feedback whenever you have time :)

@shraddhaag Thanks for your patience, I just had a chance to review. I left comments in the PR.

Here are some of my ideas how to improve the Admin interface:
For the Messages list view, we could display the sender_name, receipient_name, status and identifier (rather than the default str() text which is the identifier).
We could also use a list filter for message status and maybe add sender_approved_public and receipient_approved_public to the filter too.
Maybe we could also section the detailed message view in sections for the sender, the receiver, the message, the status and the identifier and organize some Boolean fields horizontally.

+1. This would make it easier to approve new Packets to appear in the public archive. Could you please create a new issue with your idea on improving the Messages list view?

Hi @jflory7. I think this issue can be closed :)

@shraddhaag Super. Nice work on getting this one done. :smile: Closing as complete! :clapper: