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

Add Functionality of OTP to support user 2fa #603

Merged
merged 28 commits into from Nov 13, 2019

Conversation

@madhephaestus
Copy link
Contributor

madhephaestus commented Nov 9, 2019

close #602 by adding an API to request a token using an OTP.

close #602 by adding an API to request a token using an OTP.
@madhephaestus madhephaestus changed the title Add Functionality of OTP to github Add Functionality of OTP to support user 2fa to github Nov 9, 2019
@madhephaestus madhephaestus changed the title Add Functionality of OTP to support user 2fa to github Add Functionality of OTP to support user 2fa Nov 9, 2019
@madhephaestus

This comment has been minimized.

Copy link
Owner Author

madhephaestus commented on src/main/java/org/kohsuke/github/GitHub.java in 62bf691 Nov 9, 2019

This is the only change from the normal createToken above. It adds the header section defining an otp, and passes in the OTP from the user as data.

Copy link
Collaborator

bitwiseman left a comment

This is looks like a good start.

Please add a test that can be run in CI (see the CONTRIBUTING.md).

spelling

Co-Authored-By: Liam Newman <bitwiseman@gmail.com>
@madhephaestus

This comment has been minimized.

Copy link
Contributor Author

madhephaestus commented Nov 10, 2019

I have a specific question: How do i test an API call that requires access to the users SMS 2fa system? The test would need control of the SMS receive functionality for the account used in the CI system, and the CI account would also need 2fa enabled.

Test extends AbstractGitHubWireMockTest

Test requires 2fa and the user at the command line as the test is run.
@madhephaestus

This comment has been minimized.

Copy link
Contributor Author

madhephaestus commented Nov 10, 2019

I got the tests to pass on my end.

