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

Specify the continuation API #662

Merged
merged 7 commits into from
Nov 18, 2024
Merged

Specify the continuation API #662

merged 7 commits into from
Nov 18, 2024

Conversation

cbiesinger
Copy link
Collaborator

@cbiesinger cbiesinger commented Oct 3, 2024

way.
1. Wait for one of the following conditions:
* The user closes the browsing context: return failure.
* {{IdentityProvider}}.{{IdentityProvider/close}} is called in the

Choose a reason for hiding this comment

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

Should this actually be a reject() if the completion mechanism is resolve() — borrowing naming from Promises?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yeah, that sounds nicer to me too!

Choose a reason for hiding this comment

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

That or something like abort(reason?: string) and finish(token: string, accountId?: string)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IdentityProvider.close already exists (added to support logging in), so we either need to make it do something or make a decision it should not do something in this context.

IdentityProvider.reject makes sense to me but I'd prefer waiting until we have the error API (#498) because that's what adds the error code and URL to the returned credential. We have no place to put the reason until then.

(for what it's worth, we also have no IDP feedback on an API like reject)

@ThisIsMissEm
Copy link

I'm glad to see this idea being implemented!

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

@samuelgoto samuelgoto left a comment

Choose a reason for hiding this comment

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

Minor nit

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

wseltzer commented Oct 8, 2024

Discussed at 8 October meeting [minutes to be linked]
Minutes: https://github.com/fedidcg/meetings/blob/main/2024/2024-10-08-notes.md

@@ -1251,7 +1267,8 @@ To <dfn>fetch an identity assertion</dfn> given a {{USVString}}

<xmp class="idl">
dictionary IdentityProviderToken {
required USVString token;
USVString token;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you put some thought on how/whether we should think about forwards compatibility?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will address that in a later PR

@yi-gu yi-gu added the agenda+ Regular CG meeting agenda items label Nov 6, 2024
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 Outdated Show resolved Hide resolved
@philsmart
Copy link
Contributor

FWIW, I believe the IdP needs to have the option to request user interaction during token creation, for example when the RP has requested a higher level of authentication assurance (step-up authentication). Therefore, I consider this API essential.

spec/index.bs Outdated
1. If |tokenPair| is failure, set |credential| to failure and return.
1. Let |tokenString| be the first entry of |tokenPair|.
1. If the second entry of |tokenPair| is not null, set |accountId| to that second entry.
1. Wait for |tokenString| to be set if necessary.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If necessary? This is not clear. I think we wait until either credential is set to failure or tokentString is set. In the former, we return here (note that the return in the inner algorithm does not also automatically return from here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be fixed now.

I also rebased. PTAL!

@hlflanagan
Copy link
Contributor

@npm1 npm1 merged commit 344458d into w3c-fedid:main Nov 18, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Nov 18, 2024
SHA: 344458d
Reason: push, by npm1

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@cbiesinger cbiesinger deleted the continue branch November 18, 2024 19:26
github-actions bot added a commit to mattdanielbrown/WebID that referenced this pull request Nov 19, 2024
SHA: 344458d
Reason: push, by pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agenda+ Regular CG meeting agenda items
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants