Repository navigation
Conversation
github-actions
Bot
added
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the new |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7490 +/- ##
=======================================
Coverage 86.80% 86.80%
=======================================
Files 895 896 +1
Lines 65684 65697 +13
Branches 9385 9386 +1
=======================================
+ Hits 57015 57028 +13
Misses 5189 5189
Partials 3480 3480
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| .client( | ||
| authenticatedOkHttpClient | ||
| .newBuilder() | ||
| .addInterceptor(deviceIdentifierInterceptor) |
There was a problem hiding this comment.
Why add this, I thought it was only needed for unauthenticated requests?
There was a problem hiding this comment.
Config call goes through the authentaticated requests even before the user is authenticated.
Despite the task do not requiring it specifically for this method, I have checked what happens on the web client and they do have the device-identifier parameter on the config call.
To keep parity with other clients we need this
| */ | ||
| @POST("/auth-requests") | ||
| suspend fun createAuthRequest( | ||
| @Header("Device-Identifier") deviceIdentifier: String, |
There was a problem hiding this comment.
I see that @GET("/devices/knowndevice") is using @Header(value = "X-Device-Identifier"), should that be updated?
There was a problem hiding this comment.
That is a different parameter that exists specifically on known device at server side
There was a problem hiding this comment.
Gotcha, thanks for checking!
| .client( | ||
| baseOkHttpClient | ||
| .newBuilder() | ||
| .addInterceptor(deviceIdentifierInterceptor) |
There was a problem hiding this comment.
I assume that external retrofit instances don't need this (fill assist) but what about the static ones?
This does not cover that
There was a problem hiding this comment.
It might be worth adding something like this:
private val unauthenticatedOkHttpClient: OkHttpClient by lazy {
baseOkHttpClient
.newBuilder()
.addInterceptor(deviceIdentifierInterceptor)
.build()
}
And then use it in the appropriate places
There was a problem hiding this comment.
🤔 are not all our static calls external requests?
| .client( | ||
| baseOkHttpClient | ||
| .newBuilder() | ||
| .addInterceptor(deviceIdentifierInterceptor) |
There was a problem hiding this comment.
It might be worth adding something like this:
private val unauthenticatedOkHttpClient: OkHttpClient by lazy {
baseOkHttpClient
.newBuilder()
.addInterceptor(deviceIdentifierInterceptor)
.build()
}
And then use it in the appropriate places
| override fun intercept(chain: Interceptor.Chain): Response { | ||
| val request = chain.request() | ||
| if (request.header(name = HEADER_KEY_AUTHORIZATION) != null) { | ||
| return chain.proceed(request = request) |
There was a problem hiding this comment.
I don't think you need this if we only apply this interceptor to the appropriate instances of Retrofit.
There was a problem hiding this comment.
Based on the other discussions, we do still need this.
My last question would be, could we just send the header on all requests?
This branch has not been deployed
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-43467
📔 Objective
Server scopes OTPs (new device, 2FA email) to
Device-Identifierheader. Android didn't send it onresend-new-device-otp/send-email-login.DeviceIdentifierInterceptor: addsDevice-Identifierwhen request has noAuthorization(same rule as web clients). Covers future calls too.GET /configuses it and is also called pre-login with no token (server uses the device ID for feature-flag targeting). Interceptor runs afterAuthTokenManager, so calls with a token are untouched.DeviceIdentifierfromResendEmailRequestJsonbody and redundant per-call header oncreateAuthRequest.X-Device-Identifieronknowndevice,/connect/tokenform field,/auth-requestsbodydeviceId, header oncreateAdminAuthRequest.After release, update PM-43467 to start the 3-release deprecation countdown.
🤖 AI-assisted review
code-review-localon full branch diff vsorigin/HEAD. Approved, no findings, none deferred.