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

Native API: set using PUT instead of toggle using POST #2219

Closed
bencomp opened this issue May 29, 2015 · 12 comments
Closed

Native API: set using PUT instead of toggle using POST #2219

bencomp opened this issue May 29, 2015 · 12 comments

Comments

@bencomp
Copy link
Contributor

bencomp commented May 29, 2015

In the Native API, a user can be made superuser by toggling her/his status using a POST command:

Toggles superuser mode on the AuthenticatedUser whose identifier is passed.

POST http://$SERVER/api/admin/superuser/$identifier

This is not like any other setting and requires to keep state outside the system.

Somewhat similarly, authentication providers are enabled/disabled by POSTing a status, instead of PUTting it:

Enable or disable an authentication provider (denoted by id):

POST http://$SERVER/api/admin/authenticationProviders/$id/:enabled

The body of the request should be either true or false. Content type has to be application/json, like so:

curl -H "Content-type: application/json" -X POST -d"false" http://localhost:8080/api/admin/authenticationProviders/echo-dignified/:enabled

These settings should, like all the others, be updated with PUT.

@pdurbin
Copy link
Member

pdurbin commented May 29, 2015

If we follow the same logic as #1329 I do believe @bencomp is right.

@pdurbin pdurbin added the Type: Suggestion an idea label May 29, 2015
@scolapasta scolapasta modified the milestones: Candidates for 4.0.3, Candidates for 4.0.2 Jun 1, 2015
@scolapasta scolapasta modified the milestones: Candidates for 4.0.2, In Review Jul 2, 2015
@scolapasta scolapasta removed this from the Not Assigned to a Release milestone Jan 28, 2016
@pdurbin pdurbin mentioned this issue Sep 28, 2016
@pdurbin
Copy link
Member

pdurbin commented Feb 2, 2017

Again, @bencomp is probably right but we can't follow up with him anymore since he's moved on. I'll close this issue but we can work on it if it comes up again. I will note that in #1612 there was a nice little debate about POST vs PUT as well. 😄

@bencomp
Copy link
Contributor Author

bencomp commented Feb 2, 2017

Well, I still believe this is a design flaw. (And I still see notifications from issues that I was involved with :))

@pdurbin
Copy link
Member

pdurbin commented Feb 3, 2017

Yeah, it probably is. Someday maybe we'll support both for backward compatibility, at least for a while. That's what I did recently for #2431. There's probably a way to turn off those notifications but I hope you enjoy hearing from us now and then. We miss you @bencomp! Hope you're well!

@tdilauro
Copy link
Contributor

tdilauro commented Feb 9, 2017

I think this should be re-opened. I have similar issues. There is not a deterministic way to set, for example, the superuser property on an authenticated user. You have to first determine the value before deciding whether or not to toggle it.

@pdurbin
Copy link
Member

pdurbin commented Feb 9, 2017

@tdilauro I'm happy to re-open this issue but and I see now that "set" is in the title. You want to be able to pass true or false so the operation is deterministic. Makes sense. I guess we'd keep the old toggle version for backward compatibility though. Do you have a strong opinion about PUT vs. POST for this issue? I don't.

@pdurbin pdurbin reopened this Feb 9, 2017
@tdilauro
Copy link
Contributor

tdilauro commented Feb 9, 2017

@pdurbin I think set is alright. You would be setting to either true or false.

The operation should be PUT based on RFC 2616 Section 9.6 (PUT) v. Section 9.5 (POST).

To make it RESTful, I would think the resource (URL) would be better structured as
http://$SERVER/api/user/{username}/superuser.

@michbarsinai
Copy link
Member

I agree with @tdilauro regarding the PUT vs GET. As for the proposed URL, it's technically correct, but managing superusers is very much an admin task, and so it has to go under /api/admin.
Here's another option: looking a at the superusers as a collection, managed using REST:

  • add a super user:POST http://$SERVER/api/admin/superusers/ with the username as entity (-d)
  • remove a user user: DELETE http://$SERVER/api/admin/superusers/$username
  • list superusers: GET http://$SERVER/api/admin/superusers/
  • test if someone is a superuser (optional): GET http://$SERVER/api/admin/superusers/$username (empty content, 404 if no, 200 if yes).

Also, note that this issue talks about a change in the API (that we have to support until at least 5.0) so it needs to be approved by project management before implementation.

@tdilauro
Copy link
Contributor

tdilauro commented Feb 9, 2017

@michbarsinai assume you meant PUT v. POST above.

@tdilauro
Copy link
Contributor

tdilauro commented Feb 9, 2017

@michbarsinai I don't see what you propose above as RESTful, as it is less resource-centric and more function-/implementation-based. For example, you mention that managing superusers is an admin task, so it needs to be under /api/admin (I am assuming that there is a security implementation detail bundled up in this).

To my mind, it is important to look at this from a resource perspective for a variety of reasons:

  • integrate into linked data/semantic web environments
  • support provenance, which is especially applicable for this issue of system users
  • hide implementation details, avoid architectural lock-in

It's relatively easy for me to think of superusers as a global group/collection (as you suggest above), though I would want URIs that look like more like resources and less like functions, e.g.:

  • /api/groups/superusers/
    The methods and responses that you propose above work for me; it's just the URI structure that I have a problem with.

But it's even easier for me to think of superuser (no 's') or, even better, isSuperuser as a property of a user (agent). There are a lot of different ways to handle the properties; I'm just trying to illustrate my point here.

  • add superuser priv: PUT /api/agents/$username/properties/superuser
  • drop superuser priv: DELETE /api/agents/$username/properties/superuser
  • list superusers: GET /api/agents?filter=(isSuperuser=true)&properties=id,email
    • or just: GET /api/agents?filter=(isSuperuser)&properties=id,email (since isSuperUser is boolean)
    • select matching agents and return representation of the indicated properties (or all properties, if none specified)
  • list all users could look a lot like the above, with just: GET /api/agents/
    • return all agents with representation of all properties
  • test if superuser: GET /api/agents/$username/properties/superuser (404 if no, 200 if yes).

@michbarsinai
Copy link
Member

IMHO it basically boils down to whether super users are a property of the users or of the system. Both views are fine, and are not really in contradiction (ideally we'd have both). However, the "property of the system" view is in line with the guideline of having all admin api endpoints under /api/admin/. This allows sysadmins to control them separately (we have API endpoint blocking in place for this).

@pdurbin
Copy link
Member

pdurbin commented Jun 29, 2017

@tdilauro of at " Toggle Superuser status on/off" in #3612 we're building a GUI that includes the ability to make someone a superuser. The latest screenshot is at #3614 (comment) . I hope this helps and I'm closing this issue but please open a fresh one if there's anything you need. Thanks!

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

6 participants