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

[NDD-348]: AccessToken이 계속해서 Redis에 남아있는 경우 해결 (5h / 2h) #179

Merged
merged 5 commits into from
Dec 14, 2023

Conversation

quiet-honey
Copy link
Collaborator

@quiet-honey quiet-honey commented Dec 7, 2023

NDD-348 Powered by Pull Request Badge

Why

비디오 해시값은 Redis에 영구적으로 저장되는 것이 맞지만, accessToken과 refreshToken의 경우 ttl을 주지 않고 저장을 하다보니 사용할 수 없는 Token이 되어 굳이 Redis에 계속해서 저장할 필요가 없었다. 지속적으로 Token이 쌓이다보면 저장공간에 문제가 생길 수 있기에 이를 해결해야만 했다.
또한, 악의적인 사용자에 대한 문제도 있었다. 악의적인 사용자가 지속적으로 구글 로그인을 통해 accessToken을 요청하여 Redis에 너무 많은 값을 저장될 수도 있기에 이 또한 해결이 필요했다.

그래서 이를 해결하기 위한 방법으로 아래와 같은 두 가지 방법을 사용하였다.

  • accessToken 발급 시에 accessToken:refreshToken(ttl=7d) 형식으로 Redis에 저장해 발급한 refreshToken이 사용될 수 없는 만료 후에도 Redis에 남아있는 경우를 방지하였다.
  • accessToken 발급 시에 email:accessToken(ttl=1h) 형식으로 Redis에 저장해 같은 email로 로그인하면 Redis에 저장된 accessToken을 반환하는 방식을 채택하여 악의적인 사용자가 같은 아이디로 accessToken을 계속해서 저장하는 경우를 방지하였다.

시간이 예상보다 많이 넘어간 이유는 Test 관련 문제였다. Workbook, Question, Answer에 실패하는 경우가 있었는데 이를 잡는 것이 너무나도 어려웠다. 해결한 이후 테스트가 실패하는 이유와 아래와 같다.

  1. E2E 테스트 시에 Redis는 초기화가 되지 않고, DB는 초기화가 돼서 id가 불일치하는 경우가 있었음
  2. DB의 Auto Increment로 인한 id가 테스트 시마다 쌓여서 테스트를 마칠 때마다 id Auto Increment를 초기화해주어야 했음
    이에 대한 해결 방식은 How 파트에서 설명하도록 하겠다.

How

  1. Redis SET에 ttl 부여
    Redis에 데이터를 set할 때는 아래와 같이 set에 ttl을 부여하여 저장하도록 하였다.
export const saveToRedisWithExpireIn = async (
  key: string,
  value: string,
  ttl: number,
  redis = getNewRedis(),
) => {
  try {
    await redis.set(key, value, 'EX', ttl);
  } catch (error) {
    throw new RedisSaveException();
  } finally {
    await redis.quit();
  }
};
  1. 테스트 실패 문제 해결
  • 테스트 시에 Redis는 초기화가 되지 않고, DB는 초기화가 돼서 id가 불일치하는 경우
    이 경우에 대해서는 또 두 가지의 문제가 있었는데 우선 배포 상황이 아닌 경우에는 아래와 같이 ioredis-mock을 사용하도록 변경하였다. 이 방식으로 Redis가 초기화가 되지 않아 발생하는 문제를 해결하였다. 또한 테스트 종료 시마다 clearRedis라는 메서드를 활용하여 아예 Redis가 비워질 수 있도록 하였다.
const getNewRedis = () => {
  return process.env.IS_PROD === 'true'
    ? new Redis(process.env.REDIS_URL as string)
    : new redisMock();
};

export const saveToRedis = async (
  key: string,
  value: string,
  redis: Redis = getNewRedis(),
) => {
...
};

export const saveToRedisWithExpireIn = async (
  key: string,
  value: string,
  ttl: number,
  redis = getNewRedis(),
) => {
  ...
};

export const deleteFromRedis = async (
  key: string,
  redis: Redis = getNewRedis(),
) => {
...
};

