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

Proposal to add a GET method to read account data #1339

Open
ananace opened this Issue Jun 24, 2018 · 18 comments

Comments

Projects
None yet
8 participants
@ananace
Copy link

ananace commented Jun 24, 2018

The spec provides a PUT method for setting account data, but the only way to read the data back is from the sync queries.
This is very nice if you want to be able to get a stream of data between clients, but in the case of bots that just use the account data to store configuration / state between restarts - or use the HS as a key-value store - then it's more of a nuisance.

Adding GET methods that will directly retrieve the latest data in the given id would be helpful for those use cases;

GET /_matrix/client/r0/user/%40alice%3Aexample.com/account_data/org.example.custom.config
Content-Type: application/json

{
  "custom_account_data_key": "custom_config_value"
}
GET /_matrix/client/r0/user/%40alice%3Aexample.com/rooms/%21726s6s6q%3Aexample.com/account_data/org.example.custom.room.config
Content-Type: application/json

{
  "custom_account_data_key": "custom_account_data_value"
}

A Last-Modified header could be nice to have, but most definitely not a necessity for the feature.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Aug 3, 2018

I guess the only question here is whether we want account data to be { type, key } -> event or just type -> event as they are currently.

@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Aug 3, 2018

Could also do with a plain GET /account_data for the user and room - would return the whole of account data.

@turt2live turt2live added this to To do: proposals (prioritized) in August 2018 r0 Aug 14, 2018

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Aug 15, 2018

i hadn't remembered that we currently support PUTing based on type, and so the concept of a state key is already thrown away. So i vote we just go and do this; i think the only reason it doesn't exist today is because people didn't bother to implement the GET endpoint at the point of implementation as there wasn't a use for it at that point.

@turt2live turt2live moved this from To do: proposals (not overly prioritized) to To do: client-server (prioritized) in August 2018 r0 Aug 20, 2018

@turt2live turt2live removed this from To do: client-server (prioritized) in August 2018 r0 Aug 30, 2018

@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Nov 5, 2018

@mscbot fcp merge

I think this is pretty well good to go.

@turt2live turt2live added the T-Core label Nov 5, 2018

@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Nov 5, 2018

@mscbot fcp merge

let's try this again

@mscbot

This comment has been minimized.

Copy link
Collaborator

mscbot commented Nov 5, 2018

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Nov 13, 2018

@turt2live it's not entirely clear what is included in this proposal: are we doing plain GET /_matrix/client/r0/user/{userId}/rooms/{roomId}/account_data or not?

Also: I'm hesitant to scope-creep this, but since we're in the area, the user_id in the URI is unnecessary and inconsistent with the shape of the rest of the API. It seems obtuse to add new APIs with this brokenness. How about proposing a more sensible path for both GET and PUT, and we can deprecate the existing PUT API?

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Nov 13, 2018

the reason {userId} is in there is to let you set other users' account_data if you have permission. why is it unnecessary & inconsistent?

@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Nov 13, 2018

why would you have permission to do so? We don't do the same thing for other per-account data, such as pushrules.

@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Nov 13, 2018

it's not entirely clear what is included in this proposal: are we doing plain GET /_matrix/client/r0/user/{userId}/rooms/{roomId}/account_data or not?

I'd like to, but it's probably not as needed. This would be a good point to get feedback on.

(paraphrasing) user_id in the URL

I'd rather keep it and have another proposal for dropping it everywhere else rather than scope creep this one.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Nov 13, 2018

why would you have permission to do so?

if you are a server admin helping out a user.

We don't do the same thing for other per-account data, such as pushrules.

perhaps that's an oversight?

@KitsuneRal

This comment has been minimized.

Copy link
Contributor

KitsuneRal commented Nov 14, 2018

I'd rather we introduce some kind of Client Management sub-API for things like doing something on behalf of other users. And I'd really be happy if it were secured by some capabilities-style system, the current power levels system is not really flexible; meanwhile getting/setting data of other users has sweeping consequences, especially with regards to data privacy.

I think we should keep the user id in the URL for the sake of consistency, as @turt2live proposed. Further changes towards either enabling the same for other endpoints or dropping it for the account_data endpoints need a separate discussion.

@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Nov 14, 2018

We don't do the same thing for other per-account data, such as pushrules.
perhaps that's an oversight?

where does this end? Endpoints that let you send messages on behalf of other users? Perhaps we ought to have a userId in all of our API endpoints.

I'm not that attached to this argument either way, to be honest, but I do feel it's reasonable to at least question it before we further entrench the current position.

@uhoreg

This comment has been minimized.

Copy link
Member

uhoreg commented Nov 26, 2018

@mscbot seems to be sleeping on the job, so...

🔔 This is now entering its final comment period, as per the review above. 🔔

@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Dec 11, 2018

@mscbot is still sleeping...

The final comment period, with a disposition to merge, as per the review above, is now complete.

(leaving this open to track implementation/state, given this is a grandfathered MSC)

@mscbot

This comment has been minimized.

Copy link
Collaborator

mscbot commented Dec 18, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Dec 18, 2018

Sure, mscbot, whatever you want.

turt2live added a commit to matrix-org/synapse that referenced this issue Dec 18, 2018

turt2live added a commit to matrix-org/synapse that referenced this issue Dec 18, 2018

turt2live added a commit to matrix-org/synapse that referenced this issue Dec 18, 2018

@mscbot

This comment has been minimized.

Copy link
Collaborator

mscbot commented Dec 23, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@turt2live turt2live added T-Core and removed T-Core labels Dec 24, 2018

@anoadragon453 anoadragon453 removed the T-Core label Jan 4, 2019

richvdh added a commit to matrix-org/synapse that referenced this issue Jan 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment