[Review] Quite a lot of minor improvements #1

koriaf · 2016-08-08T07:25:35Z

Hi!
Please do not merge this PR until you are 100% sure it's fine, because I didn't tested it in python3 fully yet.

I've made some minor improvements to make it works on my installation and now it's working fine. Please review this and quickly tell me if it worth going to master branch and if it safe to do it. If it's - then I will be able to create smaller commits with smaller changes subset and PR them separately (or commit directly to master if you would like to avoid merge commits).
Or feel free to pull/merge specific commits to master already, leaving problematic out of scope until they are fixed.

Thanks!

TODO: check with python3 and make correct fallback when required.

TODO: add tox and check for old django versions

…ng admin logout page is being shown

derek-pryor · 2016-08-17T15:15:46Z

Thanks for the pull request. Right now we are trying to figure out if / when to pull the changes into our branch. Once we figure out what we are doing we will let you know.

…mented

… disabled in settings

Fixes #3

Prefer that the view tries to log out from django and silently fails if the wrong OIDC client is requested.. This just fixes a specific problem I have right now where the views are wrapped in other views in the django application so this is maybe not a good general improvement. Closes #1

Will cause bugs if not overridden

Exceptions should probably be the first thing to get a full makeover in this library, right now I just copied and simplified exceptions.py/status.py from django rest framework. Things which have to be investigated are: - Should http response codes be included? (probably yes) - Should ALL oauth/oic exceptions be wrapped by corresponding local exception types? (maybe not) - Do we even need local exception types or does the ones which pyoic provide enough detail? (not really researched yet)

maiksprenger · 2017-06-30T14:11:47Z

I can confirm that this while I couldn't get master working, @koriaf's fork works a treat for me under Django 1.11 and Python 3.5. Thank you!

Update: Other users are probably still best served by yet another fork, based on @koriaf's work: https://github.com/py-pa/django-oidc

It's another hack but it's ok since a lot should be rewritten anyways.

…n_session Add auth token expires_in into django session

Merge py-pa/django-oidc work

make it more universal

…nts()

…etup of a client. This allows the use of 'private_key_jwt' for an authentication method.

…_jwt client authentication

Add ability to use "private_key_jwt" for client authentication

derek-pryor

Overall a good set of changes. Some questions on why things were done the way they were and a couple of changes that we don't want to accept (dealing with packaging)

Thank you for your contributions. We should be able to respond in a more timely manner in the future.

derek-pryor · 2017-11-04T13:28:16Z

README.rst

@@ -7,18 +7,21 @@ Behind the scenes, it uses Roland Hedberg's great pyoidc library.

 Modified by JHUAPL BOSS to support Python3

+Modified by Thomas Frössman with fixes and additional modifications.


May create an AUTHORS file / add to NOTICE instead of including this information in the README.

This line breaks setup.py, as it includes a non-ascii character and setup.py reads this file without setting the encoding to UTF-8.

derek-pryor · 2017-11-04T13:28:47Z

README.rst

 Quickstart
 ----------

 Install djangooidc::

    # Latest code - unstable!
-    pip install git+https://github.com/jhuapl-boss/django-oidc.git
-
+    pip install git+https://github.com/{desiredforkname}/django-oidc.git


We will want to keep our original version

derek-pryor · 2017-11-04T13:32:20Z

django_rp/settings.py

-            "post_logout_redirect_uris": ["http://localhost:8000/openid/callback/logout/"],
-        }
-    },
+    # "Azure Active Directory": {


Looking at the full file, this comments out all OIDC providers. What was the reason for this, as it leaves the django_rp example in an incomplete state.

derek-pryor · 2017-11-04T13:33:53Z

djangooidc/__init__.py

We will not want to downgrade our version number

derek-pryor · 2017-11-04T13:37:51Z

djangooidc/exceptions.py

+from djangooidc import status
+
+
+class OIDCException(Exception):


I like this addition. What is the reason for the comment out subclasses? It looks like they would be useful.

derek-pryor · 2017-11-07T02:32:52Z

djangooidc/views.py

+        query = parse_qs(request.GET.urlencode())
+        callback_result = client.callback(query, request.session)
+        if isinstance(callback_result, OIDCError):
+            raise callback_result


Why return the exception (instead of just raising it in client.callback)?

derek-pryor · 2017-11-07T02:40:54Z

djangooidc/views.py

+            # should include the post_logout_redirect_uri
+            request_args.update(extra_args)
+
+            url = client.provider_info['end_session_endpoint']


Thanks for cleaning this up

derek-pryor · 2017-11-07T02:45:19Z

djangooidc/views.py

        auth_logout(request)
        if next_page:
            request.session['next'] = next_page

+    return HttpResponseRedirect(next_page)


next_page may be None
Should probably redirect to '/' so similar if variable is not set

derek-pryor · 2017-11-07T02:47:20Z

