PR#214: Add config.yml and sets admin permissions - fedora-commops/fedora-happiness-packets

fedora-commops / fedora-happiness-packets

#214 Add config.yml and sets admin permissions

Merged 5 years ago by jflory7. Opened 5 years ago by shraddhaag.

fedora-commops/ shraddhaag/fedora-happiness-packets django-admin into master

Add config.yml.example and sets admin permissions

ShraddhaAg • 5 years ago

18a52b9

.gitignore

file modified

		`@@ -27,3 +27,6 @@`

		`#Adding fas-admin-details.json so that the username and password does not get pushed`
		`fas-admin-details.json`
		`+`
		`+ # YAML Configuration file for django`
		`+ config.yml`
		`\ No newline at end of file`

config.yml.example

file added

+13

		`@@ -0,0 +1,13 @@`
		`+ base:`
		`+ admins:`
		`+ # following will populate ADMIN variable in settings/base.py`
		`+ - !!python/tuple ['Anna Philips', 'algogator@fedoraproject.org']`
		`+ - !!python/tuple ['Jona Azizaj', 'jonatoni@fedoraproject.org']`
		`+ - !!python/tuple ['Bhagyashree Uday', 'bee2502@fedoraproject.org']`
		`+ auth:`
		`+ admins:`
		`+ # following will be given superuser privileges`
		`+ - jflory7`
		`+ - jonatoni`
		`+ - bt0dotninja`
		`+ - anxh3l0`

docs/setup/development.rst

file modified

		`@@ -34,6 +34,8 @@`

		`#. Create a fas-admin-details.json file and add a json object with your FAS-Username and FAS-Password. See fas-admin-details.json.example.`

		+ #. Create a config.yml file and populate it with the user's details and usernames for `ADMINS <https://docs.djangoproject.com/en/2.1/ref/settings/#admins>`_ and superuser privileges respectively.
		`+`
		`Although the Dockerfile runs the script to check if a client_secrets.json file is present, please generate it before starting the Docker container, so that client secrets are not being constantly generated every time the image is rebuilt.`

		In order to run the web server, alongside the Redis queue and celery worker instance, simply run ``docker-compose up``.

happinesspackets/messaging/auth.py

file modified

+19 -1

		`@@ -1,6 +1,23 @@`
		`from mozilla_django_oidc.auth import OIDCAuthenticationBackend`

		`+ import yaml`
		`+`
		`+ with open("config.yml", 'r') as ymlfile:`
		`+ cfg = yaml.full_load(ymlfile)`
		`+`
		`class OIDC(OIDCAuthenticationBackend):`
		`+ def update_user(self, user, claims):`
		`+ if user.username in cfg['auth']['admins']:`
		`+ if not user.is_superuser:`
		`+ user.is_superuser = True`
		`+ user.is_staff = True`
		`+ else:`
		`+ if user.is_superuser:`
		`+ user.is_staff = False`
		`+ user.is_superuser = False`
		`+ user.save()`
		`+ return user`
		`+`
		`def create_user(self, claims):`
		`user = super(OIDC, self).create_user(claims)`
		`user.username = claims.get('nickname', '')`
		`@@ -10,4 +27,5 @@`
		`except:`
		`user.first_name = user.username`
		`user.save()`
		`- return user`
		`+ return self.update_user(user,claims)`
		`+`
		`\ No newline at end of file`

happinesspackets/messaging/views.py

file modified

-2

		`@@ -98,7 +98,6 @@`
		`return HttpResponseRedirect(self.success_url)`

		`class MessageSendView(LoginRequiredMixin, FormView):`
		`- login_url = '/oidc/authenticate/'`
		`template_name = 'messaging/message_send_form.html'`
		`form_class = MessageSendForm`

		`@@ -200,7 +199,6 @@`


		`class UserMessageView(LoginRequiredMixin, ListView):`
		`- login_url = '/oidc/authenticate/'`
		`model = Message`
		`paginate_by = 5`

happinesspackets/settings/base.py

file modified

+8 -6

		`@@ -4,6 +4,11 @@`
		`from django.core.exceptions import ImproperlyConfigured`
		`from unipath import Path`

		`+ import yaml`
		`+`
		`+ with open("config.yml", 'r') as ymlfile:`
		`+ cfg = yaml.full_load(ymlfile)`
		`+`
		`PROJECT_DIR = Path(__file__).ancestor(3)`

		`# For clean_pyc to work without complaining`
		`@@ -11,11 +16,7 @@`

		`DEBUG = False`

		`- ADMINS = (`
		`- ('Anna Philips', 'algogator@fedoraproject.org'),`
		`- ('Jona Azizaj', 'jonatoni@fedoraproject.org'),`
		`- ('Bhagyashree Uday', 'bee2502@fedoraproject.org'),`
		`- )`
		`+ ADMINS = cfg['base']['admins']`
		`SERVER_EMAIL = ADMINS[0][1]`

		`DEFAULT_FROM_EMAIL = "Happiness Packets <fedora.happinesspackets@gmail.com>"`
		`@@ -140,6 +141,7 @@`
		`LOGIN_REDIRECT_URL = '/'`
		`LOGOUT_REDIRECT_URL = '/'`
		`LOGIN_REDIRECT_URL_FAILURE = '/error'`
		`+ LOGIN_URL = '/oidc/authenticate/'`
jflory7 commented 5 years ago Good idea pulling this out as a variable. :thumbsup:
		`OIDC_RP_SCOPES = 'openid profile email'`

		`LOGGING = {`
		`@@ -216,4 +218,4 @@`
		`return os.environ[var_name]`
		`except KeyError:`
		`error_msg = "Set the %s environment variable" % var_name`
		`- raise ImproperlyConfigured(error_msg)`
		`+ raise ImproperlyConfigured(error_msg)`
		`\ No newline at end of file`

