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

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Jun 7, 2019

The spec implementation of MSC2078.

Implementation proof: matrix-org/synapse#5377

* 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 Spec PRs that need review for 1.0 label Jun 7, 2019
@turt2live turt2live requested a review from a team June 7, 2019 16:56
@anoadragon453
Copy link
Member Author

image

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

@turt2live
Copy link
Member

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

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

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.

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 Show resolved Hide resolved
@anoadragon453
Copy link
Member Author

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

turt2live
turt2live previously approved these changes Jun 9, 2019
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

otherwise lgtm structurally, I guess.

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 Show resolved Hide resolved
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

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.

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 Show resolved Hide resolved
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

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

api/client-server/definitions/request_token_response.yaml Outdated Show resolved Hide resolved
changelogs/client_server/newsfragments/2101.breaking Outdated Show resolved Hide resolved
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

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 June 11, 2019 00:29
@anoadragon453
Copy link
Member Author

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

@dbkr
Copy link
Member

dbkr 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
Copy link
Member

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

@turt2live turt2live requested a review from dbkr June 11, 2019 14:50
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

lgtm then!

@turt2live turt2live merged commit fbdb56a into master Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Matrix 1.0 Spec PRs that need review for 1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants