Skip to content

Commit 668e6ac

Browse files
automagik-genieautomagik-genie
committed
fix: harden m365 oauth pr feedback
1 parent 26d67c2 commit 668e6ac

9 files changed

Lines changed: 138 additions & 78 deletions

‎internal/cmd/auth_m365.go‎

Lines changed: 7 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -42,26 +42,8 @@ func (c *AuthAddCmd) runM365(ctx context.Context, flags *RootFlags, u *ui.UI) er
4242
if c.Step != 0 && !c.Remote {
4343
return usage("--step requires --remote")
4444
}
45-
if c.Remote || c.Step == 1 {
46-
if c.Step == 2 || c.AuthURL != "" {
47-
return usage("m365 remote step 2 is not supported yet; use browser OAuth on this machine")
48-
}
49-
result, err := m365ManualAuthURL(ctx, msauth.ManualAuthURLOptions{Readonly: c.Readonly, ForceConsent: c.ForceConsent})
50-
if err != nil {
51-
return err
52-
}
53-
if outfmt.IsJSON(ctx) {
54-
return outfmt.WriteJSON(ctx, os.Stdout, map[string]any{
55-
"provider": "microsoft_graph",
56-
"auth_url": result.URL,
57-
"state": result.State,
58-
"expires_in": result.ExpiresIn,
59-
})
60-
}
61-
u.Out().Printf("provider\tmicrosoft_graph")
62-
u.Out().Printf("auth_url\t%s", result.URL)
63-
u.Out().Printf("state\t%s", result.State)
64-
return nil
45+
if c.Remote || c.Step != 0 || c.AuthURL != "" {
46+
return usage("m365 remote auth is not supported yet; use browser OAuth on this machine")
6547
}
6648
if dryRunErr := dryRunExit(ctx, flags, "auth.add.m365", map[string]any{
6749
"email": strings.TrimSpace(c.Email),
@@ -91,6 +73,10 @@ func (c *AuthAddCmd) runM365(ctx context.Context, flags *RootFlags, u *ui.UI) er
9173
}
9274

9375
func (c *AuthManageCmd) runM365(ctx context.Context) error {
76+
if !outfmt.IsJSON(ctx) && !c.PrintURL {
77+
return usage("m365 auth manage requires --print-url")
78+
}
79+
9480
result, err := m365ManualAuthURL(ctx, msauth.ManualAuthURLOptions{Readonly: true, ForceConsent: c.ForceConsent})
9581
if err != nil {
9682
return err
@@ -103,9 +89,6 @@ func (c *AuthManageCmd) runM365(ctx context.Context) error {
10389
"expires_in": result.ExpiresIn,
10490
})
10591
}
106-
u := ui.FromContext(ctx)
107-
u.Err().Println("Opening Microsoft 365 authorization in your browser…")
108-
u.Err().Println(result.URL)
10992
return nil
11093
}
11194

@@ -115,7 +98,7 @@ func storeM365Token(ctx context.Context, u *ui.UI, email string, refreshToken st
11598
return err
11699
}
117100
serviceNames := []string{"m365"}
118-
scopes := msauth.PilotAllowedScopes()
101+
scopes := append([]string(nil), msauth.PilotAllowedScopes()...)
119102
sort.Strings(scopes)
120103
if err := store.MergeToken(msauth.ClientName, email, secrets.Token{
121104
Client: msauth.ClientName,

‎internal/cmd/m365_auth_dryrun_test.go‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,17 @@
11
package cmd
22

33
import (
4+
"os"
45
"strings"
56
"testing"
7+
8+
"github.com/automagik-dev/workit/internal/config"
69
)
710

811
func TestAuthAddM365DryRunReportsPilotScopes(t *testing.T) {
912
out := captureStdout(t, func() {
1013
_ = captureStderr(t, func() {
11-
if err := Execute([]string{"--json", "--dry-run", "auth", "add", "bernardo@hapvida.com.br", "--services", "m365", "--readonly"}); err != nil {
14+
if err := Execute([]string{"--json", "--dry-run", "auth", "add", "pilot@example.com", "--services", "m365", "--readonly"}); err != nil {
1215
t.Fatalf("dry-run m365 auth: %v", err)
1316
}
1417
})
@@ -22,8 +25,21 @@ func TestAuthAddM365DryRunReportsPilotScopes(t *testing.T) {
2225
}
2326

2427
func TestAuthAddM365RealFlowFailsClosedWithoutClientID(t *testing.T) {
28+
origClientID := config.DefaultM365ClientID
29+
origEnv, hadEnv := os.LookupEnv("WK_M365_CLIENT_ID")
30+
t.Cleanup(func() {
31+
config.DefaultM365ClientID = origClientID
32+
if hadEnv {
33+
_ = os.Setenv("WK_M365_CLIENT_ID", origEnv)
34+
} else {
35+
_ = os.Unsetenv("WK_M365_CLIENT_ID")
36+
}
37+
})
38+
config.DefaultM365ClientID = ""
39+
_ = os.Unsetenv("WK_M365_CLIENT_ID")
40+
2541
_ = captureStderr(t, func() {
26-
err := Execute([]string{"--json", "auth", "add", "bernardo@hapvida.com.br", "--services", "m365", "--readonly"})
42+
err := Execute([]string{"--json", "auth", "add", "pilot@example.com", "--services", "m365", "--readonly"})
2743
if err == nil {
2844
t.Fatal("expected missing m365 client id")
2945
}

‎internal/cmd/m365_auth_edges_test.go‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,25 +6,31 @@ import (
66
)
77

88
func TestAuthAddM365RejectsUnsupportedModes(t *testing.T) {
9-
tests := [][]string{
10-
{"--json", "auth", "add", "bernardo@hapvida.com.br", "--services", "m365", "--readonly", "--headless"},
11-
{"--json", "auth", "add", "bernardo@hapvida.com.br", "--services", "m365", "--readonly", "--auth-code", "raw"},
12-
{"--json", "auth", "add", "bernardo@hapvida.com.br", "--services", "m365", "--readonly", "--remote", "--step", "2"},
9+
tests := []struct {
10+
args []string
11+
want string
12+
}{
13+
{[]string{"--json", "auth", "add", "pilot@example.com", "--services", "m365", "--readonly", "--headless"}, "headless callback-server mode is not supported yet"},
14+
{[]string{"--json", "auth", "add", "pilot@example.com", "--services", "m365", "--readonly", "--auth-code", "raw"}, "does not accept raw --auth-code"},
15+
{[]string{"--json", "auth", "add", "pilot@example.com", "--services", "m365", "--readonly", "--remote", "--step", "2"}, "remote auth is not supported yet"},
1316
}
1417

15-
for _, args := range tests {
18+
for _, tc := range tests {
1619
_ = captureStderr(t, func() {
17-
err := Execute(args)
20+
err := Execute(tc.args)
1821
if err == nil {
19-
t.Fatalf("expected error for %#v", args)
22+
t.Fatalf("expected error for %#v", tc.args)
23+
}
24+
if !strings.Contains(err.Error(), tc.want) {
25+
t.Fatalf("expected %q for %#v, got %v", tc.want, tc.args, err)
2026
}
2127
})
2228
}
2329
}
2430

2531
func TestAuthAddMixedM365AndGoogleFailsClosed(t *testing.T) {
2632
_ = captureStderr(t, func() {
27-
err := Execute([]string{"--json", "auth", "add", "bernardo@hapvida.com.br", "--services", "m365,gmail", "--readonly"})
33+
err := Execute([]string{"--json", "auth", "add", "pilot@example.com", "--services", "m365,gmail", "--readonly"})
2834
if err == nil {
2935
t.Fatal("expected mixed m365/google services to fail closed")
3036
}

‎internal/cmd/m365_auth_more_test.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package cmd
22

33
import (
44
"context"
5+
"strings"
56
"testing"
67

78
"github.com/automagik-dev/workit/internal/msauth"
@@ -29,3 +30,15 @@ func TestAuthManageM365PrintURLPropagatesForceConsent(t *testing.T) {
2930
t.Fatalf("options = %#v", got)
3031
}
3132
}
33+
34+
func TestAuthManageM365RequiresPrintURLForTextMode(t *testing.T) {
35+
_ = captureStderr(t, func() {
36+
err := Execute([]string{"auth", "manage", "--services", "m365"})
37+
if err == nil {
38+
t.Fatal("expected m365 manage text mode to fail closed")
39+
}
40+
if !strings.Contains(err.Error(), "requires --print-url") {
41+
t.Fatalf("unexpected error: %v", err)
42+
}
43+
})
44+
}

‎internal/cmd/m365_auth_test.go‎

Lines changed: 13 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,12 @@ func TestAuthAddM365UsesOAuthAndStoresToken(t *testing.T) {
2525
openSecretsStore = func() (secrets.Store, error) { return store, nil }
2626
ensureKeychainAccess = func() error { return nil }
2727
authorizeM365 = func(context.Context, msauth.AuthorizeOptions) (msauth.AuthorizeResult, error) {
28-
return msauth.AuthorizeResult{Email: "bernardo@hapvida.com.br", RefreshToken: "m365-refresh-token"}, nil
28+
return msauth.AuthorizeResult{Email: "pilot@example.com", RefreshToken: "m365-refresh-token"}, nil
2929
}
3030

3131
out := captureStdout(t, func() {
3232
_ = captureStderr(t, func() {
33-
if err := Execute([]string{"--json", "auth", "add", "bernardo@hapvida.com.br", "--services", "m365", "--readonly"}); err != nil {
33+
if err := Execute([]string{"--json", "auth", "add", "pilot@example.com", "--services", "m365", "--readonly"}); err != nil {
3434
t.Fatalf("auth add m365: %v", err)
3535
}
3636
})
@@ -43,7 +43,7 @@ func TestAuthAddM365UsesOAuthAndStoresToken(t *testing.T) {
4343
if payload["provider"] != "microsoft_graph" || payload["stored"] != true {
4444
t.Fatalf("unexpected output: %#v", payload)
4545
}
46-
tok, err := store.GetToken(msauth.ClientName, "bernardo@hapvida.com.br")
46+
tok, err := store.GetToken(msauth.ClientName, "pilot@example.com")
4747
if err != nil {
4848
t.Fatalf("stored m365 token: %v", err)
4949
}
@@ -57,7 +57,7 @@ func TestAuthAddM365UsesOAuthAndStoresToken(t *testing.T) {
5757

5858
func TestAuthAddM365RequiresReadonly(t *testing.T) {
5959
_ = captureStderr(t, func() {
60-
err := Execute([]string{"--json", "auth", "add", "bernardo@hapvida.com.br", "--services", "m365"})
60+
err := Execute([]string{"--json", "auth", "add", "pilot@example.com", "--services", "m365"})
6161
if err == nil {
6262
t.Fatal("expected missing --readonly to fail closed")
6363
}
@@ -67,23 +67,16 @@ func TestAuthAddM365RequiresReadonly(t *testing.T) {
6767
})
6868
}
6969

70-
func TestAuthAddM365RemoteStepOnePrintsMicrosoftURL(t *testing.T) {
71-
origURL := m365ManualAuthURL
72-
t.Cleanup(func() { m365ManualAuthURL = origURL })
73-
m365ManualAuthURL = func(context.Context, msauth.ManualAuthURLOptions) (msauth.ManualAuthURLResult, error) {
74-
return msauth.ManualAuthURLResult{URL: "https://login.microsoftonline.com/organizations/oauth2/v2.0/authorize?client_id=test", State: "state"}, nil
75-
}
76-
77-
out := captureStdout(t, func() {
78-
_ = captureStderr(t, func() {
79-
if err := Execute([]string{"--json", "auth", "add", "bernardo@hapvida.com.br", "--services", "m365", "--readonly", "--remote", "--step", "1"}); err != nil {
80-
t.Fatalf("remote step 1: %v", err)
81-
}
82-
})
70+
func TestAuthAddM365RemoteModeFailsClosed(t *testing.T) {
71+
_ = captureStderr(t, func() {
72+
err := Execute([]string{"--json", "auth", "add", "pilot@example.com", "--services", "m365", "--readonly", "--remote", "--step", "1"})
73+
if err == nil {
74+
t.Fatal("expected remote m365 auth to fail closed")
75+
}
76+
if !strings.Contains(err.Error(), "remote auth is not supported yet") {
77+
t.Fatalf("unexpected error: %v", err)
78+
}
8379
})
84-
if !strings.Contains(out, "login.microsoftonline.com") || !strings.Contains(out, "auth_url") {
85-
t.Fatalf("unexpected output: %s", out)
86-
}
8780
}
8881

8982
func TestAuthManageM365PrintURLIsNonTechnicalOAuthHandoff(t *testing.T) {

‎internal/msauth/oauth.go‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ const (
2424
DefaultLocalAuthPort = 8085
2525
)
2626

27+
var localAuthPort = DefaultLocalAuthPort
28+
2729
var (
2830
ErrMissingClientID = errors.New("m365 oauth client id missing")
2931
ErrMissingScopes = errors.New("m365 oauth scopes missing")
@@ -34,10 +36,13 @@ var (
3436
ErrProfileStatus = errors.New("fetch m365 profile status error")
3537
ErrProfileMissingEmail = errors.New("m365 profile missing email")
3638
ErrContextDone = errors.New("m365 oauth context done")
37-
openBrowserFn = openBrowser
38-
randomStateFn = randomURLToken
39-
oauthConfigFn = oauthConfig
40-
graphMeURL = "https://graph.microsoft.com/v1.0/me"
39+
)
40+
41+
var (
42+
openBrowserFn func(context.Context, string) error = openBrowser
43+
randomStateFn = randomURLToken
44+
oauthConfigFn = oauthConfig
45+
graphMeURL = "https://graph.microsoft.com/v1.0/me"
4146
)
4247

4348
type AuthorizeOptions struct {
@@ -93,14 +98,14 @@ func Authorize(ctx context.Context, opts AuthorizeOptions) (AuthorizeResult, err
9398
return AuthorizeResult{}, err
9499
}
95100

96-
ln, err := (&net.ListenConfig{}).Listen(ctx, "tcp", fmt.Sprintf("localhost:%d", DefaultLocalAuthPort))
101+
ln, err := (&net.ListenConfig{}).Listen(ctx, "tcp", fmt.Sprintf("localhost:%d", localAuthPort))
97102
if err != nil {
98-
return AuthorizeResult{}, fmt.Errorf("listen for m365 callback on port %d: %w", DefaultLocalAuthPort, err)
103+
return AuthorizeResult{}, fmt.Errorf("listen for m365 callback on port %d: %w", localAuthPort, err)
99104
}
100105

101106
defer func() { _ = ln.Close() }()
102107

103-
redirectURI := fmt.Sprintf("http://localhost:%d/oauth2/callback", DefaultLocalAuthPort)
108+
redirectURI := fmt.Sprintf("http://localhost:%d/oauth2/callback", localAuthPort)
104109
cfg := oauthConfigFn(settings, redirectURI, scopes)
105110
codeCh := make(chan string, 1)
106111
errCh := make(chan error, 1)
@@ -125,7 +130,7 @@ func Authorize(ctx context.Context, opts AuthorizeOptions) (AuthorizeResult, err
125130
fmt.Fprintln(os.Stderr, "Opening browser for Microsoft 365 authorization…")
126131
fmt.Fprintln(os.Stderr, "If the browser doesn't open, visit this URL:")
127132
fmt.Fprintln(os.Stderr, authURL)
128-
_ = openBrowserFn(authURL)
133+
_ = openBrowserFn(ctx, authURL)
129134

130135
select {
131136
case code := <-codeCh:
@@ -153,7 +158,7 @@ func ManualAuthURL(_ context.Context, opts ManualAuthURLOptions) (ManualAuthURLR
153158
return ManualAuthURLResult{}, err
154159
}
155160

156-
redirectURI := fmt.Sprintf("http://localhost:%d/oauth2/callback", DefaultLocalAuthPort)
161+
redirectURI := fmt.Sprintf("http://localhost:%d/oauth2/callback", localAuthPort)
157162
cfg := oauthConfigFn(settings, redirectURI, scopes)
158163
url := cfg.AuthCodeURL(state, authParams(opts.ForceConsent, challenge)...)
159164

0 commit comments

Comments
 (0)