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

Implement Additional Enterprise methods on User Administration Client #1108

Merged
merged 3 commits into from
Feb 22, 2016

Conversation

ryangribble
Copy link
Contributor

Status

🚀 Ready to merge

This is built on top of #1099 which is not yet merged.

Only the last 4 commits are actually relating to this PR. I will rebase this as soon as #1099 is merged.

Here's a diff link showing the consolidated changes for what will be in just this PR:
TattsGroup/octokit.net@8be8c41...enterprise-useradministration

Implement Additional Enterprise methods on User Administration Client

https://developer.github.com/enterprise/2.5/v3/users/administration/
Includes normal and reactive implementations plus unit and integration tests for both, including integration tests for the 4 previously implemented methods in #1068

Notes

Differences in the API docs between github.com and github enterprise are few and far between however one of the major ones I found was that on GitHub Enterprise the UserAdministration API has several additional methods over github.com - methods for Create/Rename/Delete user, Create/Delete Impersonation Tokens and List/Delete Public Keys
https://developer.github.com/v3/users/administration/
vs
https://developer.github.com/enterprise/2.5/v3/users/administration/

SuspendedAt property on User

I found that the SuspendedAt property is returned by the API (at least on GitHub Enterprise) and whether this value is null or not indicates whether a user is suspended. This seemed useful to know (and also was required in order to assert the Integration tests for the Suspend/Unsuspend methods) so Ive added this field to the User response object (I also checked whether it should be put on the base Account object, but Organization can't be suspended so User seems to be the correct place for it

ImpersonationToken method

Im not exactly sure how to actually use these! As a GHE admin through the site admin section on the webUI we can "impersonate" a user and login/interact as them, however through the API you have to specify scopes on this call, so Im not sure if this is the same thing or not. The calls as implemented do work, but Im not sure what to do with the ImperstonationToken once it's retrieved~
I also wasn't sure whether there should be a new Response type for the call or if the existing Authorization type is appropriate. The only field the response contains that Authorization doesn't have, is "token" however I believe that may be being deprecated anyway. For now I've just used Authorization as the return type.

As an aside, it looks to me like there is an issue with the Authorization object in that the field named Application is actually app in the API response and thus always null. Ill raise an issue for this tomorrow...

Naming Things (tm)

Given that this API now had a mix of methods acting on both Users, ImpersonationTokens and PublicKeys I originally wanted to name the methods CreateUser RenameUser DeleteUser. However given that the Suspend Unsuspend Promote Demote methods were already implemented with that naming (ie, no "User" suffix) and went live in the latest release, I decided for consistency to not include "User" in the Create Rename and Delete either.

Added CreateUserContext helper

Similar to the RepositoryContext and TeamContext helpers, I added a helper that allows the integration tests to create a user to run the tests, then clean it up afterwards.

Tests

The integration tests require a GitHub enterprise instance so wont run as part of Octokit builds currently. The CreateUserContext helper is working a treat, after running stacks of tests my test GHE server doesn't have any extraneious users hanging around 😀

I set a couple of tests to Skip at the moment due to #1106 which I plan to fix up in the near future and then unskip the 2 integration tests.

@ryangribble ryangribble force-pushed the enterprise-useradministration branch from 5466703 to 297b74f Compare February 15, 2016 04:36
@haacked
Copy link
Contributor

haacked commented Feb 17, 2016

This is built on top of #1108 which is not yet merged

Umm, this is 1108. 😛

@ryangribble
Copy link
Contributor Author

Lol, too many PR's! Meant to say #1099

@ryangribble ryangribble force-pushed the enterprise-useradministration branch from ea7fc21 to bd10379 Compare February 22, 2016 11:06
@ryangribble
Copy link
Contributor Author

Now #1099 is merged, this one has been rebased and squahsed, builds/tests passing...

ready for your attention @shiftkey @haacked

Thanks

using System.Threading.Tasks;

namespace Octokit
{
/// <summary>
/// A client for GitHub's User Administration API.
/// A client for GitHub's User Administration API (GitHub Enterprise)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this client is only used for Enterprise, we should consider adding a check to the constructor that ensures that the ApiConnection.Connection.BaseUrl is an Enterprise URL and if not, throw an exception that indicates that this client only works for GH:E. That would require that we create this instance lazily of course.

Perhaps an idea for a separate PR.

haacked added a commit that referenced this pull request Feb 22, 2016
Implement Additional Enterprise methods on User Administration Client
@haacked haacked merged commit 84539f7 into octokit:master Feb 22, 2016
@haacked
Copy link
Contributor

haacked commented Feb 22, 2016

@ryangribble ryangribble deleted the enterprise-useradministration branch August 6, 2016 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants