-
Notifications
You must be signed in to change notification settings - Fork 2
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
구글 소셜 로그인을 지원하는 유저, Auth 모듈 구현 #64
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
린트 깨지는 건 제가 수정해서 올려놨어요!
pull 한번 하시고 리뷰 확인해 주세요~
수정 다 하시면 이거 머지하고 #60 에서 conflict 해결하고 머지할게요!
backend/src/user/user.service.ts
Outdated
export class UserService { | ||
constructor(private readonly userRepository: UserRepository) {} | ||
|
||
async saveUser(userInfo: userInfoWithProvider) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p1. 여기서 id (PK) 를 반환해서 토큰에 담는건 어떨까요?
이후 권한 확인에서 db 조회를 생략할 수 있을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 알겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정말 고생 많으십니다.. 슬슬 끝이 보이네요
backend/src/user/user-role.ts
Outdated
ADMIN, | ||
GUEST, | ||
MEMBER, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p2: 게스트는 유저 정보가 저장되지 않을 텐데 가지고 있을 이유가 없어보입니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하 넵 정정하겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아직 안 바뀐 것 같네요!
backend/src/user/user.controller.ts
Outdated
import { Controller } from '@nestjs/common'; | ||
import { UserService } from './user.service'; | ||
|
||
@Controller('user') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p1: REST API에서 URL 도메인은 복수를 권장합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오오오 몰랐던 사실이네요.. 감사합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 아직..
async createUser(userData: Partial<User>): Promise<User> { | ||
const user = this.create(); | ||
Object.assign(user, { | ||
provider: userData.provider, | ||
oauthId: userData.oauthId, | ||
nickname: userData.nickname, | ||
role: userData.role, | ||
profileImageUrl: userData.profileImageUrl, | ||
}); | ||
return this.save(user); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p1: 참고해서 바꾸면 좋을 것 같아요
//src/place/place.service.ts
async addPlace(createPlaceRequest: CreatePlaceRequest) {
const { googlePlaceId } = createPlaceRequest;
if (await this.placeRepository.findByGooglePlaceId(googlePlaceId)) {
throw new PlaceAlreadyExistsException();
}
const place = createPlaceRequest.toEntity();
const savedPlace = await this.placeRepository.save(place);
return { id: savedPlace.id };
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createUserRequest
클래스를 만들고 나서 저장하는 방식 맞죠???
backend/src/user/user.service.ts
Outdated
profileImageUrl: picture, | ||
role: 'user', | ||
}, | ||
{ conflictPaths: ['id'] }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: { conflictPaths: ['id'] },
의 역할이 뭔가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p1. oauthId 가 conflictPaths 가 되야 할 것 같네요.
q: { conflictPaths: ['id'] }, 의 역할이 뭔가요?
존재하지 않는다면 insert 해주는 기준입니다!
backend/src/user/user.service.ts
Outdated
@@ -0,0 +1,26 @@ | |||
import { Injectable } from '@nestjs/common'; | |||
import { UserRepository } from './user.repository'; | |||
// import { userInfoWithProvider } from './userType'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p2. 주석제거
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
놓쳤네요..
provider, | ||
oauthId, | ||
); | ||
if (existingUser) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upsert 를 사용하지 않았던 이유가 처음에 member
로 role 을 정하는데 만약에 admin
이라는 role 을 가진다면 로그인 할 때마다 role 이 member
로 변하기에 선택하긴 했습니다. 다른 좋은 방법이 있을까요? upsert 를 사용하면서 role 이 로그인할 때 불변하게 되는...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하 다른 다른 문제가 엮여있었군여
어드민 토큰은 따로 발급하는 등...
그럼 일단 두고 더 이야기해 보아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 그건 따로 생각해봐야겠습니당..ㅠㅠ 권한 페이지나 권한 부여 방식? 이런 세부사항 정해지면 수정하겠습니다!
backend/src/auth/auth.controller.ts
Outdated
) {} | ||
|
||
@Post('google/signIn') | ||
@HttpCode(201) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p2: Post
기본값이 201이라 없어도 될 것 같아요
backend/src/auth/auth.service.ts
Outdated
|
||
generateJwt(payload: any): string { | ||
return jwt.sign(payload, this.jwtSecretKey, { | ||
expiresIn: '24h', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p2: jwt expiration도 env로 옮길 수 있으면 좋을 것 같아요
backend/src/user/user-role.ts
Outdated
ADMIN, | ||
GUEST, | ||
MEMBER, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아직 안 바뀐 것 같네요!
backend/src/user/user.controller.ts
Outdated
import { Controller } from '@nestjs/common'; | ||
import { UserService } from './user.service'; | ||
|
||
@Controller('user') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 아직..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^^
📄 Summary
🙋🏻 More
🕰️ Actual Time of Completion
close #8