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

feat: Force refresh personal access token #1137

Merged
merged 5 commits into from
Jun 27, 2024
Merged

feat: Force refresh personal access token #1137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
02fbcf4
feat: Force refresh personal access token
tolusha Jun 26, 2024
672f909
Fixup
tolusha Jun 27, 2024
5f46097
Fixup
tolusha Jun 27, 2024
acbbb1e
Fixup
tolusha Jun 27, 2024
09d4eb1
code cleanup
olexii4 Jun 27, 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
8 changes: 5 additions & 3 deletions ...ges/dashboard-frontend/src/contexts/WorkspaceActions/BulkDeleteButton/__mocks__/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@ export class WorkspaceActionsBulkDeleteButton extends React.Component<Props> {
const { isDisabled, onAction, workspaces } = this.props;

const handleAction = () =>
workspaces.map(workspace =>
onAction?.(WorkspaceAction.DELETE_WORKSPACE, workspace.uid, true),
);
workspaces.map(async workspace => {
if (onAction) {
await onAction(WorkspaceAction.DELETE_WORKSPACE, workspace.uid, true);
}
});

return (
<div data-testid="workspace-actions-bulk-delete">
Expand Down
20 changes: 10 additions & 10 deletions packages/dashboard-frontend/src/services/oauth/__tests__/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,23 +29,23 @@ describe('OAuth service', () => {
it('should not refresh token if no status section in devworkspace', async () => {
const devWorkspace = new DevWorkspaceBuilder().build();

await OAuthService.refreshTokenIfNeeded(devWorkspace);
await OAuthService.refreshTokenIfProjectExists(devWorkspace);

expect(refreshFactoryOauthTokenSpy).not.toHaveBeenCalled();
});
it('should not refresh token if no mainUrl in status', async () => {
const status = {};
const devWorkspace = new DevWorkspaceBuilder().withStatus(status).build();

await OAuthService.refreshTokenIfNeeded(devWorkspace);
await OAuthService.refreshTokenIfProjectExists(devWorkspace);

expect(refreshFactoryOauthTokenSpy).not.toHaveBeenCalled();
});
it('should not refresh token if no projects section in devworkspace', async () => {
const status = { mainUrl: 'https://mainUrl' };
const devWorkspace = new DevWorkspaceBuilder().withStatus(status).build();

await OAuthService.refreshTokenIfNeeded(devWorkspace);
await OAuthService.refreshTokenIfProjectExists(devWorkspace);

expect(refreshFactoryOauthTokenSpy).not.toHaveBeenCalled();
});
Expand All @@ -58,7 +58,7 @@ describe('OAuth service', () => {
.withProjects(projects)
.build();

await OAuthService.refreshTokenIfNeeded(devWorkspace);
await OAuthService.refreshTokenIfProjectExists(devWorkspace);

expect(refreshFactoryOauthTokenSpy).not.toHaveBeenCalled();
});
Expand All @@ -78,7 +78,7 @@ describe('OAuth service', () => {
.withProjects(projects)
.build();

await OAuthService.refreshTokenIfNeeded(devWorkspace);
await OAuthService.refreshTokenIfProjectExists(devWorkspace);

expect(refreshFactoryOauthTokenSpy).not.toHaveBeenCalled();
});
Expand All @@ -102,7 +102,7 @@ describe('OAuth service', () => {

refreshFactoryOauthTokenSpy.mockResolvedValueOnce();

await OAuthService.refreshTokenIfNeeded(devWorkspace);
await OAuthService.refreshTokenIfProjectExists(devWorkspace);

expect(refreshFactoryOauthTokenSpy).toHaveBeenCalledWith('origin:project');
});
Expand Down Expand Up @@ -136,7 +136,7 @@ describe('OAuth service', () => {

jest.spyOn(common.helpers.errors, 'includesAxiosResponse').mockImplementation(() => false);

await OAuthService.refreshTokenIfNeeded(devWorkspace);
await OAuthService.refreshTokenIfProjectExists(devWorkspace);

expect(refreshFactoryOauthTokenSpy).toHaveBeenCalledWith('origin:project');
expect(mockOpenOAuthPage).not.toHaveBeenCalled();
Expand Down Expand Up @@ -178,7 +178,7 @@ describe('OAuth service', () => {
jest.spyOn(common.helpers.errors, 'includesAxiosResponse').mockImplementation(() => true);

try {
await OAuthService.refreshTokenIfNeeded(devWorkspace);
await OAuthService.refreshTokenIfProjectExists(devWorkspace);
} catch (e: any) {
fail('it should not reach here');
}
Expand Down Expand Up @@ -220,7 +220,7 @@ describe('OAuth service', () => {
jest.spyOn(common.helpers.errors, 'includesAxiosResponse').mockImplementation(() => true);

try {
await OAuthService.refreshTokenIfNeeded(devWorkspace);
await OAuthService.refreshTokenIfProjectExists(devWorkspace);
} catch (e: any) {
fail('it should not reach here');
}
Expand Down Expand Up @@ -263,7 +263,7 @@ describe('OAuth service', () => {
jest.spyOn(common.helpers.errors, 'includesAxiosResponse').mockImplementation(() => true);

try {
await OAuthService.refreshTokenIfNeeded(devWorkspace);
await OAuthService.refreshTokenIfProjectExists(devWorkspace);
} catch (e: any) {
expect(e.response.status).toBe(401);
}
Expand Down
22 changes: 8 additions & 14 deletions packages/dashboard-frontend/src/services/oauth/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,22 +27,16 @@ export class OAuthService {
}
}

static async refreshTokenIfNeeded(workspace: devfileApi.DevWorkspace): Promise<void> {
// if workspace is not created yet, do not refresh token
if (!workspace.status || !workspace.status.mainUrl) {
return;
}
if (!workspace.spec.template.projects) {
return;
}

const project = workspace.spec.template.projects[0];
if (!project || !project.git) {
return;
}
static async refreshTokenIfProjectExists(workspace: devfileApi.DevWorkspace): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

@tolusha I'm a bit lost, why project presence in devfile is indicator for token refresh ?

Copy link
Member

Choose a reason for hiding this comment

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

at the very least, would be great to have some comment with method description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need a repository url to figure out the correct OAuth provider.

// Find first git project.
let project = workspace.spec.template.projects?.find(project => !!project.git);
project = project || workspace.spec.template.starterProjects?.find(project => !!project.git);
project = project || workspace.spec.template.dependentProjects?.find(project => !!project.git);

try {
await refreshFactoryOauthToken(project.git.remotes.origin);
if (project) {
await refreshFactoryOauthToken(project.git!.remotes.origin);
Copy link
Member

Choose a reason for hiding this comment

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

is it expected change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

}
} catch (e) {
if (!common.helpers.errors.includesAxiosResponse(e)) {
return;
Expand Down
2 changes: 1 addition & 1 deletion packages/dashboard-frontend/src/store/Workspaces/devWorkspaces/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ export const actionCreators: ActionCreators = {
return;
}
try {
await OAuthService.refreshTokenIfNeeded(workspace);
await OAuthService.refreshTokenIfProjectExists(workspace);
await dispatch({ type: Type.REQUEST_DEVWORKSPACE, check: AUTHORIZED });
if (!(await selectAsyncIsAuthorized(getState()))) {
const error = selectSanityCheckError(getState());
Expand Down
Loading