Add Functionality of OTP to support user 2fa

[madhephaestus]

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

[bitwiseman]

This is looks like a good start.

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

[madhephaestus]

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.

[madhephaestus]

I got the tests to pass on my end.

[madhephaestus]

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?

[bitwiseman]

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?

[madhephaestus]

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?

[PauloMigAlmeida]

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

https://github.com/github-api/github-api/blob/f28edbcf8fcb57a291aaa537bc57ea79c9bcda64/src/main/java/org/kohsuke/github/Requester.java#L761-L762

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]

@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]

@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]

@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]

@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]

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.

[madhephaestus]

@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

[madhephaestus]

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

[PauloMigAlmeida]

@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]

@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]

@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?

[bitwiseman]

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]

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

[madhephaestus]

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

[bitwiseman]

@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]

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

[bitwiseman]

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