WIP: Implements accessToken as new authType. Closes #5816 #6268
Conversation
|
Cool stuff @martinlingstuyl! |
|
Not yet no, I'll need to fix that. Thanks for pointing it out... |
src/Auth.ts
Outdated
| if (debug) { | ||
| if (!accessToken) { | ||
| await logger.logToStderr(`No token found for resource ${resource}.`); | ||
| // If the authType is accessToken, we handle returning the token later on. |
There was a problem hiding this comment.
if authType is accessToken, shouldn't we just return the access token and be done with it?
There was a problem hiding this comment.
That's possible, but in that case there's some logic that is skipped, like calling the storeConnectionInfo function and setting the identityId parameters etc. That was the reason I wanted to keep things similar to how they are with the other auth methods.
There was a problem hiding this comment.
I've rewritten it slightly. Better like this? We still should not return it right away.
| @@ -199,46 +200,43 @@ export class Auth { | |||
| } | |||
|
|
|||
| public async ensureAccessToken(resource: string, logger: Logger, debug: boolean = false, fetchNew: boolean = false): Promise<string> { | |||
| const now: Date = new Date(); | |||
There was a problem hiding this comment.
What if the specified access token expired? I suggest that we still perform this logic as early as possible to catch errors as soon as possible
There was a problem hiding this comment.
I didn't remove the logic, it's still there. also for the new authType
| throw `No token found for resource ${resource}.`; | ||
| } | ||
|
|
||
| if (this.accessTokenExpired(accessToken)) { |
There was a problem hiding this comment.
We should handle this centrally for all tokens rather than for each auth type separately. The logic is the same and having it done early allows us to speed up auth flow and avoid repetition
There was a problem hiding this comment.
Well, yes, but in the case of this new authType we want slightly different behavior: I want to throw if it's expired for example. Do you have suggestions on how to do it differently?
src/m365/commands/login.ts
Outdated
| @@ -151,6 +197,88 @@ class LoginCommand extends Command { | |||
| await this.initAction(args, logger); | |||
| await this.commandAction(logger, args); | |||
| } | |||
|
|
|||
| private tokensForMultipleTenants(accessTokenValue: string | string[] | undefined, tenantValue: string | undefined): boolean { | |||
There was a problem hiding this comment.
Let's rename this command to make it clearer what it does. Right now, without looking at its implementation, it's hard to tell what it's doing
There was a problem hiding this comment.
Would you have a suggestions here?
There was a problem hiding this comment.
How about areTokensForMultipleTenants? Adding a verb makes it clearer what the function does
|
Ok, WIP 2. |
Closes #5816
Implements accessToken as new authType.
Ok, this is a work in progress, I haven't implemented tests yet, but I'd really like some opinion here.
I've implemented the basics, you can use one or more access tokens. It doesn't matter which ones you use.
Our logic normally checked if a token for the graph was available during login. For accessToken auth I'm working around that. It checks all tokens that you've passed, it doesn't depend on the graph. If you just use a SharePoint token, you'll be able to execute SharePoint commands, but not graph commands, and the other way around.
Try it out like:
or: