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.

Sign up for GitHub

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

Jump to bottom

providers/scim: fix scim sync (#11165) #11827

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

providers/scim: fix scim sync (#11165) #11827

wants to merge 1 commit into from
+6 −6

Conversation

kpodp0ra
Copy link

Issue

Closes #11165

During SCIM synchronization, server can throw ObjectExistsSyncException to mark that one of the N objects are already synchronized. Authentik doesn't handle this error properly and crashes instead of displaying a simple warning.

Details

SCIM can throw these errors:

authentik/authentik/providers/scim/clients/base.py

Lines 72 to 77 in eac3e88

if response.status_code == HttpResponseNotFound.status_code:
raise NotFoundSyncException(response)
if response.status_code in [HTTP_TOO_MANY_REQUESTS, HTTP_SERVICE_UNAVAILABLE]:
raise TransientSyncException()
if response.status_code == HTTP_CONFLICT:
raise ObjectExistsSyncException(response)

But only BadRequestSyncException and TransientSyncException are caught and handled properly:

authentik/authentik/lib/sync/outgoing/tasks.py

Line 135 in eac3e88

except BadRequestSyncException as exc:

authentik/authentik/lib/sync/outgoing/tasks.py

Line 158 in eac3e88

except TransientSyncException as exc:

NotFoundSyncException and ObjectExistsSyncException are unhandled.

Solution

I've come up with two solutions:

  1. Add two another except cases: 'except NotFoundSyncException as exc: ' and 'except ObjectExistsSyncException as exc: ' to authentik/lib/sync/outgoing/tasks.py
  2. Subclass these two unhandled exceptions to already-handled BadRequestSyncException

In this PR I've chosen the second option and looked up the code to make sure that this will create no conflicts.

@kpodp0ra kpodp0ra requested a review from a team as a code owner October 27, 2024 01:00
@netlify Netlify
Copy link

netlify bot commented Oct 27, 2024 •
edited
Loading

✅ Deploy Preview for authentik-storybook canceled.

Name Link
🔨 Latest commit 48a8364
🔍 Latest deploy log https://app.netlify.com/sites/authentik-storybook/deploys/671d90a73c99940008472813

@netlify Netlify
Copy link

netlify bot commented Oct 27, 2024 •
edited
Loading

✅ Deploy Preview for authentik-docs canceled.

Name Link
🔨 Latest commit 48a8364
🔍 Latest deploy log https://app.netlify.com/sites/authentik-docs/deploys/671d90a752fcc700081e6e20

@BeryJu BeryJu self-assigned this Oct 31, 2024
BeryJu
BeryJu reviewed Nov 5, 2024
View reviewed changes
Copy link
Member

@BeryJu BeryJu left a comment

Choose a reason for hiding this comment

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

While this does fix the error, ObjectExistsSyncException should be handled separately as it should fetch the data from SCIM to get the remote ID and establish the connection

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BeryJu BeryJu BeryJu left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@BeryJu BeryJu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SCIM sync fails with exception
2 participants
@kpodp0ra @BeryJu