export const getValueFromRedis = async (
  key: string,
  redis: Redis = getNewRedis(),
) => {
  try {
...
};

export const clearRedis = async (redis: Redis) => {
  try {
    await redis.flushdb();
  } catch (error) {
    throw new Error('Redis 초기화 실패');
  } finally {
    await redis.quit();
  }
};

E2E 테스트를 제외하고는 위의 방식으로 문제가 해결 가능했지만, 문제는 E2E 테스트였다. 개발자가 직접 ioredis-mock 객체를 생성하여 집어넣어 줄 수 없기 때문에 프로그램이 자체적으로 생성하는 ioredis-mock 객체를 사용해야 했는데 여기서 문제가 발생하였다.
문제가 발생하는 원인은 DB에 저장되는 회원의 id와 Redis에 저장되는 토큰에 저장된 payload의 id의 불일치 때문이었는데, 이는 테스트하는 회원의 아이디를 무조건 1번으로 설정될 수 있게 DB에 저장, 즉 코드 상에서 가장 먼저 저장되는 member 객체로 둠으로써 해결하였다. => 이전 테스트들에서 redis에 저장된 토큰이 payload.id=1로 저장되었기 때문

it('question의 카테고리를 조회했을 때 카테고리가 Member의 카테고리가 아니라면 권한 없음을 발생시킨다.', async () => {
    //given
    const token = await authService.login(memberFixturesOAuthRequest); // 테스트하는 유저를 무조건 id 1번이 되도록 설정
    await memberRepository.save(otherMemberFixture); // 새로운 유저가 1번을 선점하지 않도록 테스트 유저보다 나중에 save 수행
    const workbook = await workbookRepository.save(otherWorkbookFixture);
    const question = await questionRepository.save(
      Question.of(workbook, null, 'tester'),
    );

    //when & then
    const agent = request.agent(app.getHttpServer());
    await agent
      .delete(`/api/question/${question.id}`)
      .set('Cookie', [`accessToken=${token}`])
      .expect((error) => console.log(error))
      .expect(403);
  });
  • 테스트를 마칠 때마다 id Auto Increment를 초기화
afterEach(async () => {
  ....
  await memberRepository.query('DELETE FROM sqlite_sequence'); // Auto Increment 초기화
});

위와 같은 명령어를 수행함으로써 모든 테이블에 대한 Auto Increment를 초기화 할 수 있었다.

Result

image
위와 같이 ttl 적용 후에도 모든 테스트가 잘 통과함을 확인할 수 있다.

Prize

아래의 두 가지 문제 상황에 대한 대처가 가능해짐

  1. 악의적인 사용자가 지속적으로 구글 로그인을 통해 accessToken을 요청하여 Redis에 너무 많은 값을 저장하는 것을 방지
  2. 로그인 시에 발급되어 Redis에 저장되는 accessToken과 refreshToken이 영구적으로 Redis에 저장되어 쓸데없는 공간을 차지하고 있는 상황을 방지

Reference

https://redis.github.io/ioredis/classes/Redis.html#set

@quiet-honey quiet-honey self-assigned this Dec 7, 2023
Copy link

cloudflare-workers-and-pages bot commented Dec 7, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: fd1ef93
Status: ✅  Deploy successful!
Preview URL: https://b77da946.gomterview.pages.dev
Branch Preview URL: https://feature-ndd-348.gomterview.pages.dev

View logs

@quiet-honey quiet-honey added the BE 백엔드 코드 변경사항 label Dec 7, 2023
@quiet-honey quiet-honey changed the title Feature/ndd 348 [NDD-348]: AccessToken이 계속해서 Redis에 남아있는 경우 해결 (5h / 2h) Dec 14, 2023
@quiet-honey quiet-honey added enhancement 개선 또는 피드백 반영 test 테스트코드가 변경된 경우 feature 새로운 기능이 추가 된 경우 labels Dec 14, 2023
Copy link
Collaborator

@JangAJang JangAJang left a comment

Choose a reason for hiding this comment

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

LGTM!!! 레디스에서도 트랜잭션으로 인한 데이터소스와 비슷한, DB중복 이슈같아보이는군요!! jest에서 테스트를 병렬적으로 실행해서, 모든 테스트에서 데이터베이스를 공유하는 것도 문제라고합니다!! 나중에 같이 딥다이빙해보죠

@quiet-honey quiet-honey merged commit 45f71fd into dev Dec 14, 2023
2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the feature/NDD-348 branch December 14, 2023 04:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE 백엔드 코드 변경사항 enhancement 개선 또는 피드백 반영 feature 새로운 기능이 추가 된 경우 test 테스트코드가 변경된 경우
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants