Skip to content

Remove unused credential flows in e2e tests and examples#27508

Open
jzaffiro wants to merge 10 commits into
mainfrom
test/jzaffiro/simplifyTenantSetup
Open

Remove unused credential flows in e2e tests and examples#27508
jzaffiro wants to merge 10 commits into
mainfrom
test/jzaffiro/simplifyTenantSetup

Conversation

@jzaffiro

@jzaffiro jzaffiro commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Now that the new credential format has been implemented (#26714) and tested, remove the old formats from both the real service tests and the Fluid examples.

AB#74331

Copilot AI review requested due to automatic review settings June 8, 2026 17:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (712 lines, 14 files), I've queued these reviewers:

  • Correctness — logic errors, race conditions, lifecycle issues
  • Security — vulnerabilities, secret exposure, injection
  • API Compatibility — breaking changes, release tags, type design
  • Performance — algorithmic regressions, memory leaks
  • Testing — coverage gaps, hollow tests

How this works

  • Adjust the reviewer set by ticking/unticking boxes above. Reviewer toggles alone don't trigger anything.

  • Tick Start review below to dispatch the review fleet.

  • After review finishes, tick Start review again to request another run — it auto-resets after each dispatch.

  • This comment updates as new commits land; your reviewer selections are preserved.

  • Start review

username,
password,
};
return fetchTokens(server, scope, clientConfig, credentials);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is fetchTokens used still? (fine if so, but check please :))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer used - that and its caller (refreshTokens) have been removed.

@alexvy86 alexvy86 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to examples/ concern me a bit... without understanding them 100%, it seems like things are changing such that only Microsoft devs would be able to run examples targeting odsp? (Because running tenant-setup requires interaction with internal Microsoft systems). cc @ChumpChief for thoughts on that part since he refactored things recently-ish in that space.

Comment on lines +149 to +150
_server: string,
_clientConfig: IPublicClientConfig,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two aren't used at all anymore, right? Should they just be removed?

@jzaffiro

Copy link
Copy Markdown
Contributor Author

The changes to examples/ concern me a bit... without understanding them 100%, it seems like things are changing such that only Microsoft devs would be able to run examples targeting odsp? (Because running tenant-setup requires interaction with internal Microsoft systems). cc @ChumpChief for thoughts on that part since he refactored things recently-ish in that space.

I don't think this is much different than requiring a script to get an old tenant to target ODSP, which was the case before. Please correct me if I'm wrong though.

@ChumpChief

ChumpChief commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

The changes to examples/ concern me a bit... without understanding them 100%, it seems like things are changing such that only Microsoft devs would be able to run examples targeting odsp? (Because running tenant-setup requires interaction with internal Microsoft systems). cc @ChumpChief for thoughts on that part since he refactored things recently-ish in that space.

I don't think this is much different than requiring a script to get an old tenant to target ODSP, which was the case before. Please correct me if I'm wrong though.

That side of things is probably fine, afaict this is mostly just trading one MSFT-only approach for another as you say. However please make sure the errors and documentation are sufficient for MSFT devs to figure this out - based on my read here, I don't know where the appropriate package is or how to set it up.

const packageImportLocation = process.env.token__package__import__location;
if (packageImportLocation === undefined) {
throw new Error(
'The FIC credential flow relies on a test tenant checkout client, but no client was found. Ensure that the environment variable "token__package__import__location" is set to the location of a package that exports a compatible client.',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error and the other errors below are hard to understand to the average user who is not immersed in FIC world :)

Recommend only giving directives that a customer who hits this will know how to do (e.g. "run this command" or "see this documentation page on what to do").

Also nit that we use double-quotes.

Suggested change
'The FIC credential flow relies on a test tenant checkout client, but no client was found. Ensure that the environment variable "token__package__import__location" is set to the location of a package that exports a compatible client.',
"The FIC credential flow relies on a test tenant checkout client, but no client was found. Run my really cool command to provide it",

return usernames.map((username) => ficLoginCredentials(username, odspEndpointName));
}

const ficLoginCredentials = (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, prefer a verb name

Suggested change
const ficLoginCredentials = (
const getFicLoginCredentials = (

The following environment variables must be set:
If you're a Microsoft developer, use the `@ff-internal/tenant-setup` package to populate this variable for `start:spo`:

- local\_\_testing\_\_clientId

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChumpChief @alexvy86 is getkeys still in use? I'm pretty sure it's not and that the test tenant variables have been removed from the key vault but I'm not positive. If this is still a valid use case I'll revert this change.

@github-actions

Copy link
Copy Markdown
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> [email protected] serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  290922 links
    1934 destination URLs
    2184 URLs ignored
       0 warnings
       0 errors


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants