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

Feature: Add Google Auth #128

Merged
merged 106 commits into from
Mar 28, 2024

Conversation

kyosuke-kobayashi-lvgs
Copy link
Contributor

@kyosuke-kobayashi-lvgs kyosuke-kobayashi-lvgs commented Mar 5, 2024

Issue #, if available:

Description of changes:
SSOできるようにしました
※別途GCPで認証プロバイダーのClientKeyとSecretをSecretManagerに登録する必要があります

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@kyosuke-kobayashi-lvgs kyosuke-kobayashi-lvgs changed the title Fix backend to accept access token SSO(Google Auth) Mar 5, 2024
@kyosuke-kobayashi-lvgs kyosuke-kobayashi-lvgs marked this pull request as ready for review March 6, 2024 05:16
@statefb statefb self-assigned this Mar 6, 2024
cdk/lib/constructs/auth.ts Outdated Show resolved Hide resolved
cdk/lib/constructs/auth.ts Outdated Show resolved Hide resolved
@statefb
Copy link
Contributor

statefb commented Mar 7, 2024

@kyosuke-kobayashi-lvgs PRありがとうございます!!
上記のコードのコメントのほか、簡単なもので大丈夫ですので、Googleを利用する場合の手順をdocs下またはREADME.mdへ記載いただけないでしょうか?(概要だけ記述し、詳細はリンク先参照などの体裁で大丈夫です)
お手数おかけしますが、よろしくお願いしますmm

@statefb statefb changed the title SSO(Google Auth) Feature: Add Google Auth Mar 7, 2024
@kyosuke-kobayashi-lvgs
Copy link
Contributor Author

@statefb
簡単ですが
fbdb418
こちらでREADME.md更新しました!

@statefb
Copy link
Contributor

statefb commented Mar 8, 2024

@kyosuke-kobayashi-lvgs
READMEありがとうございます!

  • リンクの作業だけでは完結しないと思いますので、必要な作業を追記いただけますか?
    • SecretsManagerの作成
      • 事前に作成が必要である点を明記してください。リンク先の手順を踏んで、client id / secretを控えた上で、どのようにSecret managerを作成すれば良いか記載ください。
      • 現在ハードコードされていると思いますので、cdk.jsonでconfigurableとなるようにしてください。例:
const oidcSecret = secretsmanager.Secret.fromSecretNameV2(this, "Secret", "cdk.jsonからの値");
  • cognitoドメインの指定
    • ユニークであることが条件であり、変更の必要があるかと思いますので、その旨記載してください
  • また、デプロイ後にリダイレクトURLの指定がGoogle側に必要となると思いますので、値をCfnOutputで出力する実装をauth.ts下に実装いただけますか?また、その値をGoogle側へ設定する必要がある旨も合わせて記載してください。
  • Google SSOをoptionalの扱いにしていただけませんか?
    • 現状の実装ですと、Google SSOを使う予定のない方でもログイン画面に選択肢が表示されてしまうと思います。また、Google SSOが不要の場合はsecrets managerの作成、値の取得およびgoogle providerの作成も不要だと思いますので、その点も考慮いただけませんか?(cdk.jsonに記載した内容を元にリソースの作成可否を制御できると良いです。git pull後何もせずにcdk deployすると、Google関連のリソースは一切作成されない挙動が望ましいです)
    • 将来的にはAzureAD連携なども考えられるため、Googleに特化しない実装を意識いただけるとありがたいです

最初にお伝えすべき点もいくつかあり後出しのような形になってしまい申し訳ありません。よろしくお願いします!

@statefb
Copy link
Contributor

statefb commented Mar 8, 2024

(本PRとは別ですが)contribution自体はとても嬉しいです!!!!ですが、機能を実装される前に、一度Issue -> Feature Requestを起票し、一度相談していただけると大変助かりますmm

@kyosuke-kobayashi-lvgs kyosuke-kobayashi-lvgs changed the title Feature: Add Google Auth [WIP]Feature: Add Google Auth Mar 8, 2024
@kyosuke-kobayashi-lvgs
Copy link
Contributor Author

@statefb
こちらcdk Fine-grained Assertions Testの実装も完了しました!
お手隙でご確認いただけますと幸いです!

@kyosuke-kobayashi-lvgs
Copy link
Contributor Author

合わせてcdk.yml にcdk test のjobを追加しております!

}
);

hasGoogleProviderTemplate.hasResource("AWS::Cognito::UserPoolClient", {
Copy link
Contributor

Choose a reason for hiding this comment

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

Dependsのテストを書かれているのには何か理由がありますか?
Logical IDはpath依存のため今後リファクタリングが行われた場合こちらのテストも書き直す必要が生じると思いますが、手間と必要性を天秤にかけると無くても良いかと思いました
参考

CDK generates the Logical IDs based on the full Construct “path”. With nested Constructs, IDs of all “higher” Constructs are used to create the unique Logical ID. So when the path changed from MyQueue to MyCustomQueue/MyQueue, the generated Logical ID changed from MyQueueE6CA6235 to MyCustomQueueMyQueue20F468EB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

論理 ID はパス依存のため今後リファクタリングが行われた場合こちらのテストも書き直す必要が生じると思いますが

こちらの観点考慮できておりませんでしたmm

現在のhasResourcePropertiesでの式でも、今回テストしたい範囲は十分にテストされているかと思うのでご指摘いただいた通り、該当テスト削除します!

});
const template = Template.fromStack(stack);

// template.hasResourceProperties("AWS::Cognito::UserPool", {
Copy link
Contributor

Choose a reason for hiding this comment

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

WIPは消してください!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

すいません!失礼しました!

@statefb statefb merged commit 58a7ee2 into aws-samples:main Mar 28, 2024
3 checks passed
@statefb
Copy link
Contributor

statefb commented Mar 28, 2024

LGTM!!

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.

3 participants