fix: add Microsoft 365 OAuth login#44
Conversation
|
Warning Review limit reached
More reviews will be available in 29 minutes and 32 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughAdds Microsoft 365 read-only OAuth: build-time credential injection, a PKCE-based msauth package (browser flow, manual URL, token exchange, Graph email fetch), command-layer M365 routing/handlers, token storage, and extensive tests. ChangesMicrosoft 365 OAuth Authentication
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Workit CLI
participant Browser
participant CallbackServer as Local Callback
participant MSGraph as Microsoft Graph
participant TokenStore as Secrets Store
User->>CLI: auth add --services m365 --readonly
CLI->>CLI: Generate PKCE state/verifier/challenge
CLI->>Browser: openBrowser(authURL)
Browser->>MSGraph: User authenticates and consents
MSGraph->>CallbackServer: POST /oauth2/callback with code
CallbackServer->>CLI: deliver code (validate state)
CLI->>MSGraph: exchange code (with PKCE verifier)
MSGraph-->>CLI: access_token + refresh_token
CLI->>MSGraph: FetchEmail (/me)
MSGraph-->>CLI: mail or userPrincipalName
CLI->>TokenStore: storeM365Token (microsoft_graph provider)
TokenStore-->>CLI: stored result (provider,email,services)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6060622f7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| u.Out().Printf("provider\tmicrosoft_graph") | ||
| u.Out().Printf("auth_url\t%s", result.URL) | ||
| u.Out().Printf("state\t%s", result.State) | ||
| return nil |
There was a problem hiding this comment.
When wk auth add <email> --services m365 --readonly --remote --step 1 takes this branch, it prints an authorization URL and exits immediately. That URL still uses redirect_uri=http://localhost:8085/oauth2/callback, but no local callback server is left running, and the step-2 path above rejects exchanging the returned redirect/code, so after the user completes Microsoft consent the redirect cannot be consumed and no token can be stored. Either keep a listener/exchange path alive for this mode or reject the remote option until it is implemented.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Microsoft 365 (M365) OAuth authentication support to the CLI, adding commands, configuration defaults, and an OAuth flow handler with a local callback server. Feedback on these changes highlights three key areas for improvement: first, AuthManageCmd.runM365 should fail closed when not requested with --print-url or JSON output, as it currently exits immediately without opening a browser or starting a callback server; second, the slice returned by msauth.PilotAllowedScopes() should be copied before sorting to prevent modifying shared package-level state; and third, the Windows browser-opening command should use cmd /c start instead of rundll32.exe to avoid enterprise security blocks and URL truncation issues with ampersands.
| scopes := msauth.PilotAllowedScopes() | ||
| sort.Strings(scopes) |
There was a problem hiding this comment.
Sorting the slice returned by msauth.PilotAllowedScopes() in-place can lead to unexpected side effects or race conditions if the underlying slice is a shared package-level variable in the msauth package. To prevent modifying shared state, copy the slice before sorting it.
| scopes := msauth.PilotAllowedScopes() | |
| sort.Strings(scopes) | |
| scopes := append([]string(nil), msauth.PilotAllowedScopes()...) | |
| sort.Strings(scopes) |
| case "windows": | ||
| cmd = exec.CommandContext(ctx, "rundll32", "url.dll,FileProtocolHandler", url) |
There was a problem hiding this comment.
Using rundll32.exe to open URLs on Windows has two major drawbacks:
- It is frequently blocked in enterprise environments (such as Hapvida) by security policies (AppLocker, WDAC) to prevent security bypasses.
rundll32 url.dll,FileProtocolHandlerhas known issues parsing URLs that contain ampersands (&), which can truncate the OAuth URL and break the flow.
Using cmd /c start is much more robust and less likely to be blocked by enterprise security policies.
| case "windows": | |
| cmd = exec.CommandContext(ctx, "rundll32", "url.dll,FileProtocolHandler", url) | |
| case "windows": | |
| cmd = exec.CommandContext(ctx, "cmd", "/c", "start", "", url) |
namastex888
force-pushed
the
fix/m365-oauth-login
branch
from
6060622 to
e83486d
Compare
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/msauth/open_browser.go (1)
11-11: 💤 Low valueConsider accepting a context parameter instead of using
context.Background().Using
context.Background()means the browser launch can't be cancelled if the parent operation times out. While the current design (fire-and-forget with error discarded) makes this low-impact, acceptingctx context.Contextas a parameter would be more idiomatic and future-proof.♻️ Proposed refactor
-func openBrowser(url string) error { - ctx := context.Background() +func openBrowser(ctx context.Context, url string) error {Then update the caller in
oauth.go:- _ = openBrowserFn(authURL) + _ = openBrowserFn(ctx, authURL)And the function variable declaration:
- openBrowserFn = openBrowser + openBrowserFn func(context.Context, string) error = openBrowser🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/msauth/open_browser.go` at line 11, The open_browser.go function should accept a context parameter instead of calling context.Background(); change the function signature (the openBrowser function) to take ctx context.Context, replace the local ctx := context.Background() with the passed ctx, and update any callers (e.g., in oauth.go where openBrowser is invoked and the openBrowser function variable declaration) to pass through the parent context (e.g., the ctx available in OAuth flow) so the browser launch can be cancelled by the caller.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cmd/m365_auth_dryrun_test.go`:
- Line 11: The test uses a real-looking email literal in the call to Execute
(the test function in internal/cmd/m365_auth_dryrun_test.go) — replace
"bernardo@hapvida.com.br" with an obviously synthetic address (for example
"pilot@example.com" or "user@example.test") in the Execute invocation and in any
other M365 test fixtures mentioned (same occurrence at lines referenced) so
tests no longer contain real user identifiers; update the string passed to
Execute([]string{...}) and any duplicate occurrences in related test files.
- Around line 24-34: In TestAuthAddM365RealFlowFailsClosedWithoutClientID, clear
both sources that resolveOAuthSettings checks by setting
config.DefaultM365ClientID = "" and clearing the environment variable
WK_M365_CLIENT_ID (e.g., os.Unsetenv("WK_M365_CLIENT_ID") or
os.Setenv("WK_M365_CLIENT_ID", "")) before calling Execute; then run the Execute
call that triggers the error check; optionally restore previous values after the
test to avoid side effects.
In `@internal/cmd/m365_auth_edges_test.go`:
- Around line 8-23: The test TestAuthAddM365RejectsUnsupportedModes is too loose
because it only checks for non-nil errors; change it to assert specific usage
error messages produced by runM365 for each unsupported mode. Update the
table-driven cases to include the expected error string for each args slice,
call Execute(args) as before, and instead of only checking err != nil, assert
that err is non-nil and that err.Error() (or the captured stderr) contains the
precise message indicating headless callback-server mode, raw --auth-code
rejection, and remote step 2 rejection; reference the Execute(...) call and
runM365/usage(...) behavior to craft the expected strings. Ensure the test fails
if the exact rejection message is not present.
In `@internal/msauth/oauth_more_test.go`:
- Around line 220-237: The test is racing with a hard-coded port and sleep;
change the code to use an ephemeral port from the actual listener instead of
DefaultLocalAuthPort and signal readiness instead of sleeping: in oauth.go (the
code that does net.Listen("tcp", fmt.Sprintf("localhost:%d",
DefaultLocalAuthPort)) and builds the redirectURI) accept/return the listener or
the resolved port (or make the port configurable) and construct the redirectURI
from listener.Addr().(*net.TCPAddr).Port; then update the test's openBrowserFn
in oauth_more_test.go to use the listener-derived redirect URL (not
"http://localhost:8085/...") and replace the fixed time.Sleep with a readiness
channel/notification that the server has started (e.g., notify after listener is
created and before serving) so the test triggers the callback only after the
server is listening.
---
Nitpick comments:
In `@internal/msauth/open_browser.go`:
- Line 11: The open_browser.go function should accept a context parameter
instead of calling context.Background(); change the function signature (the
openBrowser function) to take ctx context.Context, replace the local ctx :=
context.Background() with the passed ctx, and update any callers (e.g., in
oauth.go where openBrowser is invoked and the openBrowser function variable
declaration) to pass through the parent context (e.g., the ctx available in
OAuth flow) so the browser launch can be cancelled by the caller.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9610bfc6-8bae-47d6-a774-25f234c7263e
📒 Files selected for processing (13)
Makefileinternal/cmd/auth.gointernal/cmd/auth_m365.gointernal/cmd/m365_auth_dryrun_test.gointernal/cmd/m365_auth_edges_test.gointernal/cmd/m365_auth_more_test.gointernal/cmd/m365_auth_test.gointernal/cmd/testutil_test.gointernal/config/defaults.gointernal/msauth/oauth.gointernal/msauth/oauth_more_test.gointernal/msauth/oauth_test.gointernal/msauth/open_browser.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/config/defaults.go
- internal/cmd/auth.go
- internal/msauth/oauth_test.go
- internal/cmd/auth_m365.go
- internal/cmd/m365_auth_test.go
- internal/msauth/oauth.go
namastex888
force-pushed
the
fix/m365-oauth-login
branch
from
e83486d to
26d67c2
Compare
2 participants
Summary
Fixes #43 by adding a Microsoft 365 OAuth provider path for the read-only pilot. This keeps login simple for Hapvida employees: browser OAuth, not technical device-code/manual setup.
What changed
wk auth add <email> --services m365 --readonly.wk auth manage --services m365 --print-urlfor a clean OAuth handoff URL.m365client namespace, separate from Google tokens.offline_accessfor refresh token continuity:User.ReadMail.ReadCalendars.Read--readonlyfail-closed.WK_M365_CLIENT_ID/ build-timeDefaultM365ClientIDis missing.WK_M365_CLIENT_IDWK_M365_TENANT_IDSafety
m365, notdefault.Tests
auth add --services m365was undefined / unknown before this change.go test ./internal/msauth ./internal/cmd -run 'TestManualAuthURL|TestOAuthScopes|TestAuth(Add|Manage)M365' -count=1 -vmake lintmake deadcodego test ./...make fmt-checkafter commitDogfood without Bernardo's real account
Built local binary and verified:
m365 oauth client id missinglogin.microsoftonline.com/organizations/oauth2/v2.0/authorizecode_challenge_method=S256offline_access User.Read Mail.Read Calendars.ReadMail.Send Calendars.ReadWriteauth add ... --services m365 --readonly --remote --step 1emits Microsoft OAuth URL.auth add ... --services m365without--readonlyfails closed.Operational note
To be ready for Bernardo/Hapvida login, the deployed/internal Workit build or runtime environment needs an Entra app client id configured as
WK_M365_CLIENT_ID(and optionallyWK_M365_TENANT_ID; defaults toorganizations). Redirect URI should allow the loopback browser callback used by the CLI:Summary by CodeRabbit
New Features
Tests