Skip to content

Fix AUD default value #33

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

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/python-publish.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,6 @@ jobs:
TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }}
TWINE_PYPI_URL: ${{ secrets.PYPI_URL }}
run: |
python setup.py sdist
ls .
twine upload --repository-url $TWINE_PYPI_URL -u $TWINE_USERNAME -p $TWINE_PASSWORD dist/*
python setup.py sdist
cp qr_code.png dist
twine upload --repository-url $TWINE_PYPI_URL -u $TWINE_USERNAME -p $TWINE_PASSWORD dist/*
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ MIDDLEWARE = [

# Exempt URIS
# For example: ['core/banks', 'swagger']
# This variable only works with Django Rest Framework Views, but not for native Django views.
KEYCLOAK_EXEMPT_URIS = []

# LOCAL_DECODE: is optional and False by default. If True
Expand Down
4 changes: 2 additions & 2 deletions django_keycloak_auth/keycloak.py
Copy link
Contributor

Choose a reason for hiding this comment

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

def userinfo(self, token, audience=None, options=None, raise_exception=True):

Choose a reason for hiding this comment

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

I tried aud=None but it didnt work for me.
I am not sure if that is the case for everybody

Copy link
Owner Author

Choose a reason for hiding this comment

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

I didn't get it.
What does it mean?
Do you suggest replacing these params?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the github formatting did not work well here. I was giving an example of updating the method signatures to include the arguments to the decode function. That way any audience and options may be passed to jwt.decode.

These three methods use the decode method:

  • is_token_active
  • roles_from_token
  • userinfo

Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,8 @@ def decode(self, token, audience=None, options=None, raise_exception=True):
json: decoded token
"""

if audience is None:
audience = self.client_id
if audience is None:
audience = 'account'

jwks = self.jwks()
keys = jwks.get('keys', [])
Expand Down
2 changes: 1 addition & 1 deletion django_keycloak_auth/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@
'KEYCLOAK_SERVER_URL': 'http://localhost:8080/auth',
'KEYCLOAK_REALM': 'TEST',
'KEYCLOAK_CLIENT_ID': 'client-backend',
'KEYCLOAK_CLIENT_SECRET_KEY': 'E2n41fJgl9BPIS3nDk1DQQ7BIPf6PauH',
'KEYCLOAK_CLIENT_SECRET_KEY': '',
'KEYCLOAK_CACHE_TTL': 60,
'LOCAL_DECODE': False,

Choose a reason for hiding this comment

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

I believe the default case should be True, because you dont need to bug the auth server with every request and it will add a delay in the request lifecycle

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I agree, but wanted to make sure the author gets to make the decision.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I agree too!

}
Expand Down
35 changes: 33 additions & 2 deletions django_keycloak_auth/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from .middleware import KeycloakMiddleware, KeycloakConnect
from core import views
from unittest.mock import Mock
from django.conf import settings


class KeycloakConfigTestCase(TestCase):
Expand Down Expand Up @@ -346,8 +347,38 @@ def test_when_token_is_active_and_has_roles_request_not_authorizated(self):

# THEN not allows endpoint to be accessed (401)
self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED)

def test_when_token_is_active_and_has_roles_request_authorizated(self):

# GIVEN LOCAL_DECODE as False in settings.py
settings.KEYCLOAK_CONFIG['LOCAL_DECODE'] = False

# GIVEN token as valid
KeycloakConnect.is_token_active = Mock(return_value=True)

# GIVEN token has roles
KeycloakConnect.roles_from_token = Mock(return_value=['director'])

# GIVEN userinfo from fake token
KeycloakConnect.userinfo = Mock(return_value=self.fake_userinfo)

# GIVEN a View endpoint has existis 'diretor' role on GET method

Choose a reason for hiding this comment

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

Small typo 'diretor'

view = views.loans

# GIVEN a fake request
request = self.factory.get(self.uri)
request.META['HTTP_AUTHORIZATION'] = 'Bearer token_fake'

# WHEN middleware is processed
KeycloakMiddleware(Mock()).process_view(request, view, [], {})

# THEN allows endpoint and pass token roles and userinfo to request View
self.assertEqual(['director'], request.roles)

def test_when_cached_token_is_active_and_has_roles_request_authorizated(self):

# GIVEN LOCAL_DECODE as True in settings.py
settings.KEYCLOAK_CONFIG['LOCAL_DECODE'] = True

# GIVEN token as valid
KeycloakConnect.is_token_active = Mock(return_value=True)
Expand All @@ -369,4 +400,4 @@ def test_when_token_is_active_and_has_roles_request_authorizated(self):
KeycloakMiddleware(Mock()).process_view(request, view, [], {})

# THEN allows endpoint and pass token roles and userinfo to request View
self.assertEqual(['director'], request.roles)
self.assertEqual([], request.roles)
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

setup(
name="django-keycloak-auth",
version="0.9.9",
version="1.0.0",
packages=find_packages(),

# Project uses reStructuredText, so ensure that the docutils get
Expand Down