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

Shibboleth integration #3

Merged
merged 7 commits into from
May 31, 2017
Merged

Shibboleth integration #3

merged 7 commits into from
May 31, 2017

Conversation

helenst
Copy link

@helenst helenst commented May 24, 2017

This is similar to JiscSD/archivematica#5 and many of the comments there apply here too.

@@ -51,6 +51,9 @@ def user_list(request):
return render(request, 'administration/user_list.html', locals())

def user_edit(request, id):
if not request.user.is_superuser:
Copy link
Author

Choose a reason for hiding this comment

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

Shibboleth users shouldn't be editable. I left in the ability for superusers to do this but perhaps this should be removed entirely. Or thinking more around Shibboleth being an optional component, making this conditional on whether it's enabled?

@@ -69,6 +72,9 @@ def user_edit(request, id):
return render(request, 'administration/user_form.html', locals())

def user_create(request):
if not request.user.is_superuser:
Copy link
Author

Choose a reason for hiding this comment

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

comment for user_edit also applies here.

@@ -78,6 +84,17 @@ def user_create(request):
return render(request, 'administration/user_form.html', locals())


def user_detail(request, id):
Copy link
Author

Choose a reason for hiding this comment

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

This just summarises the user rather than allows editing. Only visible for themselves or superuser (since it shows API key)

)


class CustomShibbolethRemoteUserMiddleware(ShibbolethRemoteUserMiddleware):
Copy link
Author

Choose a reason for hiding this comment

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

This is the same as the Dashboard one. Maybe it can be common code, since the differences (for now at least) are all in config. Is there somewhere shared between the two repos that this could live?

@@ -163,6 +165,7 @@ def get_env_variable(var_name):
'django.template.context_processors.tz',
'django.template.context_processors.request',
'django.contrib.messages.context_processors.messages',
'shibboleth.context_processors.logout_link',
Copy link
Author

Choose a reason for hiding this comment

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

as with dashboard, not working yet.

@@ -52,7 +52,7 @@
<li><a href="{% url 'package_list' %}">{% trans "Packages" %}</a></li>
<li><a href="{% url 'settings_edit' %}">{% trans "Administration" %}</a></li>
{% if user.is_authenticated %}
<li><a href="{% url 'logout' %}">{% trans "Log out" %}</a></li>
<li><a href="{{ logout_link }}">{% trans "Log out" %}</a></li>
Copy link
Author

Choose a reason for hiding this comment

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

Not working yet.

@lower29
Copy link

lower29 commented May 30, 2017

I've rebased the devshib branch against the latest master to incorporate @sevein's installer changes. No merge issues encountered.

@helenst helenst changed the title [WIP] Shibboleth integration Shibboleth integration May 31, 2017
Copy link

@lower29 lower29 left a comment

Choose a reason for hiding this comment

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

This is working great now, good work. 👍
Definitely ready for merge I think.

* Auto creates user based on info received in configured X-Shib headers
* Auto logs in existing user if matches X-Shib-User header

Limitations:
* Usernames still limited to 30 characters
* Does not hook into login/logout URLs yet
* Replace existing login with Shibboleth
* Insert "logged out" message page after logout to prevent bouncing
straight back to login.
* Regular users get a read-only view of themselves (so they can see their
details and API key)
* Regular users cannot change password
* Admins can still do this for any user
Lets Django app know whether we're behind an SSL connection.
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.

3 participants