-
Notifications
You must be signed in to change notification settings - Fork 490
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
7307 get user by token #7345
7307 get user by token #7345
Conversation
will clean this up in eventual release notes, just wanted to make sure I have something to pull from. :)
|
||
Alternatively you may pass a user's api ``token`` to retrieve the same output:: | ||
|
||
GET http://$SERVER/api/admin/authenticatedUsers/token/$token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm missing something, he admin api is blocked off (rightly so), so external tools would not be able to call this. Do we have other user APIs, if so this should go there. (if not, we should create something like "/api/users/"*)
- I've never been a fan of the plural, but we should be consistent with "datasets", "dataverses", etc.
Also, we should be getting the token from the header, like we do from other APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I assumed that since it was getting the same info it should have the same permissions required. I will change/fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think as it is, the only token you can get info about is the one you already have, and it really functions like a /api/users/me call. Does anyone need to use one apitoken to check other ones? If not, and if this call should allow a header, then perhaps it shouldn't also have a token path parameter (just the header or &key= query param) to avoid scanners.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I left that part out, but by getting the token from the header, it doesn't need to have a token path parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For documentation purposes, we already have a "Get All Notifications by User" endpoint and it probably makes sense to put the new docs about getting your user info near these older docs, or vice versa: https://guides.dataverse.org/en/5.1.1/api/native-api.html#get-all-notifications-by-user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, I moved the endpoint to the api/users path. I also moved the doc as suggested by Phil. that whole native api doc needs some reorganization work especially with respect to user related end points.
I just noticed these docs as well: https://guides.dataverse.org/en/5.1.1/api/native-api.html#users-token-management They're for actions also under "api/users" (for managing API tokens). @sekmiller should we put move these under the new "User Information" section that's being added in your pull request? Or do some doc clean up in some future pull request? @scolapasta and @qqmyers have all your questions been addressed? I just looked at the latest code and it seems to be in pretty good shape. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code seems fine and @sekmiller and I decided against further consolidating the /api/users docs at this time. Approved.
What this PR does / why we need it:
Adds an endpoint to the admin API to list a single user by passing that user's token. Required for the OpenDP MVP
Which issue(s) this PR closes:
Closes #7307
Special notes for your reviewer:
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: In addition to adding the doc to the List Single User section, I also made a slight edit to the adjacent "Create Authenticated User". It was missing a header and had a typo.