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

Feat/backend/fix review/0131 #242

Merged
merged 8 commits into from
Feb 3, 2024
Merged
25 changes: 21 additions & 4 deletions backend/src/auth/auth.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,23 @@ import { AuthEntity } from './entity/auth.entity';
import { JwtGuardWithout2FA } from './jwt-auth.guard';
import { Response } from 'express';

const constants = {
loginUrl: ((): string => {
const clientId = process.env.OAUTH_42_CLIENT_ID;
const codeEndpointUrl = 'https://api.intra.42.fr/oauth/authorize';
const redirectUri =
process.env.NEST_PUBLIC_API_URL + '/auth/login/oauth2/42/callback';
return `${codeEndpointUrl}?client_id=${clientId}&redirect_uri=${redirectUri}&response_type=code`;
})(),
signupUrl: ((): string => {
const clientId = process.env.OAUTH_42_CLIENT_ID;
const codeEndpointUrl = 'https://api.intra.42.fr/oauth/authorize';
const redirectUri =
process.env.NEST_PUBLIC_API_URL + '/auth/signup/oauth2/42/callback';
return `${codeEndpointUrl}?client_id=${clientId}&redirect_uri=${redirectUri}&response_type=code`;
})(),
};

@Controller('auth')
@ApiTags('auth')
export class AuthController {
Expand All @@ -40,9 +57,8 @@ export class AuthController {
@ApiOkResponse({ type: AuthEntity })
@Redirect()
redirectToOauth42() {
return this.authService.redirectToOauth42(
'/auth/signup/oauth2/42/callback',
);
// TODO : implement state system for enhanced security
return { url: constants.signupUrl };
}

@Get('signup/oauth2/42/callback')
Expand All @@ -56,7 +72,8 @@ export class AuthController {
@ApiOkResponse({ type: AuthEntity })
@Redirect()
redirectToOauth42ToLogin() {
return this.authService.redirectToOauth42('/auth/login/oauth2/42/callback');
// TODO : implement state system for enhanced security
return { url: constants.loginUrl };
}

@Get('login/oauth2/42/callback')
Expand Down
29 changes: 10 additions & 19 deletions backend/src/auth/auth.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,23 +84,13 @@ export class AuthService {
});
}

redirectToOauth42 = (callbackUri: string) => {
const client_id = process.env.OAUTH_42_CLIENT_ID;
const redirect_uri = process.env.NEST_PUBLIC_API_URL + callbackUri;
// TODO : implement state system for enhanced security
const codeEndpointUrl = 'https://api.intra.42.fr/oauth/authorize';
return {
url: `${codeEndpointUrl}?client_id=${client_id}&redirect_uri=${redirect_uri}&response_type=code`,
};
};

getAccessTokenWith42 = async ({
async getAccessTokenWith42({
code,
redirect_uri,
}: {
code: string;
redirect_uri;
}) => {
}) {
const form = new URLSearchParams({
grant_type: 'authorization_code',
client_id: process.env.OAUTH_42_CLIENT_ID,
Expand All @@ -125,9 +115,9 @@ export class AuthService {
});
}
});
};
}

getUserInfoWith42 = async ({ access_token }: { access_token: string }) => {
async getUserInfoWith42({ access_token }: { access_token: string }) {
return fetch('https://api.intra.42.fr/v2/me', {
headers: {
Authorization: `Bearer ${access_token}`,
Expand All @@ -138,10 +128,10 @@ export class AuthService {
}
return res.json();
});
};
}

signupWithOauth42 = (code: string): Promise<User> =>
this.getAccessTokenWith42({
signupWithOauth42(code: string): Promise<User> {
return this.getAccessTokenWith42({
code,
redirect_uri: '/auth/signup/oauth2/42/callback',
}).then(
Expand All @@ -162,8 +152,9 @@ export class AuthService {
// // TODO : random password? without password?
// // TODO : save access_token in db
);
}

loginWithOauth42 = async (code: string): Promise<AuthEntity> => {
async loginWithOauth42(code: string): Promise<AuthEntity> {
return this.getAccessTokenWith42({
code,
redirect_uri: '/auth/login/oauth2/42/callback',
Expand Down Expand Up @@ -193,7 +184,7 @@ export class AuthService {
});
});
});
};
}

async pipeQrCodeStream(stream: Response, otpAuthUrl: string) {
return toFileStream(stream, otpAuthUrl);
Expand Down
46 changes: 0 additions & 46 deletions backend/test/chat-gateway.e2e-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1426,50 +1426,4 @@ describe('ChatGateway and ChatController (e2e)', () => {
});
});
});
describe('online status', () => {
let userAndSockets: UserAndSocket[];
beforeAll(async () => {
ws1.close();
ws2.close();
ws3.close();
ws4.close();
ws5.close();
ws6.close();
ws7.close();
const users = [user1, user2];
userAndSockets = await users.map((user) => {
return {
user,
ws: io('ws://localhost:3000/chat', {
extraHeaders: { cookie: 'token=' + user.accessToken },
}),
};
});
userAndSockets[1].ws.close();
await connect(userAndSockets[0].ws);
return new Promise<void>((resolve) => setTimeout(resolve, waitTime));
});
afterAll(() => {
userAndSockets.map((userAndSocket) => {
userAndSocket.ws.close();
});
});

it('connected user should be online', async () => {
const u = userAndSockets[0];
const res = await app.isOnline(u.user.id, u.user.accessToken).expect(200);
const body = res.body;
expect(body.isOnline).toEqual(true);
});
it('disconnected user should be offline', async () => {
const u = userAndSockets[1];
const res = await app.isOnline(u.user.id, u.user.accessToken).expect(200);
const body = res.body;
expect(body.isOnline).toEqual(false);
});
it('check online status with invalid access token should be unauthorized', async () => {
const u = userAndSockets[0];
await app.isOnline(u.user.id, '').expect(401);
});
});
});
89 changes: 89 additions & 0 deletions backend/test/online-status.e2e-spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import { INestApplication } from '@nestjs/common';
import { Test, TestingModule } from '@nestjs/testing';
import { Socket, io } from 'socket.io-client';
import { AppModule } from 'src/app.module';
import { TestApp } from './utils/app';