// as the exception is called, GitHub will generate and send an OTP to the users SMS
// we will prompt at the command line for the users 2fa code
try {
//token = gitHub.createTokenOtp(asList, string, "", twoFactorAuthCodePrompt());// prompt at command line for 2fa OTP code

This comment has been minimized.

Copy link
@madhephaestus

madhephaestus Nov 10, 2019

Author Contributor

I wrote this test but can't actually run it because the test framework does not have user interactions. How should i test something that is dependant on user interaction, server time sensitive date, then running the test?

This comment has been minimized.

Copy link
@bitwiseman

bitwiseman Nov 11, 2019

Collaborator

Ah, I see. Yeah, it's a little harder than I first thought.

Here's my suggestion:

  • Add checks that right information is in the GHAuthorization that came back from the server.
  • Add a thread sleep of 30 seconds after your call to createTokenOtp().
  • Add actions and checks that verify things are as they should as though the user responded to the SMS
  • Run the test with takeSnapshot with yourself as the user.
  • Respond to the SMS while the test is waiting.
  • Let the test finish successfully
  • Comment out the thread sleep with info explaining this is where a person would get an SMS message and respond
  • Add assumeFalse("Test only valid when not proxying", mockGitHub.isUseProxy()); to the top of your method.
  • Clean up the permissions your account and checkin all the files.

This will help people understand what this method is for and will ensure if someone goes to change it they have some idea of what needs to be done.

Cool?

This comment has been minimized.

Copy link
@madhephaestus

madhephaestus Nov 12, 2019

Author Contributor

ok, cool, so the takeSnapshot will record the user interaction. I would then bake in the OTP code during the capture that I get, into the unit test, to match the header data. That way it can be run in mock. Is that right?

@madhephaestus madhephaestus requested a review from bitwiseman Nov 10, 2019
@PauloMigAlmeida

This comment has been minimized.

Copy link
Contributor

PauloMigAlmeida commented Nov 11, 2019

Hi @madhephaestus, Thanks for the contribution 👍

Here are my 2 cents on what you could tweak.

[Re: Creation Token flow]
While the auth flow (when 2FA is in place) involves getting an HTTP status code 401, this library handles HTTP_UNAUTHORIZED status code by letting the IOException go up to the user-accessible API as defined here

if (responseCode == HttpURLConnection.HTTP_UNAUTHORIZED) // 401 / Unauthorized == bad creds
throw e;

I really appreciate it that you proposed a non-intrusive solution (kudos to you 😃 ) but having the regular method's utilisation depends on nested try-catch statements looks a bit unusual.
(@bitwiseman do you agree?)

GHAuthorization token=null;
try {
	token = gitHub.createToken(asList, string, "this is a test token created by a unit test");
}catch (IOException ex) {
	try {
		token = gitHub.createTokenOtp(asList, string, "", twoFactorAuthCodePrompt());
	} catch (Exception e) {
		e.printStackTrace();
	}
}

Another thing that developers can't infer with this approach is whether the exception has been thrown because GitHub sent a 401 (and the X-GitHub-OTP: required; <sms/app> header) or any other possible reason (Invalid credentials, network exception and so on).

#########

Just throwing ideas here, but what if we do the following?

We can make the Requester.handleApiError method a bit smarter so that when 401 is obtained we check for the existence of the X-GitHub-OTP header and handle it accordingly as defined here.

The implementation would be something akin to it (not tested):

PS.: We can take advantage of the existing GHIOException which extends IOException while carries the HTTP headers.

// Don't get caught up over the name of the exception, I'm really bad at choosing names :)
public class GHOTPRequiredException extends GHIOException {
// .....
}

Requester.handleApiError

if (responseCode == HttpURLConnection.HTTP_UNAUTHORIZED) // 401 Unauthorized == bad creds
    if(uc.getHeaderField("X-GitHub-OTP") != null)
        throw (IOException) new GHOTPRequiredException().withResponseHeaderFields(uc).initCause(e);
    else
        throw e; // usually org.kohsuke.github.HttpException (which extends IOException)

Once this is done, we can overload the createToken method so it can take a functional interface (that way users can integrate with any mechanism they want) while at the same time we don't break backwards compatibility. Something akin to it.

@FunctionalInterface
public interface OTPGenerator {
    String run();
}
// Original method (let's keep it that way so that we don't break BC)
public GHAuthorization createToken(Collection<String> scope, String note, String noteUrl) throws IOException {
    Requester requester = new Requester(this)
            .with("scopes", scope)
            .with("note", note)
            .with("note_url", noteUrl);

    return requester.method("POST").to("/authorizations", GHAuthorization.class).wrap(this);
}

//Overloaded method
public GHAuthorization createToken(Collection<String> scope, String note, String noteUrl, OTPGenerator otpGenerator) throws IOException{
    try{
        return this.createToken(scope, note, noteUrl);
    }catch (GHOTPRequiredException ex){
        // PS:: I'm not gonna refactor/extract the common lines of code of those 2 methods to make it easier to read on this PR
        // We can also have access to the HTTP headers using the ex.getResponseHeaderFields()

        Requester requester = new Requester(this)
                .with("scopes", scope)
                .with("note", note)
                .with("note_url", noteUrl);
        // Add the OTP from the user
        requester.setHeader("x-github-otp", otpGenerator.run());
        return requester.method("POST").to("/authorizations", GHAuthorization.class).wrap(this);
    }
}

That way, the utilisation from the user point-of-view would be like (using the example you created on the test class):

GHAuthorization token = gitHub.createToken(
        asList, 
        "TestCreateTokenOtp",
        "this is a test token created by a unit test", () -> {
            // can be anything from automated processes to user interaction.
            return whateverMethodThatGetsYouTheOTP(); 
        });
}

@madhephaestus @bitwiseman Let me know what you guys think about this approach and if you agree going forward with something along these lines. (considering that what I suggested is just a draft and many other improvements can be made on it)

also, if I'm missing something out, let me know too.

Happy coding guys 😃

@madhephaestus

This comment has been minimized.

Copy link
Contributor Author

madhephaestus commented Nov 11, 2019

@PauloMigAlmeida This! you hit the nail on the head, i was just a bit nervous to make too much of a change as my first PR with your project. I have a similar feature in my wrapping library (I add encrypted password/token storage and management of egit credential providers).

I would be happy to make the changes suggested above, i would basically take the snippets above and implement it in my PR. @bitwiseman Thoughts?

@bitwiseman

This comment has been minimized.

Copy link
Collaborator

bitwiseman commented Nov 11, 2019

@PauloMigAlmeida @madhephaestus
Yes to all of this as long as you add tests and lots of them.
This may involve adding entire new test classes in some cases because of the lack of testing up to now, but it sounds like even then this will be a win for you and a really nice addition to this library.

Question: You said SMS, so that has me a little concerned. So, there is user interaction somewhere in here? Where they library will be waiting for input right? I think seeing the tests will clarify this for me.

@madhephaestus

This comment has been minimized.

Copy link
Contributor Author

madhephaestus commented Nov 12, 2019

@bitwiseman , yes SMS as in the first token request prompts GitHub to send you a 6 digit code through a side channel, in this case SMS. To actually get the test to run to generate the moc data i think i need to make a simple swing GUI inside the test. I can't think of another way to get the test to accept the OTP code to make the test pass in the capture phase. Ill then have to delete all that and bake in the code I got to match the captured data, which should work in CI moving forward. I promise not to commit any swing cruft ;)

@bitwiseman

This comment has been minimized.

Copy link
Collaborator

bitwiseman commented Nov 12, 2019

@madhephaestus
Could run it in debug, set a break point, and set the OTP via debugger?
Or maybe run it on the commandline and enter it by hand?

Not questioning, just looking to make your life easier.

If you do create some tool for this, we should have it (and/or instructions) checked in somewhere so we can update/recreate the test later.

@madhephaestus

This comment has been minimized.

Copy link
Contributor Author

madhephaestus commented Nov 12, 2019

Could run it in debug, set a break point, and set the OTP via debugger?

This is promising, ill look into this....

Or maybe run it on the commandline and enter it by hand?

thats the code i have in there and commented out. The unit test never released the command line input back to the process so it ended up just hanging.

Not questioning, just looking to make your life easier.

I appreciate that :)

If you do create some tool for this, we should have it (and/or instructions) checked in somewhere so we can update/recreate the test later.

i guess i can leave the testing function in there and just not call it in the Mock case. UI can leave comment leave instructions for re-generating new mock data.

harrington added 5 commits Nov 12, 2019
@madhephaestus

This comment has been minimized.

Copy link
Contributor Author

madhephaestus commented Nov 12, 2019

@bitwiseman After much fiddeling i managed to take a snapshot of the OTP transaction and stor it along with the test. The trick of debugging the test and hand-inserting the OTP into the memory worked where every other method had not. Now I will dig into the changes suggested by @PauloMigAlmeida

When the OTP code is requested, then the special GHOTPRequiredException
is raised instead of the generic IOException. This differentiates
between an OTP request and a failed password.
@madhephaestus

This comment has been minimized.

Copy link
Contributor Author

madhephaestus commented Nov 12, 2019

@bitwiseman This PR is tested and ready for review now.

@PauloMigAlmeida

This comment has been minimized.

Copy link
Contributor

PauloMigAlmeida commented Nov 12, 2019

@madhephaestus Thanks 👍

what about the overloaded createToken method so that users don't need to forcefully use nested try-catchs to use it?

//Overloaded method
public GHAuthorization createToken(Collection<String> scope, String note, String noteUrl, OTPGenerator otpGenerator) throws IOException{

Did you give up on that idea for some reason? If so, share with us your thoughts 😃

@madhephaestus

This comment has been minimized.

Copy link
Contributor Author

madhephaestus commented Nov 12, 2019

@PauloMigAlmeida give up on, no, I was trying to make the minimal changes to the code base possible. I'm just enjoying the fact i was able to make the test data and get the unit test to pass. If there is a strong desire from the core devs to allow me to add that much more, then ill go ahead and set it entirely up as you suggested.

@madhephaestus

This comment has been minimized.

Copy link
Contributor Author

madhephaestus commented Nov 12, 2019

@bitwiseman @PauloMigAlmeida Ok! i have made the workflow match Paulo's suggested workflow using a lambda and hiding the try/catch in the overloaded function. Mock data with the new workflow was captured and committed, and the test tweaked to match the captured date (Baked in the OTP i used). I think this should be good to merge/release maybe?

Copy link
Collaborator

bitwiseman left a comment

This is great work! A few minor tweaks, I'd like to see. The JavaDoc/comments and test checks are required, other changes open to discussion.

@madhephaestus

This comment has been minimized.

Copy link
Contributor Author

madhephaestus commented Nov 13, 2019

@bitwiseman I think I have incorporated all of your notes :)

@bitwiseman bitwiseman merged commit d23c718 into github-api:master Nov 13, 2019
3 checks passed
3 checks passed
build (1.8.0)
Details
build (11.0.x)
Details
build (13.0.x)
Details
@madhephaestus

This comment has been minimized.

Copy link
Contributor Author

madhephaestus commented Nov 13, 2019

Awesome!! What release will this feature be availible in?

@bitwiseman

This comment has been minimized.

Copy link
Collaborator

bitwiseman commented Nov 14, 2019

@madhephaestus
1.100. I'd like to wait at least a few weeks before releasing, see if there is any fall out from the one year wait for 1.99.

@madhephaestus

This comment has been minimized.

Copy link
Contributor Author

madhephaestus commented Nov 19, 2019

any chance a pre-release or snapshot could be published so i can get my pipeline working and tested?

@bitwiseman

This comment has been minimized.

Copy link
Collaborator

bitwiseman commented Nov 20, 2019

I'll get to a release later this week. In the meanwhile, you could try https://jitpack.io/ .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.