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.

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

fix:(GitHub Auth): Deprecate getAuthUsername #585

Merged
merged 13 commits into from
Feb 19, 2025
9 changes: 4 additions & 5 deletions src/targets/awsLambdaLayer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,9 @@ import * as path from 'path';
import { Octokit } from '@octokit/rest';
import simpleGit from 'simple-git';
import {
getAuthUsername,
getGitHubApiToken,
getGitHubClient,
GitHubRemote,
getGitHubAuthHeader,
} from '../utils/githubApi';

import { TargetConfig } from '../schemas/project_config';
Expand Down Expand Up @@ -139,16 +138,16 @@ export class AwsLambdaLayerTarget extends BaseTarget {
this.logger.trace('AWS regions: ', awsRegions);

const remote = this.awsLambdaConfig.registryRemote;
const username = await getAuthUsername(this.github);
remote.setAuth(username, getGitHubApiToken());

await withTempDir(
async directory => {
const git = simpleGit(directory);
/** Add the GitHub token to the git auth header */
await git.raw(getGitHubAuthHeader());
this.logger.info(
`Cloning ${remote.getRemoteString()} to ${directory}...`
);
await git.clone(remote.getRemoteStringWithAuth(), directory);
await git.clone(remote.getRemoteString(), directory);

if (!isDryRun()) {
await this.publishRuntimes(
Expand Down
11 changes: 5 additions & 6 deletions src/targets/ghPages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ import { GitHubGlobalConfig, TargetConfig } from '../schemas/project_config';
import { ConfigurationError, reportError } from '../utils/errors';
import { withTempDir } from '../utils/files';
import {
getAuthUsername,
getGitHubApiToken,
getGitHubClient,
GitHubRemote,
getGitHubAuthHeader,
} from '../utils/githubApi';
import { isDryRun } from '../utils/helpers';
import { extractZipArchive } from '../utils/system';
Expand Down Expand Up @@ -148,11 +148,13 @@ export class GhPagesTarget extends BaseTarget {
archivePath: string,
version: string
): Promise<void> {
const git = simpleGit(directory);
/** Add the GitHub token to the git auth header */
await git.raw(getGitHubAuthHeader());
this.logger.info(
`Cloning "${remote.getRemoteString()}" to "${directory}"...`
);
await simpleGit().clone(remote.getRemoteStringWithAuth(), directory);
const git = simpleGit(directory);
await git.clone(remote.getRemoteString(), directory);
this.logger.debug(`Checking out branch: "${branch}"`);
try {
await git.checkout([branch]);
Expand Down Expand Up @@ -219,12 +221,9 @@ export class GhPagesTarget extends BaseTarget {
packageFiles[0]
);

const username = await getAuthUsername(this.github);

const remote = new GitHubRemote(
githubOwner,
githubRepo,
username,
getGitHubApiToken()
);

Expand Down
9 changes: 4 additions & 5 deletions src/targets/registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ import { GitHubGlobalConfig, TargetConfig } from '../schemas/project_config';
import { ConfigurationError, reportError } from '../utils/errors';
import { withTempDir } from '../utils/files';
import {
getAuthUsername,
getGitHubApiToken,
getGitHubAuthHeader,
getGitHubClient,
GitHubRemote,
} from '../utils/githubApi';
Expand Down Expand Up @@ -427,14 +426,14 @@ export class RegistryTarget extends BaseTarget {

private async cloneRegistry(directory: string): Promise<SimpleGit> {
const remote = this.remote;
const username = await getAuthUsername(this.github);
remote.setAuth(username, getGitHubApiToken());

const git = simpleGit(directory);
/** Add the GitHub token to the git auth header */
await git.raw(getGitHubAuthHeader());
this.logger.info(
`Cloning "${remote.getRemoteString()}" to "${directory}"...`
);
await git.clone(remote.getRemoteStringWithAuth(), directory, [
await git.clone(remote.getRemoteString(), directory, [
'--filter=tree:0',
'--single-branch',
]);
Expand Down
8 changes: 4 additions & 4 deletions src/targets/upm.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { Octokit } from '@octokit/rest';
import simpleGit from 'simple-git';
import {
getAuthUsername,
getGitHubApiToken,
getGitHubAuthHeader,
getGitHubClient,
GitHubRemote,
} from '../utils/githubApi';
Expand Down Expand Up @@ -107,11 +107,9 @@ export class UpmTarget extends BaseTarget {
packageFile
);

const username = await getAuthUsername(this.github);
const remote = new GitHubRemote(
this.config.releaseRepoOwner,
this.config.releaseRepoName,
username,
getGitHubApiToken()
);
const remoteAddr = remote.getRemoteString();
Expand All @@ -120,8 +118,10 @@ export class UpmTarget extends BaseTarget {
await withTempDir(
async directory => {
const git = simpleGit(directory);
/** Add the GitHub token to the git auth header */
await git.raw(getGitHubAuthHeader());
this.logger.info(`Cloning ${remoteAddr} to ${directory}...`);
await git.clone(remote.getRemoteStringWithAuth(), directory);
await git.clone(remote.getRemoteString(), directory);

this.logger.info('Clearing the repository.');
await git.rm(['-r', '-f', '.']);
Expand Down
55 changes: 14 additions & 41 deletions src/utils/githubApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,27 +26,14 @@
public constructor(
owner: string,
repo: string,
username?: string,

Check warning on line 29 in src/utils/githubApi.ts

View workflow job for this annotation

GitHub Actions / Lint fixes

[@typescript-eslint/no-unused-vars] 'username' is defined but never used. Allowed unused args must match /^_/u.
apiToken?: string

Check warning on line 30 in src/utils/githubApi.ts

View workflow job for this annotation

GitHub Actions / Lint fixes

[@typescript-eslint/no-unused-vars] 'apiToken' is defined but never used. Allowed unused args must match /^_/u.
) {
this.owner = owner;
this.repo = repo;
if (username && apiToken) {
this.setAuth(username, apiToken);
}
Comment on lines -34 to -36
Copy link
Member

Choose a reason for hiding this comment

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

should be able to remove these parameters entirely -- or maybe take the api key and do the header modification here?

Copy link
Member Author

Choose a reason for hiding this comment

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

parameters removed, but I'm not sure how to bake the token and header in there

this.url = `/${this.owner}/${this.repo}/`;
}

/**
* Sets authentication arguments: username and personal API token
*
* @param username GitHub username
* @param apiToken GitHub API token
*/
public setAuth(username: string, apiToken: string): void {
this.username = username;
this.apiToken = apiToken;
}

/**
* Returns an HTTP-based git remote
Expand All @@ -56,19 +43,6 @@
public getRemoteString(): string {
return this.PROTOCOL_PREFIX + this.GITHUB_HOSTNAME + this.url;
}

/**
* Returns an HTTP-based git remote with embedded HTTP basic auth
*
* It MAY contain sensitive information (e.g. API tokens)
*/
public getRemoteStringWithAuth(): string {
const authData =
this.username && this.apiToken
? `${this.username}:${this.apiToken}@`
: '';
return this.PROTOCOL_PREFIX + authData + this.GITHUB_HOSTNAME + this.url;
}
}

/**
Expand All @@ -87,6 +61,20 @@
return githubApiToken;
}

/**
* Returns the GitHub auth header
*
* @returns GitHub auth header
*/
export function getGitHubAuthHeader(): Array<string> {
Copy link
Member

Choose a reason for hiding this comment

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

I found something arguably easier and more like the old ways here: https://stackoverflow.com/a/66156992/90297

git clone https://oauth2:[email protected]/username/repo.git

So, maybe just do that to keep things simple and hopefully more compatible?

Copy link
Member

Choose a reason for hiding this comment

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

afaict both ways work with user tokens or app tokens -- fun fact the username does not matter for the clone-url approach just that it cannot be empty

Copy link
Member

Choose a reason for hiding this comment

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

Wowza, alright then let's go with the old way as it seems simpler?

return [
'config',
'--global',
'http.extraheader',
'AUTHORIZATION: bearer ' + getGitHubApiToken()
];
}

const _GitHubClientCache: Record<string, Octokit> = {};

/**
Expand Down Expand Up @@ -123,21 +111,6 @@
return _GitHubClientCache[githubApiToken];
}

/**
* Gets the currently authenticated GitHub user from the client
*
* @param github GitHub client
* @returns GitHub username
*/
export async function getAuthUsername(github: Octokit): Promise<string> {
const userData = await github.users.getAuthenticated({});
const username = (userData.data || {}).login;
if (!username) {
throw new Error('Cannot reliably detect GitHub username, aborting');
}
return username;
}

/**
* Loads a file from the context's repository
*
Expand Down
Loading