Skip to content

Add support to SOCIAL_AUTH_OIDC_PROMPT and others #1127

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rgaiacs
Copy link

@rgaiacs rgaiacs commented Apr 24, 2025

This is related to #1124.

This is my first PR to this project. Feedback is welcome, specially regarding

  • how to trigger a logging warning?
  • how to add a test to this new feature?

If this PR looks good, I will submit others to complete #1124.

@rgaiacs
Copy link
Author

rgaiacs commented Apr 24, 2025

A PR with documentation was sent, see python-social-auth/social-docs#288.

Copy link

codecov bot commented Apr 24, 2025

Codecov Report

Attention: Patch coverage is 39.21569% with 31 lines in your changes missing coverage. Please review.

Project coverage is 77.69%. Comparing base (4e5ad63) to head (86d79bc).

Files with missing lines Patch % Lines
social_core/backends/open_id_connect.py 35.89% 18 Missing and 7 partials ⚠️
social_core/exceptions.py 50.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1127      +/-   ##
==========================================
- Coverage   77.87%   77.69%   -0.19%     
==========================================
  Files         353      353              
  Lines       10794    10845      +51     
  Branches      471      484      +13     
==========================================
+ Hits         8406     8426      +20     
- Misses       2222     2246      +24     
- Partials      166      173       +7     
Flag Coverage Δ
unittests 77.69% <39.21%> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@nijel nijel left a comment

Choose a reason for hiding this comment

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

If warnings were meant to indicate errors in the configuration, I think it's better to fail directly. Nobody will notice warnings.

As for the tests, it could check whether the configured parameters are included in the request to the authentication server.

@rgaiacs rgaiacs force-pushed the 1124-add-openid-connect-optional-parameter branch from ea244d6 to 36e841d Compare April 25, 2025 12:54
@rgaiacs rgaiacs changed the title Add support to SOCIAL_AUTH_OIDC_PROMPT Add support to SOCIAL_AUTH_OIDC_PROMPT and others Apr 25, 2025
@rgaiacs
Copy link
Author

rgaiacs commented Apr 25, 2025

Thanks for the feedback. I expanded to match the list of parameters explicitly mentioned in the Authentication Request section.

I don't implemented all parameters because of my limited knowledge of OpenID.

@rgaiacs
Copy link
Author

rgaiacs commented May 2, 2025

New exceptions were defined.

display = self.setting("DISPLAY", default=self.DISPLAY)
if display is not None:
if not display:
raise AuthMissingParameter("OpenID Connect display value cannot be empty string.")
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK these should take self as a first parameter.

Comment on lines +92 to +109
class AuthInvalidParameter(AuthException):
"""Invalid value for parameter to start or complete the process."""

def __init__(self, backend, parameter, *args, **kwargs):
self.parameter = parameter
super().__init__(backend, *args, **kwargs)

def __str__(self):
return f"Invalid value for parameter {self.parameter}"


class AuthNotImplementedParameter(AuthException):
"""Optional parameter not implemented to start or complete the process."""

def __init__(self, backend, parameter, *args, **kwargs):
self.parameter = parameter
super().__init__(backend, *args, **kwargs)

Copy link
Member

Choose a reason for hiding this comment

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

Let's reuse constructor logic:

Suggested change
class AuthInvalidParameter(AuthException):
"""Invalid value for parameter to start or complete the process."""
def __init__(self, backend, parameter, *args, **kwargs):
self.parameter = parameter
super().__init__(backend, *args, **kwargs)
def __str__(self):
return f"Invalid value for parameter {self.parameter}"
class AuthNotImplementedParameter(AuthException):
"""Optional parameter not implemented to start or complete the process."""
def __init__(self, backend, parameter, *args, **kwargs):
self.parameter = parameter
super().__init__(backend, *args, **kwargs)
class AuthInvalidParameter(AuthMissingParameter):
"""Invalid value for parameter to start or complete the process."""
def __str__(self):
return f"Invalid value for parameter {self.parameter}"
class AuthNotImplementedParameter(AuthMissingParameter):
"""Optional parameter not implemented to start or complete the process."""

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants