-
Notifications
You must be signed in to change notification settings - Fork 475
feat(ui,clerk-js,shared,localizations): Add all members of a role to the SSO allow list #9826
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
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,16 @@ | ||
| --- | ||
| '@clerk/localizations': minor | ||
| '@clerk/clerk-js': minor | ||
| '@clerk/shared': minor | ||
| '@clerk/ui': minor | ||
| --- | ||
|
|
||
| The "Add members" card on the SSO allow list page of `<OrganizationProfile />` now offers two ways to add people: by email address, or every member with a given role at once. Members whose email address is not served by one of the organization's enterprise connections are skipped. When nothing could be added the card stays open and says why, and when some were added it moves to a success step that reports how many were skipped. | ||
|
|
||
| For custom flows, `organization.ssoBypassAllowlist` gains `addUsers({ userIds })`, which calls the new bulk endpoint in batches of 100 and returns the added entries together with the users that could not be added and why. | ||
|
|
||
| Inputs marked to be ignored by password managers now also carry the Bitwarden, LastPass and Dashlane opt-out attributes, so those extensions stop offering to fill fields such as the allow list email address. | ||
|
|
||
| The member picker that the "Add member" card shipped with in 4.18.0 is gone, and so are its localization keys under `organizationProfile.securityPage.ssoBypassPage.addForm`: `memberLabel`, `memberPlaceholder`, `changeButton` and `noResults`. The feature was never enabled on any instance, so no application depends on them. | ||
|
|
||
| New customization handles: the `organizationProfileSecuritySsoBypassEmailInput`, `organizationProfileSecuritySsoBypassRoleWarning`, `organizationProfileSecuritySsoBypassFailure` and `organizationProfileSecuritySsoBypassBulkResult` appearance elements. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1379,14 +1379,30 @@ export const arSA: LocalizationResource = { | |
| action__add: undefined, | ||
| action__search: undefined, | ||
| addForm: { | ||
| changeButton: undefined, | ||
| memberLabel: undefined, | ||
| memberPlaceholder: undefined, | ||
| noResults: undefined, | ||
| emailPlaceholder: undefined, | ||
|
Member
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. If a user previously customized these values in their application, how are we thinking about the fact this is a breaking change for them?
Contributor
Author
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. Fair point! I thought I could remove the keys since the feature isn’t being used in production yet, but that is not how things work in the SDK, right? I kept the existing keys and marked them as deprecated. does that work?
Member
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. ah if the feature wasn't turned on from the backend it's fine to drop these. I thought since it was merged and released in an SDK version that the feature was live. |
||
| error__allAlreadyAdded: undefined, | ||
| error__alreadyAdded: undefined, | ||
| error__memberNotFound: undefined, | ||
| modeLabel: undefined, | ||
| mode__email: undefined, | ||
| mode__role: undefined, | ||
| roleOption: undefined, | ||
| roleWarning: undefined, | ||
|
mauricioabreu marked this conversation as resolved.
|
||
| submitButton: undefined, | ||
| subtitle: undefined, | ||
| title: undefined, | ||
| }, | ||
| bulkResult: { | ||
| added: undefined, | ||
| addedMember: undefined, | ||
| added__one: undefined, | ||
| domainNotServed: undefined, | ||
| domainNotServed__one: undefined, | ||
| notMember: undefined, | ||
| notMember__one: undefined, | ||
| unknown: undefined, | ||
| unknown__one: undefined, | ||
| }, | ||
| table: { | ||
| emptyState: undefined, | ||
| emptyState__search: undefined, | ||
|
|
@@ -2049,6 +2065,7 @@ export const arSA: LocalizationResource = { | |
| protect_check_timed_out: undefined, | ||
| protect_check_unsupported_environment: undefined, | ||
| session_exists: 'لقد قمت بتسجيل الدخول بالفعل', | ||
| sso_bypass_domain_not_served: undefined, | ||
| ticket_expired_code: undefined, | ||
| ticket_invalid_code: undefined, | ||
| web3_missing_identifier: undefined, | ||
|
|
||
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.
unless I'm missing it (totally possible) I don't think we partition inputs into batches like this for other endpoints (such as the invite members flow). Is there something specific about this endpoint that requires the batching support built in? Do we think it's likely that customers will be attempting to add more than 100 members to the allowlist at a time?
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 think the problem here is that invitations and “adding all members of a given role” are a bit different.
When an admin invites members, they probably aren't typing or pasting 100+ members at once. it is more like a series of individual invites, which is different from being able to select a role with 300 members and add them all at once. does that make sense?
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 think that's fine for this PR then, but I think we should look at updating the API to support adding by role instead of expecting the client to handle it. What if you have a massive organization of 10k members with the same role and you add them? Extreme example but I'd rather tell the API "add all members with to the allowlist" than have the client individually batch those. but again, fine for this PR
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 talked to Stephen about it a few days ago. It is a follow-up, and we agreed that the current approach isn't ideal. At least we are using the same primitives, so any future work will be additive and won’t break existing customers