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

feat: 認証機構導入、いくつかでもページ作成 #115

Merged
merged 46 commits into from
Nov 20, 2021

Conversation

LumaKernel
Copy link
Collaborator

@LumaKernel LumaKernel commented Nov 20, 2021

ℹ️ Info for dev

  • firebase docker コンテナの復帰を復帰させました。使い方については CONTRIBUTIN-ja.md を確認してください。
    • 元あったものとは少し違うのでビルドを忘れずにお願いします
  • 認証が使えるようになります。書き方・使い方は web, api の /dev/auth/... にあるデモを参照してください。
  • も参照
  • Modal を少しだけリファクタリングしました

Changes

  • src/web/... 系は baseUrl を消して禁止に。jest でトラブったので (おそらくちゃんと設定すれば問題ないとは思いますが)
  • frourio --watch が消えてしまっていたので復帰。ただ、 frourio そのままだと無限ループになってか JS Stack trace (heap out of memory などのやつ) が起こる
    • とりあえず onchange で最低限のものだけ見るようにした
    • 別にこうしたいというわけではないので問題なさそうだったら戻してください
  • env 系のファイルのリファクタリング
    • もしかしたら infra 側とつなぐかもしれないというのを少し意識
  • Cookie 準備
    • の導入
    • TODO: cookie secret を Secrets Manager から流す
  • Google Cloud Identity Provider を使うための準備
    • firebase-admin とその周辺はバックエンド側のクライアント
    • firebase とその周辺はフロントエンド側のクライアント
    • 開発時は firebase emulator を使用。デフォルトで入っているユーザは contributing に記載
    • その他の詳しい話は を参照
    • セッション保持を http-only cookie でしています
    • メール認証はデバッグのしやすさのためにおいています。 staging 以降は消す、とする予定です
  • とその関連
    • pkg/api/src/service/firebase-admin 周辺で firebase-admin <-- simple convert --> google-auth-library <-- workload identity federation --> @aws-sdk/credential-providers { fromContainerCredentials } とつないでいる
      • 前半2つをつなぐために内部 API を触っているのでこれらにフィードバックしていく必要がありそう
      • 後半2つは公開 API のみ使ったが、 ECS、AWS SDK v3 対応をするための Client を作っているので、これを google-auth-library 側に提案できそう
    • google-auth-library: google-cloud-node の認証で使われている。 AWS SDK v3 で言うところの @aws-sdk/credential-providers
      • firebase-admin の内部でも使われているが、このパスだとかんたんな形ではうまくいかない
  • API に認証用のルートを用意
    • /api/auth/session:POST: id token を渡して検証し、セッションをクッキーに (サインイン)
    • /api/auth/session:DELETE: セッションをクッキーから削除 (サインアウト)
    • /api/auth/user:GET: ユーザークレームを取得
      • これに関連付けたユーザ情報は、このユーザークレーム内の (sub) と関連付けたデータを DB に保存して、別のルート (/api/user など) で返してください
  • CSRF 準備
    • の導入
    • /api/csrf:GET: CSRF トークン取得。 CSRF シークレットをクッキーにセットするのもここで行われている
    • このエンドポイントは useApi() 内で一番最初に叩かれる。
      • すべての GET は CSRF なしで使えるので、 useAspidaSWR は csrf なしの状態でキャッシュされてしまうがここは問題ない
      • TODO: 基本的には問題ないが、POST が CSRF トークン取得前に起こる場合に失敗するので、それを調整したい
      • (OPTIONS も素通りする for fetch preflight)
  • fastify logging のリファクタリング
    • 大量の情報が出てきてあまり有用ではなかった。その他、センシティブな情報は出さないようにするなど
  • bin/... の JS エントリーポイントをなくした。TS で require() を使って順序保証
  • /dev/... というルートを web, api ともに作成、コピペ用。ずっと役立ちそうだから CD ビルド時に消すか、ロードバランサで蹴ってずっと残せるのを検討中
  • Modal の軽いリファクタリング

デモ

スクリーンキャスト: GitHub OAuth 認証使うデモ

demo-local-github

  • ログインしていないと 401 が返ります
  • リロードしてもサインインしたままです
スクリーンキャスト: エミュレータでメール認証使うデモ

demo-local-email

  • ログインしていないと 401 が返ります
  • リロードしてもサインインしたままです
  • メールであればかんたんに別のアカウントを使ってのテストもできます

issues

@@ -0,0 +1,17 @@
## googleapis/google-auth-library-nodejs awsclient.ts
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

awsclient.ts をコピペして改造したのでライセンス明記しています

env_file:
- ./.env
working_dir: /opt/workspace
command: firebase emulators:start --only=auth --project=emulator --import=seeder
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

起動コマンドが以前と少し違います。ログインは必要ありません。トンネルでも使えると思います

Copy link
Collaborator

Choose a reason for hiding this comment

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

詳細理解できてないけど、私がセットアップしたときにはオフライン対応わからなくて諦めたんだよなー
これは非常にありがたい

const auth = getCachedApp({ env, logger }).auth()
// 5 days
const expiresIn = 60 * 60 * 24 * 5 * 1000
const sessionCookie = await auth.createSessionCookie(idToken, { expiresIn })
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

日数は要相談ですが、5 days でもワンクリックでログインできるのでそんなに大変でもないです

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

長期のセッション保存を大手に任せて、自分側では短くするというのは方針としては正しいと思っている
あまり長くすると、GitHub 側で revoke session してもずっと入れてしまうのでこれでも長いくらいか

Copy link
Collaborator

Choose a reason for hiding this comment

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

