TASK-033 correction: starring needs write:user AND write:repository together
Two live-confirmed 403 responses (not source reading, which had already
given one wrong answer) showed /user/starred/{owner}/{repo} requires
BOTH scopes at once: requesting write:repository alone got
required=[write:user]; requesting write:user alone then got
required=[write:repository]. Both are now requested.
Added debug-build-only HTTP status/body logging (HttpTransport
.requireSuccess) and toggleFavorite failure logging, which is how this
was actually diagnosed -- ground truth from the server beats re-reading
Go route source through a summarizing tool. Required testOptions {
unitTests { isReturnDefaultValues = true } } in build.gradle.kts so
those Log calls don't throw inside plain JVM unit tests.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
72de43d273
commit
b38ddc99e3
@ -43,6 +43,17 @@ android {
|
|||||||
packaging {
|
packaging {
|
||||||
resources.excludes += "/META-INF/{AL2.0,LGPL2.1}"
|
resources.excludes += "/META-INF/{AL2.0,LGPL2.1}"
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Debug-build-only diagnostic logging (e.g. HttpTransport.requireSuccess, RepositoryListViewModel
|
||||||
|
// .toggleFavorite) calls android.util.Log directly. Plain JVM unit tests run against an unmocked
|
||||||
|
// android.jar where every framework call throws by default; this makes them return harmless
|
||||||
|
// defaults (0/null/false) instead, so a debug-only Log.w doesn't fail tests that legitimately
|
||||||
|
// exercise the code path around it.
|
||||||
|
testOptions {
|
||||||
|
unitTests {
|
||||||
|
isReturnDefaultValues = true
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
kotlin {
|
kotlin {
|
||||||
|
|||||||
@ -13,14 +13,16 @@ data class OAuthClientConfig(
|
|||||||
require(clientId.isNotBlank()) { "OAuth client ID cannot be blank" }
|
require(clientId.isNotBlank()) { "OAuth client ID cannot be blank" }
|
||||||
require(redirectUri.isSecureRedirect()) { "OAuth redirect URI must be HTTPS without query or fragment" }
|
require(redirectUri.isSecureRedirect()) { "OAuth redirect URI must be HTTPS without query or fragment" }
|
||||||
require(scopes.isNotEmpty()) { "At least one OAuth scope is required" }
|
require(scopes.isNotEmpty()) { "At least one OAuth scope is required" }
|
||||||
// Deliberately narrow, not "any write scope goes": WRITE_REPOSITORY is the one specific
|
// Deliberately narrow, not "any write scope goes": WRITE_USER and WRITE_REPOSITORY are the
|
||||||
// exception, carved out because Gitea's star/unstar endpoints require it (verified against
|
// two specific exceptions, carved out because Gitea's star/unstar endpoints require BOTH --
|
||||||
// Gitea's own route registration -- PUT/DELETE /user/starred/{owner}/{repo} is under
|
// confirmed directly from this instance's own live error responses (not just source
|
||||||
// tokenRequiresScopes(AccessTokenScopeCategoryRepository) with a Write access level) and
|
// reading, which named only one category and turned out to be incomplete): requesting only
|
||||||
// the human product owner explicitly approved requesting it for that feature. Every other
|
// write:repository got `required=[write:user]`; requesting only write:user then got
|
||||||
// write scope (e.g. WRITE_USER) is still rejected here exactly as before.
|
// `required=[write:repository]` right back. /user/starred/{owner}/{repo} apparently checks
|
||||||
require(scopes.all { it.isReadOnly || it == OAuthScope.WRITE_REPOSITORY }) {
|
// both the /user/ prefix's own scope requirement and the target repository's, separately.
|
||||||
"Only read-only OAuth scopes (plus the narrow write:repository exception for starring) are allowed"
|
// The human product owner explicitly approved requesting write access for this feature.
|
||||||
|
require(scopes.all { it.isReadOnly || it == OAuthScope.WRITE_USER || it == OAuthScope.WRITE_REPOSITORY }) {
|
||||||
|
"Only read-only OAuth scopes (plus the narrow write:user/write:repository exception for starring) are allowed"
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -50,10 +52,13 @@ enum class OAuthScope(val wireValue: String, val isReadOnly: Boolean) {
|
|||||||
READ_ORGANIZATION("read:organization", true),
|
READ_ORGANIZATION("read:organization", true),
|
||||||
READ_ISSUE("read:issue", true),
|
READ_ISSUE("read:issue", true),
|
||||||
READ_NOTIFICATION("read:notification", true),
|
READ_NOTIFICATION("read:notification", true),
|
||||||
|
// Both required together for the favorite/star feature: Gitea's /user/starred/{owner}/{repo}
|
||||||
|
// star/unstar endpoints need both (see the require() above for how that was confirmed).
|
||||||
WRITE_USER("write:user", false),
|
WRITE_USER("write:user", false),
|
||||||
// Required for the favorite/star feature: Gitea's star/unstar endpoints need write access to
|
|
||||||
// the repository scope category, not just read (see the require() above for the source citation).
|
|
||||||
WRITE_REPOSITORY("write:repository", false),
|
WRITE_REPOSITORY("write:repository", false),
|
||||||
|
// Not requested by this app anywhere -- exists only so a test can prove the require() above
|
||||||
|
// stays narrow (exactly the two starring scopes) rather than "any write scope is fine now".
|
||||||
|
WRITE_ORGANIZATION("write:organization", false),
|
||||||
}
|
}
|
||||||
|
|
||||||
object UBGiteaOAuthDefaults {
|
object UBGiteaOAuthDefaults {
|
||||||
@ -67,6 +72,7 @@ object UBGiteaOAuthDefaults {
|
|||||||
OAuthScope.READ_ORGANIZATION,
|
OAuthScope.READ_ORGANIZATION,
|
||||||
OAuthScope.READ_ISSUE,
|
OAuthScope.READ_ISSUE,
|
||||||
OAuthScope.READ_NOTIFICATION,
|
OAuthScope.READ_NOTIFICATION,
|
||||||
|
OAuthScope.WRITE_USER,
|
||||||
OAuthScope.WRITE_REPOSITORY,
|
OAuthScope.WRITE_REPOSITORY,
|
||||||
),
|
),
|
||||||
)
|
)
|
||||||
|
|||||||
@ -139,12 +139,25 @@ sealed class ApiFailure(message: String) : Exception(message) {
|
|||||||
|
|
||||||
fun HttpResponse.requireSuccess(): HttpResponse = when (statusCode) {
|
fun HttpResponse.requireSuccess(): HttpResponse = when (statusCode) {
|
||||||
in 200..299 -> this
|
in 200..299 -> this
|
||||||
401 -> throw ApiFailure.AuthenticationExpired()
|
else -> {
|
||||||
403 -> throw ApiFailure.PermissionDenied()
|
// Debug-build-only: every non-2xx response gets collapsed to one of the typed ApiFailure
|
||||||
404 -> throw ApiFailure.NotFound()
|
// classes below with no payload, which is enough for normal error-state UI but not enough
|
||||||
409 -> throw ApiFailure.Conflict()
|
// to diagnose *why* a specific call was rejected server-side (e.g. distinguishing "OAuth
|
||||||
422 -> throw ApiFailure.Unprocessable()
|
// token is missing a scope" from some other 403 cause) without re-adding print statements
|
||||||
429 -> throw ApiFailure.RateLimited()
|
// each time. Gitea's own error responses are small JSON bodies, not sensitive by
|
||||||
in 500..599 -> throw ApiFailure.Server()
|
// themselves (no tokens/secrets), so logging them in debug builds only is safe.
|
||||||
else -> throw ApiFailure.InvalidResponse()
|
if (com.ubisam.ubgitea.BuildConfig.DEBUG) {
|
||||||
|
android.util.Log.w("UBGitea", "HTTP $statusCode: ${body.take(500)}")
|
||||||
|
}
|
||||||
|
when (statusCode) {
|
||||||
|
401 -> throw ApiFailure.AuthenticationExpired()
|
||||||
|
403 -> throw ApiFailure.PermissionDenied()
|
||||||
|
404 -> throw ApiFailure.NotFound()
|
||||||
|
409 -> throw ApiFailure.Conflict()
|
||||||
|
422 -> throw ApiFailure.Unprocessable()
|
||||||
|
429 -> throw ApiFailure.RateLimited()
|
||||||
|
in 500..599 -> throw ApiFailure.Server()
|
||||||
|
else -> throw ApiFailure.InvalidResponse()
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@ -302,6 +302,16 @@ class RepositoryListViewModel(
|
|||||||
if (failure is ApiFailure.AuthenticationExpired) {
|
if (failure is ApiFailure.AuthenticationExpired) {
|
||||||
clearSessionOnAuthenticationExpired()
|
clearSessionOnAuthenticationExpired()
|
||||||
} else {
|
} else {
|
||||||
|
// Debug-build-only, matching this file's existing debugDetail convention (see
|
||||||
|
// showCallbackError) -- there's otherwise no way to tell a genuine permission
|
||||||
|
// problem apart from a network hiccup once it's collapsed to Failed.
|
||||||
|
if (BuildConfig.DEBUG) {
|
||||||
|
android.util.Log.w(
|
||||||
|
"UBGitea",
|
||||||
|
"toggleFavorite failed for ${repository.coordinates}: ${failure::class.simpleName}: ${failure.message}",
|
||||||
|
failure,
|
||||||
|
)
|
||||||
|
}
|
||||||
updateRow(repository.coordinates) { it.copy(favoriteAction = FavoriteActionUiState.Failed) }
|
updateRow(repository.coordinates) { it.copy(favoriteAction = FavoriteActionUiState.Failed) }
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@ -8,7 +8,7 @@ class OAuthClientConfigTest {
|
|||||||
@Test
|
@Test
|
||||||
fun rejectsWriteScopeInTheProductionOAuthContract() {
|
fun rejectsWriteScopeInTheProductionOAuthContract() {
|
||||||
assertThrows(IllegalArgumentException::class.java) {
|
assertThrows(IllegalArgumentException::class.java) {
|
||||||
config(setOf(OAuthScope.READ_USER, OAuthScope.WRITE_USER))
|
config(setOf(OAuthScope.READ_USER, OAuthScope.WRITE_ORGANIZATION))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -18,14 +18,14 @@ class OAuthClientConfigTest {
|
|||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun acceptsWriteRepositoryAsTheOneNarrowExceptionForStarring() {
|
fun acceptsWriteUserAndWriteRepositoryAsTheTwoNarrowExceptionsForStarring() {
|
||||||
config(setOf(OAuthScope.READ_USER, OAuthScope.WRITE_REPOSITORY))
|
config(setOf(OAuthScope.READ_USER, OAuthScope.WRITE_USER, OAuthScope.WRITE_REPOSITORY))
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun stillRejectsWriteUserEvenAlongsideTheAllowedWriteRepositoryException() {
|
fun stillRejectsOtherWriteScopesEvenAlongsideTheAllowedStarringExceptions() {
|
||||||
assertThrows(IllegalArgumentException::class.java) {
|
assertThrows(IllegalArgumentException::class.java) {
|
||||||
config(setOf(OAuthScope.READ_USER, OAuthScope.WRITE_REPOSITORY, OAuthScope.WRITE_USER))
|
config(setOf(OAuthScope.READ_USER, OAuthScope.WRITE_USER, OAuthScope.WRITE_REPOSITORY, OAuthScope.WRITE_ORGANIZATION))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@ -555,3 +555,14 @@ This is the append-only implementation and handoff record shared by Codex and Cl
|
|||||||
- Fix (not yet done): the human product owner needs to revoke UBGitea's existing authorization from the Gitea web UI (Settings → Applications → "Authorized OAuth2 Applications" → revoke), then log in again on the device — this creates a fresh grant under the new scope and should show the consent screen listing `write:repository`.
|
- Fix (not yet done): the human product owner needs to revoke UBGitea's existing authorization from the Gitea web UI (Settings → Applications → "Authorized OAuth2 Applications" → revoke), then log in again on the device — this creates a fresh grant under the new scope and should show the consent screen listing `write:repository`.
|
||||||
- Next recommended task: wait for the human product owner to revoke the old grant and retry login, then confirm starring actually works. Everything else queued from earlier in this session remains pending live confirmation.
|
- Next recommended task: wait for the human product owner to revoke the old grant and retry login, then confirm starring actually works. Everything else queued from earlier in this session remains pending live confirmation.
|
||||||
- Next owner: Claude, continuing per the standing direction once the human product owner responds.
|
- Next owner: Claude, continuing per the standing direction once the human product owner responds.
|
||||||
|
|
||||||
|
## 2026-08-27 (late) — TASK-033 correction: starring needs write:user AND write:repository together
|
||||||
|
|
||||||
|
- Agent and role: Claude, direct continuation of the same live session — "그대론데..?" (it's still the same) after the human product owner re-tried with the write:user-only build.
|
||||||
|
- Diagnosis: added debug-build-only logging of the raw HTTP status + response body on any non-2xx response (`HttpTransport.requireSuccess`) plus the failure class/message in `toggleFavorite`'s catch block, since source-reading alone had already produced one wrong answer this session. Required adding `testOptions { unitTests { isReturnDefaultValues = true } }` to `app/build.gradle.kts` so those `android.util.Log` calls don't throw inside plain JVM unit tests. Live logcat then showed two direct, unambiguous server responses: requesting `write:repository` alone got `required=[write:user]`; requesting `write:user` alone then got `required=[write:repository]` right back. Together these confirm `/user/starred/{owner}/{repo}` requires **both** scopes at once, not either alone.
|
||||||
|
- Fix: `OAuthClientConfig` now requests both `write:user` and `write:repository`; the narrowing `require()` invariant allows exactly those two (confirmed still narrow by a new test using an unrelated write scope, `write:organization`, added purely for that test).
|
||||||
|
- Files changed: `OAuthClientConfig.kt`, `OAuthClientConfigTest.kt`, `HttpTransport.kt`, `RepositoryListViewModel.kt`, `app/build.gradle.kts`, `tasks/033-write-repository-oauth-scope-for-starring.md` (corrected throughout, including the earlier incorrect "verified against Gitea's source" claim).
|
||||||
|
- Checks and results: `./gradlew compileDebugKotlin compileDebugUnitTestKotlin testDebugUnitTest assembleDebug lintDebug` BUILD SUCCESSFUL, all unit tests passing, 0 lint errors. Installed on the human product owner's device over wireless `adb`.
|
||||||
|
- Known limitations: the device's current session still carries scope from an earlier attempt in this same investigation — needs one more revoke (Gitea Settings → Applications → Authorized OAuth2 Applications) and re-login before starring can be confirmed working. Not yet done — the human product owner was tired and this was left for a later session rather than pushed further tonight.
|
||||||
|
- Next recommended task: revoke + re-login when convenient, then confirm starring actually works; otherwise continue per the standing direction on everything else still queued from this session (TASK-030/031/032, pinch-zoom/pan, CSV, PDF, Issues/PR pagination).
|
||||||
|
- Next owner: Claude, continuing per the standing direction.
|
||||||
|
|||||||
@ -1,4 +1,4 @@
|
|||||||
# TASK-033: Request write:repository OAuth scope so starring actually works
|
# TASK-033: Request write:user + write:repository OAuth scope so starring actually works
|
||||||
|
|
||||||
Status: implemented; all required checks pass; live-installed, requires a one-time re-login on the device to take effect
|
Status: implemented; all required checks pass; live-installed, requires a one-time re-login on the device to take effect
|
||||||
Owner: Claude
|
Owner: Claude
|
||||||
@ -7,48 +7,59 @@ Reviewer: Codex
|
|||||||
## Outcome
|
## Outcome
|
||||||
|
|
||||||
Every attempt to star/unstar a repository was guaranteed to fail server-side, because Gitea's
|
Every attempt to star/unstar a repository was guaranteed to fail server-side, because Gitea's
|
||||||
`PUT`/`DELETE /user/starred/{owner}/{repo}` endpoints require `write:repository` OAuth scope
|
`PUT`/`DELETE /user/starred/{owner}/{repo}` endpoints require write-level OAuth scope while this
|
||||||
(verified against Gitea's own route registration source) while this app's `OAuthClientConfig` only
|
app's `OAuthClientConfig` only ever requested read-only scopes, enforced by a hard `require()`
|
||||||
ever requested read-only scopes, enforced by a hard `require()` invariant. Live-reported as: the
|
invariant. Live-reported as: the new favorite star turns red (this app's failed-action color) but
|
||||||
new favorite star turns red (this app's failed-action color) but never actually favorites
|
never actually favorites anything. The human product owner was given three options — add the write
|
||||||
anything. The human product owner was given three options — add the write scope, make favorites a
|
scope, make favorites a local-only bookmark, or remove the feature — and explicitly chose to add
|
||||||
local-only bookmark, or remove the feature — and explicitly chose to add the scope. This task adds
|
the scope. Which exact scope(s) that endpoint needs took two live-tested corrections to pin down
|
||||||
`write:repository` as one narrow, explicit exception to that read-only invariant.
|
(see Known limitations) — the endpoint turned out to require **both** `write:user` and
|
||||||
|
`write:repository` together, not either one alone. This task adds both as narrow, explicit
|
||||||
|
exceptions to the read-only invariant.
|
||||||
|
|
||||||
## Scope
|
## Scope
|
||||||
|
|
||||||
- Included: `core/auth/protocol/OAuthClientConfig.kt` (new `WRITE_REPOSITORY` scope value, the
|
- Included: `core/auth/protocol/OAuthClientConfig.kt` (new `WRITE_REPOSITORY`/`WRITE_ORGANIZATION`
|
||||||
read-only `require()` narrowed to allow specifically that one scope, `UBGiteaOAuthDefaults.config`
|
scope values alongside the pre-existing `WRITE_USER`, the read-only `require()` narrowed to allow
|
||||||
now requests it), matching test additions in `OAuthClientConfigTest.kt`.
|
specifically `WRITE_USER` and `WRITE_REPOSITORY`, `UBGiteaOAuthDefaults.config` now requests
|
||||||
|
both), matching test additions in `OAuthClientConfigTest.kt`; `core/network/HttpTransport.kt`
|
||||||
|
(debug-build-only logging of the raw HTTP status + response body on any non-2xx response, added
|
||||||
|
mid-investigation — see Known limitations); `feature/repositories/RepositoryListViewModel.kt`
|
||||||
|
(debug-build-only logging of `toggleFavorite`'s failure class/message); `app/build.gradle.kts`
|
||||||
|
(`testOptions { unitTests { isReturnDefaultValues = true } }`, required once `android.util.Log`
|
||||||
|
calls landed on code paths existing unit tests exercise).
|
||||||
- Excluded: No change to `toggleFavorite`, `FavoriteStar`, or any other favorite/star UI or
|
- Excluded: No change to `toggleFavorite`, `FavoriteStar`, or any other favorite/star UI or
|
||||||
ViewModel logic from TASK-018/TASK-030 — those were already correctly built against the
|
ViewModel logic from TASK-018/TASK-030 — those were already correctly built against the
|
||||||
`RepositoryFavorites` interface; the only thing wrong was the token they were being handed never
|
`RepositoryFavorites` interface; the only thing wrong was the token they were being handed never
|
||||||
had permission to actually do it. No change to any other OAuth scope (`WRITE_USER` remains
|
had permission to actually do it. No other OAuth scope is allowed by the narrowed invariant
|
||||||
rejected, confirmed by a new test asserting it's still rejected even alongside the now-allowed
|
(confirmed by a test that still rejects an unrelated write scope even alongside the two now-allowed
|
||||||
`WRITE_REPOSITORY`).
|
ones).
|
||||||
|
|
||||||
## Design notes
|
## Design notes
|
||||||
|
|
||||||
- **A narrow exception, not a blanket relaxation**: the original `require(scopes.all { it.isReadOnly })`
|
- **A narrow exception, not a blanket relaxation**: the original `require(scopes.all { it.isReadOnly })`
|
||||||
invariant existed to keep this app's OAuth footprint deliberately minimal — this task doesn't
|
invariant existed to keep this app's OAuth footprint deliberately minimal — this task doesn't
|
||||||
remove that principle, it carves out exactly the one scope this app's own already-built favorite
|
remove that principle, it carves out exactly the two scopes this app's own already-built favorite
|
||||||
feature actually needs, and only that one. `require(scopes.all { it.isReadOnly || it == OAuthScope.WRITE_REPOSITORY })`
|
feature actually needs, and only those two. `require(scopes.all { it.isReadOnly || it == OAuthScope.WRITE_USER || it == OAuthScope.WRITE_REPOSITORY })`
|
||||||
still rejects every other write scope (including the pre-existing, still-unused `WRITE_USER`
|
still rejects every other write scope — verified by a test that adds an unrelated write scope
|
||||||
value) — verified by a new test that pairs `WRITE_REPOSITORY` with `WRITE_USER` together and
|
(`WRITE_ORGANIZATION`, added purely for this test — the app doesn't request it) alongside the two
|
||||||
confirms that combination is still rejected, so a future accidental addition of some other write
|
now-allowed ones and confirms that combination is still rejected, so a future accidental addition
|
||||||
scope doesn't silently slip through just because one exception now exists.
|
of some other write scope doesn't silently slip through just because these two exceptions exist.
|
||||||
- **Verified against Gitea's actual source, not assumed**: Gitea's route registration
|
- **Confirmed live, not just from source reading**: an initial attempt (request `write:repository`
|
||||||
(`routers/api/v1/api.go`) puts the whole `/starred/{owner}/{repo}` group under
|
only) was based on a WebFetch-summarized reading of Gitea's route registration source, which
|
||||||
`tokenRequiresScopes(auth_model.AccessTokenScopeCategoryRepository)`, with the PUT/DELETE methods
|
named the wrong scope category — the actual live 403 said `required=[write:user]`. Switching to
|
||||||
specifically requiring Write-level access within that category — `read:repository` alone is
|
`write:user` only then got `required=[write:repository]` right back. Together, both responses
|
||||||
insufficient no matter what, this isn't a bug that could have been fixed any other way from the
|
show the endpoint needs both at once — it's nested under `/user/`, which needs `write:user`, but
|
||||||
client side.
|
also runs `repoAssignment()` against the target repository, which separately needs
|
||||||
|
`write:repository`. This is why the debug-only response-body logging (see Scope) was added
|
||||||
|
mid-investigation: it's dramatically faster and more reliable than re-reading Gitea's Go route
|
||||||
|
source through a summarizing tool, which had already produced one incorrect answer.
|
||||||
- **A one-time re-login is required and expected, not a regression of TASK-029**: TASK-029's
|
- **A one-time re-login is required and expected, not a regression of TASK-029**: TASK-029's
|
||||||
proactive refresh-token flow keeps a session alive indefinitely *for the scope it was originally
|
proactive refresh-token flow keeps a session alive indefinitely *for the scope it was originally
|
||||||
granted* — a refresh token from before this change carries the old (read-only) scope forward
|
granted* — a refresh token from before this change carries the old (read-only) scope forward
|
||||||
automatically, and Gitea's refresh flow doesn't expand it. The device's current session therefore
|
automatically, and Gitea's refresh flow doesn't expand it. The device's current session therefore
|
||||||
needs one explicit new login (which will show the OAuth consent screen once more, listing the new
|
needs one explicit new login (which will show the OAuth consent screen once more, listing the new
|
||||||
`write:repository` permission) before starring will actually start working; every login after
|
write permissions) before starring will actually start working; every login after
|
||||||
that continues to benefit from TASK-029's no-re-consent behavior exactly as before. This is
|
that continues to benefit from TASK-029's no-re-consent behavior exactly as before. This is
|
||||||
reported directly to the human product owner rather than silently worked around (e.g. by
|
reported directly to the human product owner rather than silently worked around (e.g. by
|
||||||
force-clearing the stored session), since forcing a logout is a disruptive, irreversible-feeling
|
force-clearing the stored session), since forcing a logout is a disruptive, irreversible-feeling
|
||||||
@ -56,9 +67,10 @@ local-only bookmark, or remove the feature — and explicitly chose to add the s
|
|||||||
|
|
||||||
## Acceptance criteria
|
## Acceptance criteria
|
||||||
|
|
||||||
- [x] `write:repository` is requested alongside the app's existing read-only scopes.
|
- [x] `write:user` and `write:repository` are both requested alongside the app's existing
|
||||||
- [x] Any other write scope (e.g. `write:user`) is still rejected by `OAuthClientConfig`'s
|
read-only scopes.
|
||||||
invariant, even when paired with the now-allowed `write:repository`.
|
- [x] Any other write scope is still rejected by `OAuthClientConfig`'s invariant, even when paired
|
||||||
|
with the two now-allowed exceptions.
|
||||||
- [x] Build, unit tests, and lint pass.
|
- [x] Build, unit tests, and lint pass.
|
||||||
- [ ] Live confirmation, after a fresh login on the device, that starring/unstarring a repository
|
- [ ] Live confirmation, after a fresh login on the device, that starring/unstarring a repository
|
||||||
now actually succeeds (star turns filled/primary-colored, not red; the repository shows up
|
now actually succeeds (star turns filled/primary-colored, not red; the repository shows up
|
||||||
@ -75,37 +87,54 @@ local-only bookmark, or remove the feature — and explicitly chose to add the s
|
|||||||
|
|
||||||
## Evidence
|
## Evidence
|
||||||
|
|
||||||
- `OAuthClientConfig.kt`: new `OAuthScope.WRITE_REPOSITORY("write:repository", false)`; the
|
- `OAuthClientConfig.kt`: new `OAuthScope.WRITE_REPOSITORY`/`WRITE_ORGANIZATION` values alongside
|
||||||
read-only `require()` narrowed with a source-cited comment; `UBGiteaOAuthDefaults.config`'s
|
the pre-existing `WRITE_USER`; the read-only `require()` narrowed to allow `WRITE_USER` and
|
||||||
scope set now includes it.
|
`WRITE_REPOSITORY` specifically; `UBGiteaOAuthDefaults.config`'s scope set now includes both.
|
||||||
- `OAuthClientConfigTest.kt`: two new tests — `write:repository` alone (with a read scope) is now
|
- `HttpTransport.kt`: `requireSuccess()` now logs the raw status + response body in debug builds
|
||||||
accepted; `write:user` is still rejected even when `write:repository` is also present in the same
|
before throwing on any non-2xx response.
|
||||||
scope set.
|
- `RepositoryListViewModel.kt`: `toggleFavorite`'s catch block now logs the failure class/message
|
||||||
|
in debug builds.
|
||||||
|
- `app/build.gradle.kts`: `testOptions { unitTests { isReturnDefaultValues = true } }` added so
|
||||||
|
those `android.util.Log` calls don't throw inside plain JVM unit tests.
|
||||||
|
- `OAuthClientConfigTest.kt`: `write:user` + `write:repository` together (with a read scope) are
|
||||||
|
now accepted; an unrelated write scope (`write:organization`) is still rejected even alongside
|
||||||
|
both now-allowed exceptions; the base "some write scope is rejected" test now uses
|
||||||
|
`write:organization` as its example instead of `write:user` (which is no longer rejected).
|
||||||
- `./gradlew compileDebugKotlin compileDebugUnitTestKotlin`: BUILD SUCCESSFUL.
|
- `./gradlew compileDebugKotlin compileDebugUnitTestKotlin`: BUILD SUCCESSFUL.
|
||||||
- `./gradlew testDebugUnitTest`: BUILD SUCCESSFUL, all unit tests passing (including the two new
|
- `./gradlew testDebugUnitTest`: BUILD SUCCESSFUL, all unit tests passing.
|
||||||
ones above; the pre-existing `rejectsWriteScopeInTheProductionOAuthContract` test, which uses
|
|
||||||
`WRITE_USER` specifically, still passes unchanged).
|
|
||||||
- `./gradlew assembleDebug lintDebug`: BUILD SUCCESSFUL, 0 lint errors.
|
- `./gradlew assembleDebug lintDebug`: BUILD SUCCESSFUL, 0 lint errors.
|
||||||
- Installed on the human product owner's device over wireless `adb` (`adb install -r`, streamed
|
- Installed on the human product owner's device over wireless `adb` (`adb install -r`, streamed
|
||||||
install succeeded); the device's current session still carries the old, read-only-only scope
|
install succeeded) three times across this investigation, most recently with the final
|
||||||
until the human product owner logs out and back in.
|
`write:user` + `write:repository` scope set; the device's current session still carries scope
|
||||||
|
from an earlier attempt in this same investigation until the human product owner revokes and logs
|
||||||
|
in again.
|
||||||
|
|
||||||
## Known limitations
|
## Known limitations
|
||||||
|
|
||||||
- Requires a manual one-time re-login on the device before starring actually works — see Design
|
- Requires a manual one-time re-login on the device before starring actually works — see Design
|
||||||
notes. Not yet done in this session.
|
notes.
|
||||||
- **The re-login itself fails with "denied:server_error" until the old grant is revoked first.**
|
- **The scope required for starring turned out to need correcting twice, both confirmed by this
|
||||||
Live-reproduced: logging in after this change showed "로그인 응답을 확인할 수 없습니다... [debug]
|
instance's own live error responses, not by source reading alone.** First attempt requested only
|
||||||
denied:server_error". Traced directly to Gitea 1.23.5's own source
|
`write:repository` (based on a WebFetch-summarized reading of Gitea's route registration source,
|
||||||
(`routers/web/auth/oauth2_provider.go`, `GrantApplicationOAuth`): `if grant.Scope != form.Scope { ... ErrorCode: ErrorCodeServerError, ErrorDescription: "a grant exists with different scope" }`
|
which turned out to name the wrong scope category) — the live 403 body said
|
||||||
— Gitea deliberately refuses to silently widen an existing OAuth2 grant's scope; the human
|
`required=[write:user]`. Second attempt switched to only `write:user` — the live 403 body then
|
||||||
product owner's account already had a grant for this app from before this change, stored under
|
said `required=[write:repository]` right back. Conclusion, confirmed by both responses together:
|
||||||
the old (read-only-only) scope string, and the new login's differently-scoped request hits this
|
`/user/starred/{owner}/{repo}` requires **both** scopes at once (it's nested under `/user/`,
|
||||||
exact check. This is Gitea's own intended behavior, not a bug introduced by this task or
|
which needs `write:user`, but also runs `repoAssignment()` against the target repository, which
|
||||||
something fixable from the client side. **Fix**: on the Gitea web UI, go to
|
needs `write:repository`). The final scope set requests both. **Lesson applied**: added
|
||||||
Settings → Applications → "Authorized OAuth2 Applications" and revoke UBGitea's existing
|
debug-build-only logging of the raw HTTP status + response body on any non-2xx response
|
||||||
authorization, then try logging in again on the device — a fresh grant will be created under the
|
(`HttpTransport.requireSuccess`), since Gitea's own error bodies name the exact missing scope —
|
||||||
new scope and the consent screen should show `write:repository` listed. Not yet confirmed live.
|
far faster and more reliable than re-reading route source through a summarizing tool a third
|
||||||
|
time. This required adding `testOptions { unitTests { isReturnDefaultValues = true } }` to
|
||||||
|
`app/build.gradle.kts`, since plain JVM unit tests otherwise throw on any unmocked
|
||||||
|
`android.util.Log` call.
|
||||||
|
- **Each scope change requires revoking the existing grant on Gitea's web UI first** (Settings →
|
||||||
|
Applications → "Authorized OAuth2 Applications" → revoke UbisamPajuApp), or the next login fails
|
||||||
|
with "denied:server_error" — traced to Gitea 1.23.5's `GrantApplicationOAuth`:
|
||||||
|
`if grant.Scope != form.Scope { ...ErrorCode: ErrorCodeServerError, ErrorDescription: "a grant exists with different scope" }`.
|
||||||
|
This is Gitea's own intended behavior (it won't silently widen an existing grant's scope), not a
|
||||||
|
bug. The human product owner needs to revoke and re-login once more for this final scope set —
|
||||||
|
not yet confirmed live as of this commit.
|
||||||
- No change was made to retroactively reflect a repository's *already*-starred-on-the-server state
|
- No change was made to retroactively reflect a repository's *already*-starred-on-the-server state
|
||||||
when the ALL list first loads (the pre-existing, unrelated `favoriteAction` limitation noted in
|
when the ALL list first loads (the pre-existing, unrelated `favoriteAction` limitation noted in
|
||||||
TASK-030 — still applies, unaffected by this task).
|
TASK-030 — still applies, unaffected by this task).
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user