#76 OAuth 계정 Refactoring

[minhyeok4dev]

#19 를 해결했습니다.

트위터는 기본적으로 이메일을 전달해주지 않기 때문에

1. 이메일을 임시로 지정해주거나,
2. 이메일을 입력하게 하거나
   둘중에 하나를 선택해야 하는데요.
   앞서 이슈에 적었던 대로 2번에 맞춰 해결해보았습니다.

[angdev]

일단 email을 고정해서 넣어두는 점 때문에 한 명 이상이 twitter로 가입해서 이메일을 변경하지 않으면 email uniqueness에 위배되어서 user 생성이 안 되는듯 ㅜㅜ

위 그림처럼 user가 안 생겨서 id도 없는 문제가 생김.

[minhyeok4dev]

그렇군.. 겹치지않는 이메일이 나오도록 다시 해봤는데 확인해볼래?

[angdev]

일단 겹치지 않는 메일이 생기긴 하는데 이메일 변경 페이지로 넘어가지 않네.. 그리고 지금 email을 user edit에서 변경할 수 있도록 되어 있는데 이거 막아야 하지 않을까 싶기도 하고 ㅋㅋ

[minhyeok4dev]

크 그랬네 timestamp가 이때쓰이면 좀 좋을거같아서 이렇게 한번 바꿔봄..

[shaynekang]

수고하셨습니다~

1. Users::OmniauthCallbacksController의 #facebook 액션을 보면, 사용자를 찾는 메소드의 이름이 #find_for_google_oauth2라고 되어있네요. #find_for_facebook_oauth 등이 올바른 네이밍이 아닐까 합니다.

2. #find_for_google_oauth2와 #find_for_twitter_oauth에 current_user를 인자로 넣는 것 같은데, 막상 사용하지는 않네요. 삭제하는 게 좋을 것 같습니다.

3. #find_for_google_oauth2와 #find_for_twitter_oauth에 Password를 auto-generate해주는 부분이 있는데, 이 부분도 수정이 필요할 것 같습니다. 패스워드를 generate하지 않고도 정상적으로 동작하도록 작업해 주세요.

4. 트위터로 로그인 할 때, 기본 이메일([email protected])을 넣을 필요는 없는 것 같습니다. 이메일을 기본으로 세팅하지 않고 진행하는 게 좀 더 올바른 구현 같네요.

5. User의 nickname필드는 OmniAuth의 name대신 nickname을 넣어주는 게 맞는 것 같습니다.

   다만 몇몇 OAuth Provider는 nickname을 제공하지 않는 경우가 있는데, 최근에 페이스북이 OAuth API에서 username(nickname)을 deprecated 시킨 것 같네요.
   이런 경우는 User의 nickname필드를 공란으로 해두고, 아닌 경우는 Omniauth의 nickname을 넣어주면 될 것 같습니다.

6. 처음 OAuth로 로그인할 때 회원가입 폼을 보여줄 이유가 없다면, 그에 관한 코드(new_user_registration_url로 redirect해주는 코드)는 지워도 될 것 같습니다.

7. 요즘 프로그래밍 트렌드 중 하나는 주석을 적지 않는 겁니다. 주석과 코드의 내용이 일치하도록 유지보수하기 어렵기 때문에, 개발을 진행할수록 주석에서 하는 말과 코드에서 하는 말이 딴판이 되죠.(..)
   즉, 주석 없이 코드만으로도 어떻게 동작하는지 명확하게 알 수 있어야 합니다. 코드를 보면 주석이 많이 보이는데, 이 주석 없이 코드만 보고 이해할 수 있도록 리펙토링 해보세요.

8. 디버깅용으로 작성한 코드는 커밋할 때 제거해 주세요. ㅎㅎ(@my_logger 라는게 있군요)

좀 더 살펴본 뒤, 또 피드백 드리겠습니다.(..)

[shaynekang]

음... OAuth로 로그인 했을 때 동일한 이메일이 있을 경우 두 계정을 Merging 하는 걸 구현하셨는데,

1. 서비스에 [email protected] 라는 이름으로 가입한 사용자가 있다.
2. 내 SNS 계정의 이메일 주소를 [email protected]로 바꾼다.
3. 내 SNS로 로그인하다.

이러면 [email protected]의 계정을 해킹할 수 있는거 아닌가요?(..)
제가 볼 땐 Merging하기 전에 패스워드를 입력하는 등의 방식으로 해커를 걸러줘야 할 것 같은데...(먼산)

