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

Break the Login Status API out of the FedCM spec #642

Merged
merged 9 commits into from
Sep 5, 2024
Merged

Conversation

samuelgoto
Copy link
Collaborator

Resolves #641.

@samuelgoto samuelgoto requested a review from npm1 August 22, 2024 23:41
@npm1
Copy link
Collaborator

npm1 commented Aug 23, 2024

Uhm is this a request from someone? Also I'm not sure but I imagine there needs to be some procedure to add a new spec to the WG (even if it is all extracted from the current spec). Finally, in terms of editor I would suggest to name the one who actually wrote this which I think is @cbiesinger

@cbiesinger
Copy link
Collaborator

Also I'm not sure but I imagine there needs to be some procedure to add a new spec to the WG (even if it is all extracted from the current spec).

As far as I know the primary requirement is that the spec is in the charter, which it is (https://www.w3.org/2024/03/wg-fedid-charter.html#deliverables)

I see this was also added to the agenda for the next call

spec/login-status.bs Outdated Show resolved Hide resolved
spec/login-status.bs Outdated Show resolved Hide resolved
@samuelgoto
Copy link
Collaborator Author

samuelgoto commented Aug 23, 2024

Uhm is this a request from someone?

Yeah, we have always intended to break the Login Status spec out of the FedCM spec, but never got to it: a browser vendor should be able to implement the Login Status API without having to implement the FedCM API.

From charter:
Currently a separate chapter in the FedCM specification, the goal is to publish it as a separate deliverable to be used by FedCM.

privacycg/is-logged-in#55 (comment)
https://www.w3.org/2023/09/13-login-status-minutes.html
https://www.w3.org/2024/03/wg-fedid-charter.html#deliverables (separate and orthogonal deliverables)

and more recently:

w3c-fedid/login-status#4

Also I'm not sure but I imagine there needs to be some procedure to add a new spec to the WG (even if it is all extracted from the current spec)

I'm already working with @hlflanagan and @wseltzer to figure out how to go about this. We would likely want to also move the spec to its own repo and the login-status repo from the CG to the WG:

https://github.com/fedidcg/login-status

We also want to coordinate with @johnwilander if he would rather have us use the FedID CG repo or the Privacy CG repo as the basis for the FedID WG repo (e.g. keeping the history and the issues). Either works for me, but we can have that discussion separately.

Finally, in terms of editor I would suggest to name the one who actually wrote this which I think is @cbiesinger

Yep, chatted with @cbiesinger who agreed to edit here too. Done.

We also want to coordinate with @johnwilander and @bvandersloot-mozilla if they still feel comfortable sharing co-editorship here (TPAC 2023 we had the three of us as co-editors).

@samuelgoto samuelgoto changed the title Refactor the Login status API out of the FedCM spec Break the Login status API out of the FedCM spec Aug 23, 2024
@samuelgoto samuelgoto changed the title Break the Login status API out of the FedCM spec Break the Login Status API out of the FedCM spec Aug 23, 2024
spec/login-status.bs Outdated Show resolved Hide resolved
spec/login-status.bs Outdated Show resolved Hide resolved
spec/login-status.bs Outdated Show resolved Hide resolved
spec/login-status.bs Outdated Show resolved Hide resolved
spec/login-status.bs Outdated Show resolved Hide resolved
1. If |client| is null, return.
1. If |origin| is not [=same origin=] with the [=/request=]'s
[=request/origin=], return.
1. If |client| is not [=same-origin with its ancestors=], return.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You seem to have copy pasted an outdated version. We updated this to same-site...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you sure? This seems to be up to date with HEAD?

https://github.com/w3c-fedid/FedCM/blob/main/spec/index.bs#L400

Copy link
Collaborator

@npm1 npm1 Sep 3, 2024

Choose a reason for hiding this comment

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

That says same-site?

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, yes, interesting, for some reason I missed this PR in my branch:

548e7b2

I sync and merged and I think this is now up to date. Sanity check?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now it seems fine. I guess the plan is to create a new repo for this, right? So anyways the change in this repo will only be to remove the existing spec text.

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 guess the plan is to create a new repo for this, right?

Yes

So anyways the change in this repo will only be to remove the existing spec text.

Yes, that's correct. I wanted to move into a single PR so that it is clear that the break was clean, rather than in two separate PRs into different repos.

spec/index.bs Show resolved Hide resolved
spec/login-status.bs Show resolved Hide resolved
spec/login-status.bs Show resolved Hide resolved
spec/Makefile Show resolved Hide resolved
Copy link
Collaborator Author

@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.

Done

Also, added John Wilander as a co-editor.

1. If |client| is null, return.
1. If |origin| is not [=same origin=] with the [=/request=]'s
[=request/origin=], return.
1. If |client| is not [=same-origin with its ancestors=], return.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you sure? This seems to be up to date with HEAD?

https://github.com/w3c-fedid/FedCM/blob/main/spec/index.bs#L400

spec/login-status.bs Show resolved Hide resolved
spec/login-status.bs Show resolved Hide resolved
spec/login-status.bs Outdated Show resolved Hide resolved
spec/login-status.bs Outdated Show resolved Hide resolved
spec/Makefile Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/index.bs Show resolved Hide resolved
spec/login-status.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.

lgtm modulo the same-origin comment

@samuelgoto samuelgoto merged commit 57d30a6 into main Sep 5, 2024
2 checks passed
@samuelgoto samuelgoto deleted the login-status branch September 5, 2024 18:32
github-actions bot added a commit that referenced this pull request Sep 5, 2024
SHA: 57d30a6
Reason: push, by samuelgoto

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
npm1 pushed a commit that referenced this pull request Sep 18, 2024
* Move the Login Status API out of the FedCM spec

* Remove the login status spec from the FedCM spec

* Set Christian as the editor

* Use the right user id for Christian

* Address feedback

* Merge with PR 548e7b2

* Add link references to the login status spec

* Remove the reference to same-origin with its ancestors

---------

Co-authored-by: Sam Goto <[email protected]>
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.

Break the Login Status API spec out of the FedCM spec
3 participants