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

Add API to show error messages from failed token fetches #498

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

npm1
Copy link
Collaborator

@npm1 npm1 commented Aug 30, 2023

Adds the capability for the FedCM API to show error dialogs in certain scenarios after the user has chosen to perform federated login with an account. For this purpose:

  • An IdentityCredentialError is created, which may contain an error which is a string that contains the specific error, and the user agent may use to customize the UI). It may also contain url, in case the IDP wants to show a url where the user could get more information.
  • Shows error UI when there are network errors or when the IDP returns that an error has occurred.

Fixes #488


Preview | Diff

@npm1 npm1 requested a review from yi-gu August 30, 2023 21:40
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@npm1
Copy link
Collaborator Author

npm1 commented Sep 1, 2023

Ready for another look @yi-gu

Copy link
Collaborator

@yi-gu yi-gu left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM!

@npm1
Copy link
Collaborator Author

npm1 commented Sep 1, 2023

Hey @bvandersloot-mozilla @martinthomson @cboozar requesting Mozilla to take a look.

spec/index.bs Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
Copy link
Collaborator

@cbiesinger cbiesinger left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
[Exposed=Window, SecureContext]
interface IdentityCredentialError : DOMException {
constructor(optional DOMString message = "", optional IdentityCredentialErrorInit options = {});
readonly attribute DOMString code;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This overwrites a deprecated value that should be 0. Another name would be good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, good catch! We missed that... here are some other naming options, any preference?

  • errorCode
  • serverError{Code} (eg with or without Code)
  • idpError{Code}
  • credentialError{Code}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • error

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ping, I'd like to merge this sooner rather than later especially if we want to make a naming change here.

@samuelgoto
Copy link
Collaborator

@npm1 just checking: is there anything else we need to act on before we merge this?

@npm1
Copy link
Collaborator Author

npm1 commented Feb 27, 2024

@npm1 just checking: is there anything else we need to act on before we merge this?

Yes, I need to rebase and change the code to whatever we agree on (awaiting @bvandersloot-mozilla's confirmation about whether error is acceptable)

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

Successfully merging this pull request may close these issues.

Users may be confused after showing intent to sign in but the sign-in is failed
6 participants