Adds multiple account functionality. Closes #3587 #5576

martinlingstuyl · 2023-10-18T08:27:50Z

Closes #3587

🚀 Adds multiple account functionality 🚀

Time for some extra juice, peeps 😊

Added logic to allow to login to multiple identities
Added a command to list available identities
Added a command to switch to another identity
Added options to logout from a specific identity

What needs to be done in another iteration:

Add prompt functionality to select an identity when using m365 identity set without options.
Add functionality to use a specific identity on just a single command execution.

Points of attention

I need to test if Managed Identity still works. That's the only one I haven't tested yet.
The discovered SharePoint root url...I had forgotten its implications. I've fixed that now, but it shows how important it is to keep a good lookout for potential issues.

Graceful fallback I've changed the shape of the data that's saved to the .cli-m365-tokens.json file. This calls for a graceful fallback to older versions of that file. I've implemented that even if I'm not entirely sure what the best route is here. Ideas appreciated.

Specifically I've added:

Name	Explanation
`identityName`	The name of the identity, can be a userName or application name
`identityId`	The localAccountId of the associated account or just the objectId of the AAD principal. Is necessary to locate the identity and find it in the MSAL store.
`availableIdentities`	A complete list of all identities, including the one that's activated.

I've also added the properties identityId and identityName to the output of commands like m365 status, m365 login, m365 identity list and m365 identity set.

...or should we rename identityName to name and identityId to id here?
Deprecated connectedAs: As I've added identityName to the output of m365 status and m365 login. This now means connectedAs and identityName have the same value. connectedAs should be deprecated and removed in the next major version.

waldekmastykarz

Nice start. Let's clean this up a little. Something I noticed across the board:

let's avoid using ! to suppress type checking. If something is optional, let's check that it's indeed set. If it's not, let update the type. Using ! exposes us to runtime exceptions for which we use TypeScript to avoid
let's stick to existing functionality where possible (returned data, exception handling, etc)
let's try to use clear name that communicate the purpose of variables, types and methods. Where it's not always possible, let's use comments.

src/m365/commands/login.ts

waldekmastykarz · 2023-11-11T08:35:54Z

src/m365/spo/commands/page/page-add.spec.ts

@@ -368,7 +368,18 @@ describe(commands.PAGE_ADD, () => {

      throw 'Invalid request';
    });
-
+    sinon.stub(Cli, 'executeCommand').callsFake(async (command): Promise<any> => {


How are these changes related to the PR?

I don't remember why, I'll recheck. I believe some tests started failing for some reason.

waldekmastykarz · 2023-11-11T08:36:00Z

src/m365/spo/commands/page/page-set.spec.ts

@@ -209,7 +209,18 @@ describe(commands.PAGE_SET, () => {

      throw 'Invalid request';
    });
-
+    sinon.stub(Cli, 'executeCommandWithOutput').callsFake(async (command): Promise<any> => {


How are these changes related to the PR?

waldekmastykarz · 2023-11-11T08:36:12Z

src/m365/spo/commands/site/site-ensure.spec.ts

@@ -191,10 +191,4 @@ describe(commands.SITE_ENSURE, () => {

    await assert.rejects(command.action(logger, { options: { url: 'https://contoso.sharepoint.com/sites/team1', title: 'Team 1', type: 'CommunicationSite' } } as any));
  });
-


How are these changes related to the PR?

src/Auth.ts

waldekmastykarz · 2023-11-11T13:25:25Z

src/m365/identity/commands/identity-set.ts

+    await logger.log(auth.getIdentityDetails(auth.service, this.debug));
+  }
+
+  public async action(logger: Logger, args: CommandArgs): Promise<void> {


Is there a reason why we're overriding the base action?

src/m365/identity/commands/identity-set.ts

waldekmastykarz · 2023-11-11T13:26:21Z

src/m365/spo/commands/file/file-get.spec.ts

@@ -441,6 +441,25 @@ describe(commands.FILE_GET, () => {
    assert.strictEqual(getStub.lastCall.args[0].url, `https://contoso.sharepoint.com/_api/web/GetFileByServerRelativePath(DecodedUrl=@f)?@f='%2FDocuments%2FTest1.docx'`);
  });

+  it('uses correct API url when tenant root URL option is passed in combination with asListItem', async () => {


How are these changes related to the PR?

src/m365/spo/commands/spo-set.ts

src/utils/spo.ts

src/Auth.ts

martinlingstuyl · 2023-11-11T18:36:21Z

Thanks @waldekmastykarz! I'll go through your feedback.

Using ! By the way is the same as casting variables as a type, which we also do a lot or in the codebase.

I agree with you in principle, though I typically only use it in situations where I've already done null-checks.

Anyway, it's a tricky thing that's true.

waldekmastykarz · 2023-11-13T07:40:29Z

Using ! By the way is the same as casting variables as a type, which we also do a lot or in the codebase.

Seems like something we should definitely pay attention to because it's circumventing design time typesafety and is exposing us to runtime errors.

I agree with you in principle, though I typically only use it in situations where I've already done null-checks.

If the types are null-checked, shouldn't TypeScript see the assertion and handle it correctly without requiring you to suppress the null-type?

martinlingstuyl · 2023-11-13T07:52:24Z

If the types are null-checked, shouldn't TypeScript see the assertion and handle it correctly without requiring you to suppress the null-type?

It should, but I've seen situations where it doesn't. As soon as i find a nice example i'll show you.

waldekmastykarz · 2023-11-13T11:27:47Z

It should, but I've seen situations where it doesn't. As soon as i find a nice example i'll show you.

It could happen when we check it in a parent method and then pass the value to a child which loses track of it. This is dangerous though because in the child we're making an assumption about how it's called, which could be a signal that we should consider refactoring the code and perhaps destructure the object to specific properties to make fewer assumptions

waldekmastykarz · 2023-11-17T07:34:51Z

docs/docs/cmd/identity/identity-list.mdx

+  <TabItem value="Text">
+
+  ```text
+  identityName                           authType


Should we also show here which one is active?

src/Auth.ts

waldekmastykarz · 2023-11-17T08:55:53Z

tl;dr:

rename with alias login to connect
rename with alias logout to disconnect
extend connect with identityId and identityName
extend disconnect with identityId and identityName
extend status with allConnections property to keep output backwards compatible
change status text output to list all connections, each with name, id, tenant id, tenant name, active (true|false)
introduce extra store for all connections
all connection is not a part of restoreAuth, is only deserialized when (dis)connecting or after access/refresh tokens updated on the current connection and need to be persisted. same with SPO URL
rename the current connection store file
drop all identity commands
rename Service class to Connection
drop Identity interface
drop AuthenticationResult interface, always resolve identityId from token

martinlingstuyl · 2023-11-17T22:12:30Z

Found some time and refactored the auth class. The rest to be done later.

martinlingstuyl · 2024-01-23T08:48:59Z

Abandoned because of a new PR #5790

martinlingstuyl force-pushed the multiple-accounts branch 4 times, most recently from d4a8bee to 2e34ede Compare October 26, 2023 07:27

martinlingstuyl force-pushed the multiple-accounts branch from 2e34ede to 7b32c69 Compare November 1, 2023 13:53

waldekmastykarz self-assigned this Nov 10, 2023

waldekmastykarz requested changes Nov 11, 2023

View reviewed changes

waldekmastykarz reviewed Nov 11, 2023

View reviewed changes

src/Auth.ts Outdated Show resolved Hide resolved

waldekmastykarz marked this pull request as draft November 11, 2023 13:32