fix(ui): key Microsoft login scopes by node protocol so the broker requests Graph scopes (#2373 to stage) - #2374
dylan-savage wants to merge 1 commit into
Conversation
…quests Graph scopes LoginWithMicrosoftButton looked up SERVICE_TIER_SCOPES[formContext.provider], but formContext.provider is the node protocol name (tool_excel, tool_word, tool_onedrive, tool_outlook_mail, tool_outlook_calendar) while the map was keyed excel/word/onedrive/outlook_mail/outlook_calendar. Every lookup missed, so no scope param was sent, the broker requested identity only, and the node rejected the token (e.g. "Missing: Files.ReadWrite"). Rekey the map by protocol name (scope values unchanged; they mirror core/microsoft_access.py) and hoist it to an exported module constant. Add a test that reads every tool_microsoft_365 services.*.json and asserts the map has non-empty scopes for each protocol and each access tier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: rocketride-org/rocketride-server/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Thanks @dylan-savage - good catch, and the test is the right shape: it reads the real services.*.json from the node, it fails on the old keys, and it will fail again the day a new Microsoft 365 service ships without scopes. I checked all five protocols against nodes/src/nodes/core/microsoft_access.py and the map mirrors the AccessSpecs exactly, User.ReadBasic.All included.
Approve
Two non-blocking notes inline; neither needs to hold the merge. This is the identical change as #2373 against stage - both files match byte for byte, so the same notes apply there.
One item has no file or line, so it goes here. The description says existing users must reconnect after this ships, and I cannot find a way for them to do that in the product. The button is disabled={authenticated} (LoginWithMicrosoftButton.tsx:178), and microsoft.userToken is "ui:disabled": true in nodes/src/nodes/core/services.common.microsoft.json, so somebody holding a pre-fix token can neither press Login again nor clear the token. Deleting and recreating the node looks like the only route. Could you confirm? If that is the answer it belongs in the release note, because their tools keep failing until they do it.
| for (const { file, tiers } of services) assert.ok(tiers.length > 0, `${file} has no <prefix>.access enum`); | ||
| }); | ||
|
|
||
| test('scope map is keyed by node protocol with scopes for every access tier', () => { |
There was a problem hiding this comment.
should fix - the same gap is open on the Google button, for four services out of five.
LoginWithGoogleButton.tsx still keeps its SERVICE_TIER_SCOPES inline inside handleHybridSignIn, and that map has exactly one key: tool_gmail.
nodes/src/nodes/tool_google_workspace/ ships five services. Every one of them has an access enum and renders GoogleButtonWidget under google.authType: user:
| services file | protocol | in the map |
|---|---|---|
services.gmail.json |
tool_gmail |
yes |
services.calendar.json |
tool_calendar |
no |
services.docs.json |
tool_docs |
no |
services.drive.json |
tool_drive |
no |
services.sheets.json |
tool_sheets |
no |
So four of them send no scope= and fall back to what the comment calls the broker's legacy default consent - the same branch that turned out to grant identity scopes only on the Microsoft side, which is the bug this PR is fixing. Whether Google's legacy default happens to cover those four is the question, and it is worth answering rather than assuming: that assumption is what shipped here.
The cheap move is to export the Google map the way you exported this one, and run this test over both - reading services.*.json from tool_google_workspace for the Google half. If the gap is real the test says so immediately. If the broker's default does cover them, a comment saying so stops the next person asking.
| * offline_access + identity scopes are appended by the broker, matching the | ||
| * Google flow. | ||
| */ | ||
| export const SERVICE_TIER_SCOPES: Record<string, Record<string, string[]>> = { |
There was a problem hiding this comment.
nit - the test pins the keys, the scope values can still drift.
This test proves every provider and every tier has an entry. It does not check which scopes, and these values are a hand-kept copy of the AccessSpecs in nodes/src/nodes/core/microsoft_access.py. They match today - I compared all five, including the User.ReadBasic.All that only the widget requests.
If someone raises a tier on the Python side, say word.write to Files.ReadWrite.All, this map keeps requesting the old scope and the symptom is the one you just fixed: consent succeeds, the tool fails at invoke time with "Missing: ...". Nothing goes red on the way there.
How far to take this is your call, and it is a design question rather than a change to make in this PR. The cheap version is a pointer comment in microsoft_access.py saying a second copy lives in this file. The real version is one source that both sides read.
2 participants
Summary
excel,word,onedrive,outlook_mail,outlook_calendar, butformContext.provideris the node protocol name (tool_excel, …). The lookup always missed, so noscope=was sent and the broker requested identity scopes only (openid email profile offline_access).authorization is missing required scopes … Missing: Files.ReadWrite.SERVICE_TIER_SCOPESby protocol name (matchingLoginWithGoogleButton'stool_gmail). Scope values are unchanged. The map moves to module scope (exported) so it can be tested.Type
fix (stage port)
Testing
LoginWithMicrosoftButton.test.tsreads everytool_microsoft_365/services.*.jsonand asserts the map has non-empty scopes for each protocol and every<prefix>.accesstier. It fails on the old keys (no entry for provider 'tool_excel').scope=Files.ReadWrite. A hand-built broker URL with that scope got the Files permissions on Microsoft's consent screen, where before only identity permissions were requested../builder testnot run locally; GitHub CI is the repository-wide gateExisting users must reconnect after this ships. Tokens issued before the fix carry no Graph scopes.
Checklist
Paired PR
Develop: #2373
Release Coordination
Merge with the release owner so the SaaS engine pin advances in the same release window (current saas pin b7764b8 has this bug).
Linked Issue
No issue filed. Found during staging release testing.
🤖 Generated with Claude Code