@W-20813610: Allow admins to enable OAuth but lock the site users can sign into to SITE_NAME #196
Conversation
| table td, | ||
| table th { | ||
| vertical-align: top; | ||
| } |
There was a problem hiding this comment.
I originally had added a big table to the OAuth docs (the one from the screenshot in the description) but I ended up removing it. Going to leave this change here though since by default table cells have a middle vertical alignment and they look weird.
| if (error === 'invalid_request') { | ||
| res.status(400).json({ | ||
| error: 'invalid_request', | ||
| error_description: `Invalid request. Did you sign into the wrong site? Expected: ${config.siteName || 'Default site'}`, | ||
| }); |
There was a problem hiding this comment.
This can happen when the user already has an active Tableau session in their browser for a site that is different than the one specified in the SITE_NAME config variable.
There was a problem hiding this comment.
Pull request overview
This pull request introduces a new OAUTH_LOCK_SITE configuration variable (default: true) to control whether users must sign in to the site specified in SITE_NAME when using OAuth authentication. This addresses issue #192 where users could inadvertently sign into a different site if they had an active Tableau session in their browser.
Key changes:
- New
OAUTH_LOCK_SITEconfig variable with default value oftrueto enforce site locking - Site validation logic in OAuth callback to verify users signed into the correct site
- URL manipulation in OAuth authorization to control site picker visibility based on lock setting
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| types/process-env.d.ts | Adds type definition for OAUTH_LOCK_SITE environment variable |
| src/config.ts | Implements lockSite configuration property with default value of true |
| src/config.test.ts | Adds test coverage for the new lockSite configuration option |
| src/server/oauth/callback.ts | Implements site validation logic and appropriate error handling for mismatched sites |
| src/server/oauth/authorize.ts | Adds getOAuthRedirectUrl helper function to control site picker behavior and sets redirected parameter when site is locked |
| src/scripts/createClaudeMcpBundleManifest.ts | Adds OAUTH_LOCK_SITE to the manifest configuration schema |
| docs/docs/configuration/mcp-config/oauth.md | Documents the new OAUTH_LOCK_SITE configuration option with clear usage guidelines |
| docs/src/css/custom.css | Minor CSS improvement for table cell vertical alignment |
| tests/oauth/testSetup.ts | Updates mock site name to mcp-test to align with test expectations |
| tests/oauth/authorizationCodeCallback.test.ts | Adds comprehensive test cases for site locking scenarios including enabled/disabled states and site mismatch |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (config.oauth.lockSite) { | ||
| // The "redirected" parameter is used by Tableau's OAuth controller to determine whether the user will be shown the site picker. | ||
| // When provided, the user will not be shown the site picker. | ||
| oauthUrl.searchParams.set('redirected', 'true'); |
There was a problem hiding this comment.
If you are internal, you can see how the redirected parameter gets used here: https://github.com/sf-analyticscloud/monolith/blob/main/workgroup/src/silos/tableau-server/services/war-vizportal/src/com/tableausoftware/api/oauth/OAuthController.java#L194
There was a problem hiding this comment.
is it just the presence of the field value? redirected: true kind of feels like the user would be redirected to the site picker. (unless im misinterpreting what redirected means)
There was a problem hiding this comment.
It needs to have some value, what the value is doesn't matter. The comments in the controller don't make it terribly clear exactly what "redirected" mean, but it doesn't mean "redirected to the site picker". When a value for "redirected" is not provided the comments say: "it means that the request is from the client side" but the end result is what we want. I think "client" refers to Tableau Desktop in this case. The PR also unfortunately has no description. https://github.com/sf-analyticscloud/monolith/pull/30596. I reached out to the author, but he's left the team.
|
|
||
| async function getOAuthRedirectUrl( | ||
| initialOAuthUrl: URL, | ||
| { lockSite }: { lockSite: boolean }, |
There was a problem hiding this comment.
cant this param just be a bool instead of an object
There was a problem hiding this comment.
Yeah, but I generally prefer objects for config args since it acts as a way to provide "named" arguments.
getOAuthRedirectUrl(url, true);
vs
getOAuthRedirectUrl(url, { lockSite: true } );
| // The response is a redirect to the Tableau OAuth login page. | ||
| // Force it to ultimately show the site picker by changing the path from #/signin to #/site. | ||
| const location = response.headers.get('location'); | ||
| if (location?.startsWith('#/signin') || location?.startsWith('/#/signin')) { |
There was a problem hiding this comment.
nit: is this cleaner
location?.includes('#/signin')
There was a problem hiding this comment.
I wanted to avoid the possibility of a false positive when #/signin appears somewhere else in the string. The reason why there are 2 parts to the condition is because the first matches on Server and the second matches on Cloud.
These changes introduce a new config variable
OAUTH_LOCK_SITE(default: true) which ensures that when users connect their agent, they sign in to the site specified in theSITE_NAMEconfig variable.This addresses #192 where users are able to sign in to some other site if they already happened to have an active Tableau session in their browser when connecting their agent.
The one major gotcha is that on Server, when site locking is enabled and the user already has an active Tableau session in their browser for some other site, then they will get an error that says
Invalid request. Did you sign into the wrong site? Expected: [SITE_NAME]. The user will need to sign out in their browser and reattempt to connect their agent.Here is the behavior matrix I compiled when doing my testing. I did test against Tableau Cloud but the oddities can be ignored since OAuth can't currently be used against Tableau Cloud outside of local testing.