Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Implements accessToken as new authType. Closes #5816 #6268

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

WIP: Implements accessToken as new authType. Closes #5816 #6268

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96b4404
Implements accessToken as new authType. Closes #5816
martinlingstuyl Aug 22, 2024
0535ec8
Resolve comments
martinlingstuyl Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 70 additions & 40 deletions src/Auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,8 @@ export enum AuthType {
Certificate,
Identity,
Browser,
Secret
Secret,
AccessToken
}

export enum CertificateType {
Expand Down Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't remove the logic, it's still there. also for the new authType

const accessToken: AccessToken | undefined = this.connection.accessTokens[resource];
const expiresOn: Date = accessToken && accessToken.expiresOn ?
// if expiresOn is serialized from the service file, it's set as a string
// if it's coming from MSAL, it's a Date
typeof accessToken.expiresOn === 'string' ? new Date(accessToken.expiresOn) : accessToken.expiresOn
: new Date(0);
let getTokenPromise: ((resource: string, logger: Logger, debug: boolean, fetchNew: boolean) => Promise<AccessToken | null>) | undefined;

if (!fetchNew && accessToken && expiresOn > now) {
if (debug) {
await logger.logToStderr(`Existing access token ${accessToken.accessToken} still valid. Returning...`);
}
return accessToken.accessToken;
}
else {
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.
Copy link
Member

Choose a reason for hiding this comment

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

if authType is accessToken, shouldn't we just return the access token and be done with it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rewritten it slightly. Better like this? We still should not return it right away.

if (this.connection.authType !== AuthType.AccessToken) {
const accessToken: AccessToken | undefined = this.connection.accessTokens[resource];

if (!fetchNew && accessToken && !this.accessTokenExpired(accessToken)) {
if (debug) {
await logger.logToStderr(`Existing access token ${accessToken.accessToken} still valid. Returning...`);
}
else {
await logger.logToStderr(`Access token expired. Token: ${accessToken.accessToken}, ExpiresAt: ${accessToken.expiresOn}`);
return accessToken.accessToken;
}
else {
if (debug) {
if (!accessToken) {
await logger.logToStderr(`No token found for resource ${resource}.`);
}
else {
await logger.logToStderr(`Access token expired. Token: ${accessToken.accessToken}, ExpiresAt: ${accessToken.expiresOn}`);
}
}
}
}

let getTokenPromise: ((resource: string, logger: Logger, debug: boolean, fetchNew: boolean) => Promise<AccessToken | null>) | undefined;

// When using an application identity, you can't retrieve the access token silently, because there is
// no account. Also (for cert auth) clientApplication is instantiated later
// after inspecting the specified cert and calculating thumbprint if one
// wasn't specified
if (this.connection.authType !== AuthType.Certificate &&
this.connection.authType !== AuthType.Secret &&
this.connection.authType !== AuthType.Identity) {
this.clientApplication = await this.getPublicClient(logger, debug);
if (this.clientApplication) {
const accounts = await this.clientApplication.getTokenCache().getAllAccounts();
// if there is an account in the cache and it's active, we can try to get the token silently
if (accounts.filter(a => a.localAccountId === this.connection.identityId).length > 0 && this.connection.active === true) {
getTokenPromise = this.ensureAccessTokenSilent.bind(this);
// When using an application identity, you can't retrieve the access token silently, because there is
// no account. Also (for cert auth) clientApplication is instantiated later
// after inspecting the specified cert and calculating thumbprint if one
// wasn't specified
if (this.connection.authType !== AuthType.Certificate &&
this.connection.authType !== AuthType.Secret &&
this.connection.authType !== AuthType.Identity) {
this.clientApplication = await this.getPublicClient(logger, debug);
if (this.clientApplication) {
const accounts = await this.clientApplication.getTokenCache().getAllAccounts();
// if there is an account in the cache and it's active, we can try to get the token silently
if (accounts.filter(a => a.localAccountId === this.connection.identityId).length > 0 && this.connection.active === true) {
getTokenPromise = this.ensureAccessTokenSilent.bind(this);
}
}
}
}
Expand All @@ -263,6 +261,9 @@ export class Auth {
case AuthType.Secret:
getTokenPromise = this.ensureAccessTokenWithSecret.bind(this);
break;
case AuthType.AccessToken:
getTokenPromise = this.ensureAccessTokenWithAccessToken.bind(this);
break;
}
}

Expand Down Expand Up @@ -304,6 +305,17 @@ export class Auth {
return response.accessToken;
}

public accessTokenExpired(accessToken: AccessToken): boolean {
const now: Date = new Date();
const expiresOn: Date = accessToken && accessToken.expiresOn ?
// if expiresOn is serialized from the service file, it's set as a string
// if it's coming from MSAL, it's a Date
typeof accessToken.expiresOn === 'string' ? new Date(accessToken.expiresOn) : accessToken.expiresOn
: new Date(0);

return expiresOn <= now;
}

private async getAuthClientConfiguration(logger: Logger, debug: boolean, certificateThumbprint?: string, certificatePrivateKey?: string, clientSecret?: string): Promise<Msal.Configuration> {
const msal: typeof Msal = await import('@azure/msal-node');
const { LogLevel } = msal;
Expand Down Expand Up @@ -420,13 +432,13 @@ export class Auth {
}

// Asserting identityId because it is expected to be available at this point.
assert(this.connection.identityId !== undefined);
assert(this.connection.identityId !== undefined, "identityId is undefined");
martinlingstuyl marked this conversation as resolved.
Show resolved Hide resolved

const account = await (this.clientApplication as Msal.ClientApplication)
.getTokenCache().getAccountByLocalId(this.connection.identityId);

// Asserting account because it is expected to be available at this point.
assert(account !== null);
assert(account !== null, "account is null");

return (this.clientApplication as Msal.ClientApplication).acquireTokenSilent({
account: account,
Expand Down Expand Up @@ -706,6 +718,24 @@ export class Auth {
});
}

private async ensureAccessTokenWithAccessToken(resource: string, logger: Logger, debug: boolean): Promise<AccessToken | null> {
martinlingstuyl marked this conversation as resolved.
Show resolved Hide resolved
const accessToken: AccessToken | undefined = this.connection.accessTokens[resource];

if (!accessToken) {
throw `No token found for resource ${resource}.`;
}

if (this.accessTokenExpired(accessToken)) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

throw `Access token expired. Token: ${accessToken.accessToken}, ExpiresAt: ${accessToken.expiresOn}`;
}

if (debug) {
await logger.logToStderr(`Existing access token ${accessToken.accessToken} still valid. Returning...`);
}

return accessToken;
}

private async calculateThumbprint(certificate: NodeForge.pki.Certificate): Promise<string> {
const nodeForge = (await import('node-forge')).default;
const { md, asn1, pki } = nodeForge;
Expand Down Expand Up @@ -878,8 +908,8 @@ export class Auth {

public getConnectionDetails(connection: Connection): ConnectionDetails {
// Asserting name and identityId because they are optional, but required at this point.
assert(connection.identityName !== undefined);
assert(connection.name !== undefined);
assert(connection.identityName !== undefined, "identity name is undefined");
assert(connection.name !== undefined, "connection name is undefined");

const details: ConnectionDetails = {
connectionName: connection.name,
Expand Down
1 change: 1 addition & 0 deletions src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export default {
applicationName: `CLI for Microsoft 365 v${app.packageJson().version}`,
delimiter: 'm365\$',
cliEntraAppId: process.env.CLIMICROSOFT365_ENTRAAPPID || process.env.CLIMICROSOFT365_AADAPPID || cliEntraAppId,
cliEnvEntraAppId: process.env.CLIMICROSOFT365_ENTRAAPPID || process.env.CLIMICROSOFT365_AADAPPID,
Copy link
Member

Choose a reason for hiding this comment

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

This seems duplicate of the line above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed to circumvent it defaulting to the pnp management shell Id... I only need the value if it's in an environment variable.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Since we'll be no longer using the hardcoded value, do we still need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If your PR comes through first, no we don't...

tenant: process.env.CLIMICROSOFT365_TENANT || 'common',
configstoreName: 'cli-m365-config'
};
134 changes: 131 additions & 3 deletions src/m365/commands/login.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,11 @@ import config from '../../config.js';
import { settingsNames } from '../../settingsNames.js';
import { zod } from '../../utils/zod.js';
import commands from './commands.js';
import * as accessTokenUtil from '../../utils/accessToken.js';
martinlingstuyl marked this conversation as resolved.
Show resolved Hide resolved

const options = globalOptionsZod
.extend({
authType: zod.alias('t', z.enum(['certificate', 'deviceCode', 'password', 'identity', 'browser', 'secret']).optional()),
authType: zod.alias('t', z.enum(['certificate', 'deviceCode', 'password', 'identity', 'browser', 'secret', 'accessToken']).optional()),
cloud: z.nativeEnum(CloudType).optional().default(CloudType.Public),
userName: zod.alias('u', z.string().optional()),
password: zod.alias('p', z.string().optional()),
Expand All @@ -26,6 +27,7 @@ const options = globalOptionsZod
appId: z.string().optional(),
tenant: z.string().optional(),
secret: zod.alias('s', z.string().optional()),
accessToken: zod.alias('a', z.string().or(z.array(z.string())).optional()),
connectionName: z.string().optional()
})
.strict();
Expand Down Expand Up @@ -64,6 +66,21 @@ class LoginCommand extends Command {
})
.refine(options => options.authType !== 'secret' || options.secret, {
message: 'Secret is required when using secret authentication'
})
.refine(options => options.authType !== 'accessToken' || options.accessToken, {
message: 'accessToken is required when using accessToken authentication'
})
.refine(options => !(options.authType === 'accessToken' && options.accessToken && this.tokensForMultipleTenants(options.accessToken, options.tenant)), {
message: 'The provided accessToken is not for the specified tenant or the access tokens are not for the same tenant'
})
.refine(options => !(options.authType === 'accessToken' && options.accessToken && this.tokensForMultipleApps(options.accessToken, options.appId)), {
message: 'The provided access token is not for the specified app or the access tokens are not for the same app'
})
.refine(options => !(options.authType === 'accessToken' && options.accessToken && this.tokensForTheSameResources(options.accessToken)), {
message: 'Specify access tokens that are not for the same resource'
})
.refine(options => !(options.authType === 'accessToken' && options.accessToken && this.tokenExpired(options.accessToken)), {
message: 'The provided access token has expired'
});
}

Expand Down Expand Up @@ -107,13 +124,37 @@ class LoginCommand extends Command {
case 'secret':
auth.connection.authType = AuthType.Secret;
auth.connection.secret = args.options.secret;
break;
case 'accessToken':
const accessTokens = typeof args.options.accessToken === "string" ? [args.options.accessToken] : args.options.accessToken as string[];
auth.connection.authType = AuthType.AccessToken;
auth.connection.appId = accessTokenUtil.accessToken.getTenantIdFromAccessToken(accessTokens[0]);
auth.connection.tenant = accessTokenUtil.accessToken.getAppIdFromAccessToken(accessTokens[0]);

for (const token of accessTokens) {
const resource = accessTokenUtil.accessToken.getAudienceFromAccessToken(token);
const expiresOn = accessTokenUtil.accessToken.getExpirationFromAccessToken(token);

auth.connection.accessTokens[resource] = {
expiresOn: expiresOn as Date || null,
waldekmastykarz marked this conversation as resolved.
Show resolved Hide resolved
accessToken: token
};
};

break;
}

auth.connection.cloudType = args.options.cloud;

try {
await auth.ensureAccessToken(auth.defaultResource, logger, this.debug);
if (auth.connection.authType !== AuthType.AccessToken) {
await auth.ensureAccessToken(auth.defaultResource, logger, this.debug);
}
else {
for (const resource of Object.keys(auth.connection.accessTokens)) {
await auth.ensureAccessToken(resource, logger, this.debug);
}
}
auth.connection.active = true;
}
catch (error: any) {
Expand All @@ -123,7 +164,12 @@ class LoginCommand extends Command {
await logger.logToStderr('');
}

throw new CommandError(error.message);
if (error instanceof Error) {
throw new CommandError(error.message);
}
else {
throw new CommandError(error);
}
}

const details = auth.getConnectionDetails(auth.connection);
Expand Down Expand Up @@ -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 {
martinlingstuyl marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you have a suggestions here?

Copy link
Member

Choose a reason for hiding this comment

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

How about areTokensForMultipleTenants? Adding a verb makes it clearer what the function does

const accessTokens = typeof accessTokenValue === "string" ? [accessTokenValue] : accessTokenValue as string[];;
martinlingstuyl marked this conversation as resolved.
Show resolved Hide resolved
waldekmastykarz marked this conversation as resolved.
Show resolved Hide resolved
let tenant = tenantValue || config.tenant;
let forMultipleTenants: boolean = false;

for (const token of accessTokens) {
const tenantIdInAccessToken = accessTokenUtil.accessToken.getTenantIdFromAccessToken(token);

if (tenant !== 'common' && tenant !== tenantIdInAccessToken) {
forMultipleTenants = true;
martinlingstuyl marked this conversation as resolved.
Show resolved Hide resolved
break;
}

tenant = tenantIdInAccessToken;
};

return forMultipleTenants;
}

private tokensForMultipleApps(accessTokenValue: string | string[] | undefined, appIdValue: string | undefined): boolean {
const accessTokens = typeof accessTokenValue === "string" ? [accessTokenValue] : accessTokenValue as string[];;
let appId = appIdValue || config.cliEnvEntraAppId || '';
let forMultipleApps: boolean = false;

for (const token of accessTokens) {
const appIdInAccessToken = accessTokenUtil.accessToken.getAppIdFromAccessToken(token);

if (appId !== '' && appId !== appIdInAccessToken) {
forMultipleApps = true;
break;
}

appId = appIdInAccessToken;
};

return forMultipleApps;
}

private tokensForTheSameResources(accessTokenValue: string | string[] | undefined): boolean {
const accessTokens = typeof accessTokenValue === "string" ? [accessTokenValue] : accessTokenValue as string[];;
let forTheSameResources: boolean = false;
const resources: string[] = [];

if ((accessTokens as string[]).length === 1) {
martinlingstuyl marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

for (const token of accessTokens) {
const resource = accessTokenUtil.accessToken.getAudienceFromAccessToken(token);

if (resources.indexOf(resource) > -1) {
forTheSameResources = true;
break;
}

resources.push(resource);
};

return forTheSameResources;
}

private tokenExpired(accessTokenValue: string | string[] | undefined): boolean {
const accessTokens = typeof accessTokenValue === "string" ? [accessTokenValue] : accessTokenValue as string[];;
let tokenExpired: boolean = false;

for (const token of accessTokens) {
const expiresOn = accessTokenUtil.accessToken.getExpirationFromAccessToken(token);

const accessToken = {
expiresOn: expiresOn as Date || null,
accessToken: token
};

if (auth.accessTokenExpired(accessToken)) {
tokenExpired = true;
break;
}
};

return tokenExpired;
}
}

export default new LoginCommand();
11 changes: 9 additions & 2 deletions src/m365/commands/status.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import auth from '../../Auth.js';
import auth, { AuthType } from '../../Auth.js';
import { Logger } from '../../cli/Logger.js';
import Command, { CommandArgs, CommandError } from '../../Command.js';
import commands from './commands.js';
Expand All @@ -15,7 +15,14 @@ class StatusCommand extends Command {
public async commandAction(logger: Logger): Promise<void> {
if (auth.connection.active) {
try {
await auth.ensureAccessToken(auth.defaultResource, logger, this.debug);
if (auth.connection.authType !== AuthType.AccessToken) {
await auth.ensureAccessToken(auth.defaultResource, logger, this.debug);
}
else {
for (const resource of Object.keys(auth.connection.accessTokens)) {
await auth.ensureAccessToken(resource, logger, this.debug);
}
}
}
catch (err: any) {
if (this.debug) {
Expand Down
Loading
Loading