happinesspackets/urls.py

file modified

+6 -3

		`@@ -5,6 +5,7 @@`
		`from django.urls import include, re_path, path`
		`from django.conf.urls.static import static`
		`from django.contrib import admin`
		`+ from django.views.generic.base import RedirectView`

		`urlpatterns = [`
		`re_path(r'^oidc/', include('mozilla_django_oidc.urls')),`
		`@@ -12,11 +13,13 @@`
		`]`

		`if settings.ADMIN_ENABLED or settings.DEBUG:`
		`- urlpatterns.append(re_path(r'^drunken-octo-lama/', admin.site.urls))`
		`+ urlpatterns += [`
		`+ re_path(r'^drunken-octo-lama/login/', RedirectView.as_view(url=settings.LOGIN_URL, permanent=True, query_string=True)),`
		`+ re_path(r'^drunken-octo-lama/', admin.site.urls)`
		`+ ]`

		`if settings.DEBUG:`
		`import debug_toolbar`
		`urlpatterns += [`
		`path('__debug__/', include(debug_toolbar.urls))`
		`- ] + static(settings.MEDIA_URL, document_root=settings.MEDIA_ROOT)`
		`-`
		`+ ] + static(settings.MEDIA_URL, document_root=settings.MEDIA_ROOT)`
		`\ No newline at end of file`

requirements/base.txt

file modified

		`@@ -44,3 +44,6 @@`

		`#python-fedora for f-a-s API`
		`python-fedora==0.10.0`
		`+`
		`+ # Dependency for YAML file`
		`+ pyyaml==5.1`

shraddhaag commented 5 years ago

This commit adds the following:

Adds update_user method in auth.py to set admin permissions for
new accounts and pre-existing accounts of admin users. It also
clears the permissions when a user is removed from the admin list.
A config.yml is added to get the admin list in auth.py and ADMINS
in settings/base.py.

The ADMINS can be verified by checking its value in the Django Debug
Toolbar's Settings panel.

Metadata Update from @jflory7:
- Pull-request tagged with: improvement, needs testing, type - backend, type - summer coding

5 years ago

rebased onto 95d6a87ff41b6cb4db029c1532b13896e19eb0c3

5 years ago

Metadata Update from @jflory7:
- Request assigned

5 years ago

Metadata Update from @jflory7:
- Pull-request untagged with: needs testing

5 years ago

jflory7 commented 5 years ago

Thanks @shraddhaag, nice work :thumbsup: I tested this and everything worked as expected. :tada: I had three comments when I reviewed:

1. Navigating to admin portal URL when unauthenticated shows Django authentication backend

In happinesspackets/messaging/views.py, some classes set the login_url as follows:

login_url = '/oidc/authenticate/'

The admin portal authentication does not happen in views.py. I assume it happens somewhere else, possibly happinesspackets/messaging/admin.py. I'm not familiar enough with Django to be certain. cc: @jonatoni @anxh3l0 @bt0dotninja

For now, if there is an easy fix to add an OIDC auth to the admin portal, that can go into this PR. Either way, we should also file a new issue to refactor authentication code into a single module to reuse across FHP.

2. Change config.yml to config.yml.example, update .gitignore

This way, we can ship an example config file without conflicting with an actual production config file. The .gitignore file should ignore the path to the config file. Even though this is minor, it makes FHP easier to deploy later.

3. Add documentation comments to config file

I suggest adding a few comment lines in the example config file to briefly explain what they mean, what data input is expected, and how they should be formatted.

Otherwise, this PR looks great. :smile:

Metadata Update from @jflory7:
- Pull-request tagged with: needs changes

5 years ago

1 new commit added

new

5 years ago

rebased onto 06f3dbb8a9ee40898232eb91c691401c1199261f

5 years ago

rebased onto d1691745df6282e5717a85b94ee5fa9241431c8c

5 years ago

rebased onto 4ef5ef7b9cff8b59921bdd751a4483768ce94fa6

5 years ago

rebased onto 18a52b9

5 years ago

shraddhaag commented 5 years ago

Hi @jflory7! I have incorporated all the changes as suggested.

Added login to Django Admin SIte using OIDC.
Renamed config.yml and added changes to .gitignore.
Added comments to config.yml and also added documentation in docs/setup/development.rst.

The admin portal authentication does not happen in views.py. I assume it happens somewhere else, possibly happinesspackets/messaging/admin.py. I'm not familiar enough with Django to be certain. cc: @jonatoni @anxh3l0 @bt0dotninja
For now, if there is an easy fix to add an OIDC auth to the admin portal, that can go into this PR. Either way, we should also file a new issue to refactor authentication code into a single module to reuse across FHP.

I have incorporated login to Admin site using OIDC. What's remaining is prompting for credentials using OIDC auth again if a non admin logged in user tries to access the admin site. This is due to the fact that when users are logged out they are not logged out of the OpenID Connect provider. Hence when trying to login again, users are not prompted for credentials. This is detailed here and needs to be handled in a custom logout function as detailed in the link. I'll add this in the issue with refactoring authentication code.

Please let me know if I need to make anymore changes here! :)

jflory7 commented on line 30 of happinesspackets/settings/base.py 5 years ago

Good idea pulling this out as a variable. :thumbsup:

jflory7 commented 5 years ago

I tested this and everything works as expected. Thanks @shraddhaag, nice work! Merging. :clapper: