Finish Shibboleth Integration #48

ArmaanT · 2019-07-29T16:50:38Z

This PR finalizes the Shibboleth login integration.
When a user logs in through Shibboleth, if it is their first time logging in a new user will be created with the supplied username, first name, last name, email. Note it appears Penn does not send an email through Shibboleth
On every login, user's affiliations are updated.

This PR should be heavily reviewed to ensure no authentication bypasses are possible.

coveralls · 2019-07-29T16:58:17Z

Coverage remained the same at 100.0% when pulling b75f0af on fix/shibboleth into 301adcc on master.

kirubarajan · 2019-07-29T17:59:44Z

Is there a TLDR on how this is tested?

kirubarajan · 2019-07-29T18:00:27Z

I see the unit tests for exception handling but wondering if its rigorous enough for route authorization and whatnot

ArmaanT · 2019-07-29T18:03:42Z

The unit tests are primarily to ensure that a user can log in though Shibboleth. Testing to make sure no one can break it should just involve a code review (including nginx configs) and sending requests to accounts/login/ with fake data to determine if we can log in as another user, or change user data.

I've done a few quick tests, and I'm pretty sure it's secure, but a few additional eyes wouldn't hurt.

kirubarajan · 2019-07-29T18:14:19Z

accounts/views.py

@@ -36,8 +33,7 @@ def get(self, request):
        user = auth.authenticate(remote_user=pennkey, shibboleth_attributes=shibboleth_attributes)
        if user:
            auth.login(request, user)
-            params = request.get_full_path().split('next=')[1]
-            return redirect('https://platform.pennlabs.org' + params)
+            return redirect(request.GET.get('next'))


Does GET['next'] contain the redirect URL for success? Is pennlabs.org hardcoded anywhere? Just curious about design choices.

Yep, that's it. It's a relative URL to redirect once logged in. There's no reference to pennlabs.org. In the normal login process next should be accounts/authorize?x=..., Since users will be logging in through OAuth2

Gotcha. If we don't hardcode pennlabs.org is there any way somebody could spoof the redirect URL back to ours with their own hosted instance of Shibboleth?

Nope, the redirect doesn't contain any user information. At that point the user is already logged into platform and a cookie is set. The redirect is just to let the user continue the OAuth2 flow. There shouldn't be any way to leak data through this.

kirubarajan · 2019-07-29T18:18:43Z

On every login, user's affiliations are updated.

Why is it on every login?

kirubarajan · 2019-07-29T18:21:05Z

Note it appears Penn does not send an email through Shibboleth

When should Shibboleth send emails? Is this functionality we need for forgotten passwords or something? Do we handle that?

ArmaanT · 2019-07-29T18:21:40Z

On every login, user's affiliations are updated.

Why is it on every login?

I don't really have a great reason for it, but my thinking was when people become alumni their affiliations will change. Additionally if a student teaches a class like CIS 19x, then their affiliations may change.

Eventually we may want to use this affiliations to determine what people can do. An example being letting professors choose comments on PCR

ArmaanT · 2019-07-29T18:24:10Z

Note it appears Penn does not send an email through Shibboleth

When should Shibboleth send emails? Is this functionality we need for forgotten passwords or something? Do we handle that?

Sorry, this is poor wording. I meant Penn does not send us a user's email through Shibboleth. In certain configurations (not Penn's) Shibboleth provides additional metadata on users who are logging in. It would be nice for us if we got students emails through Shibboleth, but it seems like we won't be able to do that. Penn would have to make that change. If we can't get a directory API key, then it's worth asking them to change, if possible.

kirubarajan · 2019-07-29T18:25:55Z

Got it. Can't find the affiliation changing code in this PR but it should be fine if we're using the standard Django abstractions. I can't really think of a better way anyway to update affiliation so this flow LGTM

kirubarajan · 2019-07-29T18:28:27Z

Is searchPennDirectory not implemented yet? Not sure why you want to include the stub but not important enough to request changes so up to your discretion.

ArmaanT · 2019-07-29T18:28:28Z

Got it. Can't find the affiliation changing code in this PR but it should be fine if we're using the standard Django abstractions. I can't really think of a better way anyway to update affiliation so this flow LGTM

This block in backends.py handles the affiliation updating. It just adds the correct affiliation in a many-to-many field in the user.

ArmaanT · 2019-07-29T18:30:27Z

Is searchPennDirectory not implemented yet? Not sure why you want to include the stub but not important enough to request changes so up to your discretion.

We don't have a directory API key that lets us search for the data we want (the data is hidden behind a PennKey login). So I can't yet implement the method. I've requested a key with elevated privileges, so we'll have to wait and see if we can get one.

kirubarajan · 2019-07-29T18:31:20Z

This block in backends.py handles the affiliation updating. It just adds the correct affiliation in a many-to-many field in the user.

Gotcha. I would personally abstract that into a helper function somewhere, just because from a dev perspective I guess it's not authentication code specifically? But non-breaking of course so maybe a later refactor could handle this

kirubarajan · 2019-07-29T18:35:25Z

Also for the sake of security updating the Django version number in requirements.txt?

kirubarajan · 2019-07-29T18:37:40Z

Overall, LGTM. Follows standard Shibboleth flow and OAuth2 conventions/best practices with no glaring vulnerabilities. From a functional standpoint ready to merge, but awaiting approval from @davish and @ezwang.

ArmaanT · 2019-07-29T18:38:37Z

Also for the sake of security updating the Django version number in requirements.txt?

That should be done, but I was waiting for coveralls to update. It pins the urllib3 version to something that is not supported by requests. I've already made a PR on that project to fix it (TheKevJames/coveralls-python#204).

Edit: updated in 37f8d6b

davish

Overall looks good to me! Most pressing open question for me is how this works as part of the reusable app. But assuming that that's fine, this looks good! Excited for integrating this.

davish · 2019-07-29T20:03:53Z

accounts/backends.py

-                        setattr(user, key, self.searchPennDirectory(username, key))
-
+                    if key != 'affiliation':
+                        setattr(user, key, value if value else self.searchPennDirectory(username, key))


Are we worried about latency issues for an external request to the Penn directory? What if the directory API is down, but shibboleth is still up? We should probably still accept logins in that case.

Read the thread with @kirubarajan, but still think this is something to think about at least.

That's a good question. Polling the directory only happens on a user's first login, so I don't think latency will be a real issue. We probably should add some error checking if the directory is down, but since we don't have access to it yet, I think we should leave it as is.

davish · 2019-07-29T20:05:14Z

accounts/backends.py

            user.save()
            user = self.configure_user(request, user)

+        # Update affiliations with every log in
+        if shibboleth_attributes is not None and 'affiliation' in shibboleth_attributes:


Probably could move the not-None check to encompass the if created branch as well. Not sure exactly how shibboleth_attributes is populated, but would it be a successful login if it was None?

See comment below

davish · 2019-07-29T20:06:17Z

accounts/views.py

        pennkey = request.META.get('HTTP_EPPN', '').lower().split('@')[0]
        first_name = request.META.get('HTTP_GIVENNAME', '').lower().capitalize()
        last_name = request.META.get('HTTP_SN', '').lower().capitalize()
        email = request.META.get('HTTP_MAIL', '').lower()
-        shibboleth_attributes = {'first_name': first_name, 'last_name': last_name, 'email': email}
+        affiliation = request.META.get('HTTP_UNSCOPED_AFFILIATION', '').split(';')
+        shibboleth_attributes = {'first_name': first_name, 'last_name': last_name, 'email': email,


From my comment above, looks like shibboleth_attributes being None would mean that something went wrong.

That's true. I added the check since I had previously written a test where shibboleth_attributes was empty, but you're right, it would be impossible for that to happen.

Fixed in b75f0af

davish · 2019-07-29T20:13:04Z

accounts/views.py

    """
    Log in a user.
+    WARNING: You must ensure this page is protected by Shibboleth and Clean Headers
+    See https://github.com/nginx-shib/nginx-http-shibboleth


From a cursory read of the linked page, I'm guessing this means that not everyone can hit the LoginView endpoint. I see this is in the accounts package... will we have to add a custom nginx rule on every dokku app that uses this package?

We just need to make sure that when platform is (re)deployed in production, we configure nginx to require Shibboleth authentication on the accounts/login/ route. This only needs to be done on platform though.

ArmaanT · 2019-07-29T20:27:20Z

Overall looks good to me! Most pressing open question for me is how this works as part of the reusable app. But assuming that that's fine, this looks good! Excited for integrating this.

The reusable app portion will be django-labs-accounts which I can finish once this PR is merged. The reusable app will just act as an OAuth2 client to interface with platform.

davish · 2019-07-29T20:52:58Z

Overall looks good to me! Most pressing open question for me is how this works as part of the reusable app. But assuming that that's fine, this looks good! Excited for integrating this.

The reusable app portion will be django-labs-accounts which I can finish once this PR is merged. The reusable app will just act as an OAuth2 client to interface with platform.

Does this repo pennlabs/platform not use django-labs-accounts? If it doesn't, I think it might be a good idea to switch it over to the reusable app for the sake of dogfooding once the app has been updated as well.

kirubarajan · 2019-07-29T20:57:38Z

Does this repo pennlabs/platform not use django-labs-accounts? If it doesn't, I think it might be a good idea to switch it over to the reusable app for the sake of dogfooding once the app has been updated as well.

Completely forgot about django-labs-accounts but 100% agree with this.

ArmaanT · 2019-07-29T21:02:38Z

Does this repo pennlabs/platform not use django-labs-accounts? If it doesn't, I think it might be a good idea to switch it over to the reusable app for the sake of dogfooding once the app has been updated as well.

So this is where I need to document things better. The way things are set up, platform lets users log in through Shibboleth. Users then have a valid session with platform which is also an OAuth2 provider. django-labs-accounts doesn't interface with Shibboleth at all. It is installed on other apps like clubs-backend and allows clubs-backend to log in users through platform's master database of users. django-labs-accounts gets its information about users from platform. So it doesn't really make sense to put it on platform. Not sure if I'm doing a good job explaining. I'll document the entire auth flow soon.

ezwang · 2019-07-31T03:22:00Z

accounts/views.py

        user = auth.authenticate(remote_user=pennkey, shibboleth_attributes=shibboleth_attributes)
        if user:
            auth.login(request, user)
-            params = request.get_full_path().split('next=')[1]
-            return redirect('https://platform.pennlabs.org' + params)
+            return redirect(request.GET.get('next', '/'))


I would protect this using a whitelist, since it could possibly open us up to phishing attacks by making the URL appear to come from us.

It might be a bit difficult, since there are a lot of different products and domains we could use this with. Maybe a simple list of domain names for now and expanding that into a model later?

I don't believe the redirect exposes us to any potential attack vectors. the user.authenticate method creates a cookie valid only on platform.pennlabs.org to verify the user is logged in, that information doesn't get sent through the redirect.

ArmaanT · 2019-08-01T03:16:53Z

Note CircleCI tests are failing due to the updated version of shortener. This is fixed in #49.

ArmaanT added 6 commits July 29, 2019 11:17

Fix Shibboleth Integration

6ab5b8b

Update Shibboleth Tests

a52b44d

Store Shibboleth User Affiliations

96ad1ee

Update Backend Tests

a304ef8

Compact Backend code

d64c5d7

Add Backend Comments

e0cfbe3

ArmaanT requested review from davish and ezwang July 29, 2019 16:51

Add login redirect fallback

23656b5

ArmaanT force-pushed the fix/shibboleth branch from 766c10a to 23656b5 Compare July 29, 2019 16:56

ArmaanT requested a review from kirubarajan July 29, 2019 18:11

kirubarajan reviewed Jul 29, 2019

View reviewed changes

Clear affilations on every log in

7cf2e4c

Use normal Django view for LoginView

36e575b

Update Packages

37f8d6b

davish approved these changes Jul 29, 2019

View reviewed changes

Remove unnecessary shibboleth_attributes check

b75f0af

ArmaanT requested a review from davish July 29, 2019 20:37

ezwang reviewed Jul 31, 2019

View reviewed changes

ArmaanT added 2 commits July 31, 2019 23:09

Add logout view

54826d5

Fix logout tests

fba1cbc

ArmaanT merged commit ee2d2ed into master Aug 1, 2019

ArmaanT deleted the fix/shibboleth branch August 1, 2019 03:22

Finish Shibboleth Integration #48

Finish Shibboleth Integration #48

Conversation

ArmaanT commented Jul 29, 2019

coveralls commented Jul 29, 2019 • edited Loading

kirubarajan commented Jul 29, 2019

kirubarajan commented Jul 29, 2019

ArmaanT commented Jul 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirubarajan commented Jul 29, 2019

kirubarajan commented Jul 29, 2019

ArmaanT commented Jul 29, 2019

ArmaanT commented Jul 29, 2019

kirubarajan commented Jul 29, 2019

kirubarajan commented Jul 29, 2019

ArmaanT commented Jul 29, 2019

ArmaanT commented Jul 29, 2019

kirubarajan commented Jul 29, 2019

kirubarajan commented Jul 29, 2019

kirubarajan commented Jul 29, 2019

ArmaanT commented Jul 29, 2019 • edited Loading

davish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArmaanT commented Jul 29, 2019

davish commented Jul 29, 2019

kirubarajan commented Jul 29, 2019

ArmaanT commented Jul 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArmaanT commented Aug 1, 2019 • edited Loading

coveralls commented Jul 29, 2019 •

edited

Loading

ArmaanT commented Jul 29, 2019 •

edited

Loading

ArmaanT commented Aug 1, 2019 •

edited

Loading