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

Fix AUD default value #33

wants to merge 6 commits into from

Conversation

marcelo225
Copy link
Owner

Fix the #32 issue

# 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'

@@ -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!

@hassaanalansary
Copy link

hassaanalansary commented Dec 14, 2023

Dont you think there should be a test for the aud='account' case

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

@marcelo225
Copy link
Owner Author

marcelo225 commented Dec 15, 2023

Dont you think there should be a test for the aud='account' case

@hassaanalansary I'm gonna create it

@marcelo225 marcelo225 closed this Feb 24, 2024
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