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

GET /api/me permissions query parameter #1284

Open
davidbrochart opened this issue May 29, 2023 · 5 comments
Open

GET /api/me permissions query parameter #1284

davidbrochart opened this issue May 29, 2023 · 5 comments

Comments

@davidbrochart
Copy link
Contributor

Problem

The IdentityHandler expects GET /api/me to have a "permissions" query parameter whose value is a stringified JSON blob. From a user point of view, this looks like an opaque API, and the structure of the permissions parameter is not apparent. For instance in Jupyverse, the Swagger UI cannot show a nice input, it basically lets the user enter a string which is not guaranteed to be a valid JSON.

Proposed Solution

I'm not sure how/if the situation can be improved. Basically permissions has the following structure:

{
    "kernels": ["read", "write", "execute"],
    "contents": ["read", "write"]
}

Maybe the standard application/x-www-form-urlencoded format should be used? That would look like:

GET /api/me?kernels[]=read&kernels[]=write&kernels[]=execute&contents[]=read&contents[]=write
@minrk
Copy link
Contributor

minrk commented May 31, 2023

For instance in Jupyverse, the Swagger UI cannot show a nice input

I don't follow why it can't. This is an optional parameter, so it can be ignored. Is jupyverse forcing a usually-empty field to be non-empty?

the structure of the permissions parameter is not apparent

The structure of the permissions parameter is the structure of the permissions model in the response, and I think there's value in it matching exactly, which is how it is currently (I have a dict of lists, return me the same dict with items filtered to only those whose permissions I currently hold). JSON seems more sensible than form-encoding for a JSON API, but if it should be form-encoded, it should all be nested under permissions, not top-level kernels arguments, e.g. permissions.kernels=read&permissions.kernels=write&permissions.kernels=execute. That would be quite a bit more complex for both the request and the response than JSON, and result in far-longer URLs, so I'm not quite sure what the improvement or reason would be to make that change.

It seems the solution is to fill in the missing documentation for the optional permissions parameter.

@davidbrochart
Copy link
Contributor Author

By lack of structure, I mean that Swagger just shows a box where one has to manually enter some free-form text. I was hoping it could be a bit more "user-friendly", for instance by having a line for each resource where one could enter the permissions.
Also, it's almost impossible to write a URL by hand, since it's URL-encoded.
But I understand your argument about passing the exact same structure that is returned. It just feels weird to me that it's in a query parameter. Passing it as a JSON body parameter would feel more natural, but GET requests cannot have a body.

@minrk
Copy link
Contributor

minrk commented Jun 1, 2023

Passing it as a JSON body parameter would feel more natural, but GET requests cannot have a body.

I agree 100%, it would have been in the body if that were allowed. We could also split checking permissions into its own endpoint so it can be a POST. What do you think about that? POST to /api/me for checking permissions, rather than folding it into the identity model?

I don't think folks should be creating the URLs by hand. I'd try to omit it from openapi UI, if you can. But I know those tools don't usually allow for any kind of nuance.

@davidbrochart
Copy link
Contributor Author

We could also split checking permissions into its own endpoint so it can be a POST.

Yes I think it would be better, but isn't a POST required only when a state change occurs in the server?

@minrk
Copy link
Contributor

minrk commented Jun 1, 2023

but isn't a POST required only when a state change occurs in the server?

In a strict, RESTful resource sense, sure. But /api/me isn't really a REST resource so much as a general information endpoint. But you can use POST for whatever you want. For example, graphQL read-only search use POST. POST is often used when a request needs a body and url parameters are unwieldy (or size-limited), nothing more. I think I've seen several 'do this query' search APIs use POST. Arguably, POST is appropriate here because it's asking the server to 'do something' because checking individual permissions is an action (it's not existing readable state, the server has to go and check each one), not purely a read operation. But that's a detail. We can use POST if it suits our needs better than GET.

It's not perfect, and GET with a body would make the most sense, but HTTP is annoying that way.

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

No branches or pull requests

2 participants