-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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(okta): custom properties for okta user #12773
feat(okta): custom properties for okta user #12773
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
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.
Mostly looks good
@@ -671,6 +671,21 @@ def _map_okta_user_profile(self, profile: UserProfile) -> CorpUserInfoClass: | |||
# TODO: Extract user's manager if provided. | |||
# Source: https://developer.okta.com/docs/reference/api/users/#default-profile-properties | |||
full_name = f"{profile.firstName} {profile.lastName}" | |||
custom_properties = { | |||
k: v | |||
for k, v in profile.__dict__.items() |
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.
is this a safe thing to do? maybe we should also check that not k.startswith('_')
?
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.
addressed in 79a201f
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.
about safety... well, it was safe for UserProfile
Checklist