個人的にはサイトを開いている間自動でリフレッシュしてくれればトークンは短命でもいい

@@ -0,0 +1,55 @@
import { Portal } from '@violet/web/src/components/atoms/Portal'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cardmodal

modal のリファクタリング。かなり適当ではありますが、とりあえず最低限。
Card は molecules に移せるかもということでこの命名。

onClose?: () => void
}

export const CardModal: React.FC<Props> = ({ open, children, onClose }: Props) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

open は prop で渡したほうがよいかなと思います。 1. HTML 標準に合わせる 2. たぶんアニメーションを CSS の範囲外のことをしようとしたときに柔軟に対応できるのはこっちの書き方

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • 親側の複雑度が1減る
  • フェードアウトアニメーションが実装できる
    という利点もあるのでPropsで制御するのは良い

@@ -11,7 +11,7 @@ const InputFormProject = styled.form`
justify-content: center;
`

export const ProjectNameInput = (props: { inputCompleted: () => void }) => {
export const ProjectNameInput = (props: { onComplete?: () => void }) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

細かいリネーム。 on... 使う、 => void という形式にして、 optional にする、といった統一をすると良いかも

@maihrs55 @shuheiest

border: none;
border-radius: 16px;
`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

一箇所だけコメントのこします。 SecondaryButton は Button を先にatom にして molecules においたほうがいいやつかな。ちょっと全部にベタ書きして雑にやりましたが… Column も同様です

Copy link
Collaborator

Choose a reason for hiding this comment

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

教育観点を除けば、実際に再利用するときまでべた書きで良いと思っている
200行ルールだけ気を付ければ後からコンポーネント分割に苦労することは経験的にほぼない

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

実際に再利用するときまでべた書き

これはよくやりますね。単純にこの PR でやると詰め込み過ぎなのと、あまりフロントエンドまで手が回らないのでおまかせしたい。

"target": "es5",
"lib": ["dom", "dom.iterable", "esnext"],
"baseUrl": ".",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

'src/...' の形式で import できていたのはこれです。 別にいいと思いますが jest 側のエラーが出たので @violet/web/src/... で一旦統一

it.skip('matches snapshot', async () => {
const { asFragment, findByText } = render(
<>
<ApiProvider>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

テストするなら (場合によってはモックした) Provider をセットしてあげる必要がありますが、どちらにせよこのままだと Auth 関連で動かないので skip

if (typeof window === 'undefined') return null
if (process.env.NODE_ENV === 'production') return createAuth()
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- prevent HMR
return ((window as any)._firebase_auth = (window as any)._firebase_auth ?? createAuth())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

HMR 環境でキャッシュさせるにはこうする必要があります

prisma のドキュメント で見かけたのを参考にした

@LumaKernel
Copy link
Collaborator Author

LumaKernel commented Nov 20, 2021

  • ローカルでの upload & convert
  • プレビュー環境での upload & convert

上動かないかも、これだけちょっと解決します…

@LumaKernel
Copy link
Collaborator Author

_csrf クッキーが path=/ 以外に残っているのが原因だった。実装は問題なし

// その後、Firebase Admin Library で使える形にする
// ExternalAccountClient や AwsClient を直接使わないのは、これらが instance metadata にしか対応していないため

/* eslint-disable max-lines,@typescript-eslint/no-explicit-any */
Copy link
Collaborator

@solufa solufa Nov 20, 2021

Choose a reason for hiding this comment

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

max-linesを無効にしているのは今後200行超える予定があるとか?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

失礼しました、消し忘れです! resolved by f7c0648

photoURL: string
displayName: string
const focusRefreshIntervalMilliseconds = 1000 * 60 * 5
const autoRefreshIntervalMilliseconds = 1000 * 60 * 60
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

うーん。普通に SWR でいい気がしてきた。申し訳ない、ちょっと調整します

Copy link
Collaborator

Choose a reason for hiding this comment

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

おk、待ちます

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

resolved by 47b6596

  • ローカルで使用感が変わらないことを確認

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • プレビュー環境も確認

オッケーです!

Copy link
Collaborator

@solufa solufa left a comment

Choose a reason for hiding this comment

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

単純に行数多くて把握しきれていないが、一つ一つの目的は理解できた
ローカル開発の認証をFirebaseエミュレータに切り替える発想は大変面白い
コントリビュートのハードルを低く維持出来るのが良い

useEffect(() => {
setCurrentUser({ photoURL: '', displayName: '' })
}, [])
const initialized = useMemo(() => userClaims !== undefined, [userClaims])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

useAuth の使用者に !== undefined とさせることもできますが、ライブラリが undefinednull の意味が違う API を露出させるとバグを不用意に増やしそうなので、ライブラリ側で分離

@solufa
Copy link
Collaborator

solufa commented Nov 20, 2021

毎度ながら素晴らしい仕事ぶり

@solufa solufa merged commit d02e18a into violet-ts:main Nov 20, 2021
@LumaKernel LumaKernel deleted the feat/auth branch November 20, 2021 09:20
@LumaKernel
Copy link
Collaborator Author

どうなるかはわからんが Google Auth Library への PR 作ってみた。
googleapis/google-auth-library-nodejs#1321

@solufa
Copy link
Collaborator

solufa commented Nov 21, 2021

めっちゃ丁寧なPRだ

@LumaKernel
Copy link
Collaborator Author

関連 issue をもう一つ作った: googleapis/google-auth-library-nodejs#1322

@LumaKernel LumaKernel added aws AWS relevant gcloud Google Cloud relevant security labels Nov 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

認証 (Cognito、Identity Platform、...) 関連の実装
2 participants