Skip to content

Set limit on size of providers #711

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

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

Set limit on size of providers #711

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions spec/index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -794,6 +794,7 @@ the exception thrown.
{{IdentityCredentialRequestOptionsMode/passive}} on |globalObject|'s [=Window/navigable=]'s
[=navigable/top-level traversable=] or on any of its descendants, reject the pending
request with a "{{NetworkError}}" {{DOMException}}.
1. If |providerList|'s [=list/size=] is greater than 10, return (failure, true).
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems arbitrarily small. I understand some of the risks if it is too large, but it seems to me it should not be hard-coded at the spec level. Relevant admin users should be able to adjust this as appropriate to a given deployment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can add enterprise settings in order to make this larger for admins that control browser deployments if needed. But I think it is more interoperable to have a concrete specific (and yes, arbitrary) value here so that the behavior is consistent across browsers and developers do not accidentally request a number of IdPs which works on some browsers but not on others.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when there are ten large IdPs and some number of small? Or is there an unstated goal to limit the overall number of IdPs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure what the size of the IdP has to do with anything? We want to limit the number of IdPs that a single RP can request in one go. Each IdP in the request can potentially introduce 5+ fetches so it would be too slow to include too many IdPs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Imagine, limiting the size of the IdP pool to 5 providers. Who might they be? Probably GoogleIdP, XIdP, AppleIdP, MicrosoftIdP, maybe GitHubIdP. What about JoeSmithIdP? What about DisneyIdP?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Federation is not only done through FedCM, so the question about today is about the current state of the world.

I'll reiterate that what you are imagining is not feasible without IDP registration, because the RP would need to embed one SDK per IDP, and we'd need to perform multiple fetches per IDP. So a get() with hundreds of IDPs is not feasible.

This limit is intended to only be limiting to people attempting to abuse the API. We can always change it or increase it if there are real-world requests where they would need a larger limit. Today, the largest I've seen is 5, so the limit I am proposing is double that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But IDP registration is something we want to do, and that use case is entirely boxed out by this approach to limiting only to 10.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why is that? Usually you just need 1 IDP in your get call to request any registered IDP. We can also tweak the limit to completely exclude registration cases if needed, when we spec that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see, this is the provided idp config list, not the actual set that shows up in the UI. I was a little turned around. Then I don't have strong opinions against this, but it really isn't needed in Firefox because of the IDP chooser.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. We don't need to add this to the spec, as I think the spec already provides enough leeway for the user agent to reject a request for whatever reason. But I figured it might be helpful to have a shared limit to reduce possible compat issues.

1. The user agent MAY wait an arbitrary amount of time before continuing with the next steps.
The user agent MAY also return (failure, false) at this point or after some arbitrary wait.
The user agent MAY also set |providerList| to a subset of itself after some arbitrary wait.
Expand Down