async function createNestApp(): Promise<INestApplication> {
const moduleFixture: TestingModule = await Test.createTestingModule({
imports: [AppModule],
}).compile();

const app = moduleFixture.createNestApplication();
return app;
}
Comment on lines +7 to +14
Copy link
Contributor

Choose a reason for hiding this comment

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

The createNestApp function is well-implemented, leveraging async/await for asynchronous operations. This function is crucial for creating a new instance of the NestJS application for testing purposes. However, consider adding error handling to manage potential exceptions during the application setup.

async function createNestApp(): Promise<INestApplication> {
  try {
    const moduleFixture: TestingModule = await Test.createTestingModule({
      imports: [AppModule],
    }).compile();

    const app = moduleFixture.createNestApplication();
    return app;
  } catch (error) {
    console.error('Error creating Nest application', error);
    throw error;
  }
}

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
async function createNestApp(): Promise<INestApplication> {
const moduleFixture: TestingModule = await Test.createTestingModule({
imports: [AppModule],
}).compile();
const app = moduleFixture.createNestApplication();
return app;
}
async function createNestApp(): Promise<INestApplication> {
try {
const moduleFixture: TestingModule = await Test.createTestingModule({
imports: [AppModule],
}).compile();
const app = moduleFixture.createNestApplication();
return app;
} catch (error) {
console.error('Error creating Nest application', error);
throw error;
}
}


describe('ChatGateway and ChatController (e2e)', () => {
let app: TestApp;
let user1;
let user2;

beforeAll(async () => {
//app = await initializeApp();
const _app = await createNestApp();
await _app.listen(3000);
app = new TestApp(_app);
const dto1 = {
name: 'test-user1',
email: '[email protected]',
password: 'test-password',
};
const dto2 = {
name: 'test-user2',
email: '[email protected]',
password: 'test-password',
};
user1 = await app.createAndLoginUser(dto1);
user2 = await app.createAndLoginUser(dto2);
});

afterAll(async () => {
await app.deleteUser(user1.id, user1.accessToken).expect(204);
await app.deleteUser(user2.id, user2.accessToken).expect(204);
await app.close();
});
Comment on lines +16 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

The setup and teardown logic within beforeAll and afterAll hooks are correctly implemented, ensuring that the test environment is correctly initialized and cleaned up. This includes creating test users and deleting them after tests are completed. However, hardcoding the port number (3000) in the setup could limit flexibility. Consider making the port number configurable or deriving it from the application's configuration.


const connect = (ws: Socket) => {
return new Promise<void>((resolve) => {
ws.on('connect', () => {
resolve();
});
});
};

describe('online status', () => {
let onlineUser;
let onlineUserSocket;
let offlineUser;

beforeAll(async () => {
onlineUser = user1;
onlineUserSocket = io('ws://localhost:3000/chat', {
extraHeaders: { cookie: 'token=' + onlineUser.accessToken },
});
await connect(onlineUserSocket);
offlineUser = user2;
});
afterAll(() => {
onlineUserSocket.close();
});

it('online user should be true (online)', async () => {
Copy link
Owner

Choose a reason for hiding this comment

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

it英語として意味をなしてないので、テストdescの書き方もすこしかんがえてみて〜
it is xxxとはいえるけど
it A is xxxとはいえないでしょ、英語的に

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

この it って英文の一部だったんですね!!w
test 関数を変に名付けてるだけなのかと思ってました

const res = await app
.isOnline(onlineUser.id, onlineUser.accessToken)
.expect(200);
const body = res.body;
expect(body.isOnline).toEqual(true);
});
it('offline user should be false (offline)', async () => {
const res = await app
.isOnline(offlineUser.id, offlineUser.accessToken)
.expect(200);
const body = res.body;
expect(body.isOnline).toEqual(false);
});
it('check online status with invalid access token should be unauthorized', async () => {
await app.isOnline(onlineUser.id, '').expect(401);
});
});
Comment on lines +54 to +88
Copy link
Contributor

Choose a reason for hiding this comment

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

The test cases within the describe block for online status are well-structured and cover essential scenarios, including checking the online status of an online user, an offline user, and unauthorized access with an invalid token. These tests effectively validate the online status functionality. However, the tests rely on the ws://localhost:3000/chat URL, which includes a hardcoded port number and assumes the local environment. For better flexibility and to support different environments or CI pipelines, consider making the WebSocket URL and port configurable.

});
Loading