[shaynekang]

1. ActiveRecord의 create나 save의 경우 뱅(!) 메소드를 사용하는 걸 권장합니다. 자세한 사항은 #1 사용자는 History에 유저를 assign할 수 있다에 달아놓은 코멘트를 참고해 주세요. ㅎㅎ
2. Merging할 때 라우트 컨벤션은 users/merge/9/facebook_data/callback 보다 users/:id/merge/:provider 가 맞는 것 같습니다. 전반적으로 라우팅이 RESTful하지 않은데, 레일즈 라우팅 가이드를 천천히 읽어보시고 RESTful하게 수정해 주세요. ㅎㅎ
3. Merging할때 session에 Omniauth 값을 넣고 계신데, 세션은 크기가 작기 때문에 해시(Hash)등을 넣기 시작하면 터져 버립니다. 만일 info값을 이용할 생각이 없다면, 세션에는 provider, uid 두 개만 넣으면 될 것 같네요. ㅎㅎ
4. 세션명이 "devise-#{provider_name}"인데, Devise 컨벤션 상으로 꼭 이렇게 해줘야 하는게 아니라면, provider_name을 붙여줄 필요가 있을까요? Merging할때 데이터값을 찾아오기 불편할 것 같네요. ㅎㅎ
5. merging을 완료했으면, 바로 로그인 상태도 만들어줘도 될 것 같습니다.

역시나 조금 더 살펴보고, 더 피드백 드리겠습니다.(..)

[shaynekang]

1. UsersController의 merge 액션을 보면, if params[:callback] == 'callback' 등으로 별도 처리를 해 준 부분이 있는데, 요런건 아예 라우트-액션을 따로 나누는 걸 추천합니다. /users/:id/merge/:provider/callback으로 들어가면 merge_callback 액션으로 간다던가. 요렇게 해 주는 것이 좋습니다. ㅎㅎ

2. 비슷한 예로 UsersController의 sign_up_from_twitter 액션에서 if request.post?로 나눠주는 부분이 있는데 이런 것도 라우트-액션을 통채로 분리하는 걸 권장합니다.

3. 트위터 OAuth에서 사용자가 가입을 완료하지 않았음에도(이메일을 안 넣은 상황) 데이터베이스에 저장을 하도록 구현하셨는데, 이럴 경우에는 User 모델에 플래그 컬럼(activate 등)을 하나 둬서 플래그가 세워져 있지 않을 경우 사용자를 비활성화 처리해야 합니다.

   그렇지 않으면 서비스에서 이 미가입 사용자가 드러나 버리는 상황이 발생합니다.(가령 친구가 서비스에 초대하려고 하는데 이미 가입되었다고 나온다던가 등등)

여담인데, SNS 로그인이 실패했을 경우 redirect해줄 필요가 있을까요? 그냥 실패를 확인한 액션(UsersController의 twitter 액션)에서 render 메소드 등으로 사용자 수정 폼 등을 보여주면 될 것 같은데... 그렇게 하면 더 쉽게 구현할 수 있지 않을까요? ㅋ

[shaynekang]

여튼 이정돕니다.(헉헉)
어려운 기능인데 구현하느라 수고하셨고, 조금 더 보완해서 깔끔하게 완성하도록 합시다. -_-)/~

[shaynekang]

아, 사용하지 않는 코드가 종종 보이는데, 이런 건 꼼꼼하게 지워주셔야 코드가 비대해 지는 걸 막을 수 있습니다.
틈틈히 확인해서 제거해 주세요. ㅋ

[yhoonkim]

여기까지 이 이쓔 트랙팅 완료.... 후아 민혁이 화이팅!

[MoojinChae]

확인 했습니다!

[shaynekang]

음... 좀 더 살펴봐야 할텐데, 일단 현재까지 살펴본 몇 개만 피드백 하겠습니다.

1. controller :users do; end와 같은 Non-Resourceful하지 않은 라우트보다, Resourceful한 라우트를 사용해 주세요.

   레일즈의 라우트 가이드에 보면 크게 Resourceful 라우트와 Non-Resourceful 라우트, 크게 두 가지가 있습니다. 이 중 Resourceful 라우트를 적극적으로 활용하는 걸 권장합니다. 그래야 레일즈에서 제공하는 부가 혜택을 받을 수 있습니다. Non-Resourceful는 혜택이 없어서, 하다보면 이것저것 옵션을 추가해줘야 합니다.

