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

OAuth implementation. #91

Merged
merged 7 commits into from
Oct 13, 2024
Merged

OAuth implementation. #91

merged 7 commits into from
Oct 13, 2024

Conversation

uakihir0
Copy link
Owner

@uakihir0 uakihir0 commented Oct 11, 2024

全体的にまだ色々ブラッシュアップをする予定

  • ハッシュのためだけにライブラリを入れているのでそれを削除
  • デバッグプリントが至る所にあるので削除
  • OAuth の流れについて document を書く (別 PR 予定)

終わったら 0.3 のバージョンを切る予定。

@uakihir0
Copy link
Owner Author

@takke だいぶ参考にさせてもらったので、レビューお願いしたいです!

@uakihir0 uakihir0 self-assigned this Oct 11, 2024
Copy link
Contributor

@takke takke left a comment

Choose a reason for hiding this comment

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

ざっとコードを読んだだけですが(手元での動作確認はできていませんが)、

  • 従来の accessJwtAuthProvider にラップして、BearerTokenAuthProviderOAuthProvider に分離する
  • getWithAuth, postWithAuth でリクエストの前後をフックして、認証ヘッダーの設定と(DPoPNonceの)収集を行う

という流れは自分が OAuth 対応するならやろうと思っていたことのイメージと合っているので、この流れでいい感じだと思います!

Comment on lines 12 to 16
fun postProcess(
method: String,
request: HttpRequest,
response: HttpResponse,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

postProcess という名前がちょっぴり HTTP POST Reqeust と勘違いしそうです(最初、勘違いしました・・・)。
できれば変更してもらえると読みやすくなるかなーと思いました。

他の候補を copilot に聞いてみました。

  • afterRequestProcessing
  • postRequestHook
  • postRequestHandler
  • finalizeRequest
  • afterRequestCompletion

@@ -59,7 +63,7 @@ object _InternalUtility {
inline fun <reified T> proceed(function: () -> HttpResponse): Response<T> {
try {
val response: HttpResponse = function()
if (response.status == 200) {
if ((response.status >= 200) && (response.status < 300)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (response.status in 200..299) {
のように書くと直感的かもしれません

Comment on lines 9 to 12
var redirectUri: String? = null

var state: String? = null
var codeVerifier: String? = null
Copy link
Contributor

Choose a reason for hiding this comment

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

ライブラリの利用者は AuthProvider として OAuthProviderOAuthContext を用意して作ることになると思います。
redirectUri, state, codeVerifier は厳密には OAuth の認証・認可の処理だけで必要なので、理想的には別のクラスに(つまり認証・認可用とそれ以降のAPIアクセス用に分離)したほうがいいのかなーと思いました。

が、影響がけっこう大きいので今後作成されるREADMEにどのメンバーを用意すればいいかが書かれていればひとまず十分かなーと思いました。

@uakihir0
Copy link
Owner Author

@coderabbitai please review

Copy link

coderabbitai bot commented Oct 11, 2024

@uakihir0, I will review the changes.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@uakihir0 uakihir0 merged commit 68e1a97 into main Oct 13, 2024
1 check passed
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.

2 participants