Qiita login#8
Conversation
tomoasleep
force-pushed
the
qiita-login
branch
2 times, most recently
from
e3b5ad2 to
4cb7f0e
Compare
|
@takashi @yujinakayama レビューお願いします 🙏 |
|
@takashi の助言でCookieStoreにAuthHash保存するようにした。 |
|
そういえば、メール認証をしないとログインできないので新規登録までに、
…と使えるようになるまでに2回Qiitaで参加ボタンを押す必要があるのが面倒だ… |
There was a problem hiding this comment.
OauthAuthorization という名前、 Authorization と2回行っているようで不自然なのと、 Oauth(という仕様)とのAuthorization のようにとれ、違和感があります。
この場合 Qiita との Authorization なわけなので、QiitaAuthorization とするのはどうでしょうか
There was a problem hiding this comment.
ここで password をユーザの代わりに追加しているけど、このパスワードはユーザーに知らされる？
There was a problem hiding this comment.
知らされないので、パスワードを再発行するまでパスワード認証は実質利用できない状態です。
There was a problem hiding this comment.
dummy_password_flag を足して初回だけ現在のパスワードを入力しなくてもパスワードを変更できるようにした
初回のみ現在のパスワードを入力するinputを非表示にしている。
There was a problem hiding this comment.
最初にパスワードを変更する場合だけ 新しい じゃなくした
There was a problem hiding this comment.
cookie store を使用しているならば、serializer を marshal にしていても json にしていても hash はそのまま格納できるはずです。 to_json 必要なさそう
tomoasleep
force-pushed
the
qiita-login
branch
4 times, most recently
from
0057a13 to
b7aee85
Compare
新規登録の認証の仕様の変更現在の仕様だと、Qiitaアカウントで新規登録してログインした状態になるまで都度2回QiitaのOAuthの認可画面を表示する必要があるので、新規登録してから1日の間はメール認証しなくてもログイン出来るように仕様を変更する。その場合、画面上部にアラートが表示される。
|
負荷対策用の、新規登録の制限(負荷対策のために、)管理画面からQiita連携以外での新規登録を停止することも出来るようにする。
|
|
@takashi 再度レビューお願いします |
There was a problem hiding this comment.
今更なのですが、 ここで行っているのは結局 QiitaAuthorization モデルの作成ですし、Setting の方も QiitaAuthorizationsController なのでAuth::QiitaAuthorizationsController が適当なのではないかと思います 🙇
There was a problem hiding this comment.
QiitaAuthorizationsを確かに作成してはいるのですが、どちらかと言うとここはDeviseの提供する仕組みの上でOmniauthのcallbackを処理する性格のほうが強いので、Deviseの命名に寄せていた方が良いと考えているのですが、どう思いますか？
There was a problem hiding this comment.
Omniauthのcallbackを処理するが、実際には Oauth Server 側 (今回だとQiita) で明示的にResource Owner のリソースを OauthClient 受け渡すための callback url を記述しているので、omniauth という gem が提供している仕組みではあるが QiitaAuthorization を作成する、という考え方がしっくりくるのかな、と思った。
けど特に強い意見があるわけではないのでこのままでも大丈夫です 🙆♂️
There was a problem hiding this comment.
補足すると、 qiita というアクションに違和感があって、 アクションは原則 crud のみで構成されるべき、という rails の controller の実装的にこうすればいいんじゃないか、とおもった、というのもあります (Qiita:Authorization#create でもいいのではという意味)
There was a problem hiding this comment.
qiita というアクションに違和感があって、 アクションは原則 crud のみで構成されるべき、という rails の controller の実装的にこうすればいいんじゃないか
これはたしかにそうだと思いますが、今回は素直に提供する仕組みに乗っかって様子見ようと思います。forkなので。
There was a problem hiding this comment.
付けといたほうが良さそうですね、thx 🙇
There was a problem hiding this comment.
Mastodonはja, en以外のlocaleを用意していて、それらの他のlocaleを使用している場合に使われます。
There was a problem hiding this comment.
この 「current_user が存在した場合」というのは 既存の password 認証のアカウントに Qiita アカウントに紐付ける というプロセスなので、今すぐには必要ない部分ですか？(消す必要はないと思っているが確認です)
|
@takashi 修正 or コメントしたので再度レビューお願いします 🙏 |
There was a problem hiding this comment.
1日の期限をもってログインできなくなるよ、という旨をここで書いたほうが親切かなと思ったけどどうでしょう？
|
@takashi レビューthx 🙏 |
Conflicts: Gemfile Gemfile.lock app/javascript/mastodon/features/compose/index.js app/serializers/initial_state_serializer.rb app/views/about/_registration.html.haml app/views/admin/settings/edit.html.haml app/views/auth/registrations/edit.html.haml app/views/shared/_og.html.haml config/initializers/omniauth.rb package.json Look at f6df9d0 . The diff is too difficult to read however, in file app/views/auth/registrations/edit.html.haml, `simple_form.labels.defaults.password` was suddenly replaced to `simple_form.labels.defaults.new_password`. This can be proved by this file on ef27a0b which is a parent commit of that. However, this replacement has been continued for about 3 years. And there is no significant difference. So, I decided not to recover this replacement. ref: - 1cd0497 - increments#8 - f4d549d - mastodon#8703
2 participants
QiitaのOAuthを利用してアカウント作成とログインが行えるようにする。
仕様
/about)とログイン画面 (/session/new) に「Qiitaアカウントで参加ボタン」が追加される- ユーザーには知らされないので再設定するまでパスワードログインできない
- 設定画面からパスワードを設定することが出来る。最初の1回だけ現在のパスワードを入れずにパスワードを変更することが出来る
動作を見たい場合の注意
QIitaで自分のアカウントでアプリケーションを作成し、
.envに適宜以下の内容を追加しておいてください。