-
Notifications
You must be signed in to change notification settings - Fork 58
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
Ensure SSO works on admin level #237
Conversation
Reviewer's Guide by SourceryThis PR refactors the SSO implementation by moving it from the organizer level to the admin level and implementing it as an external plugin. The changes include removing SSO-related code from the organizer views and forms, creating new dedicated SSO views and templates, and updating the authentication flow to use dynamic SSO provider configuration. Updated class diagram for SSO implementationclassDiagram
class SSOConfigureView {
+get_object()
+get_success_url()
+form_valid(form)
+form_invalid(form)
+get_context_data(**kwargs)
}
class SSODeleteView {
+get_object(queryset=None)
+action_object_name()
+action_back_url
+post(*args, **kwargs)
}
class SSOClientForm {
+fields: client_id, secret
}
SSOConfigureView --> SSOClientForm
SSODeleteView --> SocialApp
SSOConfigureView --> SocialApp
class SocialApp {
+provider
+name
+client_id
+secret
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @lcduong - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please add tests to cover the new SSO configuration functionality and authentication flow changes, as these are security-critical features.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -39,8 +43,11 @@ def oauth2_login_view(request, *args, **kwargs): | |||
|
|||
|
|||
def oauth2_callback(request): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Add error handling for missing SSO provider
The function should handle the case where no SSO provider exists, possibly redirecting to a configuration page with an appropriate message.
form_class = SSOClientForm | ||
model = SocialApp | ||
|
||
def get_object(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider caching the get_object result and simplifying form handling to reduce code complexity.
The current implementation has unnecessary complexity that can be simplified while maintaining Django patterns:
- Cache the get_object result as a property to avoid multiple database queries:
@property
def sso_provider(self):
if not hasattr(self, '_sso_provider'):
self._sso_provider = SocialApp.objects.filter(
provider=settings.EVENTYAY_SSO_PROVIDER
).first()
return self._sso_provider
- Simplify form handling by leveraging Django's built-in functionality:
def form_valid(self, form):
instance = form.save(commit=False)
instance.provider = settings.EVENTYAY_SSO_PROVIDER
instance.name = "Eventyay Ticket Provider"
instance.save()
instance.sites.add(Site.objects.get(pk=settings.SITE_ID))
messages.success(self.request, phrases.base.saved)
return super().form_valid(form)
def get_success_url(self):
return self.request.path
This removes the need for a separate form_invalid method since the parent class already handles it appropriately. The property caching pattern reduces database queries and improves code organization while maintaining Django conventions.
oauth2_session = OAuth2Session( | ||
settings.OAUTH2_PROVIDER["CLIENT_ID"], | ||
sso_provider.client_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.first()
returns nullable object. sso_provider
can be None
and sso_provider.client_id
will raise error.
* Ensure SSO works on admin level and change implementation to be an external plugin * remove unsed template * fix pipeline * using provider name constant instead of text * update transaction for storing key * handle case sso_provider not configured --------- Co-authored-by: odkhang <[email protected]>
This PR closes/references issue #234 .
Implement new page /orga/admin/sso/settings which allow Admin to set key pair for SSO, Admin able to add/delete key pair
How has this been tested?
Checklist
Summary by Sourcery
Implement a new admin-level SSO settings page and refactor SSO functionality to be managed as an external plugin, enhancing the system's modularity and allowing administrators to manage SSO key pairs directly.
New Features:
Enhancements: