Skip to content
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

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open
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
82 changes: 82 additions & 0 deletions dojo/backends.py
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)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
all_saml_groups = {group.name: group for group in Dojo_Group.objects.all() if self.group_re.match(group.name)}
all_saml_groups = {group.name: group for group in Dojo_Group.objects.filter(social_provider=Dojo_Group.SAML) if self.group_re.match(group.name)}

Copy link
Contributor Author

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?


# 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
2 changes: 1 addition & 1 deletion dojo/settings/.settings.dist.py.sha256sum
Original file line number Diff line number Diff line change
@@ -1 +1 @@
01215b397651163c0403b028adb08b18fa83c4abb188b0536dfb9e43eddcd9cd
a3b00e1e7ef4d362201f8f2cb4b217fa4c72695008b8f0b6996853ed3371c29c
9 changes: 8 additions & 1 deletion dojo/settings/settings.dist.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand All @@ -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, ""),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you call it _READERS as it adds a Reader role? In the future

Suggested change
DD_SAML2_GROUPS_FILTER=(str, ""),
DD_SAML2_GROUPS_FILTER_READERS=(str, ""),

Or even more future-proof (if new roles would be created): dict with mapping to roles

Suggested change
DD_SAML2_GROUPS_FILTER=(str, ""),
DD_SAML2_GROUPS_FILTER=(dict, {"readers": ""}),

More info, about how dict works with environ

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Expand Down Expand Up @@ -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
Expand Down