setup.py

    packages=[
        'djangooidc',
    ],
    include_package_data=True,
    install_requires=[
-        'django>=1.8',
-        'oic>=0.7.6',
+        'django>=1.10',


This works with earlier versions of Django. We are running Django 1.9.1 and this update messes with the pip install for us.
Any particular reason for the version upgrades?

derek-pryor · 2017-11-07T02:47:36Z

setup.py

@@ -27,20 +27,20 @@
 history = open('HISTORY.rst').read().replace('.. :changelog:', '')

 setup(
-    name='django-oidc',
+    name='django-oidc-tf',


We will want to keep the original name

Thanks for the review! Some changes came from the fork py-pa/django-oidc which I merged to get both py3 and py2 support (I use this library both for 2 and 3), so can't say why it's been done what way - like this one.
But I'll try to handle issues which may be handled at all. Looks like some potential bugs were introduced so my projects are affected so it will be fixed.

* fix it up so that django 2.2 is tested and works * nonce/state are too small, need to make acr_value something that can be configured * added django 2.2 required request parameter, try using email address to log in, set unusable password for created users #3

Arkadiy Korotaev added 10 commits July 12, 2016 10:35

Fix python2 import back

7ddc980

Avoid bytes usage for python 2.7

dcc8b07

TODO: check with python3 and make correct fallback when required.

Avoid annoying deprecation warnings for django 1.10

4f09d92

TODO: add tox and check for old django versions

Get logout url from provider_info dict; pep8

37ad2bb

Make raw id_token available for Client class

4374b48

Disable fail on unavailable backend on application init; pep8

6b4553a

fix exception when session[next] not defined

a399a40

Gently redirect user to LOGOUT_REDIRECT_URL if it's defined, preventi…

85747e6

…ng admin logout page is being shown

Correctly handle client.callback error if it's returned, not raised

5cab86a

Add tests and tox runner; comment outdated Azure IDP configuration

0dbc898

Arkadiy Korotaev and others added 19 commits September 8, 2016 16:39

accept OP urls both with slash and without slash

bdc16cb

urls with optional slash at the end

68842b9

assume requestcontext is used by default for django 1.10

b7b2ad2

Logout even if user was logged in not with OIDC

f93a653

Support for acr_value parameter

bc11cbc

Allow normal auth flow even if userinfo endpoint ommited or not imple…

9d576b8

…mented

Raise readable error if response is missing or faulty

fe226cf

Better error handling for authz_cb view

5237ad8

Raise exception when someone tries to create dynamic client when it's…

33bce98

… disabled in settings

Fix py3 compat

4d0fd15

Modify package publishing meta data

b2f1011

Require django 1.10 or newer

ef63d55

Only care about python 3 support

79a445f

Update readme with additions for fork

ae304ba

Set version to 0.0.1

8623375

Add isort config

0b3d46d

Sort imports

81b1614

Remove everything older than py3.5 dj1.1 from tox env list

33ba4d9

Remove requirements file

1083673

thomasf added 11 commits May 14, 2017 13:46

Send kwargs to configure_user function

66c54c1

Version 0.0.5

c3a8dd1

Return http status 404 when dynamic clients are disabled

beeca78

Fixes #3

Simplify tox.ini

1bf2287

WIP drone

ae1bd79

Version 0.0.6

e6de2e0

Update drone config

e48e1d5

Add **kwargs to default backend impl

f8c2860

Will cause bugs if not overridden

Bump version

7932d0e

thomasf and others added 13 commits August 30, 2017 12:19

Add auth token expires_in into django session

fa8c30a

It's another hack but it's ok since a lot should be rewritten anyways.

Merge pull request jhuapl-boss#5 from py-pa/expose_token_expires_in_i…

57323b3

…n_session Add auth token expires_in into django session

v0.0.8

530264d

Merge with py-pa/django-oidc

c94d037

Visually fix py2.7 compatibility

527cfbc

Use less ugly unicode compat approach

fce378a

Merge pull request #1 from koriaf/test_merge

b9e9646

Merge py-pa/django-oidc work

Update README.rst

3b7aa3e

make it more universal

Fix naive datetime to timezone-aware for default backend

48b7b75

Added a check for "keyset_jwk_file" in setting up clients in OIDCClie…

82630cd

…nts()

Added in looking for a keyset_jwk_file in the 'client_registration' s…

1930d37

…etup of a client. This allows the use of 'private_key_jwt' for an authentication method.

Updated README.rs to include how to specify a client with private_key…

acdcbe6

…_jwt client authentication

Merge pull request #2 from srmoore/master

53f4350

Add ability to use "private_key_jwt" for client authentication

derek-pryor suggested changes Nov 7, 2017

View reviewed changes

jhuapl-boss deleted a comment from Allan-Nava Jan 11, 2018

timothy-spencer and others added 3 commits October 12, 2019 12:55

Django 2.2 support

2427f75

* fix it up so that django 2.2 is tested and works * nonce/state are too small, need to make acr_value something that can be configured * added django 2.2 required request parameter, try using email address to log in, set unusable password for created users #3

Replaced render_to_response with render

3c99327

More Django 3.0 changes

ae4a0ba

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Review] Quite a lot of minor improvements #1

[Review] Quite a lot of minor improvements #1

koriaf commented Aug 8, 2016

derek-pryor commented Aug 17, 2016

maiksprenger commented Jun 30, 2017 •

edited

Loading

derek-pryor left a comment

derek-pryor Nov 4, 2017

derek-pryor Nov 4, 2017

derek-pryor Nov 4, 2017

derek-pryor Nov 4, 2017

derek-pryor Nov 4, 2017

derek-pryor Nov 7, 2017

derek-pryor Nov 7, 2017

derek-pryor Nov 7, 2017

derek-pryor Nov 7, 2017

derek-pryor Nov 7, 2017

koriaf Nov 7, 2017 •

edited

Loading

		@@ -7,18 +7,21 @@ Behind the scenes, it uses Roland Hedberg's great pyoidc library.

		Modified by JHUAPL BOSS to support Python3

		Modified by Thomas Frössman with fixes and additional modifications.

		from djangooidc import status


		class OIDCException(Exception):

[Review] Quite a lot of minor improvements #1

Are you sure you want to change the base?

[Review] Quite a lot of minor improvements #1

Conversation

koriaf commented Aug 8, 2016

derek-pryor commented Aug 17, 2016

maiksprenger commented Jun 30, 2017 • edited Loading

derek-pryor 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

koriaf Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

maiksprenger commented Jun 30, 2017 •

edited

Loading

koriaf Nov 7, 2017 •

edited

Loading