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 submit_url field to requestToken responses, clarify HS's can send tokens themselves #2101

Merged
merged 22 commits into from Jun 11, 2019

Conversation

Projects
None yet
4 participants
@anoadragon453
Copy link
Member

commented Jun 7, 2019

The spec implementation of MSC2078.

Implementation proof: matrix-org/synapse#5377

anoadragon453 added some commits Jun 7, 2019

Merge branch 'master' into anoa/hs_3pid_tokens
* master:
  Update example
  Fix 404s in links from room v1 spec
  Provide a more complete example of a "minimally-sized event"
  Revert signature change for redactable event test
  Clarify how many PDUs are in a given transaction object
  Clarify that the server shouldn't process retries for UIA
  Clarify when authorization and rate-limiting are not applicable
  Skip over partial event definitions in examples
  Rename example to invite_room_state
  Shorten references to StrippedState in s2s spec
  Fix examples of StrippedState in s2s spec
  Clarify exactly what StrippedState is
  Clarify that UIA stages cannot be attempted twice
  Fix test vectors with invalid JSON and signature
  Spec 3PID unbind API
  Spec MSISDN UIA support

@turt2live turt2live added the Matrix 1.0 label Jun 7, 2019

@turt2live turt2live requested a review from matrix-org/spec-core-team Jun 7, 2019

@anoadragon453

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

image

Odd that the added section has an increase line spacing height. Is this due to having multiple paragraphs perhaps?

@turt2live

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

I wouldn't worry too much about the line height tbh - it's almost certainly because of the paragraphs, but I think we can fix that with css on Monday if we care (I don't, at least not enough to rush in a fix).

@turt2live
Copy link
Member

left a comment

I haven't done a review for accuracy - that probably needs an identity person. I have done a structural review though.

Because the same information is repeated several times, I stopped pointing out duplicate comments about halfway through - the feedback applies to more than what's pointed out.

Show resolved Hide resolved api/client-server/administrative_contact.yaml Outdated
Show resolved Hide resolved api/client-server/administrative_contact.yaml Outdated
Show resolved Hide resolved api/client-server/administrative_contact.yaml Outdated
Show resolved Hide resolved api/client-server/administrative_contact.yaml Outdated
Show resolved Hide resolved api/client-server/administrative_contact.yaml Outdated
Show resolved Hide resolved api/client-server/administrative_contact.yaml Outdated
Show resolved Hide resolved api/client-server/administrative_contact.yaml Outdated
Show resolved Hide resolved api/client-server/registration.yaml Outdated
Show resolved Hide resolved api/client-server/registration.yaml Outdated
Show resolved Hide resolved api/client-server/registration.yaml Outdated
@anoadragon453

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2019

@dbkr could you be an identity person and check this PR for accuracy?

@turt2live
Copy link
Member

left a comment

otherwise lgtm structurally, I guess.

Show resolved Hide resolved api/client-server/administrative_contact.yaml Outdated
Show resolved Hide resolved api/client-server/administrative_contact.yaml Outdated
Show resolved Hide resolved api/client-server/registration.yaml Outdated
Show resolved Hide resolved api/client-server/registration.yaml
Show resolved Hide resolved api/client-server/registration.yaml Outdated
Show resolved Hide resolved api/client-server/registration.yaml Outdated
Show resolved Hide resolved api/client-server/registration.yaml Outdated
Show resolved Hide resolved api/client-server/registration.yaml Outdated
Show resolved Hide resolved api/client-server/registration.yaml Outdated

anoadragon453 added some commits Jun 9, 2019

@turt2live
Copy link
Member

left a comment

More structural concerns to be addressed.

This PR also needs to address #2030 (comment) - the proposal should also mention this.

Also: Please resolve comment threads once the related concern is addressed. They do not auto-close.

Show resolved Hide resolved api/client-server/administrative_contact.yaml Outdated
Show resolved Hide resolved api/client-server/definitions/sid.yaml Outdated
Show resolved Hide resolved api/client-server/definitions/sid.yaml Outdated
Show resolved Hide resolved api/client-server/registration.yaml
Show resolved Hide resolved api/client-server/registration.yaml Outdated
Show resolved Hide resolved api/client-server/registration.yaml Outdated
@turt2live
Copy link
Member

left a comment

also the id_server and next_link parameter docs should mention that they are ignored when the homeserver handles the token collection.

@turt2live
Copy link
Member

left a comment

and a changelog, highlighting the breaking change (see #2078 (comment) )

@turt2live
Copy link
Member

left a comment

if volume of comments is anything to go by, this is getting closer :)

anoadragon453 and others added some commits Jun 10, 2019

Update changelogs/client_server/newsfragments/2101.breaking
Co-Authored-By: Travis Ralston <travpc@gmail.com>

@anoadragon453 anoadragon453 requested a review from turt2live Jun 10, 2019

@turt2live
Copy link
Member

left a comment

Last request: please link to the implementation in the PR description as proof this works in practice.

otherwise lgtm, thanks for suffering my nitpicking editor hat. I would recommend a checkmark from an identity person, but I believe this accurately represents the proposal and implementation.

@anoadragon453 anoadragon453 requested a review from dbkr Jun 11, 2019

@anoadragon453

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

Thanks for sticking with me on it! I've requested a final review from @dbkr as an 'identity person'.

@dbkr

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

also may be a good idea to add a few red flags to the impl proof just in case someone copies it with the vuln.

@turt2live

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

have added yelling to the PR description of matrix-org/synapse#5377

@turt2live turt2live requested a review from dbkr Jun 11, 2019

@dbkr

dbkr approved these changes Jun 11, 2019

Copy link
Member

left a comment

lgtm then!

@turt2live turt2live merged commit fbdb56a into master Jun 11, 2019

8 checks passed

buildkite/matrix-doc Build #318 passed (52 seconds)
Details
ci/circleci: build-dev-scripts Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
ci/circleci: check-docs Your tests passed on CircleCI!
Details
ci/circleci: validate-docs Your tests passed on CircleCI!
Details
docs Click details to preview the HTML documentation.
Details
swagger Click to preview the swagger build.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.