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

Spec /directory/list #1141

Merged
merged 6 commits into from
Mar 6, 2018
Merged

Conversation

turt2live
Copy link
Member

Adds #417

Adds matrix-org#417

Signed-off-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Travis Ralston <travpc@gmail.com>
@@ -23,6 +23,8 @@ Unreleased changes
(`#1137 <https://github.com/matrix-org/matrix-doc/pull/1137>`_).
- Clarify that ``m.tag`` ordering is done with numbers, not strings
(`#1139 <https://github.com/matrix-org/matrix-doc/pull/1139>`_).
- Add the room visibility options for the room directory
Copy link
Member

Choose a reason for hiding this comment

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

this should be under "Changes to the API which will be backwards-compatible for clients".

Gets the visibility of a given room on the server's public room directory.
operationId: getRoomVisibilityOnDirectory
security:
- accessToken: []
Copy link
Member

Choose a reason for hiding this comment

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

synapse appears happy without an access token. I think this is deliberate?

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes sense to be deliberate - let's go with that.

@@ -23,6 +23,8 @@ Unreleased changes
(`#1137 <https://github.com/matrix-org/matrix-doc/pull/1137>`_).
- Clarify that ``m.tag`` ordering is done with numbers, not strings
(`#1139 <https://github.com/matrix-org/matrix-doc/pull/1139>`_).
- Add the room visibility options for the room directory
(`#1141 <https://github.com/matrix-org/matrix-doc/pull/1141`_).
Copy link
Member

Choose a reason for hiding this comment

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

this has lost its closing >

description: The room is not known to the server
examples:
application/json: {
"errcode": "M_UNKNOWN",
Copy link
Member

Choose a reason for hiding this comment

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

this probably ought to be M_NOT_FOUND, and we should treat the fact synapse returns M_UNKNOWN as a bug.

"error": "Room not found"
}
delete:
summary: Sets a room to be private on the room directory
Copy link
Member

Choose a reason for hiding this comment

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

how is this different from PUT private?

Copy link
Member Author

Choose a reason for hiding this comment

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

"synapse" is my only answer.

summary: Sets the visibility of a room in the room directory
description: |-
Sets the visibility of a given room in the server's public room
directory.
Copy link
Member

Choose a reason for hiding this comment

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

Could we clarify who is allowed to call this? Is it controlled by power_levels or a server admin flag? If the latter you could take inspiration from https://matrix.org/docs/spec/client_server/unstable.html#delete-matrix-client-r0-directory-room-roomalias for some wording

Copy link
Member Author

Choose a reason for hiding this comment

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

I took a look at what synapse does, and it seems to require one of the conditions to be met:

  • The user isn't spamming (as far as the anti-scam stuff is concerned)
  • The user is a server admin
  • The user is in the room and can change the m.room.aliases event (moderator)

Because of the above, inspiration has been taken from the alias delete route.

examples:
application/json: {
"visibility": "public"
}
Copy link
Member

Choose a reason for hiding this comment

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

indentation is weird here. this should line up with "application/json", and the content is over-indented. Likewise below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Both are indented according to a random sampling of other docs in the area.

I'm pretty sure if it were lined up then yaml will think it's an object, not a string.

Copy link
Member

Choose a reason for hiding this comment

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

It is an object. Please don't ask me what happens if you want to give an example which isn't application/json.

Copy link
Member

Choose a reason for hiding this comment

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

See 820704a for some background here. The reason for the odd indentation everywhere else is that they were turned from a string into an object with some hacky perl which didn't bother to realign the closing ".

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright. I don't have a way to verify the fix at the moment (matrix-doc barfs on windows and I'm not really able to justify the time in fighting it at the moment), but I'll make the change.

Signed-off-by: Travis Ralston <travpc@gmail.com>
* 404 for room not found instead of 400
* GET doesn't require an access token
* PUT (and therefore DELETE) can have it's own access control checks
* DELETE is implemented because of synapse

Signed-off-by: Travis Ralston <travpc@gmail.com>
summary: Sets a room to be private on the room directory
description: |-
Updates the visibility of a room to be private on the server's room
directory. This is the same as using the PUT operation with a private
Copy link
Member

Choose a reason for hiding this comment

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

ok, I'm in danger of channeling jzk, but I feel like this has no justification in existing. If we can remove the listing with a PUT, then everything should be using that, and we should treat failure to do so as a bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like Riot doesn't use it anyways. I've removed it - some sort of transition period should probably still be added to remove it from synapse.

Signed-off-by: Travis Ralston <travpc@gmail.com>
@richvdh
Copy link
Member

richvdh commented Mar 6, 2018

thanks!

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.

2 participants