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.

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

Finalize naming for the IDP sign-in status API #430

Closed
cbiesinger opened this issue Feb 9, 2023 · 2 comments · Fixed by #436
Closed

Finalize naming for the IDP sign-in status API #430

cbiesinger opened this issue Feb 9, 2023 · 2 comments · Fixed by #436

Comments

@cbiesinger
Copy link
Collaborator

https://github.com/fedidcg/FedCM/blob/main/proposals/idp-sign-in-status-api.md

We currently use these names:

IdP-SignIn-Status: action=signin
IdP-SignIn-Status: action=signout-all

interface IdentityProvider {
  static void login();
  static void logout();
}

We should make sure these are the names we want to use. I believe @bvandersloot-mozilla / @martinthomson had thoughts on this.

@samuelgoto samuelgoto added agenda+ Regular CG meeting agenda items spec review labels Feb 10, 2023
@bvandersloot-mozilla
Copy link
Collaborator

Alternative proposal, leaving the headers alone but altering the Javascript version.

IdP-SignIn-Status: action=signin
IdP-SignIn-Status: action=signout-all
interface IdentityProvider {
  static void recordSignIn(IdentityProviderSignInOptions? options);
  static void recordSignOut(IdentityProviderSignOutOptions? options);
}

dictionary IdentityProviderSignInOptions {
};

dictionary IdentityProviderSignOutOptions {
};

Three changes:

  1. prepend record to the function names to make clear that this is not inducing the browser to perform actions, but is instead mutating some internal state about the provider in the browser.
  2. unify on Sign In rather than splitting on log in and sign in.
  3. Add a nullable options arguments that are initially empty to allow extensions.

@cbiesinger
Copy link
Collaborator Author

Thanks! That basically sounds good.

I don't think we need the empty dictionaries; we can just add them when needed, right? Adding a new optional parameter is generally backwards compatible.

Also, just to confirm, does that match what Martin wanted (since he talked about this in that meeting)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants