-
Notifications
You must be signed in to change notification settings - Fork 320
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
feat: scim #566
base: main
Are you sure you want to change the base?
feat: scim #566
Conversation
|
# Conflicts: # src/backend/config/routers.py # src/backend/main.py # src/backend/tests/factories/__init__.py
# Conflicts: # src/backend/crud/user.py
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #566 /- ##
==========================================
Coverage 81.38% 83.50% 2.11%
==========================================
Files 199 243 44
Lines 8354 11037 2683
==========================================
Hits 6799 9216 2417
- Misses 1555 1821 266 ☔ View full report in Codecov by Sentry. |
src/backend/config/routers.py
Outdated
Depends(get_session), | ||
Depends(ScimAuthValidation()), | ||
], | ||
"auth": [], |
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.
Why none in auth?
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.
misunderstand how the dependency system work, also added to auth now
async def get_users( | ||
session: DBSessionDep, | ||
count: int = 100, | ||
start_index: int = 1, |
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.
Why 1 not 0?
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.
This is in the spec 🤷
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.
https://datatracker.ietf.org/doc/html/rfc7644
startIndex The 1-based index of the first result in the current set
of list results. REQUIRED when partial results are returned due
to pagination.
src/backend/routers/scim.py
Outdated
) -> ListUserResponse: | ||
if filter: | ||
single_filter = filter.split(" ") | ||
filter_value = single_filter[2].strip('"') |
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.
Should we do validation here?
session: DBSessionDep, | ||
count: int = 100, | ||
start_index: int = 1, | ||
filter: Optional[str] = None, |
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.
Why have filter and the one below for username? Can you only filter by username?
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.
For okta yes. That's all they support. Might be different for other providers.
async def get_groups( | ||
session: DBSessionDep, | ||
count: int = 100, | ||
start_index: int = 1, |
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.
Same questions about start index, fitler (just by username) etc
This PR adds SCIM functionality to the project. It allows an organisation to sync their users and groups from their idp.
Allows synchronisation of users and groups from Okta to toolkit. While SCIM is an open standard, this implementation was only tested with Okta. If we want to add more providers we would need to retest, but as long as they are following the spec it should be fine (similar to OIDC).
General Notes
Resources
PR Template
Thank you for contributing to the Cohere Toolkit!
PR title: "area: description"
PR message: Delete this entire checklist and replace with
Add tests and docs: Please include testing and documentation for your changes
Lint and test: Run
make lint
andmake run-tests
AI Description
This PR introduces the System for Cross-domain Identity Management (SCIM) to the backend. SCIM is a standard for automating the exchange of user identity information between identity domains, such as between an enterprise identity management system and a service provider.
The changes include:
.env-template
file.SCIMAuthValidation
class inrequest_validators.py
for SCIM authentication.SCIM
router inrouters.py
.secrets.template.yaml
andsettings.py
files.group.py
anduser.py
, respectively.scim.py
.schemas/scim.py
.ScimAuthValidation
class inrequest_validators.py
for SCIM authentication.get_or_create_user
function inutils.py
to use theget_user_by_external_id
function.test_scim.py
.secrets.yaml
file.