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

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

Closed
ananace opened this issue Jun 24, 2018 · 19 comments
Closed

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

ananace opened this issue Jun 24, 2018 · 19 comments
Assignees
Labels
kind:maintenance MSC which clarifies/updates existing spec merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal

Comments

@ananace
Copy link
Contributor

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.

@turt2live turt2live added proposal-ready-for-review proposal A matrix spec change proposal labels Jul 10, 2018
@ara4n
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
Copy link
Member

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
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 turt2live added the client-server Client-Server API label Sep 6, 2018
@turt2live
Copy link
Member

@mscbot fcp merge

I think this is pretty well good to go.

@turt2live
Copy link
Member

@mscbot fcp merge

let's try this again

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

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Nov 5, 2018
@richvdh
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
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
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
Copy link
Member

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

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
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 uhoreg added final-comment-period and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Nov 26, 2018
@uhoreg
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
Copy link
Member

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

@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec finished-final-comment-period labels Dec 11, 2018
turt2live added a commit to matrix-org/synapse that referenced this issue Dec 18, 2018
@mscbot mscbot added finished-final-comment-period and removed final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Dec 23, 2018
@mscbot
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.

@richvdh richvdh removed the spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec label Dec 24, 2018
@turt2live turt2live added T-Core and removed T-Core labels Dec 24, 2018
richvdh pushed a commit to matrix-org/synapse that referenced this issue Jan 7, 2019
@richvdh richvdh added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed client-server Client-Server API disposition-merge finished-final-comment-period labels Jan 7, 2019
@turt2live turt2live self-assigned this Feb 9, 2019
turt2live added a commit that referenced this issue Feb 11, 2019
Original proposal: #1339

This contains no known differences to what was ultimately decided upon and implemented.
@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review merged A proposal whose PR has merged into the spec! and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Feb 11, 2019
@turt2live
Copy link
Member

Merged via #1873

@turt2live turt2live added the kind:maintenance MSC which clarifies/updates existing spec label Apr 21, 2020
RiotTranslateBot pushed a commit to RiotTranslateBot/matrix-doc that referenced this issue Aug 22, 2023
Stick a `definition-` on the front of the autogenerated anchors for definition
blocks.

This solves a problem where, for example,
https://spec.matrix.org/unstable/application-service-api/#registration could
refer to either the "Registration" section or the `Registration` definition
therein.

(These anchors are relatively recent: they were added in #1191.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:maintenance MSC which clarifies/updates existing spec merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
None yet
Development

No branches or pull requests

8 participants