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

Testing observations #14

Closed
backside180 opened this issue Jun 15, 2017 · 7 comments
Closed

Testing observations #14

backside180 opened this issue Jun 15, 2017 · 7 comments

Comments

@backside180
Copy link

backside180 commented Jun 15, 2017

Having done some testing as part of my setup and I have noticed the following:

Site return redirection:
When a user hits a view/page that requires authentication the user is sent to the ADFS login screen, the user signs in and is returned to the URL defined in the “LOGIN_REDIRECT_URL”

  • Is there any way of passing the current location over to ADFS so the user can return to the same page, I’m not sure if this is a limitation of this module or ADFS. (It’s my understanding that ADFS can do this.)

  • So for me this solution works well on sites that require a login to the whole site where the user hits the login page as the first point of call rather than a site that has mixed content which displays differently depending on your authentication state.

Logout:
I’m getting mixed results when it comes to logging out a user.

  • I assume I would need to logout of both Django and close the ADFS session, logging out of Django is not enough as I found that when I revisit the login process I get automatically authenticated (no ADFS sign in required)

  • Having a function in your code to provide this in a single template call much like the {{ ADFS_AUTH_URL }} context call would be helpful.

User “Staff” and “Superuser” Status
I didn't see an option to be able to add a user with "Staff" or "Superuser" rights

  • My thoughts were to have two groups one for Users and another for Admins, I have this working to a point, my user gets placed in to the relevant group in django based on the AD group membership however I'm unable to assign "Staff" or "Supperuser" rights to the user as its added.
@jobec
Copy link
Collaborator

jobec commented Jun 15, 2017

Site return redirection:
This is indeed a limitation of the OAuth2 authorization code grant type itself. You have to register a redirect URI and it's only this URI that ADFS will redirect to. This is a security measure so the authorization code is only sent to a trusted URI and not to an attacker.

A bypass could be something that remembers the page that triggered the redirect to ADFS an direct the user again to this page when authentication is completed. But the possible ways of doing that are so diverse that it's outside the scope of this package.

Logout:
The ADFS server remembers your authentication in a cookie. So as soon as you are redirected again to the ADFS server, you will automatically be logged in again. That was also the scope of this package. Seamless (e.g. without user interaction) single sign on to your site with ADFS as the back end.

While there is a logout URL on ADFS (/adfs/ls/?wa=wsignout1.0), I never got it to work.

@backside180
Copy link
Author

UPDATE

User “Staff” and “Superuser” Status:
I fixed my own point here, I realized that I could send a claim field to the app to apply the "is_staff" option when creating the user.

I did this by updating my CLAIM_MAPPING and by adding the following claim rules:

"CLAIM_MAPPING" : {"first_name": "given_name",
"last_name": "family_name",
"is_staff": "is_staff",
"email": "email"},

RULES
Admin Group / is_staff = True

Rule Template Claim Rule Name User's Group Outgoing Claim Type Outgoing claim value
Send Group Membership as a Claim is_staff_[ADMIN_GROUP]_True [AD Admin Group] is_staff True

User Group / is_staff = False

Rule Template Claim Rule Name User's Group Outgoing Claim Type Outgoing claim value
Send Group Membership as a Claim is_staff_[USER_GROUP]_False [AD User Group] is_staff False

@jobec
Copy link
Collaborator

jobec commented Jun 15, 2017

As for the is_staff and is_superuser fields, they can indeed be set like that. This is because both are a BooleanField and those convert from a string value to a boolean automatically.

Two things are possible here:

  1. Document setting those fields that way
  2. Adding extra functionality to support those 2 specific fields.

My preference goes to 1 as it allows to support all possible fields you might want to add to the user model of Django.

@backside180
Copy link
Author

RE User “Staff” and “Superuser” Status:
I guess this is your call.

RE Logout:
I have been testing options for the logout and have come up with the following:

setting.py add:

LOGOUT_REDIRECT_URL = 'https://[ADFS SERVER FQDN]/adfs/ls/?wa=wsignout1.0'

django-auth-urls.py update as follows:

from django.conf.urls import url
from django.contrib.auth.views import logout
from . import views

urlpatterns
= [
url(r'^login$', views.OAuth2View.as_view(), name='login'),
url(r'^logout/$', logout, name='logout'),
]

Now when I visit the https://[WEB SERVER FQDN]/oauth2/logout I get logged out of django and redirected to the ADFS logout page.

Its just one method which might work for some.

@jobec
Copy link
Collaborator

jobec commented Jun 15, 2017

While testing I found an issue with setting is_staff via a claim. While it's easy to give a user staff access, it's very difficult to take it away again. Also, if your user is not in one of the groups you specify, the claim isn't sent, triggering an error. So this doesn't scale.

@backside180
Copy link
Author

OK thanks for the update on this.

In my case I have the two groups defined in my "Issuance Authorization Rules" So I guess I will only ever get a user that is in one group or the other.

@jobec
Copy link
Collaborator

jobec commented Jun 16, 2017

I've opened separate issue for your 3 remarks.

@jobec jobec closed this as completed Jun 16, 2017
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

No branches or pull requests

2 participants