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

ユーザー認証のmiddleware作成 #28

Merged
merged 14 commits into from
Oct 16, 2023

Conversation

Yuya-Furusawa
Copy link
Owner

@Yuya-Furusawa Yuya-Furusawa commented Sep 2, 2023

#21

@Yuya-Furusawa Yuya-Furusawa marked this pull request as ready for review September 17, 2023 16:28
@Yuya-Furusawa
Copy link
Owner Author

@AtsukiTak
こちらも問題なければMergeお願いします🙏

@AtsukiTak
Copy link
Collaborator

auth_middleware のテストの追加と、 認証が必要な他のhandlerにも auth_middleware を追加するのをやってほしいです!

@Yuya-Furusawa
Copy link
Owner Author

それ忘れていた!追加します🙇‍♀️

@Yuya-Furusawa Yuya-Furusawa force-pushed the feat/create-user-auth-middleware branch from 24ba8f2 to fdb631e Compare October 13, 2023 05:24
@Yuya-Furusawa
Copy link
Owner Author

以下に認証を追加

  • ユーザー情報の取得(/users/:id
  • ユーザー情報の削除(/users/:id
  • クエスト参加(/quests/:id/participate
  • チャレンジ達成(/challenges/:id/complete

Comment on lines +48 to +53
let secret_key = "secret_key".to_string();
let test_user_id = "test_user".to_string();
let now = Utc::now();
let iat = now.timestamp();
let exp = (now + Duration::hours(8)).timestamp();
let valid_session_token = create_jwt(&test_user_id, iat, &exp, &secret_key);
Copy link
Owner Author

Choose a reason for hiding this comment

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

特にuser_idを使用するわけではないのに、user_idをトークン化してミドルウェアで認証する設計になっているのが個人的に違和感があるのだがそういうもん?

Copy link
Collaborator

Choose a reason for hiding this comment

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

user_id いろんなとこで使ってるくない?

Copy link
Collaborator

Choose a reason for hiding this comment

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

各handlerで使ってるけどそういうことではない?

Copy link
Owner Author

Choose a reason for hiding this comment

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

例えば/users/:idでもauth_middlewareを使ってuser_idを取り出しているんだけど、user_id自体はPathで渡してるからミドルウェアでuser_idを取り出してるのは意味ないなぁと思った(ちゃんと)
/quests/:id/participateとかもuser_idはリクエストボディで渡してるからミドルウェアでuser_idを取り出している意味がなさそうだけど、それはリクエストボディでuser_idを渡すんじゃなくて認証で取り出せば良いように設計し直すことはできそう

Copy link
Owner Author

Choose a reason for hiding this comment

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

ミドルウェアで取り出したuser_idがPathのuser_idとかリクエストボディのuser_idと一致するかどうかをチェックするってことか(?)

@Yuya-Furusawa
Copy link
Owner Author

@AtsukiTak
レビューお願いします!🙇‍♀️
現状、認証情報が間違っていても(headerにsession tokenが入ってないとか)エラーにならず空のレスポンスが返ってくる感じなんだけど、ちゃんとエラーを返すようにするべき?

@AtsukiTak
Copy link
Collaborator

ユーザー情報の取得とか削除時に、user_idのチェック行ってないっぽい?

@Yuya-Furusawa
Copy link
Owner Author

めっちゃ勘違いしてたw
Tokenをdecodeするだけでは認証じゃないな

@Yuya-Furusawa
Copy link
Owner Author

@AtsukiTak
再度修正しました🙏
PR大きくなってすまない

@AtsukiTak
Copy link
Collaborator

基本的にはLGTMです!!
何点か、直してもいいかなって箇所あるので共有しておく。

  1. Requestのpathとかbodyで user_id を渡す必要はないかも。クライアントが自分の user_id を知っている必要がなくなるし、認証漏れもなくなる。
  2. 認証は、middlewareでやるより Extractor でやるほうが筋が良さそう。参考実装

別PRでやるのであればこのPRはマージしてそれぞれissue立てておいてほしい!

@Yuya-Furusawa
Copy link
Owner Author

了解!一旦このPRはMergeしてIssue立てます!

@Yuya-Furusawa Yuya-Furusawa merged commit 572c5f8 into main Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants