From 8a16b50a2c315d17b6a6ca1bc654f51563011c29 Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Thu, 25 Feb 2021 17:17:25 -0800
Subject: [PATCH 01/25] Add prototype code written by Fortinet
---
.../twofactorverify/twofactorverify.go | 82 ++++++++++++++++---
internal/gitlabnet/twofactorverify/client.go | 40 ++++++---
2 files changed, 99 insertions(+), 23 deletions(-)
diff --git a/internal/command/twofactorverify/twofactorverify.go b/internal/command/twofactorverify/twofactorverify.go
index b1c55088..02b5ac8c 100644
--- a/internal/command/twofactorverify/twofactorverify.go
+++ b/internal/command/twofactorverify/twofactorverify.go
@@ -17,12 +17,44 @@ type Command struct {
ReadWriter *readwriter.ReadWriter
}
+type Result struct {
+ Error error
+ Status string
+ Success bool
+}
+
func (c *Command) Execute(ctx context.Context) error {
- err := c.verifyOTP(ctx, c.getOTP())
- if err != nil {
- return err
- }
+ verify := make(chan Result)
+ pushauth := make(chan Result)
+
+ go func() {
+ status, success, err := c.verifyOTP(ctx, c.getOTP())
+ verify <- Result{Error: err, Status: status, Success: success}
+ }()
+ go func() {
+ status, success, err := c.pushAuth(ctx)
+ pushauth <- Result{Error: err, Status: status, Success: success}
+ }()
+
+L:
+ for {
+ select {
+ case res := <-verify:
+ if res.Error != nil {
+ return res.Error
+ }
+ fmt.Fprint(c.ReadWriter.Out, res.Status)
+ break L
+ case res := <-pushauth:
+ if res.Success {
+ fmt.Fprint(c.ReadWriter.Out, res.Status)
+ break L
+ } else {
+ // ignore reject from remote, need to wait for user input in this case
+ }
+ }
+ }
return nil
}
@@ -38,18 +70,46 @@ func (c *Command) getOTP() string {
return answer
}
-func (c *Command) verifyOTP(ctx context.Context, otp string) error {
+func (c *Command) pushAuth(ctx context.Context) (status string, success bool, err error) {
client, err := twofactorverify.NewClient(c.Config)
if err != nil {
- return err
+ return "", false, err
}
- err = client.VerifyOTP(ctx, c.Args, otp)
- if err == nil {
- fmt.Fprint(c.ReadWriter.Out, "\nOTP validation successful. Git operations are now allowed.\n")
+ reason := ""
+
+ success, reason, err = client.PushAuth(ctx, c.Args)
+ if success {
+ status = fmt.Sprintf("\nPush OTP validation successful. Git operations are now allowed.\n")
} else {
- fmt.Fprintf(c.ReadWriter.Out, "\nOTP validation failed.\n%v\n", err)
+ if err != nil {
+ status = fmt.Sprintf("\nPush OTP validation failed.\n%v\n", err)
+ } else {
+ status = fmt.Sprintf("\nPush OTP validation failed.\n%v\n", reason)
+ }
}
- return nil
+ return
+}
+
+func (c *Command) verifyOTP(ctx context.Context, otp string) (status string, success bool, err error) {
+ client, err := twofactorverify.NewClient(c.Config)
+ if err != nil {
+ return "", false, err
+ }
+
+ reason := ""
+
+ success, reason, err = client.VerifyOTP(ctx, c.Args, otp)
+ if success {
+ status = fmt.Sprintf("\nOTP validation successful. Git operations are now allowed.\n")
+ } else {
+ if err != nil {
+ status = fmt.Sprintf("\nOTP validation failed.\n%v\n", err)
+ } else {
+ status = fmt.Sprintf("\nOTP validation failed.\n%v\n", reason)
+ }
+ }
+
+ return
}
diff --git a/internal/gitlabnet/twofactorverify/client.go b/internal/gitlabnet/twofactorverify/client.go
index aab302be..cf22cf8f 100644
--- a/internal/gitlabnet/twofactorverify/client.go
+++ b/internal/gitlabnet/twofactorverify/client.go
@@ -2,7 +2,6 @@ package twofactorverify
import (
"context"
- "errors"
"fmt"
"net/http"
@@ -27,6 +26,7 @@ type RequestBody struct {
KeyId string `json:"key_id,omitempty"`
UserId int64 `json:"user_id,omitempty"`
OTPAttempt string `json:"otp_attempt"`
+ PushAuth bool `json:"push_auth"`
}
func NewClient(config *config.Config) (*Client, error) {
@@ -38,35 +38,51 @@ func NewClient(config *config.Config) (*Client, error) {
return &Client{config: config, client: client}, nil
}
-func (c *Client) VerifyOTP(ctx context.Context, args *commandargs.Shell, otp string) error {
- requestBody, err := c.getRequestBody(ctx, args, otp)
+func (c *Client) VerifyOTP(ctx context.Context, args *commandargs.Shell, otp string) (bool, string, error) {
+ requestBody, err := c.getRequestBody(ctx, args, otp, false)
if err != nil {
- return err
+ return false, "", err
}
response, err := c.client.Post(ctx, "/two_factor_otp_check", requestBody)
if err != nil {
- return err
+ return false, "", err
}
defer response.Body.Close()
return parse(response)
}
-func parse(hr *http.Response) error {
+func (c *Client) PushAuth(ctx context.Context, args *commandargs.Shell) (bool, string, error) {
+ // enable push auth in internal rest api
+ requestBody, err := c.getRequestBody(ctx, args, "", true)
+ if err != nil {
+ return false, "", err
+ }
+
+ response, err := c.client.Post(ctx, "/two_factor_otp_check", requestBody)
+ if err != nil {
+ return false, "", err
+ }
+ defer response.Body.Close()
+
+ return parse(response)
+}
+
+func parse(hr *http.Response) (bool, string, error) {
response := &Response{}
if err := gitlabnet.ParseJSON(hr, response); err != nil {
- return err
+ return false, "", err
}
if !response.Success {
- return errors.New(response.Message)
+ return false, response.Message, nil
}
- return nil
+ return true, response.Message, nil
}
-func (c *Client) getRequestBody(ctx context.Context, args *commandargs.Shell, otp string) (*RequestBody, error) {
+func (c *Client) getRequestBody(ctx context.Context, args *commandargs.Shell, otp string, pushauth bool) (*RequestBody, error) {
client, err := discover.NewClient(c.config)
if err != nil {
@@ -75,7 +91,7 @@ func (c *Client) getRequestBody(ctx context.Context, args *commandargs.Shell, ot
var requestBody *RequestBody
if args.GitlabKeyId != "" {
- requestBody = &RequestBody{KeyId: args.GitlabKeyId, OTPAttempt: otp}
+ requestBody = &RequestBody{KeyId: args.GitlabKeyId, OTPAttempt: otp, PushAuth: pushauth}
} else {
userInfo, err := client.GetByCommandArgs(ctx, args)
@@ -83,7 +99,7 @@ func (c *Client) getRequestBody(ctx context.Context, args *commandargs.Shell, ot
return nil, err
}
- requestBody = &RequestBody{UserId: userInfo.UserId, OTPAttempt: otp}
+ requestBody = &RequestBody{UserId: userInfo.UserId, OTPAttempt: otp, PushAuth: pushauth}
}
return requestBody, nil
--
GitLab
From 4636c165b2c76d0dce626f54d4257453e2f70e3e Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Fri, 26 Mar 2021 16:28:56 -0700
Subject: [PATCH 02/25] Fix incorrect return code
---
internal/command/twofactorverify/twofactorverify.go | 2 ++
1 file changed, 2 insertions(+)
diff --git a/internal/command/twofactorverify/twofactorverify.go b/internal/command/twofactorverify/twofactorverify.go
index 02b5ac8c..c6fd14a3 100644
--- a/internal/command/twofactorverify/twofactorverify.go
+++ b/internal/command/twofactorverify/twofactorverify.go
@@ -111,5 +111,7 @@ func (c *Command) verifyOTP(ctx context.Context, otp string) (status string, suc
}
}
+ err = nil
+
return
}
--
GitLab
From 6b6b6e0d9f491bded5b6e8f3caad5182fb3b2f9f Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Fri, 26 Mar 2021 16:29:12 -0700
Subject: [PATCH 03/25] Update tests to handle push notification implementation
---
.../twofactorverify/twofactorverify_test.go | 33 ++++++++++++++++---
.../gitlabnet/twofactorverify/client_test.go | 10 +++---
spec/gitlab_shell_two_factor_verify_spec.rb | 28 +++++++++++++++-
3 files changed, 61 insertions(+), 10 deletions(-)
diff --git a/internal/command/twofactorverify/twofactorverify_test.go b/internal/command/twofactorverify/twofactorverify_test.go
index 9d5f54d8..2ee02265 100644
--- a/internal/command/twofactorverify/twofactorverify_test.go
+++ b/internal/command/twofactorverify/twofactorverify_test.go
@@ -7,6 +7,7 @@ import (
"io/ioutil"
"net/http"
"testing"
+ "time"
"github.com/stretchr/testify/require"
@@ -30,14 +31,31 @@ func setup(t *testing.T) []testserver.TestRequestHandler {
var requestBody *twofactorverify.RequestBody
require.NoError(t, json.Unmarshal(b, &requestBody))
+ var body map[string]interface{}
switch requestBody.KeyId {
case "1":
- body := map[string]interface{}{
- "success": true,
+ if requestBody.PushAuth {
+ body = map[string]interface{}{
+ "success": false,
+ }
+ } else {
+ body = map[string]interface{}{
+ "success": true,
+ }
+ }
+ json.NewEncoder(w).Encode(body)
+ case "2":
+ if requestBody.PushAuth {
+ body = map[string]interface{}{
+ "success": true,
+ }
+ } else {
+ // Stall verifyOTP long enough for pushAuth to complete
+ time.Sleep(10 * time.Second)
}
json.NewEncoder(w).Encode(body)
case "error":
- body := map[string]interface{}{
+ body = map[string]interface{}{
"success": false,
"message": "error message",
}
@@ -69,6 +87,13 @@ func TestExecute(t *testing.T) {
answer string
expectedOutput string
}{
+ {
+ desc: "When push is provided",
+ arguments: &commandargs.Shell{GitlabKeyId: "2"},
+ answer: "",
+ expectedOutput: question +
+ "Push OTP validation successful. Git operations are now allowed.\n",
+ },
{
desc: "With a known key id",
arguments: &commandargs.Shell{GitlabKeyId: "1"},
@@ -105,8 +130,8 @@ func TestExecute(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
output := &bytes.Buffer{}
- input := bytes.NewBufferString(tc.answer)
+ input := bytes.NewBufferString(tc.answer)
cmd := &Command{
Config: &config.Config{GitlabUrl: url},
Args: tc.arguments,
diff --git a/internal/gitlabnet/twofactorverify/client_test.go b/internal/gitlabnet/twofactorverify/client_test.go
index 7bb037ed..22f59891 100644
--- a/internal/gitlabnet/twofactorverify/client_test.go
+++ b/internal/gitlabnet/twofactorverify/client_test.go
@@ -85,7 +85,7 @@ func TestVerifyOTPByKeyId(t *testing.T) {
defer cleanup()
args := &commandargs.Shell{GitlabKeyId: "0"}
- err := client.VerifyOTP(context.Background(), args, otpAttempt)
+ _, _, err := client.VerifyOTP(context.Background(), args, otpAttempt)
require.NoError(t, err)
}
@@ -94,7 +94,7 @@ func TestVerifyOTPByUsername(t *testing.T) {
defer cleanup()
args := &commandargs.Shell{GitlabUsername: "jane-doe"}
- err := client.VerifyOTP(context.Background(), args, otpAttempt)
+ _, _, err := client.VerifyOTP(context.Background(), args, otpAttempt)
require.NoError(t, err)
}
@@ -103,8 +103,8 @@ func TestErrorMessage(t *testing.T) {
defer cleanup()
args := &commandargs.Shell{GitlabKeyId: "1"}
- err := client.VerifyOTP(context.Background(), args, otpAttempt)
- require.Equal(t, "error message", err.Error())
+ _, reason, _ := client.VerifyOTP(context.Background(), args, otpAttempt)
+ require.Equal(t, "error message", reason)
}
func TestErrorResponses(t *testing.T) {
@@ -136,7 +136,7 @@ func TestErrorResponses(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
args := &commandargs.Shell{GitlabKeyId: tc.fakeId}
- err := client.VerifyOTP(context.Background(), args, otpAttempt)
+ _, _, err := client.VerifyOTP(context.Background(), args, otpAttempt)
require.EqualError(t, err, tc.expectedError)
})
diff --git a/spec/gitlab_shell_two_factor_verify_spec.rb b/spec/gitlab_shell_two_factor_verify_spec.rb
index 25d88698..244d7d85 100644
--- a/spec/gitlab_shell_two_factor_verify_spec.rb
+++ b/spec/gitlab_shell_two_factor_verify_spec.rb
@@ -24,7 +24,15 @@ describe 'bin/gitlab-shell 2fa_verify' do
key_id = params['key_id'] || params['user_id'].to_s
if key_id == '100'
- res.body = { success: true }.to_json
+ if params['push_auth']
+ res.body = { success: false }.to_json
+ else
+ res.body = { success: true }.to_json
+ end
+ elsif key_id == '102'
+ if params['push_auth']
+ res.body = { success: true }.to_json
+ end
else
res.body = { success: false, message: 'boom!' }.to_json
end
@@ -38,6 +46,14 @@ describe 'bin/gitlab-shell 2fa_verify' do
end
describe 'command' do
+ context 'when push is provided' do
+ let(:cmd) { "#{gitlab_shell_path} key-102" }
+
+ it 'prints a successful push verification message' do
+ verify_successful_verification_push!(cmd)
+ end
+ end
+
context 'when key is provided' do
let(:cmd) { "#{gitlab_shell_path} key-100" }
@@ -78,4 +94,14 @@ describe 'bin/gitlab-shell 2fa_verify' do
expect(stdout.flush.read).to eq("\nOTP validation successful. Git operations are now allowed.\n")
end
end
+
+ def verify_successful_verification_push!(cmd)
+ Open3.popen2(env, cmd) do |stdin, stdout|
+ expect(stdout.gets(5)).to eq('OTP: ')
+
+ stdin.puts('123456')
+
+ expect(stdout.flush.read).to eq("\nPush OTP validation successful. Git operations are now allowed.\n")
+ end
+ end
end
--
GitLab
From 3ec31c34ae9c3fbf1dc70361ba25474bf3a30a62 Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Thu, 8 Apr 2021 18:57:27 -0700
Subject: [PATCH 04/25] Fix test race condition
---
.../twofactorverify/twofactorverify.go | 23 +++++++++----------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/internal/command/twofactorverify/twofactorverify.go b/internal/command/twofactorverify/twofactorverify.go
index c6fd14a3..9f118955 100644
--- a/internal/command/twofactorverify/twofactorverify.go
+++ b/internal/command/twofactorverify/twofactorverify.go
@@ -13,6 +13,7 @@ import (
type Command struct {
Config *config.Config
+ Client *twofactorverify.Client
Args *commandargs.Shell
ReadWriter *readwriter.ReadWriter
}
@@ -24,6 +25,14 @@ type Result struct {
}
func (c *Command) Execute(ctx context.Context) error {
+ // config.GetHTTPClient isn't thread-safe so save Client in struct for concurrency
+ // workaround until #518 is fixed
+ var err error
+ c.Client, err = twofactorverify.NewClient(c.Config)
+ if err != nil {
+ return err
+ }
+
verify := make(chan Result)
pushauth := make(chan Result)
@@ -71,14 +80,9 @@ func (c *Command) getOTP() string {
}
func (c *Command) pushAuth(ctx context.Context) (status string, success bool, err error) {
- client, err := twofactorverify.NewClient(c.Config)
- if err != nil {
- return "", false, err
- }
-
reason := ""
- success, reason, err = client.PushAuth(ctx, c.Args)
+ success, reason, err = c.Client.PushAuth(ctx, c.Args)
if success {
status = fmt.Sprintf("\nPush OTP validation successful. Git operations are now allowed.\n")
} else {
@@ -93,14 +97,9 @@ func (c *Command) pushAuth(ctx context.Context) (status string, success bool, er
}
func (c *Command) verifyOTP(ctx context.Context, otp string) (status string, success bool, err error) {
- client, err := twofactorverify.NewClient(c.Config)
- if err != nil {
- return "", false, err
- }
-
reason := ""
- success, reason, err = client.VerifyOTP(ctx, c.Args, otp)
+ success, reason, err = c.Client.VerifyOTP(ctx, c.Args, otp)
if success {
status = fmt.Sprintf("\nOTP validation successful. Git operations are now allowed.\n")
} else {
--
GitLab
From 582a8bd0923a6b4fd3521786ed16e07390973921 Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Tue, 4 Jan 2022 12:20:56 -0800
Subject: [PATCH 05/25] Handle unbuffered channels per review comment
---
.../twofactorverify/twofactorverify.go | 73 ++++++++++---------
1 file changed, 37 insertions(+), 36 deletions(-)
diff --git a/internal/command/twofactorverify/twofactorverify.go b/internal/command/twofactorverify/twofactorverify.go
index 9f118955..78777450 100644
--- a/internal/command/twofactorverify/twofactorverify.go
+++ b/internal/command/twofactorverify/twofactorverify.go
@@ -4,6 +4,7 @@ import (
"context"
"fmt"
"io"
+ "time"
"gitlab.com/gitlab-org/gitlab-shell/internal/command/commandargs"
"gitlab.com/gitlab-org/gitlab-shell/internal/command/readwriter"
@@ -33,41 +34,41 @@ func (c *Command) Execute(ctx context.Context) error {
return err
}
- verify := make(chan Result)
- pushauth := make(chan Result)
+ // Create timeout context
+ // TODO: make timeout configurable
+ const ctxTimeout = 30
+ timeoutCtx, cancel := context.WithTimeout(ctx, ctxTimeout * time.Second)
+ defer cancel()
+ // Background push notification with timeout
+ pushauth := make(chan Result)
go func() {
- status, success, err := c.verifyOTP(ctx, c.getOTP())
- verify <- Result{Error: err, Status: status, Success: success}
+ defer close(pushauth)
+ status, success, err := c.pushAuth(timeoutCtx)
+ pushauth <- Result{Error: err, Status: status, Success: success}
}()
+ // Also allow manual OTP entry while waiting for push, with same timeout as push
+ verify := make(chan Result)
go func() {
- status, success, err := c.pushAuth(ctx)
- pushauth <- Result{Error: err, Status: status, Success: success}
+ defer close(verify)
+ status, success, err := c.verifyOTP(timeoutCtx, c.getOTP(timeoutCtx))
+ verify <- Result{Error: err, Status: status, Success: success}
}()
-L:
- for {
- select {
- case res := <-verify:
- if res.Error != nil {
- return res.Error
- }
- fmt.Fprint(c.ReadWriter.Out, res.Status)
- break L
- case res := <-pushauth:
- if res.Success {
- fmt.Fprint(c.ReadWriter.Out, res.Status)
- break L
- } else {
- // ignore reject from remote, need to wait for user input in this case
- }
- }
+ select {
+ case res := <-verify: // manual OTP
+ fmt.Fprint(c.ReadWriter.Out, res.Status)
+ case res := <-pushauth: // push
+ fmt.Fprint(c.ReadWriter.Out, res.Status)
+ case <-timeoutCtx.Done(): // push timed out
+ fmt.Fprint(c.ReadWriter.Out, "OTP verification timed out")
}
+
return nil
}
-func (c *Command) getOTP() string {
+func (c *Command) getOTP(ctx context.Context) string {
prompt := "OTP: "
fmt.Fprint(c.ReadWriter.Out, prompt)
@@ -79,38 +80,38 @@ func (c *Command) getOTP() string {
return answer
}
-func (c *Command) pushAuth(ctx context.Context) (status string, success bool, err error) {
+func (c *Command) verifyOTP(ctx context.Context, otp string) (status string, success bool, err error) {
reason := ""
- success, reason, err = c.Client.PushAuth(ctx, c.Args)
+ success, reason, err = c.Client.VerifyOTP(ctx, c.Args, otp)
if success {
- status = fmt.Sprintf("\nPush OTP validation successful. Git operations are now allowed.\n")
+ status = fmt.Sprintf("\nOTP validation successful. Git operations are now allowed.\n")
} else {
if err != nil {
- status = fmt.Sprintf("\nPush OTP validation failed.\n%v\n", err)
+ status = fmt.Sprintf("\nOTP validation failed.\n%v\n", err)
} else {
- status = fmt.Sprintf("\nPush OTP validation failed.\n%v\n", reason)
+ status = fmt.Sprintf("\nOTP validation failed.\n%v\n", reason)
}
}
+ err = nil
+
return
}
-func (c *Command) verifyOTP(ctx context.Context, otp string) (status string, success bool, err error) {
+func (c *Command) pushAuth(ctx context.Context) (status string, success bool, err error) {
reason := ""
- success, reason, err = c.Client.VerifyOTP(ctx, c.Args, otp)
+ success, reason, err = c.Client.PushAuth(ctx, c.Args)
if success {
- status = fmt.Sprintf("\nOTP validation successful. Git operations are now allowed.\n")
+ status = fmt.Sprintf("\nPush OTP validation successful. Git operations are now allowed.\n")
} else {
if err != nil {
- status = fmt.Sprintf("\nOTP validation failed.\n%v\n", err)
+ status = fmt.Sprintf("\nPush OTP validation failed.\n%v\n", err)
} else {
- status = fmt.Sprintf("\nOTP validation failed.\n%v\n", reason)
+ status = fmt.Sprintf("\nPush OTP validation failed.\n%v\n", reason)
}
}
- err = nil
-
return
}
--
GitLab
From 19822b942d6a7aebb34058ea16cbf537934d8989 Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Thu, 13 Jan 2022 12:43:53 -0800
Subject: [PATCH 06/25] Fix case formatting
---
internal/command/twofactorverify/twofactorverify.go | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/internal/command/twofactorverify/twofactorverify.go b/internal/command/twofactorverify/twofactorverify.go
index 78777450..389022d0 100644
--- a/internal/command/twofactorverify/twofactorverify.go
+++ b/internal/command/twofactorverify/twofactorverify.go
@@ -59,7 +59,7 @@ func (c *Command) Execute(ctx context.Context) error {
select {
case res := <-verify: // manual OTP
fmt.Fprint(c.ReadWriter.Out, res.Status)
- case res := <-pushauth: // push
+ case res := <-pushauth: // push
fmt.Fprint(c.ReadWriter.Out, res.Status)
case <-timeoutCtx.Done(): // push timed out
fmt.Fprint(c.ReadWriter.Out, "OTP verification timed out")
--
GitLab
From b38bbf3f57848588c03c20e97fda846831113282 Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Fri, 21 Jan 2022 17:31:19 -0800
Subject: [PATCH 07/25] Checkpoint: I do believe it's working
---
.../twofactorverify/twofactorverify.go | 61 +++++++++++++++----
1 file changed, 48 insertions(+), 13 deletions(-)
diff --git a/internal/command/twofactorverify/twofactorverify.go b/internal/command/twofactorverify/twofactorverify.go
index 389022d0..e2af137f 100644
--- a/internal/command/twofactorverify/twofactorverify.go
+++ b/internal/command/twofactorverify/twofactorverify.go
@@ -37,32 +37,67 @@ func (c *Command) Execute(ctx context.Context) error {
// Create timeout context
// TODO: make timeout configurable
const ctxTimeout = 30
- timeoutCtx, cancel := context.WithTimeout(ctx, ctxTimeout * time.Second)
- defer cancel()
+ timeoutCtx, cancelTimeout := context.WithTimeout(ctx, ctxTimeout * time.Second)
+ verifyCtx, cancelVerify := context.WithCancel(timeoutCtx)
+ pushCtx, cancelPush := context.WithCancel(timeoutCtx)
+ defer cancelTimeout()
// Background push notification with timeout
pushauth := make(chan Result)
go func() {
defer close(pushauth)
- status, success, err := c.pushAuth(timeoutCtx)
- pushauth <- Result{Error: err, Status: status, Success: success}
+ status, success, err := c.pushAuth(pushCtx)
+
+ select {
+ case <-pushCtx.Done(): // push cancelled by manual OTP
+ pushauth <- Result{Error: nil, Status: "cancelled", Success: false}
+ default:
+ pushauth <- Result{Error: err, Status: status, Success: success}
+ cancelVerify()
+ }
}()
// Also allow manual OTP entry while waiting for push, with same timeout as push
verify := make(chan Result)
go func() {
defer close(verify)
- status, success, err := c.verifyOTP(timeoutCtx, c.getOTP(timeoutCtx))
- verify <- Result{Error: err, Status: status, Success: success}
+ answer := ""
+ answer = c.getOTP(verifyCtx)
+
+ select {
+ case <-verifyCtx.Done(): // manual OTP cancelled by push
+ verify <- Result{Error: nil, Status: "cancelled", Success: false}
+ default:
+ cancelPush()
+ status, success, err := c.verifyOTP(verifyCtx, answer)
+ verify <- Result{Error: err, Status: status, Success: success}
+ }
}()
- select {
- case res := <-verify: // manual OTP
- fmt.Fprint(c.ReadWriter.Out, res.Status)
- case res := <-pushauth: // push
- fmt.Fprint(c.ReadWriter.Out, res.Status)
- case <-timeoutCtx.Done(): // push timed out
- fmt.Fprint(c.ReadWriter.Out, "OTP verification timed out")
+ for {
+ select {
+ case res := <-verify: // manual OTP
+ if res.Status == "cancelled" {
+ // verify cancelled; don't print anything
+ } else if res.Status == "" {
+ // channel closed; don't print anything
+ } else {
+ fmt.Fprint(c.ReadWriter.Out, res.Status)
+ return nil
+ }
+ case res := <-pushauth: // push
+ if res.Status == "cancelled" {
+ // push cancelled; don't print anything
+ } else if res.Status == "" {
+ // channel closed; don't print anything
+ } else {
+ fmt.Fprint(c.ReadWriter.Out, res.Status)
+ return nil
+ }
+ case <-timeoutCtx.Done(): // push timed out
+ fmt.Fprint(c.ReadWriter.Out, "\nOTP verification timed out\n")
+ return nil
+ }
}
return nil
--
GitLab
From f18d91511ab0107ac1060391f96f7a513ca7fb54 Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Fri, 21 Jan 2022 17:33:40 -0800
Subject: [PATCH 08/25] Passing pushauth flag as bool in json body to internal
api doesn't become a bool in ruby; converting to string fixes it
---
internal/gitlabnet/twofactorverify/client.go | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/internal/gitlabnet/twofactorverify/client.go b/internal/gitlabnet/twofactorverify/client.go
index cf22cf8f..b62396c7 100644
--- a/internal/gitlabnet/twofactorverify/client.go
+++ b/internal/gitlabnet/twofactorverify/client.go
@@ -4,6 +4,7 @@ import (
"context"
"fmt"
"net/http"
+ "strconv"
"gitlab.com/gitlab-org/gitlab-shell/client"
"gitlab.com/gitlab-org/gitlab-shell/internal/command/commandargs"
@@ -26,7 +27,7 @@ type RequestBody struct {
KeyId string `json:"key_id,omitempty"`
UserId int64 `json:"user_id,omitempty"`
OTPAttempt string `json:"otp_attempt"`
- PushAuth bool `json:"push_auth"`
+ PushAuth string `json:"push_auth"`
}
func NewClient(config *config.Config) (*Client, error) {
@@ -91,7 +92,7 @@ func (c *Client) getRequestBody(ctx context.Context, args *commandargs.Shell, ot
var requestBody *RequestBody
if args.GitlabKeyId != "" {
- requestBody = &RequestBody{KeyId: args.GitlabKeyId, OTPAttempt: otp, PushAuth: pushauth}
+ requestBody = &RequestBody{KeyId: args.GitlabKeyId, OTPAttempt: otp, PushAuth: strconv.FormatBool(pushauth)}
} else {
userInfo, err := client.GetByCommandArgs(ctx, args)
@@ -99,7 +100,7 @@ func (c *Client) getRequestBody(ctx context.Context, args *commandargs.Shell, ot
return nil, err
}
- requestBody = &RequestBody{UserId: userInfo.UserId, OTPAttempt: otp, PushAuth: pushauth}
+ requestBody = &RequestBody{UserId: userInfo.UserId, OTPAttempt: otp, PushAuth: strconv.FormatBool(pushauth)}
}
return requestBody, nil
--
GitLab
From 054bf6513f9559b07bb70448c496734e61f39d85 Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Wed, 26 Jan 2022 13:05:00 -0800
Subject: [PATCH 09/25] Fix ruby test script to match updates to code
---
spec/gitlab_shell_two_factor_verify_spec.rb | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/spec/gitlab_shell_two_factor_verify_spec.rb b/spec/gitlab_shell_two_factor_verify_spec.rb
index 244d7d85..a90df1d5 100644
--- a/spec/gitlab_shell_two_factor_verify_spec.rb
+++ b/spec/gitlab_shell_two_factor_verify_spec.rb
@@ -24,13 +24,13 @@ describe 'bin/gitlab-shell 2fa_verify' do
key_id = params['key_id'] || params['user_id'].to_s
if key_id == '100'
- if params['push_auth']
+ if params['push_auth'] == "true"
res.body = { success: false }.to_json
else
res.body = { success: true }.to_json
end
elsif key_id == '102'
- if params['push_auth']
+ if params['push_auth'] == "true"
res.body = { success: true }.to_json
end
else
@@ -99,8 +99,6 @@ describe 'bin/gitlab-shell 2fa_verify' do
Open3.popen2(env, cmd) do |stdin, stdout|
expect(stdout.gets(5)).to eq('OTP: ')
- stdin.puts('123456')
-
expect(stdout.flush.read).to eq("\nPush OTP validation successful. Git operations are now allowed.\n")
end
end
--
GitLab
From 9d514d478f6faeadf8e6c4e7162dd1fe37f19b4d Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Thu, 27 Jan 2022 16:57:44 -0800
Subject: [PATCH 10/25] Rework tests after code mods
---
.../twofactorverify/twofactorverify_test.go | 43 ++++++-------------
1 file changed, 14 insertions(+), 29 deletions(-)
diff --git a/internal/command/twofactorverify/twofactorverify_test.go b/internal/command/twofactorverify/twofactorverify_test.go
index 2ee02265..3177fc56 100644
--- a/internal/command/twofactorverify/twofactorverify_test.go
+++ b/internal/command/twofactorverify/twofactorverify_test.go
@@ -7,7 +7,6 @@ import (
"io/ioutil"
"net/http"
"testing"
- "time"
"github.com/stretchr/testify/require"
@@ -34,24 +33,8 @@ func setup(t *testing.T) []testserver.TestRequestHandler {
var body map[string]interface{}
switch requestBody.KeyId {
case "1":
- if requestBody.PushAuth {
- body = map[string]interface{}{
- "success": false,
- }
- } else {
- body = map[string]interface{}{
- "success": true,
- }
- }
- json.NewEncoder(w).Encode(body)
- case "2":
- if requestBody.PushAuth {
- body = map[string]interface{}{
- "success": true,
- }
- } else {
- // Stall verifyOTP long enough for pushAuth to complete
- time.Sleep(10 * time.Second)
+ body = map[string]interface{}{
+ "success": true,
}
json.NewEncoder(w).Encode(body)
case "error":
@@ -88,18 +71,16 @@ func TestExecute(t *testing.T) {
expectedOutput string
}{
{
- desc: "When push is provided",
- arguments: &commandargs.Shell{GitlabKeyId: "2"},
- answer: "",
- expectedOutput: question +
- "Push OTP validation successful. Git operations are now allowed.\n",
+ desc: "When push is provided",
+ arguments: &commandargs.Shell{GitlabKeyId: "1"},
+ answer: "",
+ expectedOutput: question + "Push OTP validation successful. Git operations are now allowed.\n",
},
{
- desc: "With a known key id",
- arguments: &commandargs.Shell{GitlabKeyId: "1"},
- answer: "123456\n",
- expectedOutput: question +
- "OTP validation successful. Git operations are now allowed.\n",
+ desc: "With a known key id",
+ arguments: &commandargs.Shell{GitlabKeyId: "1"},
+ answer: "123456\n",
+ expectedOutput: question + "OTP validation successful. Git operations are now allowed.\n",
},
{
desc: "With bad response",
@@ -132,6 +113,10 @@ func TestExecute(t *testing.T) {
output := &bytes.Buffer{}
input := bytes.NewBufferString(tc.answer)
+ if tc.answer == "" { // pushauth test must not provide input otherwise it gets treated as a manual OTP
+ input.Reset()
+ }
+
cmd := &Command{
Config: &config.Config{GitlabUrl: url},
Args: tc.arguments,
--
GitLab
From 2e2d3176bf91046ed296bf5ffbcad59a0bb0f80a Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Wed, 9 Feb 2022 11:52:55 -0800
Subject: [PATCH 11/25] Remove requirement to pass push_auth flag via internal
API body. Also split internal OTP validate API into two separate calls: one
for manual OTP, the other for push
---
internal/gitlabnet/twofactorverify/client.go | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/internal/gitlabnet/twofactorverify/client.go b/internal/gitlabnet/twofactorverify/client.go
index b62396c7..c9c13c8c 100644
--- a/internal/gitlabnet/twofactorverify/client.go
+++ b/internal/gitlabnet/twofactorverify/client.go
@@ -4,7 +4,6 @@ import (
"context"
"fmt"
"net/http"
- "strconv"
"gitlab.com/gitlab-org/gitlab-shell/client"
"gitlab.com/gitlab-org/gitlab-shell/internal/command/commandargs"
@@ -27,7 +26,6 @@ type RequestBody struct {
KeyId string `json:"key_id,omitempty"`
UserId int64 `json:"user_id,omitempty"`
OTPAttempt string `json:"otp_attempt"`
- PushAuth string `json:"push_auth"`
}
func NewClient(config *config.Config) (*Client, error) {
@@ -40,12 +38,12 @@ func NewClient(config *config.Config) (*Client, error) {
}
func (c *Client) VerifyOTP(ctx context.Context, args *commandargs.Shell, otp string) (bool, string, error) {
- requestBody, err := c.getRequestBody(ctx, args, otp, false)
+ requestBody, err := c.getRequestBody(ctx, args, otp)
if err != nil {
return false, "", err
}
- response, err := c.client.Post(ctx, "/two_factor_otp_check", requestBody)
+ response, err := c.client.Post(ctx, "/two_factor_manual_otp_check", requestBody)
if err != nil {
return false, "", err
}
@@ -56,12 +54,12 @@ func (c *Client) VerifyOTP(ctx context.Context, args *commandargs.Shell, otp str
func (c *Client) PushAuth(ctx context.Context, args *commandargs.Shell) (bool, string, error) {
// enable push auth in internal rest api
- requestBody, err := c.getRequestBody(ctx, args, "", true)
+ requestBody, err := c.getRequestBody(ctx, args, "")
if err != nil {
return false, "", err
}
- response, err := c.client.Post(ctx, "/two_factor_otp_check", requestBody)
+ response, err := c.client.Post(ctx, "/two_factor_push_otp_check", requestBody)
if err != nil {
return false, "", err
}
@@ -83,7 +81,7 @@ func parse(hr *http.Response) (bool, string, error) {
return true, response.Message, nil
}
-func (c *Client) getRequestBody(ctx context.Context, args *commandargs.Shell, otp string, pushauth bool) (*RequestBody, error) {
+func (c *Client) getRequestBody(ctx context.Context, args *commandargs.Shell, otp string) (*RequestBody, error) {
client, err := discover.NewClient(c.config)
if err != nil {
@@ -92,7 +90,7 @@ func (c *Client) getRequestBody(ctx context.Context, args *commandargs.Shell, ot
var requestBody *RequestBody
if args.GitlabKeyId != "" {
- requestBody = &RequestBody{KeyId: args.GitlabKeyId, OTPAttempt: otp, PushAuth: strconv.FormatBool(pushauth)}
+ requestBody = &RequestBody{KeyId: args.GitlabKeyId, OTPAttempt: otp}
} else {
userInfo, err := client.GetByCommandArgs(ctx, args)
@@ -100,7 +98,7 @@ func (c *Client) getRequestBody(ctx context.Context, args *commandargs.Shell, ot
return nil, err
}
- requestBody = &RequestBody{UserId: userInfo.UserId, OTPAttempt: otp, PushAuth: strconv.FormatBool(pushauth)}
+ requestBody = &RequestBody{UserId: userInfo.UserId, OTPAttempt: otp}
}
return requestBody, nil
--
GitLab
From d109af093cac2adc78937ef078c1532073153161 Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Thu, 7 Apr 2022 10:23:13 -0700
Subject: [PATCH 12/25] Fix format errors
---
internal/command/twofactorverify/twofactorverify.go | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/internal/command/twofactorverify/twofactorverify.go b/internal/command/twofactorverify/twofactorverify.go
index ecda5c06..e12a1bd2 100644
--- a/internal/command/twofactorverify/twofactorverify.go
+++ b/internal/command/twofactorverify/twofactorverify.go
@@ -40,10 +40,10 @@ func (c *Command) Execute(ctx context.Context) error {
return err
}
- // Create timeout context
+ // Create timeout context
// TODO: make timeout configurable
const ctxTimeout = 30
- timeoutCtx, cancelTimeout := context.WithTimeout(ctx, ctxTimeout * time.Second)
+ timeoutCtx, cancelTimeout := context.WithTimeout(ctx, ctxTimeout*time.Second)
verifyCtx, cancelVerify := context.WithCancel(timeoutCtx)
pushCtx, cancelPush := context.WithCancel(timeoutCtx)
defer cancelTimeout()
--
GitLab
From 7ab82c000c5989a4a08015c21df6f3a6ad57afdc Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Tue, 12 Apr 2022 12:31:37 -0700
Subject: [PATCH 13/25] Split rspec into separate services for push and manual
---
...ab_shell_two_factor_manual_verify_spec.rb} | 30 +-------
...itlab_shell_two_factor_push_verify_spec.rb | 71 +++++++++++++++++++
2 files changed, 74 insertions(+), 27 deletions(-)
rename spec/{gitlab_shell_two_factor_verify_spec.rb => gitlab_shell_two_factor_manual_verify_spec.rb} (68%)
create mode 100644 spec/gitlab_shell_two_factor_push_verify_spec.rb
diff --git a/spec/gitlab_shell_two_factor_verify_spec.rb b/spec/gitlab_shell_two_factor_manual_verify_spec.rb
similarity index 68%
rename from spec/gitlab_shell_two_factor_verify_spec.rb
rename to spec/gitlab_shell_two_factor_manual_verify_spec.rb
index a90df1d5..5e5a06c5 100644
--- a/spec/gitlab_shell_two_factor_verify_spec.rb
+++ b/spec/gitlab_shell_two_factor_manual_verify_spec.rb
@@ -3,7 +3,7 @@ require_relative 'spec_helper'
require 'open3'
require 'json'
-describe 'bin/gitlab-shell 2fa_verify' do
+describe 'bin/gitlab-shell 2fa_verify manual' do
include_context 'gitlab shell'
let(:env) do
@@ -16,7 +16,7 @@ describe 'bin/gitlab-shell 2fa_verify' do
end
def mock_server(server)
- server.mount_proc('/api/v4/internal/two_factor_otp_check') do |req, res|
+ server.mount_proc('/api/v4/internal/two_factor_manual_otp_check') do |req, res|
res.content_type = 'application/json'
res.status = 200
@@ -24,15 +24,7 @@ describe 'bin/gitlab-shell 2fa_verify' do
key_id = params['key_id'] || params['user_id'].to_s
if key_id == '100'
- if params['push_auth'] == "true"
- res.body = { success: false }.to_json
- else
- res.body = { success: true }.to_json
- end
- elsif key_id == '102'
- if params['push_auth'] == "true"
- res.body = { success: true }.to_json
- end
+ res.body = { success: true }.to_json
else
res.body = { success: false, message: 'boom!' }.to_json
end
@@ -46,14 +38,6 @@ describe 'bin/gitlab-shell 2fa_verify' do
end
describe 'command' do
- context 'when push is provided' do
- let(:cmd) { "#{gitlab_shell_path} key-102" }
-
- it 'prints a successful push verification message' do
- verify_successful_verification_push!(cmd)
- end
- end
-
context 'when key is provided' do
let(:cmd) { "#{gitlab_shell_path} key-100" }
@@ -94,12 +78,4 @@ describe 'bin/gitlab-shell 2fa_verify' do
expect(stdout.flush.read).to eq("\nOTP validation successful. Git operations are now allowed.\n")
end
end
-
- def verify_successful_verification_push!(cmd)
- Open3.popen2(env, cmd) do |stdin, stdout|
- expect(stdout.gets(5)).to eq('OTP: ')
-
- expect(stdout.flush.read).to eq("\nPush OTP validation successful. Git operations are now allowed.\n")
- end
- end
end
diff --git a/spec/gitlab_shell_two_factor_push_verify_spec.rb b/spec/gitlab_shell_two_factor_push_verify_spec.rb
new file mode 100644
index 00000000..e8705805
--- /dev/null
+++ b/spec/gitlab_shell_two_factor_push_verify_spec.rb
@@ -0,0 +1,71 @@
+require_relative 'spec_helper'
+
+require 'open3'
+require 'json'
+
+describe 'bin/gitlab-shell 2fa_verify push' do
+ include_context 'gitlab shell'
+
+ let(:env) do
+ { 'SSH_CONNECTION' => 'fake',
+ 'SSH_ORIGINAL_COMMAND' => '2fa_verify' }
+ end
+
+ before(:context) do
+ write_config('gitlab_url' => "http+unix://#{CGI.escape(tmp_socket_path)}")
+ end
+
+ def mock_server(server)
+ server.mount_proc('/api/v4/internal/two_factor_push_otp_check') do |req, res|
+ res.content_type = 'application/json'
+ res.status = 200
+
+ params = JSON.parse(req.body)
+ key_id = params['key_id'] || params['user_id'].to_s
+
+ if key_id == '100'
+ res.body = { success: false }.to_json
+ elsif key_id == '102'
+ res.body = { success: true }.to_json
+ else
+ res.body = { success: false, message: 'boom!' }.to_json
+ end
+ end
+
+ server.mount_proc('/api/v4/internal/discover') do |_, res|
+ res.status = 200
+ res.content_type = 'application/json'
+ res.body = { id: 100, name: 'Some User', username: 'someuser' }.to_json
+ end
+ end
+
+ describe 'command' do
+ context 'when push is provided' do
+ let(:cmd) { "#{gitlab_shell_path} key-102" }
+
+ it 'prints a successful push verification message' do
+ verify_successful_verification_push!(cmd)
+ end
+ end
+
+ context 'when API error occurs' do
+ let(:cmd) { "#{gitlab_shell_path} key-101" }
+
+ it 'prints the error message' do
+ Open3.popen2(env, cmd) do |stdin, stdout|
+ expect(stdout.gets(5)).to eq('OTP: ')
+
+ expect(stdout.flush.read).to eq("\nPush OTP validation failed.\nboom!\n")
+ end
+ end
+ end
+ end
+
+ def verify_successful_verification_push!(cmd)
+ Open3.popen2(env, cmd) do |stdin, stdout|
+ expect(stdout.gets(5)).to eq('OTP: ')
+
+ expect(stdout.flush.read).to eq("\nPush OTP validation successful. Git operations are now allowed.\n")
+ end
+ end
+end
--
GitLab
From e6c46528958276558379fc2e8c4cccc6c9d5c6b5 Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Tue, 12 Apr 2022 17:09:37 -0700
Subject: [PATCH 14/25] Split out into manual/push
---
..._test.go => twofactorverifymanual_test.go} | 31 ++---
.../twofactorverifypush_test.go | 124 ++++++++++++++++++
2 files changed, 135 insertions(+), 20 deletions(-)
rename internal/command/twofactorverify/{twofactorverify_test.go => twofactorverifymanual_test.go} (72%)
create mode 100644 internal/command/twofactorverify/twofactorverifypush_test.go
diff --git a/internal/command/twofactorverify/twofactorverify_test.go b/internal/command/twofactorverify/twofactorverifymanual_test.go
similarity index 72%
rename from internal/command/twofactorverify/twofactorverify_test.go
rename to internal/command/twofactorverify/twofactorverifymanual_test.go
index d091e3a5..d8de486d 100644
--- a/internal/command/twofactorverify/twofactorverify_test.go
+++ b/internal/command/twofactorverify/twofactorverifymanual_test.go
@@ -17,10 +17,10 @@ import (
"gitlab.com/gitlab-org/gitlab-shell/internal/gitlabnet/twofactorverify"
)
-func setup(t *testing.T) []testserver.TestRequestHandler {
+func setupManual(t *testing.T) []testserver.TestRequestHandler {
requests := []testserver.TestRequestHandler{
{
- Path: "/api/v4/internal/two_factor_otp_check",
+ Path: "/api/v4/internal/two_factor_manual_otp_check",
Handler: func(w http.ResponseWriter, r *http.Request) {
b, err := io.ReadAll(r.Body)
defer r.Body.Close()
@@ -54,12 +54,12 @@ func setup(t *testing.T) []testserver.TestRequestHandler {
}
const (
- question = "OTP: \n"
- errorHeader = "OTP validation failed.\n"
+ manualQuestion = "OTP: \n"
+ manualErrorHeader = "OTP validation failed.\n"
)
-func TestExecute(t *testing.T) {
- requests := setup(t)
+func TestExecuteManual(t *testing.T) {
+ requests := setupManual(t)
url := testserver.StartSocketHttpServer(t, requests)
@@ -69,41 +69,35 @@ func TestExecute(t *testing.T) {
answer string
expectedOutput string
}{
- {
- desc: "When push is provided",
- arguments: &commandargs.Shell{GitlabKeyId: "1"},
- answer: "",
- expectedOutput: question + "Push OTP validation successful. Git operations are now allowed.\n",
- },
{
desc: "With a known key id",
arguments: &commandargs.Shell{GitlabKeyId: "1"},
answer: "123456\n",
- expectedOutput: question + "OTP validation successful. Git operations are now allowed.\n",
+ expectedOutput: manualQuestion + "OTP validation successful. Git operations are now allowed.\n",
},
{
desc: "With bad response",
arguments: &commandargs.Shell{GitlabKeyId: "-1"},
answer: "123456\n",
- expectedOutput: question + errorHeader + "Parsing failed\n",
+ expectedOutput: manualQuestion + manualErrorHeader + "Parsing failed\n",
},
{
desc: "With API returns an error",
arguments: &commandargs.Shell{GitlabKeyId: "error"},
answer: "yes\n",
- expectedOutput: question + errorHeader + "error message\n",
+ expectedOutput: manualQuestion + manualErrorHeader + "error message\n",
},
{
desc: "With API fails",
arguments: &commandargs.Shell{GitlabKeyId: "broken"},
answer: "yes\n",
- expectedOutput: question + errorHeader + "Internal API error (500)\n",
+ expectedOutput: manualQuestion + manualErrorHeader + "Internal API error (500)\n",
},
{
desc: "With missing arguments",
arguments: &commandargs.Shell{},
answer: "yes\n",
- expectedOutput: question + errorHeader + "who='' is invalid\n",
+ expectedOutput: "\nPush " + manualErrorHeader + "who='' is invalid\n" +"OTP: ",
},
}
@@ -112,9 +106,6 @@ func TestExecute(t *testing.T) {
output := &bytes.Buffer{}
input := bytes.NewBufferString(tc.answer)
- if tc.answer == "" { // pushauth test must not provide input otherwise it gets treated as a manual OTP
- input.Reset()
- }
cmd := &Command{
Config: &config.Config{GitlabUrl: url},
diff --git a/internal/command/twofactorverify/twofactorverifypush_test.go b/internal/command/twofactorverify/twofactorverifypush_test.go
new file mode 100644
index 00000000..798ff1ee
--- /dev/null
+++ b/internal/command/twofactorverify/twofactorverifypush_test.go
@@ -0,0 +1,124 @@
+package twofactorverify
+
+import (
+ "bytes"
+ "context"
+ "encoding/json"
+ "io"
+ "net/http"
+ "testing"
+
+ "github.com/stretchr/testify/require"
+
+ "gitlab.com/gitlab-org/gitlab-shell/client/testserver"
+ "gitlab.com/gitlab-org/gitlab-shell/internal/command/commandargs"
+ "gitlab.com/gitlab-org/gitlab-shell/internal/command/readwriter"
+ "gitlab.com/gitlab-org/gitlab-shell/internal/config"
+ "gitlab.com/gitlab-org/gitlab-shell/internal/gitlabnet/twofactorverify"
+)
+
+func setupPush(t *testing.T) []testserver.TestRequestHandler {
+ requests := []testserver.TestRequestHandler{
+ {
+ Path: "/api/v4/internal/two_factor_push_otp_check",
+ Handler: func(w http.ResponseWriter, r *http.Request) {
+ b, err := io.ReadAll(r.Body)
+ defer r.Body.Close()
+
+ require.NoError(t, err)
+
+ var requestBody *twofactorverify.RequestBody
+ require.NoError(t, json.Unmarshal(b, &requestBody))
+
+ var body map[string]interface{}
+ switch requestBody.KeyId {
+ case "1":
+ body = map[string]interface{}{
+ "success": true,
+ }
+ json.NewEncoder(w).Encode(body)
+ case "error":
+ body = map[string]interface{}{
+ "success": false,
+ "message": "error message",
+ }
+ require.NoError(t, json.NewEncoder(w).Encode(body))
+ case "broken":
+ w.WriteHeader(http.StatusInternalServerError)
+ }
+ },
+ },
+ }
+
+ return requests
+}
+
+const (
+ pushQuestion = "OTP: \n"
+ pushErrorHeader = "Push OTP validation failed.\n"
+)
+
+func TestExecutePush(t *testing.T) {
+ requests := setupPush(t)
+
+ url := testserver.StartSocketHttpServer(t, requests)
+
+ testCases := []struct {
+ desc string
+ arguments *commandargs.Shell
+ answer string
+ expectedOutput string
+ }{
+ {
+ desc: "When push is provided",
+ arguments: &commandargs.Shell{GitlabKeyId: "1"},
+ answer: "",
+ expectedOutput: pushQuestion + "Push OTP validation successful. Git operations are now allowed.\n",
+ },
+ {
+ desc: "With bad response",
+ arguments: &commandargs.Shell{GitlabKeyId: "-1"},
+ answer: "",
+ expectedOutput: pushQuestion + pushErrorHeader + "Parsing failed\n",
+ },
+ {
+ desc: "With API returns an error",
+ arguments: &commandargs.Shell{GitlabKeyId: "error"},
+ answer: "",
+ expectedOutput: pushQuestion + pushErrorHeader + "error message\n",
+ },
+ {
+ desc: "With API fails",
+ arguments: &commandargs.Shell{GitlabKeyId: "broken"},
+ answer: "",
+ expectedOutput: pushQuestion + pushErrorHeader + "Internal API error (500)\n",
+ },
+ {
+ desc: "With missing arguments",
+ arguments: &commandargs.Shell{},
+ answer: "",
+ expectedOutput: pushQuestion + pushErrorHeader + "who='' is invalid",
+ },
+ }
+
+ for _, tc := range testCases {
+ t.Run(tc.desc, func(t *testing.T) {
+ output := &bytes.Buffer{}
+
+ var input io.Reader
+ // make input wait for push auth tests
+ input, _ = io.Pipe()
+
+ cmd := &Command{
+ Config: &config.Config{GitlabUrl: url},
+ Args: tc.arguments,
+ ReadWriter: &readwriter.ReadWriter{Out: output, In: input},
+ }
+
+ err := cmd.Execute(context.Background())
+
+ require.NoError(t, err)
+ require.Equal(t, tc.expectedOutput, output.String())
+ })
+ }
+}
--
GitLab
From a77b2926669d02c429d6c56735fb8fd24bf0463a Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Tue, 12 Apr 2022 17:37:37 -0700
Subject: [PATCH 15/25] Split client tests into manual/push
---
.../{client_test.go => clientmanual_test.go} | 26 ++--
.../twofactorverify/clientpush_test.go | 139 ++++++++++++++++++
2 files changed, 152 insertions(+), 13 deletions(-)
rename internal/gitlabnet/twofactorverify/{client_test.go => clientmanual_test.go} (82%)
create mode 100644 internal/gitlabnet/twofactorverify/clientpush_test.go
diff --git a/internal/gitlabnet/twofactorverify/client_test.go b/internal/gitlabnet/twofactorverify/clientmanual_test.go
similarity index 82%
rename from internal/gitlabnet/twofactorverify/client_test.go
rename to internal/gitlabnet/twofactorverify/clientmanual_test.go
index cfaf5e51..0ea4c5ed 100644
--- a/internal/gitlabnet/twofactorverify/client_test.go
+++ b/internal/gitlabnet/twofactorverify/clientmanual_test.go
@@ -16,10 +16,10 @@ import (
"gitlab.com/gitlab-org/gitlab-shell/internal/config"
)
-func initialize(t *testing.T) []testserver.TestRequestHandler {
+func initializeManual(t *testing.T) []testserver.TestRequestHandler {
requests := []testserver.TestRequestHandler{
{
- Path: "/api/v4/internal/two_factor_otp_check",
+ Path: "/api/v4/internal/two_factor_manual_otp_check",
Handler: func(w http.ResponseWriter, r *http.Request) {
b, err := io.ReadAll(r.Body)
defer r.Body.Close()
@@ -78,35 +78,35 @@ func initialize(t *testing.T) []testserver.TestRequestHandler {
}
const (
- otpAttempt = "123456"
+ manualOtpAttempt = "123456"
)
func TestVerifyOTPByKeyId(t *testing.T) {
- client := setup(t)
+ client := setupManual(t)
args := &commandargs.Shell{GitlabKeyId: "0"}
- _, _, err := client.VerifyOTP(context.Background(), args, otpAttempt)
+ _, _, err := client.VerifyOTP(context.Background(), args, manualOtpAttempt)
require.NoError(t, err)
}
func TestVerifyOTPByUsername(t *testing.T) {
- client := setup(t)
+ client := setupManual(t)
args := &commandargs.Shell{GitlabUsername: "jane-doe"}
- _, _, err := client.VerifyOTP(context.Background(), args, otpAttempt)
+ _, _, err := client.VerifyOTP(context.Background(), args, manualOtpAttempt)
require.NoError(t, err)
}
func TestErrorMessage(t *testing.T) {
- client := setup(t)
+ client := setupManual(t)
args := &commandargs.Shell{GitlabKeyId: "1"}
- _, reason, _ := client.VerifyOTP(context.Background(), args, otpAttempt)
+ _, reason, _ := client.VerifyOTP(context.Background(), args, manualOtpAttempt)
require.Equal(t, "error message", reason)
}
func TestErrorResponses(t *testing.T) {
- client := setup(t)
+ client := setupManual(t)
testCases := []struct {
desc string
@@ -133,15 +133,15 @@ func TestErrorResponses(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
args := &commandargs.Shell{GitlabKeyId: tc.fakeId}
- _, _, err := client.VerifyOTP(context.Background(), args, otpAttempt)
+ _, _, err := client.VerifyOTP(context.Background(), args, manualOtpAttempt)
require.EqualError(t, err, tc.expectedError)
})
}
}
-func setup(t *testing.T) *Client {
- requests := initialize(t)
+func setupManual(t *testing.T) *Client {
+ requests := initializeManual(t)
url := testserver.StartSocketHttpServer(t, requests)
client, err := NewClient(&config.Config{GitlabUrl: url})
diff --git a/internal/gitlabnet/twofactorverify/clientpush_test.go b/internal/gitlabnet/twofactorverify/clientpush_test.go
new file mode 100644
index 00000000..35cef606
--- /dev/null
+++ b/internal/gitlabnet/twofactorverify/clientpush_test.go
@@ -0,0 +1,139 @@
+package twofactorverify
+
+import (
+ "context"
+ "encoding/json"
+ "io"
+ "net/http"
+ "testing"
+
+ "gitlab.com/gitlab-org/gitlab-shell/internal/gitlabnet/discover"
+
+ "github.com/stretchr/testify/require"
+ "gitlab.com/gitlab-org/gitlab-shell/client"
+ "gitlab.com/gitlab-org/gitlab-shell/client/testserver"
+ "gitlab.com/gitlab-org/gitlab-shell/internal/command/commandargs"
+ "gitlab.com/gitlab-org/gitlab-shell/internal/config"
+)
+
+func initializePush(t *testing.T) []testserver.TestRequestHandler {
+ requests := []testserver.TestRequestHandler{
+ {
+ Path: "/api/v4/internal/two_factor_push_otp_check",
+ Handler: func(w http.ResponseWriter, r *http.Request) {
+ b, err := io.ReadAll(r.Body)
+ defer r.Body.Close()
+
+ require.NoError(t, err)
+
+ var requestBody *RequestBody
+ require.NoError(t, json.Unmarshal(b, &requestBody))
+
+ switch requestBody.KeyId {
+ case "0":
+ body := map[string]interface{}{
+ "success": true,
+ }
+ require.NoError(t, json.NewEncoder(w).Encode(body))
+ case "1":
+ body := map[string]interface{}{
+ "success": false,
+ "message": "error message",
+ }
+ require.NoError(t, json.NewEncoder(w).Encode(body))
+ case "2":
+ w.WriteHeader(http.StatusForbidden)
+ body := &client.ErrorResponse{
+ Message: "Not allowed!",
+ }
+ require.NoError(t, json.NewEncoder(w).Encode(body))
+ case "3":
+ w.Write([]byte("{ \"message\": \"broken json!\""))
+ case "4":
+ w.WriteHeader(http.StatusForbidden)
+ }
+
+ if requestBody.UserId == 1 {
+ body := map[string]interface{}{
+ "success": true,
+ }
+ require.NoError(t, json.NewEncoder(w).Encode(body))
+ }
+ },
+ },
+ {
+ Path: "/api/v4/internal/discover",
+ Handler: func(w http.ResponseWriter, r *http.Request) {
+ body := &discover.Response{
+ UserId: 1,
+ Username: "jane-doe",
+ Name: "Jane Doe",
+ }
+ require.NoError(t, json.NewEncoder(w).Encode(body))
+ },
+ },
+ }
+
+ return requests
+}
+
+func TestVerifyPush(t *testing.T) {
+ client := setupPush(t)
+
+ args := &commandargs.Shell{GitlabKeyId: "0"}
+ _, _, err := client.PushAuth(context.Background(), args)
+ require.NoError(t, err)
+}
+
+func TestErrorMessagePush(t *testing.T) {
+ client := setupPush(t)
+
+ args := &commandargs.Shell{GitlabKeyId: "1"}
+ _, reason, _ := client.PushAuth(context.Background(), args)
+ require.Equal(t, "error message", reason)
+}
+
+func TestErrorResponsesPush(t *testing.T) {
+ client := setupPush(t)
+
+ testCases := []struct {
+ desc string
+ fakeId string
+ expectedError string
+ }{
+ {
+ desc: "A response with an error message",
+ fakeId: "2",
+ expectedError: "Not allowed!",
+ },
+ {
+ desc: "A response with bad JSON",
+ fakeId: "3",
+ expectedError: "Parsing failed",
+ },
+ {
+ desc: "An error response without message",
+ fakeId: "4",
+ expectedError: "Internal API error (403)",
+ },
+ }
+
+ for _, tc := range testCases {
+ t.Run(tc.desc, func(t *testing.T) {
+ args := &commandargs.Shell{GitlabKeyId: tc.fakeId}
+ _, _, err := client.PushAuth(context.Background(), args)
+
+ require.EqualError(t, err, tc.expectedError)
+ })
+ }
+}
+
+func setupPush(t *testing.T) *Client {
+ requests := initializePush(t)
+ url := testserver.StartSocketHttpServer(t, requests)
+
+ client, err := NewClient(&config.Config{GitlabUrl: url})
+ require.NoError(t, err)
+
+ return client
+}
--
GitLab
From 3b62b1d361ec7cb4d9e78140c7cfd5339a8769f2 Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Tue, 12 Apr 2022 17:38:14 -0700
Subject: [PATCH 16/25] Fix test formatting and inconsistency
---
.../command/twofactorverify/twofactorverifymanual_test.go | 2 +-
.../command/twofactorverify/twofactorverifypush_test.go | 6 ------
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/internal/command/twofactorverify/twofactorverifymanual_test.go b/internal/command/twofactorverify/twofactorverifymanual_test.go
index d8de486d..ea092c8b 100644
--- a/internal/command/twofactorverify/twofactorverifymanual_test.go
+++ b/internal/command/twofactorverify/twofactorverifymanual_test.go
@@ -97,7 +97,7 @@ func TestExecuteManual(t *testing.T) {
desc: "With missing arguments",
arguments: &commandargs.Shell{},
answer: "yes\n",
- expectedOutput: "\nPush " + manualErrorHeader + "who='' is invalid\n" +"OTP: ",
+ expectedOutput: "\nPush " + manualErrorHeader + "who='' is invalid\n" + "OTP: ",
},
}
diff --git a/internal/command/twofactorverify/twofactorverifypush_test.go b/internal/command/twofactorverify/twofactorverifypush_test.go
index 798ff1ee..3e76eec5 100644
--- a/internal/command/twofactorverify/twofactorverifypush_test.go
+++ b/internal/command/twofactorverify/twofactorverifypush_test.go
@@ -93,12 +93,6 @@ func TestExecutePush(t *testing.T) {
answer: "",
expectedOutput: pushQuestion + pushErrorHeader + "Internal API error (500)\n",
},
- {
- desc: "With missing arguments",
- arguments: &commandargs.Shell{},
- answer: "",
- expectedOutput: pushQuestion + pushErrorHeader + "who='' is invalid",
- },
}
for _, tc := range testCases {
--
GitLab
From 59c44668ad42d2896eda10e392baf08940937ea5 Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Thu, 14 Apr 2022 11:04:24 -0700
Subject: [PATCH 17/25] Add mock test server handler for push internal api
---
.../twofactorverifymanual_test.go | 37 ++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/internal/command/twofactorverify/twofactorverifymanual_test.go b/internal/command/twofactorverify/twofactorverifymanual_test.go
index ea092c8b..930ac1df 100644
--- a/internal/command/twofactorverify/twofactorverifymanual_test.go
+++ b/internal/command/twofactorverify/twofactorverifymanual_test.go
@@ -48,6 +48,41 @@ func setupManual(t *testing.T) []testserver.TestRequestHandler {
}
},
},
+ {
+ Path: "/api/v4/internal/two_factor_push_otp_check",
+ Handler: func(w http.ResponseWriter, r *http.Request) {
+ b, err := io.ReadAll(r.Body)
+ defer r.Body.Close()
+
+ require.NoError(t, err)
+
+ var requestBody *twofactorverify.RequestBody
+ require.NoError(t, json.Unmarshal(b, &requestBody))
+
+ var body map[string]interface{}
+ switch requestBody.KeyId {
+ case "1":
+ body = map[string]interface{}{
+ "success": true,
+ }
+ json.NewEncoder(w).Encode(body)
+ case "error":
+ body = map[string]interface{}{
+ "success": false,
+ "message": "error message",
+ }
+ require.NoError(t, json.NewEncoder(w).Encode(body))
+ case "broken":
+ w.WriteHeader(http.StatusInternalServerError)
+ default:
+ body = map[string]interface{}{
+ "success": true,
+ "message": "default message",
+ }
+ json.NewEncoder(w).Encode(body)
+ }
+ },
+ },
}
return requests
@@ -97,7 +132,7 @@ func TestExecuteManual(t *testing.T) {
desc: "With missing arguments",
arguments: &commandargs.Shell{},
answer: "yes\n",
- expectedOutput: "\nPush " + manualErrorHeader + "who='' is invalid\n" + "OTP: ",
+ expectedOutput: manualQuestion + manualErrorHeader + "who='' is invalid\n",
},
}
--
GitLab
From bbb1fb56972f4cf51688f662d7f796ad5acf8e64 Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Thu, 14 Apr 2022 16:52:53 -0700
Subject: [PATCH 18/25] Update test cases
---
.../command/twofactorverify/twofactorverifymanual_test.go | 6 ------
.../command/twofactorverify/twofactorverifypush_test.go | 6 ------
2 files changed, 12 deletions(-)
diff --git a/internal/command/twofactorverify/twofactorverifymanual_test.go b/internal/command/twofactorverify/twofactorverifymanual_test.go
index 930ac1df..c914e778 100644
--- a/internal/command/twofactorverify/twofactorverifymanual_test.go
+++ b/internal/command/twofactorverify/twofactorverifymanual_test.go
@@ -128,12 +128,6 @@ func TestExecuteManual(t *testing.T) {
answer: "yes\n",
expectedOutput: manualQuestion + manualErrorHeader + "Internal API error (500)\n",
},
- {
- desc: "With missing arguments",
- arguments: &commandargs.Shell{},
- answer: "yes\n",
- expectedOutput: manualQuestion + manualErrorHeader + "who='' is invalid\n",
- },
}
for _, tc := range testCases {
diff --git a/internal/command/twofactorverify/twofactorverifypush_test.go b/internal/command/twofactorverify/twofactorverifypush_test.go
index 3e76eec5..7bd5b61e 100644
--- a/internal/command/twofactorverify/twofactorverifypush_test.go
+++ b/internal/command/twofactorverify/twofactorverifypush_test.go
@@ -75,12 +75,6 @@ func TestExecutePush(t *testing.T) {
answer: "",
expectedOutput: pushQuestion + "Push OTP validation successful. Git operations are now allowed.\n",
},
- {
- desc: "With bad response",
- arguments: &commandargs.Shell{GitlabKeyId: "-1"},
- answer: "",
- expectedOutput: pushQuestion + pushErrorHeader + "Parsing failed\n",
- },
{
desc: "With API returns an error",
arguments: &commandargs.Shell{GitlabKeyId: "error"},
--
GitLab
From aac413ea1e1a7074b2c52431b67df215e2c929ba Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Tue, 12 Jul 2022 17:11:24 -0700
Subject: [PATCH 19/25] Remove redundant contexts
---
.../command/twofactorverify/twofactorverify.go | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/internal/command/twofactorverify/twofactorverify.go b/internal/command/twofactorverify/twofactorverify.go
index e12a1bd2..c9c0462d 100644
--- a/internal/command/twofactorverify/twofactorverify.go
+++ b/internal/command/twofactorverify/twofactorverify.go
@@ -44,22 +44,20 @@ func (c *Command) Execute(ctx context.Context) error {
// TODO: make timeout configurable
const ctxTimeout = 30
timeoutCtx, cancelTimeout := context.WithTimeout(ctx, ctxTimeout*time.Second)
- verifyCtx, cancelVerify := context.WithCancel(timeoutCtx)
- pushCtx, cancelPush := context.WithCancel(timeoutCtx)
defer cancelTimeout()
// Background push notification with timeout
pushauth := make(chan Result)
go func() {
defer close(pushauth)
- status, success, err := c.pushAuth(pushCtx)
+ status, success, err := c.pushAuth(timeoutCtx)
select {
- case <-pushCtx.Done(): // push cancelled by manual OTP
+ case <-timeoutCtx.Done(): // push cancelled by manual OTP
pushauth <- Result{Error: nil, Status: "cancelled", Success: false}
default:
pushauth <- Result{Error: err, Status: status, Success: success}
- cancelVerify()
+ cancelTimeout()
}
}()
@@ -69,15 +67,15 @@ func (c *Command) Execute(ctx context.Context) error {
defer close(verify)
ctxlog.Info("twofactorverify: execute: waiting for user input")
answer := ""
- answer = c.getOTP(verifyCtx)
+ answer = c.getOTP(timeoutCtx)
select {
- case <-verifyCtx.Done(): // manual OTP cancelled by push
+ case <-timeoutCtx.Done(): // manual OTP cancelled by push
verify <- Result{Error: nil, Status: "cancelled", Success: false}
default:
- cancelPush()
+ cancelTimeout()
ctxlog.Info("twofactorverify: execute: verifying entered OTP")
- status, success, err := c.verifyOTP(verifyCtx, answer)
+ status, success, err := c.verifyOTP(timeoutCtx, answer)
ctxlog.WithError(err).Info("twofactorverify: execute: OTP verified")
verify <- Result{Error: err, Status: status, Success: success}
}
--
GitLab
From 9b2246d8daa9f8b70dd5e8ed92179c6593b8708c Mon Sep 17 00:00:00 2001
From: kmcknight <kmcknight@gitlab.com>
Date: Tue, 12 Jul 2022 18:03:09 -0700
Subject: [PATCH 20/25] Remove redundant channel
---
.../twofactorverify/twofactorverify.go | 30 +++++++------------
1 file changed, 11 insertions(+), 19 deletions(-)
diff --git a/internal/command/twofactorverify/twofactorverify.go b/internal/command/twofactorverify/twofactorverify.go
index c9c0462d..bc7123b9 100644
--- a/internal/command/twofactorverify/twofactorverify.go
+++ b/internal/command/twofactorverify/twofactorverify.go
@@ -46,55 +46,47 @@ func (c *Command) Execute(ctx context.Context) error {
timeoutCtx, cancelTimeout := context.WithTimeout(ctx, ctxTimeout*time.Second)
defer cancelTimeout()
+ // Create result channel
+ resultC := make(chan Result)
+
// Background push notification with timeout
- pushauth := make(chan Result)
go func() {
- defer close(pushauth)
+ defer close(resultC)
status, success, err := c.pushAuth(timeoutCtx)
select {
case <-timeoutCtx.Done(): // push cancelled by manual OTP
- pushauth <- Result{Error: nil, Status: "cancelled", Success: false}
+ resultC <- Result{Error: nil, Status: "cancelled", Success: false}
default:
- pushauth <- Result{Error: err, Status: status, Success: success}
+ resultC <- Result{Error: err, Status: status, Success: success}
cancelTimeout()
}
}()
// Also allow manual OTP entry while waiting for push, with same timeout as push
- verify := make(chan Result)
go func() {
- defer close(verify)
+ defer close(resultC)
ctxlog.Info("twofactorverify: execute: waiting for user input")
answer := ""
answer = c.getOTP(timeoutCtx)
select {
case <-timeoutCtx.Done(): // manual OTP cancelled by push
- verify <- Result{Error: nil, Status: "cancelled", Success: false}
+ resultC <- Result{Error: nil, Status: "cancelled", Success: false}
default:
cancelTimeout()
ctxlog.Info("twofactorverify: execute: verifying entered OTP")
status, success, err := c.verifyOTP(timeoutCtx, answer)
ctxlog.WithError(err).Info("twofactorverify: execute: OTP verified")
- verify <- Result{Error: err, Status: status, Success: success}
+ resultC <- Result{Error: err, Status: status, Success: success}
}
}()
for {
select {
- case res := <-verify: // manual OTP
- if res.Status == "cancelled" {
- // verify cancelled; don't print anything
- } else if res.Status == "" {
- // channel closed; don't print anything
- } else {
- fmt.Fprint(c.ReadWriter.Out, res.Status)
- return nil
- }
- case res := <-pushauth: // push
+ case res := <-resultC:
if res.Status == "cancelled" {
- // push cancelled; don't print anything
+ // request cancelled; don't print anything
} else if res.Status == "" {
// channel closed; don't print anything
} else {
--
GitLab
From ac9d9bd756b0b215ea7e2519a3748eec8124d91b Mon Sep 17 00:00:00 2001
From: James Sandlin <jsandlin@gitlab.com>
Date: Fri, 15 Jul 2022 09:01:18 -0700
Subject: [PATCH 21/25] reorg
---
.../twofactorverify/twofactorverify.go | 215 ++++++++++++++++--
.../twofactorverifymanual_test.go | 36 +--
2 files changed, 212 insertions(+), 39 deletions(-)
diff --git a/internal/command/twofactorverify/twofactorverify.go b/internal/command/twofactorverify/twofactorverify.go
index bc7123b9..26daedcc 100644
--- a/internal/command/twofactorverify/twofactorverify.go
+++ b/internal/command/twofactorverify/twofactorverify.go
@@ -4,6 +4,7 @@ import (
"context"
"fmt"
"io"
+ "sync"
"time"
"gitlab.com/gitlab-org/labkit/log"
@@ -27,7 +28,13 @@ type Result struct {
Success bool
}
-func (c *Command) Execute(ctx context.Context) error {
+var (
+ mu sync.RWMutex
+ // TODO: make timeout configurable
+ ctxMaxTime = time.Second + 30
+)
+
+func (c *Command) Execute1(ctx context.Context) error {
ctxlog := log.ContextLogger(ctx)
// config.GetHTTPClient isn't thread-safe so save Client in struct for concurrency
@@ -44,49 +51,71 @@ func (c *Command) Execute(ctx context.Context) error {
// TODO: make timeout configurable
const ctxTimeout = 30
timeoutCtx, cancelTimeout := context.WithTimeout(ctx, ctxTimeout*time.Second)
+ verifyCtx, cancelVerify := context.WithCancel(timeoutCtx)
+ pushCtx, cancelPush := context.WithCancel(timeoutCtx)
defer cancelTimeout()
- // Create result channel
- resultC := make(chan Result)
-
// Background push notification with timeout
+ pushauth := make(chan Result)
go func() {
- defer close(resultC)
- status, success, err := c.pushAuth(timeoutCtx)
+ defer close(pushauth)
+ status, success, err := c.pushAuth(pushCtx)
select {
- case <-timeoutCtx.Done(): // push cancelled by manual OTP
- resultC <- Result{Error: nil, Status: "cancelled", Success: false}
+ case <-pushCtx.Done(): // push cancelled by manual OTP
+ pushauth <- Result{Error: nil, Status: "cancelled", Success: false}
default:
- resultC <- Result{Error: err, Status: status, Success: success}
- cancelTimeout()
+ pushauth <- Result{Error: err, Status: status, Success: success}
+ cancelVerify()
}
}()
// Also allow manual OTP entry while waiting for push, with same timeout as push
+ verify := make(chan Result)
go func() {
- defer close(resultC)
+ defer close(verify)
ctxlog.Info("twofactorverify: execute: waiting for user input")
answer := ""
- answer = c.getOTP(timeoutCtx)
+ answer = c.getOTP(verifyCtx)
select {
- case <-timeoutCtx.Done(): // manual OTP cancelled by push
- resultC <- Result{Error: nil, Status: "cancelled", Success: false}
+ case <-verifyCtx.Done(): // manual OTP cancelled by push
+ verify <- Result{Error: nil, Status: "cancelled", Success: false}
default:
- cancelTimeout()
+ cancelPush()
ctxlog.Info("twofactorverify: execute: verifying entered OTP")
- status, success, err := c.verifyOTP(timeoutCtx, answer)
+ status, success, err := c.verifyOTP(verifyCtx, answer)
+ fmt.Println("-------------")
+ fmt.Println("pushAuth.status = ", status)
+ fmt.Println("pushAuth.success = ", success)
+ fmt.Println("pushAuth.err = ", err)
+ fmt.Println("-------------")
ctxlog.WithError(err).Info("twofactorverify: execute: OTP verified")
- resultC <- Result{Error: err, Status: status, Success: success}
+ verify <- Result{Error: err, Status: status, Success: success}
}
}()
+
for {
select {
- case res := <-resultC:
+ case res := <-verify: // manual OTP
+ fmt.Println("-------------")
+ fmt.Println("verify.res = ", res)
+ fmt.Println("-------------")
if res.Status == "cancelled" {
- // request cancelled; don't print anything
+ // verify cancelled; don't print anything
+ } else if res.Status == "" {
+ // channel closed; don't print anything
+ } else {
+ fmt.Fprint(c.ReadWriter.Out, res.Status)
+ return nil
+ }
+ case res := <-pushauth: // push
+ fmt.Println("-------------")
+ fmt.Println("pushauth.res = ", res)
+ fmt.Println("-------------")
+ if res.Status == "cancelled" {
+ // push cancelled; don't print anything
} else if res.Status == "" {
// channel closed; don't print anything
} else {
@@ -101,6 +130,70 @@ func (c *Command) Execute(ctx context.Context) error {
return nil
}
+func (c *Command) Execute(ctx context.Context) error {
+ ctxlog := log.ContextLogger(ctx)
+
+ // config.GetHTTPClient isn't thread-safe so save Client in struct for concurrency
+ // workaround until #518 is fixed
+ var err error
+ c.Client, err = twofactorverify.NewClient(c.Config)
+
+ if err != nil {
+ ctxlog.WithError(err).Error("twofactorverify: execute: OTP verification failed")
+ return err
+ }
+
+ // Create timeout context
+ // TODO: make timeout configurable
+ const ctxTimeout = 30
+ myctx, mycancel := context.WithCancel(ctx)
+ defer mycancel()
+ //defer cancelTimeout()
+ //
+ //// Create result channel
+ //resultC := make(chan Result)
+
+ c.processCmd(myctx, mycancel)
+
+ //
+ for {
+ fmt.Println("for")
+ fmt.Println(myctx)
+ select {
+ case <- myctx.Done():
+ fmt.Println("myctx.Done")
+ //case resPush := <-pushAuth.D:
+ // fmt.Println("resPush => ", resPush.Status)
+ // //if resPush.Status == "cancelled" {
+ // // // request cancelled; don't print anything
+ // //} else if resPush.Status == "" {
+ // // // channel closed; don't print anything
+ // //} else {
+ // // fmt.Fprint(c.ReadWriter.Out, resPush.Status)
+ // // return nil
+ // //}
+ //case resOtp := <-otpAuth:
+ // fmt.Println("otpAuth => ", resOtp)
+ // //if resOtp.Status == "cancelled" {
+ // // // request cancelled; don't print anything
+ // //} else if resOtp.Status == "" {
+ // // // channel closed; don't print anything
+ // //} else {
+ // // fmt.Fprint(c.ReadWriter.Out, resOtp.Status)
+ // // return nil
+ // //}
+
+ case <-myctx.Done(): // push timed out
+ fmt.Fprint(c.ReadWriter.Out, "\nOTP verification timed out\n")
+ return nil
+ default:
+ fmt.Println("myctx == ", myctx)
+
+ }
+ }
+
+ return nil
+}
func (c *Command) getOTP(ctx context.Context) string {
prompt := "OTP: "
@@ -116,9 +209,84 @@ func (c *Command) getOTP(ctx context.Context) string {
return answer
}
+func (c *Command) processCmd(ctx context.Context, cancelTimeout context.CancelFunc) (status string, success bool, err error) {
+ ctxlog := log.ContextLogger(ctx)
+ // Background push notification with timeout
+ pushAuth := make(chan Result)
+ go func() {
+ defer close(pushAuth)
+ status, success, err := c.pushAuth(ctx)
+ fmt.Println("-------------")
+ fmt.Println("pushAuth.status = ", status)
+ fmt.Println("pushAuth.success = ", success)
+ fmt.Println("pushAuth.err = ", err)
+ fmt.Println("-------------")
+
+ select {
+ case <-ctx.Done(): // push cancelled by manual OTP
+ fmt.Println("pushAuth.func.timeoutCtx.Done()")
+ pushAuth <- Result{Error: nil, Status: "cancelled", Success: false}
+ default:
+ fmt.Println("pushAuth.func.default")
+ pushAuth <- Result{Error: err, Status: status, Success: success}
+ cancelTimeout()
+ }
+ }()
+
+ // Also allow manual OTP entry while waiting for push, with same timeout as push
+ otpAuth := make(chan Result)
+ go func() {
+ defer close(otpAuth)
+ fmt.Println("twofactorverify: execute: waiting for user input")
+ otpAnswer := c.getOTP(ctx)
+ fmt.Println("otpAnswer = ", otpAnswer)
+
+ select {
+ case <-ctx.Done(): // manual OTP cancelled by push
+ fmt.Println("otpAuth.func.timeoutCtx.Done()")
+ otpAuth <- Result{Error: nil, Status: "cancelled", Success: false}
+ default:
+ fmt.Println("otpAuth.func.timeoutCtx.default")
+ cancelTimeout()
+ fmt.Println("twofactorverify: execute: verifying entered OTP")
+ status, success, err := c.verifyOTP(ctx, otpAnswer)
+ ctxlog.WithError(err).Info("twofactorverify: execute: OTP verified")
+ otpAuth <- Result{Error: err, Status: status, Success: success}
+ }
+ }()
+ for {
+ select {
+ case pres := <- pushAuth:
+ fmt.Println("-------------")
+ fmt.Println("pushAuth = ", pres)
+ fmt.Println("-------------")
+ if len(pres.Status) > 0 {
+ fmt.Println("-------------")
+ fmt.Println("pushAuth = ", pres.Status)
+ fmt.Println("-------------")
+ }
+ case ores := <- otpAuth:
+ fmt.Println("-------------")
+ fmt.Println("otpAuth = ", ores)
+ fmt.Println("-------------")
+ if len(ores.Status) > 0 {
+ fmt.Println("-------------")
+ fmt.Println("otpAuth = ", ores.Status)
+ fmt.Println("-------------")
+
+ }
+ }
+ }
+ return
+}
+
+
+
+
+
func (c *Command) verifyOTP(ctx context.Context, otp string) (status string, success bool, err error) {
reason := ""
-
+ fmt.Println("verifyOTP(ctx, ",otp,")")
success, reason, err = c.Client.VerifyOTP(ctx, c.Args, otp)
if success {
status = fmt.Sprintf("\nOTP validation successful. Git operations are now allowed.\n")
@@ -136,9 +304,14 @@ func (c *Command) verifyOTP(ctx context.Context, otp string) (status string, suc
}
func (c *Command) pushAuth(ctx context.Context) (status string, success bool, err error) {
+ fmt.Println("---------------------------------------")
reason := ""
-
+ fmt.Println(c.Args)
success, reason, err = c.Client.PushAuth(ctx, c.Args)
+ fmt.Println("pushAuth.reason = ", reason)
+ fmt.Println("pushAuth.success = ", success)
+ fmt.Println("pushAuth.err = ", err)
+ fmt.Println("---------------------------------------")
if success {
status = fmt.Sprintf("\nPush OTP validation successful. Git operations are now allowed.\n")
} else {
diff --git a/internal/command/twofactorverify/twofactorverifymanual_test.go b/internal/command/twofactorverify/twofactorverifymanual_test.go
index c914e778..71372798 100644
--- a/internal/command/twofactorverify/twofactorverifymanual_test.go
+++ b/internal/command/twofactorverify/twofactorverifymanual_test.go
@@ -110,24 +110,24 @@ func TestExecuteManual(t *testing.T) {
answer: "123456\n",
expectedOutput: manualQuestion + "OTP validation successful. Git operations are now allowed.\n",
},
- {
- desc: "With bad response",
- arguments: &commandargs.Shell{GitlabKeyId: "-1"},
- answer: "123456\n",
- expectedOutput: manualQuestion + manualErrorHeader + "Parsing failed\n",
- },
- {
- desc: "With API returns an error",
- arguments: &commandargs.Shell{GitlabKeyId: "error"},
- answer: "yes\n",
- expectedOutput: manualQuestion + manualErrorHeader + "error message\n",
- },
- {
- desc: "With API fails",
- arguments: &commandargs.Shell{GitlabKeyId: "broken"},
- answer: "yes\n",
- expectedOutput: manualQuestion + manualErrorHeader + "Internal API error (500)\n",
- },
+ //{
+ // desc: "With bad response",
+ // arguments: &commandargs.Shell{GitlabKeyId: "-1"},
+ // answer: "123456\n",
+ // expectedOutput: manualQuestion + manualErrorHeader + "Parsing failed\n",
+ //},
+ //{
+ // desc: "With API returns an error",
+ // arguments: &commandargs.Shell{GitlabKeyId: "error"},
+ // answer: "yes\n",
+ // expectedOutput: manualQuestion + manualErrorHeader + "error message\n",
+ //},
+ //{
+ // desc: "With API fails",
+ // arguments: &commandargs.Shell{GitlabKeyId: "broken"},
+ // answer: "yes\n",
+ // expectedOutput: manualQuestion + manualErrorHeader + "Internal API error (500)\n",
+ //},
}
for _, tc := range testCases {
--
GitLab
From d791ce333dd072c181cb519163e26eda4ea7e7cc Mon Sep 17 00:00:00 2001
From: James Sandlin <jsandlin@gitlab.com>
Date: Fri, 15 Jul 2022 09:03:44 -0700
Subject: [PATCH 22/25] add temp files
---
.../twofactorverify/twofactorverify.go.james | 233 ++++++++++++++++++
.../twofactorverify/twofactorverify.go.new | 157 ++++++++++++
.../twofactorverify/twofactorverify.go.old | 163 ++++++++++++
3 files changed, 553 insertions(+)
create mode 100644 internal/command/twofactorverify/twofactorverify.go.james
create mode 100644 internal/command/twofactorverify/twofactorverify.go.new
create mode 100644 internal/command/twofactorverify/twofactorverify.go.old
diff --git a/internal/command/twofactorverify/twofactorverify.go.james b/internal/command/twofactorverify/twofactorverify.go.james
new file mode 100644
index 00000000..8ba501aa
--- /dev/null
+++ b/internal/command/twofactorverify/twofactorverify.go.james
@@ -0,0 +1,233 @@
+package twofactorverify
+
+import (
+ "context"
+ "fmt"
+ "io"
+ "sync"
+ "time"
+
+ "gitlab.com/gitlab-org/labkit/log"
+
+ "gitlab.com/gitlab-org/gitlab-shell/internal/command/commandargs"
+ "gitlab.com/gitlab-org/gitlab-shell/internal/command/readwriter"
+ "gitlab.com/gitlab-org/gitlab-shell/internal/config"
+ "gitlab.com/gitlab-org/gitlab-shell/internal/gitlabnet/twofactorverify"
+)
+
+type Command struct {
+ Config *config.Config
+ Client *twofactorverify.Client
+ Args *commandargs.Shell
+ ReadWriter *readwriter.ReadWriter
+}
+
+type Result struct {
+ Error error
+ Status string
+ Success bool
+}
+
+// A context to be used as a merged context for both push && OTP entry
+type waitContext struct {
+ mu sync.Mutex
+ mainCtx context.Context
+ ctx context.Context
+ done chan struct {}
+ err error
+}
+
+
+var (
+ mu sync.RWMutex
+ // TODO: make timeout configurable
+ ctxMaxTime = time.Second + 30
+)
+
+func (c *Command) Execute(ctx context.Context) error {
+ ctxlog := log.ContextLogger(ctx)
+
+ // config.GetHTTPClient isn't thread-safe so save Client in struct for concurrency
+ // workaround until #518 is fixed
+ var err error
+ c.Client, err = twofactorverify.NewClient(c.Config)
+
+ if err != nil {
+ ctxlog.WithError(err).Error("twofactorverify: execute: OTP verification failed")
+ return err
+ }
+ //var mainCancel context.CancelFunc
+ //mainContext, mainCancel = context.WithCancel(ctx)
+ timeoutCtx, timeoutCancel := context.WithTimeout(ctx, ctxMaxTime)
+ defer timeoutCancel()
+
+ // Create Result Channel
+ // It seems to me that if we use the same channel for each request & defer it multiple times things will fail. So I'm not doing that.
+ //resultChannel := make(chan Result)
+
+ // Time before forced task cancellation
+ timeoutCh := time.After(ctxMaxTime)
+
+ // Send push Auth Request
+ pushAuth := make(chan Result)
+ go func() {
+ defer close(pushAuth)
+ status, success, err := c.pushAuth(timeoutCtx)
+ ctxlog.Info("pushAuth.status = ", status)
+ ctxlog.Info("pushAuth.success = ", success)
+ ctxlog.Info("pushAuth.err = ", err)
+ select {
+ case <-timeoutCtx.Done(): // push cancelled by manual OTP
+ resultC <- Result{Error: nil, Status: "cancelled", Success: false}
+ default:
+ resultC <- Result{Error: err, Status: status, Success: success}
+ cancelTimeout()
+ }
+
+ }()
+
+ // Send OTP Auth Request
+ otpAuth := make(chan Result)
+ go func(){
+ defer close(otpAuth)
+ ctxlog.Info("twofactorverify: waiting for user input.")
+ otpAnswer := c.getOTP(mainCtx)
+ ctxlog.Info("otpAnswer = ", otpAnswer)
+ }()
+
+
+
+ //
+ //// Wait until tasks completed or time.After event occurred.
+ select {
+ case <- tasksCancelled:
+ ctxlog.Info("case tasksCancelled")
+ ctxlog.Info(pushAuth)
+ case <- timeoutCh:
+ ctxlog.Info("case Timeout")
+ // Wait until all done.
+ <-tasksCancelled
+ }
+ //
+ ////timeoutCtx, cancelTimeout := context.WithTimeout(ctx, ctxTimeout)
+ //context, cancelVerify := context.WithCancel(timeoutCtx)
+ //pushCtx, cancelPush := context.WithCancel(timeoutCtx)
+ //defer cancelTimeout()
+ //
+ //// Background push notification with timeout
+ //pushauth := make(chan Result)
+ //go func() {
+ // defer close(pushauth)
+ // status, success, err := c.pushAuth(pushCtx)
+ //
+ // select {
+ // case <-pushCtx.Done(): // push cancelled by manual OTP
+ // pushauth <- Result{Error: nil, Status: "cancelled", Success: false}
+ // default:
+ // pushauth <- Result{Error: err, Status: status, Success: success}
+ // cancelVerify()
+ // }
+ //}()
+ //
+ //// Also allow manual OTP entry while waiting for push, with same timeout as push
+ //verify := make(chan Result)
+ //go func() {
+ // defer close(verify)
+ // ctxlog.Info("twofactorverify: execute: waiting for user input")
+ // answer := ""
+ // answer = c.getOTP(verifyCtx)
+ //
+ // select {
+ // case <-verifyCtx.Done(): // manual OTP cancelled by push
+ // verify <- Result{Error: nil, Status: "cancelled", Success: false}
+ // default:
+ // cancelPush()
+ // ctxlog.Info("twofactorverify: execute: verifying entered OTP")
+ // status, success, err := c.verifyOTP(verifyCtx, answer)
+ // ctxlog.WithError(err).Info("twofactorverify: execute: OTP verified")
+ // verify <- Result{Error: err, Status: status, Success: success}
+ // }
+ //}()
+ //
+ //for {
+ // select {
+ // case res := <-verify: // manual OTP
+ // if res.Status == "cancelled" {
+ // // verify cancelled; don't print anything
+ // } else if res.Status == "" {
+ // // channel closed; don't print anything
+ // } else {
+ // fmt.Fprint(c.ReadWriter.Out, res.Status)
+ // return nil
+ // }
+ // case res := <-pushauth: // push
+ // if res.Status == "cancelled" {
+ // // push cancelled; don't print anything
+ // } else if res.Status == "" {
+ // // channel closed; don't print anything
+ // } else {
+ // fmt.Fprint(c.ReadWriter.Out, res.Status)
+ // return nil
+ // }
+ // case <-timeoutCtx.Done(): // push timed out
+ // fmt.Fprint(c.ReadWriter.Out, "\nOTP verification timed out\n")
+ // return nil
+ // }
+ //}
+ //
+ return nil
+}
+
+
+func (c *Command) getOTP(ctx context.Context) string {
+ prompt := "OTP: "
+ fmt.Fprint(c.ReadWriter.Out, prompt)
+
+ var answer string
+ otpLength := int64(64)
+ reader := io.LimitReader(c.ReadWriter.In, otpLength)
+ if _, err := fmt.Fscanln(reader, &answer); err != nil {
+ log.ContextLogger(ctx).WithError(err).Debug("twofactorverify: getOTP: Failed to get user input")
+ }
+
+ return answer
+}
+
+func (c *Command) verifyOTP(ctx context.Context, otp string) (status string, success bool, err error) {
+ reason := ""
+
+ success, reason, err = c.Client.VerifyOTP(ctx, c.Args, otp)
+ fmt.Println("success = " , success)
+ fmt.Println("reason = ", reason)
+ fmt.Println("err = ", err)
+ if success {
+ status = fmt.Sprintf("\nOTP validation successful. Git operations are now allowed.\n")
+ } else {
+ if err != nil {
+ status = fmt.Sprintf("\nOTP validation failed.\n%v\n", err)
+ } else {
+ status = fmt.Sprintf("\nOTP validation failed.\n%v\n", reason)
+ }
+ }
+
+ err = nil
+
+ return
+}
+
+func (c *Command) pushAuth(ctx context.Context) (status string, success bool, err error) {
+ reason := ""
+
+ success, reason, err = c.Client.PushAuth(ctx, c.Args)
+ if success {
+ status = fmt.Sprintf("\nPush OTP validation successful. Git operations are now allowed.\n")
+ } else {
+ if err != nil {
+ status = fmt.Sprintf("\nPush OTP validation failed.\n%v\n", err)
+ } else {
+ status = fmt.Sprintf("\nPush OTP validation failed.\n%v\n", reason)
+ }
+ }
+
+ return
+}
diff --git a/internal/command/twofactorverify/twofactorverify.go.new b/internal/command/twofactorverify/twofactorverify.go.new
new file mode 100644
index 00000000..447b663c
--- /dev/null
+++ b/internal/command/twofactorverify/twofactorverify.go.new
@@ -0,0 +1,157 @@
+package twofactorverify
+
+import (
+ "context"
+ "fmt"
+ "io"
+ "time"
+
+ "gitlab.com/gitlab-org/labkit/log"
+
+ "gitlab.com/gitlab-org/gitlab-shell/internal/command/commandargs"
+ "gitlab.com/gitlab-org/gitlab-shell/internal/command/readwriter"
+ "gitlab.com/gitlab-org/gitlab-shell/internal/config"
+ "gitlab.com/gitlab-org/gitlab-shell/internal/gitlabnet/twofactorverify"
+)
+
+type Command struct {
+ Config *config.Config
+ Client *twofactorverify.Client
+ Args *commandargs.Shell
+ ReadWriter *readwriter.ReadWriter
+}
+
+type Result struct {
+ Error error
+ Status string
+ Success bool
+}
+
+func (c *Command) Execute(ctx context.Context) error {
+ ctxlog := log.ContextLogger(ctx)
+
+ // config.GetHTTPClient isn't thread-safe so save Client in struct for concurrency
+ // workaround until #518 is fixed
+ var err error
+ c.Client, err = twofactorverify.NewClient(c.Config)
+
+ if err != nil {
+ ctxlog.WithError(err).Error("twofactorverify: execute: OTP verification failed")
+ return err
+ }
+
+ // Create timeout context
+ // TODO: make timeout configurable
+ const ctxTimeout = 30
+ timeoutCtx, cancelTimeout := context.WithTimeout(ctx, ctxTimeout*time.Second)
+ defer cancelTimeout()
+
+ // Create result channel
+ resultC := make(chan Result)
+
+ // Background push notification with timeout
+ go func() {
+ defer close(resultC)
+ status, success, err := c.pushAuth(timeoutCtx)
+
+ select {
+ case <-timeoutCtx.Done(): // push cancelled by manual OTP
+ resultC <- Result{Error: nil, Status: "cancelled", Success: false}
+ default:
+ resultC <- Result{Error: err, Status: status, Success: success}
+ cancelTimeout()
+ }
+ }()
+
+ // Also allow manual OTP entry while waiting for push, with same timeout as push
+ go func() {
+ defer close(resultC)
+ ctxlog.Info("twofactorverify: execute: waiting for user input")
+ answer := ""
+ answer = c.getOTP(timeoutCtx)
+
+ select {
+ case <-timeoutCtx.Done(): // manual OTP cancelled by push
+ resultC <- Result{Error: nil, Status: "cancelled", Success: false}
+ default:
+ cancelTimeout()
+ ctxlog.Info("twofactorverify: execute: verifying entered OTP")
+ status, success, err := c.verifyOTP(timeoutCtx, answer)
+ ctxlog.WithError(err).Info("twofactorverify: execute: OTP verified")
+ resultC <- Result{Error: err, Status: status, Success: success}
+ }
+ }()
+
+ for {
+ select {
+ case res := <-resultC:
+ if res.Status == "cancelled" {
+ // request cancelled; don't print anything
+ } else if res.Status == "" {
+ // channel closed; don't print anything
+ } else {
+ fmt.Fprint(c.ReadWriter.Out, res.Status)
+ return nil
+ }
+ case <-timeoutCtx.Done(): // push timed out
+ fmt.Fprint(c.ReadWriter.Out, "\nOTP verification timed out\n")
+ return nil
+ }
+ }
+
+ return nil
+}
+
+func (c *Command) getOTP(ctx context.Context) string {
+ prompt := "OTP: "
+ fmt.Fprint(c.ReadWriter.Out, prompt)
+
+ var answer string
+ otpLength := int64(64)
+ reader := io.LimitReader(c.ReadWriter.In, otpLength)
+ if _, err := fmt.Fscanln(reader, &answer); err != nil {
+ log.ContextLogger(ctx).WithError(err).Debug("twofactorverify: getOTP: Failed to get user input")
+ }
+
+ return answer
+}
+
+func (c *Command) verifyOTP(ctx context.Context, otp string) (status string, success bool, err error) {
+ fmt.Sprintf("verifyOTP")
+ reason := ""
+ success, reason, err = c.Client.VerifyOTP(ctx, c.Args, otp)
+
+ fmt.Sprintf("\nSUCCESS = #{success}\n")
+ fmt.Sprintf("\nREASON = #{reason}\n")
+ fmt.Sprintf("\nERR = #{err}\n")
+ if success {
+ status = fmt.Sprintf("\nOTP validation successful. Git operations are now allowed.\n")
+ } else {
+ if err != nil {
+ status = fmt.Sprintf("\nOTP validation failed.\n%v\n", err)
+ } else {
+ status = fmt.Sprintf("\nOTP validation failed.\n%v\n", reason)
+ }
+ }
+
+ err = nil
+
+ return
+}
+
+func (c *Command) pushAuth(ctx context.Context) (status string, success bool, err error) {
+ reason := ""
+
+ success, reason, err = c.Client.PushAuth(ctx, c.Args)
+ if success {
+ status = fmt.Sprintf("\nPush OTP validation successful. Git operations are now allowed.\n")
+ } else {
+ if err != nil {
+ status = fmt.Sprintf("\nPush OTP validation failed.\n%v\n", err)
+ } else {
+ status = fmt.Sprintf("\nPush OTP validation failed.\n%v\n", reason)
+ }
+ }
+
+ return
+}
diff --git a/internal/command/twofactorverify/twofactorverify.go.old b/internal/command/twofactorverify/twofactorverify.go.old
new file mode 100644
index 00000000..e12a1bd2
--- /dev/null
+++ b/internal/command/twofactorverify/twofactorverify.go.old
@@ -0,0 +1,163 @@
+package twofactorverify
+
+import (
+ "context"
+ "fmt"
+ "io"
+ "time"
+
+ "gitlab.com/gitlab-org/labkit/log"
+
+ "gitlab.com/gitlab-org/gitlab-shell/internal/command/commandargs"
+ "gitlab.com/gitlab-org/gitlab-shell/internal/command/readwriter"
+ "gitlab.com/gitlab-org/gitlab-shell/internal/config"
+ "gitlab.com/gitlab-org/gitlab-shell/internal/gitlabnet/twofactorverify"
+)
+
+type Command struct {
+ Config *config.Config
+ Client *twofactorverify.Client
+ Args *commandargs.Shell
+ ReadWriter *readwriter.ReadWriter
+}
+
+type Result struct {
+ Error error
+ Status string
+ Success bool
+}
+
+func (c *Command) Execute(ctx context.Context) error {
+ ctxlog := log.ContextLogger(ctx)
+
+ // config.GetHTTPClient isn't thread-safe so save Client in struct for concurrency
+ // workaround until #518 is fixed
+ var err error
+ c.Client, err = twofactorverify.NewClient(c.Config)
+
+ if err != nil {
+ ctxlog.WithError(err).Error("twofactorverify: execute: OTP verification failed")
+ return err
+ }
+
+ // Create timeout context
+ // TODO: make timeout configurable
+ const ctxTimeout = 30
+ timeoutCtx, cancelTimeout := context.WithTimeout(ctx, ctxTimeout*time.Second)
+ verifyCtx, cancelVerify := context.WithCancel(timeoutCtx)
+ pushCtx, cancelPush := context.WithCancel(timeoutCtx)
+ defer cancelTimeout()
+
+ // Background push notification with timeout
+ pushauth := make(chan Result)
+ go func() {
+ defer close(pushauth)
+ status, success, err := c.pushAuth(pushCtx)
+
+ select {
+ case <-pushCtx.Done(): // push cancelled by manual OTP
+ pushauth <- Result{Error: nil, Status: "cancelled", Success: false}
+ default:
+ pushauth <- Result{Error: err, Status: status, Success: success}
+ cancelVerify()
+ }
+ }()
+
+ // Also allow manual OTP entry while waiting for push, with same timeout as push
+ verify := make(chan Result)
+ go func() {
+ defer close(verify)
+ ctxlog.Info("twofactorverify: execute: waiting for user input")
+ answer := ""
+ answer = c.getOTP(verifyCtx)
+
+ select {
+ case <-verifyCtx.Done(): // manual OTP cancelled by push
+ verify <- Result{Error: nil, Status: "cancelled", Success: false}
+ default:
+ cancelPush()
+ ctxlog.Info("twofactorverify: execute: verifying entered OTP")
+ status, success, err := c.verifyOTP(verifyCtx, answer)
+ ctxlog.WithError(err).Info("twofactorverify: execute: OTP verified")
+ verify <- Result{Error: err, Status: status, Success: success}
+ }
+ }()
+
+ for {
+ select {
+ case res := <-verify: // manual OTP
+ if res.Status == "cancelled" {
+ // verify cancelled; don't print anything
+ } else if res.Status == "" {
+ // channel closed; don't print anything
+ } else {
+ fmt.Fprint(c.ReadWriter.Out, res.Status)
+ return nil
+ }
+ case res := <-pushauth: // push
+ if res.Status == "cancelled" {
+ // push cancelled; don't print anything
+ } else if res.Status == "" {
+ // channel closed; don't print anything
+ } else {
+ fmt.Fprint(c.ReadWriter.Out, res.Status)
+ return nil
+ }
+ case <-timeoutCtx.Done(): // push timed out
+ fmt.Fprint(c.ReadWriter.Out, "\nOTP verification timed out\n")
+ return nil
+ }
+ }
+
+ return nil
+}
+
+func (c *Command) getOTP(ctx context.Context) string {
+ prompt := "OTP: "
+ fmt.Fprint(c.ReadWriter.Out, prompt)
+
+ var answer string
+ otpLength := int64(64)
+ reader := io.LimitReader(c.ReadWriter.In, otpLength)
+ if _, err := fmt.Fscanln(reader, &answer); err != nil {
+ log.ContextLogger(ctx).WithError(err).Debug("twofactorverify: getOTP: Failed to get user input")
+ }
+
+ return answer
+}
+
+func (c *Command) verifyOTP(ctx context.Context, otp string) (status string, success bool, err error) {
+ reason := ""
+
+ success, reason, err = c.Client.VerifyOTP(ctx, c.Args, otp)
+ if success {
+ status = fmt.Sprintf("\nOTP validation successful. Git operations are now allowed.\n")
+ } else {
+ if err != nil {
+ status = fmt.Sprintf("\nOTP validation failed.\n%v\n", err)
+ } else {
+ status = fmt.Sprintf("\nOTP validation failed.\n%v\n", reason)
+ }
+ }
+
+ err = nil
+
+ return
+}
+
+func (c *Command) pushAuth(ctx context.Context) (status string, success bool, err error) {
+ reason := ""
+
+ success, reason, err = c.Client.PushAuth(ctx, c.Args)
+ if success {
+ status = fmt.Sprintf("\nPush OTP validation successful. Git operations are now allowed.\n")
+ } else {
+ if err != nil {
+ status = fmt.Sprintf("\nPush OTP validation failed.\n%v\n", err)
+ } else {
+ status = fmt.Sprintf("\nPush OTP validation failed.\n%v\n", reason)
+ }
+ }
+
+ return
+}
--
GitLab
From c93f5456da5a0696d974c071d43c5d0faf31bb88 Mon Sep 17 00:00:00 2001
From: James Sandlin <jsandlin@gitlab.com>
Date: Fri, 15 Jul 2022 13:38:38 -0700
Subject: [PATCH 23/25] All testing for otpAuth now works.
---
.../twofactorverify/twofactorverify.go | 219 ++++++++----------
.../twofactorverifymanual_test.go | 36 +--
internal/gitlabnet/twofactorverify/client.go | 5 +
3 files changed, 116 insertions(+), 144 deletions(-)
diff --git a/internal/command/twofactorverify/twofactorverify.go b/internal/command/twofactorverify/twofactorverify.go
index 26daedcc..23f3f8bd 100644
--- a/internal/command/twofactorverify/twofactorverify.go
+++ b/internal/command/twofactorverify/twofactorverify.go
@@ -34,14 +34,17 @@ var (
ctxMaxTime = time.Second + 30
)
-func (c *Command) Execute1(ctx context.Context) error {
+func (c *Command) Execute2(ctx context.Context) error {
ctxlog := log.ContextLogger(ctx)
// config.GetHTTPClient isn't thread-safe so save Client in struct for concurrency
// workaround until #518 is fixed
var err error
c.Client, err = twofactorverify.NewClient(c.Config)
-
+ fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
+ fmt.Println("client = ", c.Client)
+ fmt.Println("err = ", err)
+ fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
if err != nil {
ctxlog.WithError(err).Error("twofactorverify: execute: OTP verification failed")
return err
@@ -54,7 +57,9 @@ func (c *Command) Execute1(ctx context.Context) error {
verifyCtx, cancelVerify := context.WithCancel(timeoutCtx)
pushCtx, cancelPush := context.WithCancel(timeoutCtx)
defer cancelTimeout()
-
+ //fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
+ //fmt.Println(verifyCtx, ", ", cancelVerify)
+ //fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
// Background push notification with timeout
pushauth := make(chan Result)
go func() {
@@ -85,11 +90,11 @@ func (c *Command) Execute1(ctx context.Context) error {
cancelPush()
ctxlog.Info("twofactorverify: execute: verifying entered OTP")
status, success, err := c.verifyOTP(verifyCtx, answer)
- fmt.Println("-------------")
- fmt.Println("pushAuth.status = ", status)
- fmt.Println("pushAuth.success = ", success)
- fmt.Println("pushAuth.err = ", err)
- fmt.Println("-------------")
+ //fmt.Println("-------------")
+ //fmt.Println("pushAuth.status = ", status)
+ //fmt.Println("pushAuth.success = ", success)
+ //fmt.Println("pushAuth.err = ", err)
+ //fmt.Println("-------------")
ctxlog.WithError(err).Info("twofactorverify: execute: OTP verified")
verify <- Result{Error: err, Status: status, Success: success}
}
@@ -99,9 +104,9 @@ func (c *Command) Execute1(ctx context.Context) error {
for {
select {
case res := <-verify: // manual OTP
- fmt.Println("-------------")
- fmt.Println("verify.res = ", res)
- fmt.Println("-------------")
+ //fmt.Println("-------------")
+ //fmt.Println("verify.res = ", res)
+ //fmt.Println("-------------")
if res.Status == "cancelled" {
// verify cancelled; don't print anything
} else if res.Status == "" {
@@ -111,9 +116,9 @@ func (c *Command) Execute1(ctx context.Context) error {
return nil
}
case res := <-pushauth: // push
- fmt.Println("-------------")
- fmt.Println("pushauth.res = ", res)
- fmt.Println("-------------")
+ //fmt.Println("-------------")
+ //fmt.Println("pushauth.res = ", res)
+ //fmt.Println("-------------")
if res.Status == "cancelled" {
// push cancelled; don't print anything
} else if res.Status == "" {
@@ -144,136 +149,79 @@ func (c *Command) Execute(ctx context.Context) error {
}
// Create timeout context
- // TODO: make timeout configurable
- const ctxTimeout = 30
myctx, mycancel := context.WithCancel(ctx)
defer mycancel()
- //defer cancelTimeout()
- //
- //// Create result channel
- //resultC := make(chan Result)
- c.processCmd(myctx, mycancel)
+ otpAuth := make(chan Result)
+ go func() {
+ defer close(otpAuth)
+ ctxlog.Info("twofactorverify: execute: waiting for user input")
+ otpAnswer := c.getOTP(myctx)
- //
+ select {
+ case <-ctx.Done(): // manual OTP cancelled by push
+ otpAuth <- Result{Error: nil, Status: "cancelled", Success: false}
+ default:
+ status, success, err := c.verifyOTP(myctx, otpAnswer)
+ otpAuth <- Result{Error: err, Status: status, Success: success}
+ }
+ }()
for {
- fmt.Println("for")
- fmt.Println(myctx)
select {
- case <- myctx.Done():
- fmt.Println("myctx.Done")
- //case resPush := <-pushAuth.D:
- // fmt.Println("resPush => ", resPush.Status)
- // //if resPush.Status == "cancelled" {
- // // // request cancelled; don't print anything
- // //} else if resPush.Status == "" {
- // // // channel closed; don't print anything
- // //} else {
- // // fmt.Fprint(c.ReadWriter.Out, resPush.Status)
- // // return nil
- // //}
- //case resOtp := <-otpAuth:
- // fmt.Println("otpAuth => ", resOtp)
- // //if resOtp.Status == "cancelled" {
- // // // request cancelled; don't print anything
- // //} else if resOtp.Status == "" {
- // // // channel closed; don't print anything
- // //} else {
- // // fmt.Fprint(c.ReadWriter.Out, resOtp.Status)
- // // return nil
- // //}
-
- case <-myctx.Done(): // push timed out
- fmt.Fprint(c.ReadWriter.Out, "\nOTP verification timed out\n")
+ case res := <- otpAuth:
+ if len(res.Status) > 0 && res.Status != "cancelled"{
+ fmt.Fprint(c.ReadWriter.Out, res.Status)
return nil
- default:
- fmt.Println("myctx == ", myctx)
-
+ }
}
}
return nil
}
-func (c *Command) getOTP(ctx context.Context) string {
- prompt := "OTP: "
- fmt.Fprint(c.ReadWriter.Out, prompt)
-
- var answer string
- otpLength := int64(64)
- reader := io.LimitReader(c.ReadWriter.In, otpLength)
- if _, err := fmt.Fscanln(reader, &answer); err != nil {
- log.ContextLogger(ctx).WithError(err).Debug("twofactorverify: getOTP: Failed to get user input")
- }
-
- return answer
-}
-
-func (c *Command) processCmd(ctx context.Context, cancelTimeout context.CancelFunc) (status string, success bool, err error) {
+func (c Command) processCmd(ctx context.Context, cancelTimeout context.CancelFunc) (result Result) {
ctxlog := log.ContextLogger(ctx)
- // Background push notification with timeout
- pushAuth := make(chan Result)
- go func() {
- defer close(pushAuth)
- status, success, err := c.pushAuth(ctx)
- fmt.Println("-------------")
- fmt.Println("pushAuth.status = ", status)
- fmt.Println("pushAuth.success = ", success)
- fmt.Println("pushAuth.err = ", err)
- fmt.Println("-------------")
-
- select {
- case <-ctx.Done(): // push cancelled by manual OTP
- fmt.Println("pushAuth.func.timeoutCtx.Done()")
- pushAuth <- Result{Error: nil, Status: "cancelled", Success: false}
- default:
- fmt.Println("pushAuth.func.default")
- pushAuth <- Result{Error: err, Status: status, Success: success}
- cancelTimeout()
- }
- }()
- // Also allow manual OTP entry while waiting for push, with same timeout as push
otpAuth := make(chan Result)
go func() {
defer close(otpAuth)
- fmt.Println("twofactorverify: execute: waiting for user input")
+ ctxlog.Info("twofactorverify: execute: waiting for user input")
otpAnswer := c.getOTP(ctx)
- fmt.Println("otpAnswer = ", otpAnswer)
select {
- case <-ctx.Done(): // manual OTP cancelled by push
- fmt.Println("otpAuth.func.timeoutCtx.Done()")
- otpAuth <- Result{Error: nil, Status: "cancelled", Success: false}
- default:
- fmt.Println("otpAuth.func.timeoutCtx.default")
- cancelTimeout()
- fmt.Println("twofactorverify: execute: verifying entered OTP")
- status, success, err := c.verifyOTP(ctx, otpAnswer)
- ctxlog.WithError(err).Info("twofactorverify: execute: OTP verified")
- otpAuth <- Result{Error: err, Status: status, Success: success}
+ case <-ctx.Done(): // manual OTP cancelled by push
+ fmt.Println("otpAuth.ctx.Done()")
+ otpAuth <- Result{Error: nil, Status: "cancelled", Success: false}
+ fmt.Println("----------------------------------------------------")
+ fmt.Println("otpAuth = ", otpAuth)
+ fmt.Println("----------------------------------------------------")
+ default:
+ fmt.Println("otpAuth.default")
+ cancelTimeout()
+ fmt.Println("Call c.verifyOTP(", ctx, ", ", otpAnswer, ")")
+ status, success, err := c.verifyOTP(ctx, otpAnswer)
+ fmt.Println("otpAnswer.status = ", status)
+ fmt.Println("otpAnswer.success = ", success)
+ fmt.Println("otpAnswer.err = ", err)
+ otpAuth <- Result{Error: err, Status: status, Success: success}
+ fmt.Println("----------------------------------------------------")
+ fmt.Println("otpAuth = ", otpAuth)
+ fmt.Println("----------------------------------------------------")
}
}()
for {
+ //fmt.Println("for loop")
select {
- case pres := <- pushAuth:
- fmt.Println("-------------")
- fmt.Println("pushAuth = ", pres)
- fmt.Println("-------------")
- if len(pres.Status) > 0 {
- fmt.Println("-------------")
- fmt.Println("pushAuth = ", pres.Status)
- fmt.Println("-------------")
- }
- case ores := <- otpAuth:
- fmt.Println("-------------")
- fmt.Println("otpAuth = ", ores)
- fmt.Println("-------------")
- if len(ores.Status) > 0 {
- fmt.Println("-------------")
- fmt.Println("otpAuth = ", ores.Status)
- fmt.Println("-------------")
-
+ case res := <- otpAuth:
+ fmt.Println(res)
+ //fmt.Println("-------------")
+ //fmt.Println("otpAuth = ", ores)
+ //fmt.Println("-------------")
+ if len(res.Status) > 0 && res.Status != "cancelled"{
+ //fmt.Println("-------------")
+ //fmt.Println("otpAuth = ", res.Status)
+ //fmt.Println("-------------")
+ return res
}
}
}
@@ -283,11 +231,30 @@ func (c *Command) processCmd(ctx context.Context, cancelTimeout context.CancelFu
+func (c *Command) getOTP(ctx context.Context) string {
+ prompt := "OTP: "
+ fmt.Fprint(c.ReadWriter.Out, prompt)
+
+ var answer string
+ otpLength := int64(64)
+ reader := io.LimitReader(c.ReadWriter.In, otpLength)
+ if _, err := fmt.Fscanln(reader, &answer); err != nil {
+ log.ContextLogger(ctx).WithError(err).Debug("twofactorverify: getOTP: Failed to get user input")
+ }
+
+ return answer
+}
+
func (c *Command) verifyOTP(ctx context.Context, otp string) (status string, success bool, err error) {
reason := ""
- fmt.Println("verifyOTP(ctx, ",otp,")")
+ fmt.Println("verifyOTP(", ctx, ", ", c.Args, ", ",otp,")")
success, reason, err = c.Client.VerifyOTP(ctx, c.Args, otp)
+ fmt.Println("----------------------------------------------------")
+ fmt.Println("verifyOTP.status = ", status)
+ fmt.Println("verifyOTP.success = ", success)
+ fmt.Println("verifyOTP.err = ", err)
+ fmt.Println("----------------------------------------------------")
if success {
status = fmt.Sprintf("\nOTP validation successful. Git operations are now allowed.\n")
} else {
@@ -304,14 +271,14 @@ func (c *Command) verifyOTP(ctx context.Context, otp string) (status string, suc
}
func (c *Command) pushAuth(ctx context.Context) (status string, success bool, err error) {
- fmt.Println("---------------------------------------")
+ //fmt.Println("---------------------------------------")
reason := ""
- fmt.Println(c.Args)
+ //fmt.Println(c.Args)
success, reason, err = c.Client.PushAuth(ctx, c.Args)
- fmt.Println("pushAuth.reason = ", reason)
- fmt.Println("pushAuth.success = ", success)
- fmt.Println("pushAuth.err = ", err)
- fmt.Println("---------------------------------------")
+ //fmt.Println("pushAuth.reason = ", reason)
+ //fmt.Println("pushAuth.success = ", success)
+ //fmt.Println("pushAuth.err = ", err)
+ //fmt.Println("---------------------------------------")
if success {
status = fmt.Sprintf("\nPush OTP validation successful. Git operations are now allowed.\n")
} else {
diff --git a/internal/command/twofactorverify/twofactorverifymanual_test.go b/internal/command/twofactorverify/twofactorverifymanual_test.go
index 71372798..c914e778 100644
--- a/internal/command/twofactorverify/twofactorverifymanual_test.go
+++ b/internal/command/twofactorverify/twofactorverifymanual_test.go
@@ -110,24 +110,24 @@ func TestExecuteManual(t *testing.T) {
answer: "123456\n",
expectedOutput: manualQuestion + "OTP validation successful. Git operations are now allowed.\n",
},
- //{
- // desc: "With bad response",
- // arguments: &commandargs.Shell{GitlabKeyId: "-1"},
- // answer: "123456\n",
- // expectedOutput: manualQuestion + manualErrorHeader + "Parsing failed\n",
- //},
- //{
- // desc: "With API returns an error",
- // arguments: &commandargs.Shell{GitlabKeyId: "error"},
- // answer: "yes\n",
- // expectedOutput: manualQuestion + manualErrorHeader + "error message\n",
- //},
- //{
- // desc: "With API fails",
- // arguments: &commandargs.Shell{GitlabKeyId: "broken"},
- // answer: "yes\n",
- // expectedOutput: manualQuestion + manualErrorHeader + "Internal API error (500)\n",
- //},
+ {
+ desc: "With bad response",
+ arguments: &commandargs.Shell{GitlabKeyId: "-1"},
+ answer: "123456\n",
+ expectedOutput: manualQuestion + manualErrorHeader + "Parsing failed\n",
+ },
+ {
+ desc: "With API returns an error",
+ arguments: &commandargs.Shell{GitlabKeyId: "error"},
+ answer: "yes\n",
+ expectedOutput: manualQuestion + manualErrorHeader + "error message\n",
+ },
+ {
+ desc: "With API fails",
+ arguments: &commandargs.Shell{GitlabKeyId: "broken"},
+ answer: "yes\n",
+ expectedOutput: manualQuestion + manualErrorHeader + "Internal API error (500)\n",
+ },
}
for _, tc := range testCases {
diff --git a/internal/gitlabnet/twofactorverify/client.go b/internal/gitlabnet/twofactorverify/client.go
index c9c13c8c..79498150 100644
--- a/internal/gitlabnet/twofactorverify/client.go
+++ b/internal/gitlabnet/twofactorverify/client.go
@@ -44,6 +44,11 @@ func (c *Client) VerifyOTP(ctx context.Context, args *commandargs.Shell, otp str
}
response, err := c.client.Post(ctx, "/two_factor_manual_otp_check", requestBody)
+ fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
+ fmt.Println("c.client = ", c.client)
+ fmt.Println("client.VerifyOTP.response = ", response)
+ fmt.Println("client.VerifyOTP.err = ", err)
+ fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
if err != nil {
return false, "", err
}
--
GitLab
From 1fa30cb09115b72707d14b3b42d4c14a7ea6965b Mon Sep 17 00:00:00 2001
From: James Sandlin <jsandlin@gitlab.com>
Date: Fri, 15 Jul 2022 15:00:17 -0700
Subject: [PATCH 24/25] waitgroup implemented & otp testing works
---
.../twofactorverify/twofactorverify.go | 177 +++++++++++-------
internal/gitlabnet/twofactorverify/client.go | 10 +-
2 files changed, 116 insertions(+), 71 deletions(-)
diff --git a/internal/command/twofactorverify/twofactorverify.go b/internal/command/twofactorverify/twofactorverify.go
index 23f3f8bd..88c49789 100644
--- a/internal/command/twofactorverify/twofactorverify.go
+++ b/internal/command/twofactorverify/twofactorverify.go
@@ -148,87 +148,132 @@ func (c *Command) Execute(ctx context.Context) error {
return err
}
- // Create timeout context
- myctx, mycancel := context.WithCancel(ctx)
- defer mycancel()
+ waitGroup := sync.WaitGroup{}
+
+ myctx, cancelCtx := context.WithTimeout(ctx, ctxMaxTime)
+ defer cancelCtx()
+ //myctx, mycancel := context.WithCancel(timeoutCtx)
+
+
+
+ // Also allow manual OTP entry while waiting for push, with same timeout as push
+ otpChannel := make(chan Result)
+ waitGroup.Add(1)
+ //defer close(otpChannel)
- otpAuth := make(chan Result)
go func() {
- defer close(otpAuth)
+ defer waitGroup.Done()
ctxlog.Info("twofactorverify: execute: waiting for user input")
otpAnswer := c.getOTP(myctx)
select {
case <-ctx.Done(): // manual OTP cancelled by push
- otpAuth <- Result{Error: nil, Status: "cancelled", Success: false}
+ otpChannel <- Result{Error: nil, Status: "cancelled", Success: false}
default:
status, success, err := c.verifyOTP(myctx, otpAnswer)
- otpAuth <- Result{Error: err, Status: status, Success: success}
+ otpChannel <- Result{Error: err, Status: status, Success: success}
}
+ //cancelCtx()
}()
- for {
- select {
- case res := <- otpAuth:
- if len(res.Status) > 0 && res.Status != "cancelled"{
- fmt.Fprint(c.ReadWriter.Out, res.Status)
- return nil
- }
- }
- }
-
- return nil
-}
-
-func (c Command) processCmd(ctx context.Context, cancelTimeout context.CancelFunc) (result Result) {
- ctxlog := log.ContextLogger(ctx)
-
- otpAuth := make(chan Result)
+ //// Background push notification with timeout
+ pushChannel := make(chan Result)
+ waitGroup.Add(1)
go func() {
- defer close(otpAuth)
- ctxlog.Info("twofactorverify: execute: waiting for user input")
- otpAnswer := c.getOTP(ctx)
+ defer waitGroup.Done()
+ //defer close(pushChannel)
+ ctxlog.Info("twofactorverify: execute: waiting for push auth")
+ //status, success, err := c.pushAuth(myctx)
+ //ctxlog.WithError(err).Info("twofactorverify: execute: push auth verified")
select {
- case <-ctx.Done(): // manual OTP cancelled by push
- fmt.Println("otpAuth.ctx.Done()")
- otpAuth <- Result{Error: nil, Status: "cancelled", Success: false}
- fmt.Println("----------------------------------------------------")
- fmt.Println("otpAuth = ", otpAuth)
- fmt.Println("----------------------------------------------------")
- default:
- fmt.Println("otpAuth.default")
- cancelTimeout()
- fmt.Println("Call c.verifyOTP(", ctx, ", ", otpAnswer, ")")
- status, success, err := c.verifyOTP(ctx, otpAnswer)
- fmt.Println("otpAnswer.status = ", status)
- fmt.Println("otpAnswer.success = ", success)
- fmt.Println("otpAnswer.err = ", err)
- otpAuth <- Result{Error: err, Status: status, Success: success}
- fmt.Println("----------------------------------------------------")
- fmt.Println("otpAuth = ", otpAuth)
- fmt.Println("----------------------------------------------------")
+ case <-myctx.Done(): // push cancelled by manual OTP
+ // skip writing to channel
+ pushChannel <- Result{Error: nil, Status: "cancelled", Success: false}
+ ctxlog.Info("twofactorverify: execute: push auth cancelled")
+ //default:
+ // pushChannel <- Result{Error: err, Status: status, Success: success}
}
}()
- for {
- //fmt.Println("for loop")
+ //for {
select {
- case res := <- otpAuth:
- fmt.Println(res)
- //fmt.Println("-------------")
- //fmt.Println("otpAuth = ", ores)
- //fmt.Println("-------------")
- if len(res.Status) > 0 && res.Status != "cancelled"{
- //fmt.Println("-------------")
- //fmt.Println("otpAuth = ", res.Status)
- //fmt.Println("-------------")
- return res
+ case res := <-otpChannel:
+ //fmt.Println("Received from otpChannel => ", res)
+ if len(res.Status) > 0 && res.Status != "cancelled" {
+ fmt.Fprint(c.ReadWriter.Out, res.Status)
+ return nil
}
+ case res := <-pushChannel:
+ if len(res.Status) > 0 && res.Status != "cancelled" {
+ //fmt.Println("Received from pushChannel => ", res)
+ fmt.Println("res.Status == ", res.Status, " -> ", len(res.Status))
+ // //fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
+ // //fmt.Println(res)
+ // //fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
+ fmt.Fprint(c.ReadWriter.Out, res.Status)
+ return nil
+ }
+ //case <- myctx.Done():
+ // fmt.Fprint(c.ReadWriter.Out, "\nOTP verification timed out\n")
+ // return nil
+
}
- }
- return
-}
+ waitGroup.Wait()
+ //}
+ return nil
+}
+//
+//func (c Command) processCmd(ctx context.Context, cancelTimeout context.CancelFunc) (result Result) {
+// ctxlog := log.ContextLogger(ctx)
+//
+// otpAuth := make(chan Result)
+// go func() {
+// defer close(otpAuth)
+// ctxlog.Info("twofactorverify: execute: waiting for user input")
+// otpAnswer := c.getOTP(ctx)
+//
+// select {
+// case <-ctx.Done(): // manual OTP cancelled by push
+// fmt.Println("otpAuth.ctx.Done()")
+// otpAuth <- Result{Error: nil, Status: "cancelled", Success: false}
+// fmt.Println("----------------------------------------------------")
+// fmt.Println("otpAuth = ", otpAuth)
+// fmt.Println("----------------------------------------------------")
+// default:
+// fmt.Println("otpAuth.default")
+// cancelTimeout()
+// fmt.Println("Call c.verifyOTP(", ctx, ", ", otpAnswer, ")")
+// status, success, err := c.verifyOTP(ctx, otpAnswer)
+// fmt.Println("otpAnswer.status = ", status)
+// fmt.Println("otpAnswer.success = ", success)
+// fmt.Println("otpAnswer.err = ", err)
+// otpAuth <- Result{Error: err, Status: status, Success: success}
+// fmt.Println("----------------------------------------------------")
+// fmt.Println("otpAuth = ", otpAuth)
+// fmt.Println("----------------------------------------------------")
+// }
+// }()
+// for {
+// //fmt.Println("for loop")
+// select {
+// case res := <- otpAuth:
+// fmt.Println(res)
+// //fmt.Println("-------------")
+// //fmt.Println("otpAuth = ", ores)
+// //fmt.Println("-------------")
+// if len(res.Status) > 0 && res.Status != "cancelled"{
+// //fmt.Println("-------------")
+// //fmt.Println("otpAuth = ", res.Status)
+// //fmt.Println("-------------")
+// return res
+// }
+// }
+// }
+// return
+//}
+//
+//
func (c *Command) getOTP(ctx context.Context) string {
@@ -248,13 +293,13 @@ func (c *Command) getOTP(ctx context.Context) string {
func (c *Command) verifyOTP(ctx context.Context, otp string) (status string, success bool, err error) {
reason := ""
- fmt.Println("verifyOTP(", ctx, ", ", c.Args, ", ",otp,")")
+ //fmt.Println("verifyOTP(", ctx, ", ", c.Args, ", ",otp,")")
success, reason, err = c.Client.VerifyOTP(ctx, c.Args, otp)
- fmt.Println("----------------------------------------------------")
- fmt.Println("verifyOTP.status = ", status)
- fmt.Println("verifyOTP.success = ", success)
- fmt.Println("verifyOTP.err = ", err)
- fmt.Println("----------------------------------------------------")
+ //fmt.Println("----------------------------------------------------")
+ //fmt.Println("verifyOTP.status = ", status)
+ //fmt.Println("verifyOTP.success = ", success)
+ //fmt.Println("verifyOTP.err = ", err)
+ //fmt.Println("----------------------------------------------------")
if success {
status = fmt.Sprintf("\nOTP validation successful. Git operations are now allowed.\n")
} else {
diff --git a/internal/gitlabnet/twofactorverify/client.go b/internal/gitlabnet/twofactorverify/client.go
index 79498150..fdd6d938 100644
--- a/internal/gitlabnet/twofactorverify/client.go
+++ b/internal/gitlabnet/twofactorverify/client.go
@@ -44,11 +44,11 @@ func (c *Client) VerifyOTP(ctx context.Context, args *commandargs.Shell, otp str
}
response, err := c.client.Post(ctx, "/two_factor_manual_otp_check", requestBody)
- fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
- fmt.Println("c.client = ", c.client)
- fmt.Println("client.VerifyOTP.response = ", response)
- fmt.Println("client.VerifyOTP.err = ", err)
- fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
+ //fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
+ //fmt.Println("c.client = ", c.client)
+ //fmt.Println("client.VerifyOTP.response = ", response)
+ //fmt.Println("client.VerifyOTP.err = ", err)
+ //fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
if err != nil {
return false, "", err
}
--
GitLab
From eb6033a66e054425fd43c8436fbc448907c0f160 Mon Sep 17 00:00:00 2001
From: James Sandlin <jsandlin@gitlab.com>
Date: Fri, 15 Jul 2022 15:21:02 -0700
Subject: [PATCH 25/25] close; things are colliding
---
.../twofactorverify/twofactorverify.go | 156 +++---------------
1 file changed, 27 insertions(+), 129 deletions(-)
diff --git a/internal/command/twofactorverify/twofactorverify.go b/internal/command/twofactorverify/twofactorverify.go
index 88c49789..20e3df28 100644
--- a/internal/command/twofactorverify/twofactorverify.go
+++ b/internal/command/twofactorverify/twofactorverify.go
@@ -34,107 +34,6 @@ var (
ctxMaxTime = time.Second + 30
)
-func (c *Command) Execute2(ctx context.Context) error {
- ctxlog := log.ContextLogger(ctx)
-
- // config.GetHTTPClient isn't thread-safe so save Client in struct for concurrency
- // workaround until #518 is fixed
- var err error
- c.Client, err = twofactorverify.NewClient(c.Config)
- fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
- fmt.Println("client = ", c.Client)
- fmt.Println("err = ", err)
- fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
- if err != nil {
- ctxlog.WithError(err).Error("twofactorverify: execute: OTP verification failed")
- return err
- }
-
- // Create timeout context
- // TODO: make timeout configurable
- const ctxTimeout = 30
- timeoutCtx, cancelTimeout := context.WithTimeout(ctx, ctxTimeout*time.Second)
- verifyCtx, cancelVerify := context.WithCancel(timeoutCtx)
- pushCtx, cancelPush := context.WithCancel(timeoutCtx)
- defer cancelTimeout()
- //fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
- //fmt.Println(verifyCtx, ", ", cancelVerify)
- //fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
- // Background push notification with timeout
- pushauth := make(chan Result)
- go func() {
- defer close(pushauth)
- status, success, err := c.pushAuth(pushCtx)
-
- select {
- case <-pushCtx.Done(): // push cancelled by manual OTP
- pushauth <- Result{Error: nil, Status: "cancelled", Success: false}
- default:
- pushauth <- Result{Error: err, Status: status, Success: success}
- cancelVerify()
- }
- }()
-
- // Also allow manual OTP entry while waiting for push, with same timeout as push
- verify := make(chan Result)
- go func() {
- defer close(verify)
- ctxlog.Info("twofactorverify: execute: waiting for user input")
- answer := ""
- answer = c.getOTP(verifyCtx)
-
- select {
- case <-verifyCtx.Done(): // manual OTP cancelled by push
- verify <- Result{Error: nil, Status: "cancelled", Success: false}
- default:
- cancelPush()
- ctxlog.Info("twofactorverify: execute: verifying entered OTP")
- status, success, err := c.verifyOTP(verifyCtx, answer)
- //fmt.Println("-------------")
- //fmt.Println("pushAuth.status = ", status)
- //fmt.Println("pushAuth.success = ", success)
- //fmt.Println("pushAuth.err = ", err)
- //fmt.Println("-------------")
- ctxlog.WithError(err).Info("twofactorverify: execute: OTP verified")
- verify <- Result{Error: err, Status: status, Success: success}
- }
- }()
-
-
- for {
- select {
- case res := <-verify: // manual OTP
- //fmt.Println("-------------")
- //fmt.Println("verify.res = ", res)
- //fmt.Println("-------------")
- if res.Status == "cancelled" {
- // verify cancelled; don't print anything
- } else if res.Status == "" {
- // channel closed; don't print anything
- } else {
- fmt.Fprint(c.ReadWriter.Out, res.Status)
- return nil
- }
- case res := <-pushauth: // push
- //fmt.Println("-------------")
- //fmt.Println("pushauth.res = ", res)
- //fmt.Println("-------------")
- if res.Status == "cancelled" {
- // push cancelled; don't print anything
- } else if res.Status == "" {
- // channel closed; don't print anything
- } else {
- fmt.Fprint(c.ReadWriter.Out, res.Status)
- return nil
- }
- case <-timeoutCtx.Done(): // push timed out
- fmt.Fprint(c.ReadWriter.Out, "\nOTP verification timed out\n")
- return nil
- }
- }
-
- return nil
-}
func (c *Command) Execute(ctx context.Context) error {
ctxlog := log.ContextLogger(ctx)
@@ -154,6 +53,8 @@ func (c *Command) Execute(ctx context.Context) error {
defer cancelCtx()
//myctx, mycancel := context.WithCancel(timeoutCtx)
+ myctx2, cancelCtx2 := context.WithTimeout(ctx, ctxMaxTime)
+ defer cancelCtx2()
// Also allow manual OTP entry while waiting for push, with same timeout as push
@@ -182,44 +83,41 @@ func (c *Command) Execute(ctx context.Context) error {
defer waitGroup.Done()
//defer close(pushChannel)
ctxlog.Info("twofactorverify: execute: waiting for push auth")
- //status, success, err := c.pushAuth(myctx)
- //ctxlog.WithError(err).Info("twofactorverify: execute: push auth verified")
+ ctxlog.WithError(err).Info("twofactorverify: execute: push auth verified")
select {
- case <-myctx.Done(): // push cancelled by manual OTP
+ case <-myctx2.Done(): // push cancelled by manual OTP
// skip writing to channel
pushChannel <- Result{Error: nil, Status: "cancelled", Success: false}
ctxlog.Info("twofactorverify: execute: push auth cancelled")
- //default:
- // pushChannel <- Result{Error: err, Status: status, Success: success}
+ default:
+ status, success, err := c.pushAuth(myctx2)
+ pushChannel <- Result{Error: err, Status: status, Success: success}
}
}()
- //for {
- select {
- case res := <-otpChannel:
- //fmt.Println("Received from otpChannel => ", res)
- if len(res.Status) > 0 && res.Status != "cancelled" {
- fmt.Fprint(c.ReadWriter.Out, res.Status)
- return nil
- }
- case res := <-pushChannel:
- if len(res.Status) > 0 && res.Status != "cancelled" {
- //fmt.Println("Received from pushChannel => ", res)
- fmt.Println("res.Status == ", res.Status, " -> ", len(res.Status))
- // //fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
- // //fmt.Println(res)
- // //fmt.Println("xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
- fmt.Fprint(c.ReadWriter.Out, res.Status)
- return nil
- }
- //case <- myctx.Done():
- // fmt.Fprint(c.ReadWriter.Out, "\nOTP verification timed out\n")
- // return nil
- }
+ select {
+ case res := <-otpChannel:
+ //fmt.Println("Received from otpChannel => ", res)
+ if len(res.Status) > 0 && res.Status != "cancelled" {
+ fmt.Fprint(c.ReadWriter.Out, res.Status)
+ return nil
+ }
+ case res := <-pushChannel:
+ if len(res.Status) > 0 && res.Status != "cancelled" {
+ //fmt.Println("Received from pushChannel => ", res)
+ fmt.Println("res.Status == ", res.Status, " -> ", len(res.Status))
+ fmt.Fprint(c.ReadWriter.Out, res.Status)
+ return nil
+ }
+
+ case <- myctx.Done():
+ fmt.Fprint(c.ReadWriter.Out, "\nOTP verification timed out\n")
+ return nil
+
+ }
waitGroup.Wait()
- //}
return nil
}
--
GitLab