-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
SAML2 group mapping #11273
base: dev
Are you sure you want to change the base?
SAML2 group mapping #11273
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
import logging | ||
import re | ||
from functools import cached_property | ||
|
||
from django.conf import settings | ||
from djangosaml2.backends import Saml2Backend as _Saml2Backend | ||
|
||
from dojo.authorization.roles_permissions import Roles | ||
from dojo.models import Dojo_Group, Dojo_Group_Member, Role | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class Saml2Backend(_Saml2Backend): | ||
|
||
"""Subclass to handle adding SAML2 groups as DefectDojo/Django groups to a user""" | ||
|
||
@cached_property | ||
def group_re(self): | ||
if settings.SAML2_ENABLED and settings.SAML2_GROUPS_ATTRIBUTE and settings.SAML2_GROUPS_FILTER: | ||
return re.compile(settings.SAML2_GROUPS_FILTER) | ||
return None | ||
|
||
def _update_user( | ||
self, | ||
user, | ||
attributes: dict, | ||
attribute_mapping: dict, | ||
force_save=False, | ||
): | ||
""" | ||
Method overriden to handle groups after user object is saved. | ||
Ideally we would only override "public" methods: in this case, get_or_create_user() would be the one but it doesn't save the NEW user | ||
We could override that AND save_user() (each to handle new or existing users) but the latter does not receive the attributes which include the groups... | ||
|
||
This does NOT create the groups if they do not exist. They have to be created in the UI | ||
This is not a big issue and it works around an existing bug with dojo/group/utils.py::group_post_save_handler (user does not yet exist and he is forcefully added to the new group - boom) | ||
""" | ||
user = super()._update_user(user, attributes, attribute_mapping, force_save=force_save) | ||
if self.group_re is None: | ||
return user | ||
|
||
# list of all existing "SAML2-mapped" groups | ||
all_saml_groups = {group.name: group for group in Dojo_Group.objects.all() if self.group_re.match(group.name)} | ||
|
||
# list of groups user MUST have | ||
needs_groups = set() | ||
if attributes[settings.SAML2_GROUPS_ATTRIBUTE]: | ||
needs_groups.update( | ||
group_name | ||
for group_name in attributes[settings.SAML2_GROUPS_ATTRIBUTE] | ||
if self.group_re.match(group_name) | ||
) | ||
|
||
# list of groups user ALREADY has | ||
has_groups = { | ||
dgm.group.name: dgm | ||
for dgm in Dojo_Group_Member.objects.filter(user_id=user.id).select_related("group") | ||
if dgm.group.name in all_saml_groups | ||
} | ||
|
||
groups_to_remove = has_groups.keys() - needs_groups | ||
groups_to_add = needs_groups - has_groups.keys() | ||
|
||
if groups_to_remove: | ||
# bulk .delete() can be used as it emits post_delete signal | ||
deleted, _ = Dojo_Group_Member.objects.filter(user_id=user.id, group__name__in=groups_to_remove).delete() | ||
logger.info("User %s removed from SAML2 groups: %s", user, ", ".join(groups_to_remove)) | ||
if deleted != len(groups_to_remove): | ||
logger.error("User %s had %d groups to be removed but %d were", user, len(groups_to_remove), deleted) | ||
|
||
if groups_to_add: | ||
# .bulk_create() cannot be used as it does NOT emit post_save signal | ||
reader_role = Role.objects.get(id=Roles.Reader) | ||
for group_name in groups_to_add: | ||
group = all_saml_groups.get(group_name) | ||
if group is None: | ||
logger.error("Group %s is mapped for SAML2 but it does not exist in Dojo", group_name) | ||
else: | ||
Dojo_Group_Member.objects.create(group=group, user_id=user.pk, role=reader_role) | ||
logger.debug("User %s became member of SAML2 group: %s", user, group.name) | ||
return user |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
01215b397651163c0403b028adb08b18fa83c4abb188b0536dfb9e43eddcd9cd | ||
a3b00e1e7ef4d362201f8f2cb4b217fa4c72695008b8f0b6996853ed3371c29c |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -155,7 +155,7 @@ | |||||||||
DD_SOCIAL_AUTH_GITHUB_ENTERPRISE_SECRET=(str, ""), | ||||||||||
DD_SAML2_ENABLED=(bool, False), | ||||||||||
# Allows to override default SAML authentication backend. Check https://djangosaml2.readthedocs.io/contents/setup.html#custom-user-attributes-processing | ||||||||||
DD_SAML2_AUTHENTICATION_BACKENDS=(str, "djangosaml2.backends.Saml2Backend"), | ||||||||||
DD_SAML2_AUTHENTICATION_BACKENDS=(str, "dojo.backends.Saml2Backend"), | ||||||||||
# Force Authentication to make SSO possible with SAML2 | ||||||||||
DD_SAML2_FORCE_AUTH=(bool, True), | ||||||||||
DD_SAML2_LOGIN_BUTTON_TEXT=(str, "Login with SAML"), | ||||||||||
|
@@ -177,6 +177,11 @@ | |||||||||
"Lastname": "last_name", | ||||||||||
}), | ||||||||||
DD_SAML2_ALLOW_UNKNOWN_ATTRIBUTE=(bool, False), | ||||||||||
# similar to DD_SOCIAL_AUTH_AZUREAD_TENANT_OAUTH2_GROUPS_FILTER, regular expression for which SAML2 groups to map to Dojo groups (with same name) | ||||||||||
# Groups need to already exist in Dojo. And if value is not set, no group processing is done | ||||||||||
DD_SAML2_GROUPS_FILTER=(str, ""), | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you call it
Suggested change
Or even more future-proof (if new roles would be created): dict with mapping to roles
Suggested change
More info, about how dict works with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At first sight, I agree this would make sense, but does it really? When would anyone not be just reader in a group where the membership is expected to be managed by IdP? Noone should be able to add/remove people from the group in Dojo, they should do it in IdP, otherwise that person will eventually be removed from the group (upon login) Maybe I'm missing some use case in my mind, please let me know if so. Otherwise this would just increase complexity of the configuration without any added benefit |
||||||||||
# SAML2 attribute with groups to match in Dojo. And if value is not set, no group processing is done | ||||||||||
DD_SAML2_GROUPS_ATTRIBUTE=(str, ""), | ||||||||||
# Authentication via HTTP Proxy which put username to HTTP Header REMOTE_USER | ||||||||||
DD_AUTH_REMOTEUSER_ENABLED=(bool, False), | ||||||||||
# Names of headers which will be used for processing user data. | ||||||||||
|
@@ -1054,6 +1059,8 @@ def saml2_attrib_map_format(dict): | |||||||||
}, | ||||||||||
"valid_for": 24, # how long is our metadata valid | ||||||||||
} | ||||||||||
SAML2_GROUPS_ATTRIBUTE = env("DD_SAML2_GROUPS_ATTRIBUTE") | ||||||||||
SAML2_GROUPS_FILTER = env("DD_SAML2_GROUPS_FILTER") | ||||||||||
|
||||||||||
# ------------------------------------------------------------------------------ | ||||||||||
# REMOTE_USER | ||||||||||
|
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.
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.
I guess this makes sense thinking of the AzureAD pipeline yet it "forces" groups to have been created by this (instead of added manually to match IdP groups), right?