2. 코드를 확인해보니 ApplicationController에 @my_logger ||= Logger.new("#{Rails.root}/log/my.log") 라는 파일이 있더라구요. 여기서 @my_logger는 사실상 클래스 변수처럼 동작하기 때문에, 멀티쓰레드 환경에서 문제가 생길 확률이 있습니다. 인스턴스 메소드로 빼는 걸 추천합니다.

나머지는 오늘내일 중으로 살펴보겠습니다~

[shaynekang]

#76 의 본문에서 이 Pull Request에 대한 태그를 달았습니다.
요렇게 하면 어떤 리퀘스트가 어떤 이슈를 담당하는가를 좀 더 명확하게 판단할 수 있을 듯 합니다.

앞으로는 "어떻게하면 다른 사람들이 쉽게 리뷰할 수 있을까?"를 고민해주면서 작업하시면, 다른 사람들이 좀 더 편하게 리뷰할 것 같습니다. ㅎㅎ

[minhyeok4dev]

@shaynekang 네. 리퀘스트 제목도 바꾸긴 했는데 76번 이슈에 이 리퀘스트를 참조하는 것은 깜빡했네요. 주의하겠습니다..

[shaynekang]

1. Session이 터지지 않기 위해 form의 hidden_field에 값을 집어넣어서 해결하기로 한건데, request.env["omniauth.auth"] -> session["omniauth"] -> hidden_field 순으로 집어넣으면 결국 세션에 넣는것과 마찬가지 아닐까 합니다. session을 전혀 거치지 않도록 짤 수 있을 것 같아요. 한 번 해보시겠어요?

2. 음... 저는 duplicate를 저장하는 것 보다, find_for_oauth2에서 반환해주는 게 더 깔끔한 방법이지 않을까 합니다.

   def self.find_for_oauth2(access_token)
      data = access_token.info
   
      user = User.where(provider: access_token.provider, uid: access_token.uid).first
   
      unless user
        user = User.new
   
        if User.find_by_email(data[:email])
          [user, :duplicated]
        else
         [user, :first_login]
        end
      end
   
      [user, :success]
   end

   그리고 바깥에서 switch-case를 돌려주는거죠. ㅎㅎ
   그리고 저 #find_for_oauth2 함수도 좀 더 깔끔하게 고칠 수 있을 것 같아요. 한 번 고쳐보시겠어요?

[shaynekang]

테스트해보고 있는데 다음과 같은 에러가 나네요.

앞으로는 고치고 리뷰를 요청하기 전에, 한 번 테스트를 해주세요. ㅎㅎ
테스트는 모든 상황에 대해서 해주셔야 합니다. [트위터, 페이스북, 구글] x [첫 로그인, 이메일 중복, 로그인 성공]. 그리고 몇몇 특이 케이스(트위터라던가, 이메일 중복 처리할 때 패스워드를 잘못 입력했다던가)

[minhyeok4dev]

@shaynekang 네 한번 해보겠습니다. 그리고 저 화면은 에러는 아니고 테스트하다가 값을 출력했던건데 지운다는걸 미처 확인하지 못했네요. 수정하겠습니다

[minhyeok4dev]

@DonutWorks/owners 76번 이슈의 7~9번에 대해 리팩토링 진행했습니다.

• omniauth_callbacks_controller에서 3개의 메소드로 나눠져 있던 것을 하나로 묶어 중복제거했습니다.
• 세션은 이제 더 이상 없습니다. 필요했던 변수 몇개들은 인스턴스 변수로 관리됩니다
• 로그인 시 중복 등의 상태를 리턴하는 것을 해시로 리턴하도록 했습니다.

[shaynekang]

크억, 난 class_eval 쓴거 정말 안 좋아 보이는데(..)
요렇게 작성하는게 좀 더 깔끔하고 이해하기 쉽지 않을까요?

class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController
  def authenticate(provider)
    @provider = provider
    @uid = request.env["omniauth.auth"].uid
    result = User.find_for_oauth(@provider, request.env["omniauth.auth"])

    case @provider
    when :twitter
      @nickname = request.env["omniauth.auth"]["extra"]["raw_info"].screen_name

      case result[:status]
      when :success
        @user = result[:data]
        sign_in_and_redirect @user, :event => :authentication

      when :first_login
        @user = User.new
        render sign_up_from_twitter_users_path
      end

    else
      @email = request.env["omniauth.auth"]["info"].email

      case result[:status]
      when :success
        @user = result[:data]
        sign_in_and_redirect @user, :event => :authentication

      when :first_login
        @user = User.new
        render nickname_new_users_path

      when :duplicated
        @user = User.find_by_email(@email)
        render merge_users_path
       end

    end
  end

  def twitter
    authenticate(:twitter)
  end

  def facebook
    authenticate(:facebook)
  end

  def google_oauth2
    authenticate(:google_oauth2)
  end
end

근데 왠지 메소드가 하나인 것 보다 세개인 게 더 깔끔해 보이네요.(..)
제가 볼 땐 1. twitter인 경우, 2. twitter가 아닌 경우. 요렇게 두 개를 다른 메소드로 빼면 좀 더 깔끔하지 않을까 생각합니다.

[minhyeok4dev]

@shaynekang 음 네 알겠습니다

[shaynekang]

음... 간단히 테스트를 해 봤는데, 생각보다 버그가 많네요. ㅎㄷ
코드를 수정할 때 마다, 귀찮더라도 꼼꼼하게 테스트해주시면 좋을 것 같습니다. ㅎㅎ

1. facebook으로 처음 로그인 했을 때 nickname을 입력해야 하는데, 아무것도 입력하지 않고 submit하면 에러가 납니다.(Validation failed: Nickname can't be blank)
2. 비슷하게 facebook으로 처음 로그인하고 nickname을 입력해야 하는데, nickname이 중복이면 에러가 납니다.(Validation failed: Nickname has already been taken)
3. [email protected]이라는 계정으로 회원가입을 하고, 같은 이메일을 가진 facebook 계정으로 로그인 할 경우 병합을 위해 패스워드를 입력하라고 합니다. 이때 패스워드를 잘못 입력할 경우 아무런 에러 메시지 없이 다시 로그인 페이지로 돌아갑니다.
4. 트위터로 처음 로그인할 경우 이메일을 입력해야 하는데, 아무것도 입력하지 않고 submit하면 에러가 납니다.(Validation failed: Email can't be blank, Nickname has already been taken)
5. 비슷하게 트위터로 처음 로그인할 경우 이메일을 입력해야 하는데, 중복 이메일을 입력하면 아무런 에러 메시지 없이 다시 이메일 입력 페이지로 돌아옵니다.
6. 비슷하게 트위터로 처음 로그인할 경우 이메일을 입력해야 하는데, 잘못된 이메일을 입력하면(foo@bar) 에러가 납니다.(Validation failed: Email is invalid, Nickname has already been taken)
7. 트위터로 처음 로그인하고 이메일을 입력하면 에러가 납니다.(Validation failed: Nickname can't be blank)
8. [email protected]이라는 계정으로 회원가입을 하고, 같은 이메일을 가진 facebook 계정으로 로그인한다. 패스워드를 정상적으로 입력하면 기존 계정과 facebook이 연결되는데, 이후 같은 이메일을 가진 google 계정으로 로그인합니다. 마찬가지로 패스워드를 정상적으로 입력하면 기존 계정과 google 계정이 연결됩니다. 다만 이 때 자동적으로 facebook 계정과의 연결이 끊어집니다.(로그인하고 패스워드를 다시 입력해야 함)
9. facebook으로 로그인하고, google로 로그인했을 때, 두 계정의 이메일이 동일하다면 병합할 수 없습니다.(패스워드가 없으므로)

[minhyeok4dev]

헐..

[shaynekang]

ㅋㅋㅋㅋ 오늘 밤 새야겠구만!

[shaynekang]

9번은 SNS 로그인이 아닐경우 패스워드를, SNS 로그인일 경우 해당 SNS로 로그인을 시도하는 것으로 검증해야 할 것 같습니다. 다만 이건 시간이 오래 걸릴테니, 일단 제외하고 나머지를 고치는 데 집중해주세요.
시간이 부족해서 도저히 여유가 안 난다면 8번도 일단 제외하셔도 됩니다. 나머지를 우선적으로 고쳐주시구요, 수정 후에는 꼭 테스트를 해주세요. ㅎㅎ

[minhyeok4dev]

테스트 코드 도입이 시급합니다. ㅠㅠ

[shaynekang]

흑흑... 하지만 뭐 어쩔 수 없죠. ㅋ
일단은 맨손으로 수동테스트를 하는 수 밖에...ㅠㅠ

[minhyeok4dev]

코멘트1, 코멘트2에 해당하는 예외사항들을 처리했습니다.

• validation 오류처리들은 dynamic_form gem의 error.messages를 활용했습니다
• 말씀드렸던것처럼, 한 서비스로 로그인에 성공했을 경우 이메일이 같은 다른 서비스로 로그인을 시도할 때 두 요청이 같은 사람한테서 이뤄졌다는 점을 고려하여 새로운 계정을 만들도록 하기보다는 이미 가입한 서비스로 로그인 해줄 것을 요구했습니다
• quiet_assets gem을 활용해 불필요한 로그들을 삭제했습니다

[angdev]

확인했습니다.
view에 있는 flash 보여주는 코드 부분은 #62 에서 해결했으니 굳이 안 넣어줘도 될 것 같아! (그만큼 이 이슈는 오래됐지..)
한편, User 생성할 때 param.permit!을 쓰는 건 strong parameter를 무력화시키는 것 같은데 지금 상황에서 문제가 생길 상황은 없을지 궁금한데..

[shaynekang]

@fegs 음... 문제는 없을 것 같은데, 굳이 params.permit!을 쓸 필요가 있을까요? 가령 params[:user][:email] 요렇게 해도 될 것 같은데... ㅎㄷ

[shaynekang]

1. 버그 하나 추가요~
   1. 'Test'라는 Nickname을 가진 새로운 계정을 생성한다.
   2. 'Test'라는 Nickname을 가진 트위터 계정으로 SNS 로그인을 시도한다.
   3. 이메일 입력창이 나오고, 이메일 입력을 시도한다.
   4. 'Nickname has already been taken'이라는 메시지가 나온다. 하지만 Nickname을 입력하는 필드가 없으므로, 가입할 수 없다.
2. 버그 두 개 추가요~
   1. [email protected] 이라는 이메일로 회원가입을 합니다.
   2. [email protected] 이라는 이메일을 가진 facebook 계정으로 SNS로그인을 시도합니다.
   3. 계정을 병합하기 위해 비밀번호를 입력하라는 페이지가 나오고, 비밀번호를 입력하면 성공적으로 계정을 병합한다.
   4. 로그아웃 후 다시 facebook 계정으로 로그인하면, "서비스로 이미 회원가입 되어있습니다. 해당 서비스로 로그인해주세요." 라는 메시지가 나오면서 로그인 할 수 없다.

[minhyeok4dev]

위 버그를 수정했습니다

1. 폼으로 에러 메시지가 렌더될때 nickname에 의한 에러 메시지가 전해진다면, 새로운 nickname 필드를 입력받도록 했습니다
2. 폼 뷰를 전체적으로 user 객체 하나로 활용할수 있도록 리팩토링하다보니 merge를 할 때 provider, uid를 넘겨주지 않는 실수를 했었습니다. 이를 수정했습니다.

[shaynekang]

1. if f.error_message_on(:nickname) == "" 보다는 if f.error_message_on(:nickname).blank? 가 조금 더 루비다운 문법 같습니다. ㅎㅎ

2. 병합 과정에러 이런 에러가 나네요.

   

   확인해보니 계정 병합에 관한 모든 상황에서 동일한 에러가 나는 것 같습니다.
   테스트를 꼼꼼하게 하고 계시지 않는 것 같아서 걱정입니다. 이렇게 검수할 때 마다 버그가 발견되는 건 문제가 있는 것 같아요. 이전에 이야기한대로, 기능을 구현한 다음 '일어날 수 있는 모든 상황'에 대해서 테스트를 진행해주세요.(제가 해 본 바로는 경우의 수가 대략 10~20개 정도 되네요.) 커밋은 그 다음입니다.

[minhyeok4dev]

• blank? 를 사용했습니다
• 실수가 있었던 부분의 버그를 수정합니다.

또한 아래처럼 테스트 상황을 직접 적어보고 이에 따라 한번씩 다 테스트를 진행해봤습니다

[shaynekang]

ㅇㅋ 확인했습니다. 수고하셨습니다. ㅎㅎ
다들 한 번 확인해보시고, 마지막으로 확인한 사람이 Merge하도록 하죠. ㅎㅎ @MujinChae @yhoonkim @fegs

[minhyeok4dev]

우와앙 감사합니다

[MoojinChae]

추카추카 확인!

[angdev]

확인했습니다! 수고하셨습니다 ㅋㅋ