Implement OAuth PKCE protocol core
This commit is contained in:
parent
177786ef11
commit
6da016cf7b
@ -0,0 +1,37 @@
|
||||
package com.ubisam.ubgitea.core.auth.protocol
|
||||
|
||||
import java.net.URI
|
||||
import java.net.URLEncoder
|
||||
|
||||
class OAuthAuthorizationRequest(
|
||||
val uri: URI,
|
||||
val transaction: OAuthTransaction,
|
||||
) {
|
||||
override fun toString(): String = "OAuthAuthorizationRequest(REDACTED)"
|
||||
}
|
||||
|
||||
class OAuthAuthorizationRequestFactory(
|
||||
private val config: OAuthClientConfig,
|
||||
private val pkceGenerator: PkceGenerator = PkceGenerator(),
|
||||
) {
|
||||
fun create(nowEpochMillis: Long): OAuthAuthorizationRequest {
|
||||
val transaction = pkceGenerator.createTransaction(nowEpochMillis)
|
||||
val query = listOf(
|
||||
"client_id" to config.clientId,
|
||||
"redirect_uri" to config.redirectUri.toASCIIString(),
|
||||
"response_type" to "code",
|
||||
"code_challenge" to PkceGenerator.challenge(transaction.verifier),
|
||||
"code_challenge_method" to "S256",
|
||||
"state" to transaction.state,
|
||||
"scope" to config.scopes.map(OAuthScope::wireValue).sorted().joinToString(" "),
|
||||
).joinToString("&") { (name, value) -> "${name.queryEncode()}=${value.queryEncode()}" }
|
||||
|
||||
return OAuthAuthorizationRequest(
|
||||
uri = URI("${config.authorizationEndpoint}?$query"),
|
||||
transaction = transaction,
|
||||
)
|
||||
}
|
||||
|
||||
private fun String.queryEncode(): String =
|
||||
URLEncoder.encode(this, Charsets.UTF_8.name()).replace("+", "%20")
|
||||
}
|
||||
@ -0,0 +1,197 @@
|
||||
package com.ubisam.ubgitea.core.auth.protocol
|
||||
|
||||
import java.io.ByteArrayOutputStream
|
||||
import java.net.URI
|
||||
import java.nio.CharBuffer
|
||||
import java.nio.charset.CharacterCodingException
|
||||
import java.nio.charset.CodingErrorAction
|
||||
import java.nio.charset.StandardCharsets
|
||||
import java.security.MessageDigest
|
||||
|
||||
sealed interface OAuthCallbackResult {
|
||||
class Success(val authorizationCode: String) : OAuthCallbackResult {
|
||||
override fun toString(): String = "OAuthCallbackResult.Success(REDACTED)"
|
||||
}
|
||||
|
||||
class Denied(
|
||||
val error: String,
|
||||
val description: String?,
|
||||
) : OAuthCallbackResult {
|
||||
override fun toString(): String = "OAuthCallbackResult.Denied(REDACTED)"
|
||||
}
|
||||
|
||||
data class Invalid(val reason: InvalidCallbackReason) : OAuthCallbackResult
|
||||
}
|
||||
|
||||
enum class InvalidCallbackReason {
|
||||
UNEXPECTED_REDIRECT,
|
||||
MALFORMED_QUERY,
|
||||
MISSING_STATE,
|
||||
DUPLICATE_STATE,
|
||||
MISMATCHED_STATE,
|
||||
EXPIRED_TRANSACTION,
|
||||
FUTURE_TRANSACTION,
|
||||
DUPLICATE_ERROR,
|
||||
AMBIGUOUS_RESPONSE,
|
||||
MISSING_CODE,
|
||||
DUPLICATE_CODE,
|
||||
}
|
||||
|
||||
class OAuthCallbackValidator(
|
||||
private val config: OAuthClientConfig,
|
||||
private val transactionLifetimeMillis: Long = 10 * 60 * 1_000L,
|
||||
private val allowedClockSkewMillis: Long = 30_000L,
|
||||
) {
|
||||
init {
|
||||
require(transactionLifetimeMillis > 0) { "Transaction lifetime must be positive" }
|
||||
require(allowedClockSkewMillis >= 0) { "Clock skew cannot be negative" }
|
||||
}
|
||||
|
||||
fun validate(
|
||||
callbackUri: URI,
|
||||
transaction: OAuthTransaction,
|
||||
nowEpochMillis: Long,
|
||||
): OAuthCallbackResult {
|
||||
if (!callbackUri.matchesRedirect(config.redirectUri)) {
|
||||
return OAuthCallbackResult.Invalid(InvalidCallbackReason.UNEXPECTED_REDIRECT)
|
||||
}
|
||||
|
||||
val parameters = parseQuery(callbackUri.rawQuery)
|
||||
?: return OAuthCallbackResult.Invalid(InvalidCallbackReason.MALFORMED_QUERY)
|
||||
|
||||
val states = parameters["state"].orEmpty()
|
||||
if (states.size > 1) {
|
||||
return OAuthCallbackResult.Invalid(InvalidCallbackReason.DUPLICATE_STATE)
|
||||
}
|
||||
if (states.isEmpty() || states.single().isEmpty()) {
|
||||
return OAuthCallbackResult.Invalid(InvalidCallbackReason.MISSING_STATE)
|
||||
}
|
||||
if (!constantTimeEquals(states.single(), transaction.state)) {
|
||||
return OAuthCallbackResult.Invalid(InvalidCallbackReason.MISMATCHED_STATE)
|
||||
}
|
||||
val latestAllowedCreation = nowEpochMillis.saturatingAdd(allowedClockSkewMillis)
|
||||
if (transaction.createdAtEpochMillis > latestAllowedCreation) {
|
||||
return OAuthCallbackResult.Invalid(InvalidCallbackReason.FUTURE_TRANSACTION)
|
||||
}
|
||||
val earliestAllowedCreation = nowEpochMillis.saturatingSubtract(transactionLifetimeMillis)
|
||||
if (transaction.createdAtEpochMillis < earliestAllowedCreation) {
|
||||
return OAuthCallbackResult.Invalid(InvalidCallbackReason.EXPIRED_TRANSACTION)
|
||||
}
|
||||
|
||||
val errors = parameters["error"].orEmpty()
|
||||
if (errors.size > 1) {
|
||||
return OAuthCallbackResult.Invalid(InvalidCallbackReason.DUPLICATE_ERROR)
|
||||
}
|
||||
val codes = parameters["code"].orEmpty()
|
||||
if (parameters.containsKey("error") && parameters.containsKey("code")) {
|
||||
return OAuthCallbackResult.Invalid(InvalidCallbackReason.AMBIGUOUS_RESPONSE)
|
||||
}
|
||||
if (parameters["error_description"].orEmpty().size > 1 ||
|
||||
parameters["error_uri"].orEmpty().size > 1
|
||||
) {
|
||||
return OAuthCallbackResult.Invalid(InvalidCallbackReason.MALFORMED_QUERY)
|
||||
}
|
||||
if (errors.size == 1 && errors.single().isNotBlank()) {
|
||||
return OAuthCallbackResult.Denied(
|
||||
error = errors.single(),
|
||||
description = parameters["error_description"]?.singleOrNull(),
|
||||
)
|
||||
}
|
||||
|
||||
if (codes.size > 1) {
|
||||
return OAuthCallbackResult.Invalid(InvalidCallbackReason.DUPLICATE_CODE)
|
||||
}
|
||||
if (codes.isEmpty() || codes.single().isBlank()) {
|
||||
return OAuthCallbackResult.Invalid(InvalidCallbackReason.MISSING_CODE)
|
||||
}
|
||||
return OAuthCallbackResult.Success(codes.single())
|
||||
}
|
||||
|
||||
private fun URI.matchesRedirect(expected: URI): Boolean =
|
||||
scheme.equals(expected.scheme, ignoreCase = true) &&
|
||||
host.equals(expected.host, ignoreCase = true) &&
|
||||
effectivePort() == expected.effectivePort() &&
|
||||
rawPath == expected.rawPath &&
|
||||
userInfo == null &&
|
||||
fragment == null
|
||||
|
||||
private fun URI.effectivePort(): Int = if (port == -1) 443 else port
|
||||
|
||||
private fun parseQuery(rawQuery: String?): Map<String, List<String>>? {
|
||||
if (rawQuery.isNullOrEmpty()) return emptyMap()
|
||||
return try {
|
||||
rawQuery.split('&').fold(linkedMapOf()) { result, item ->
|
||||
if (item.isEmpty()) throw IllegalArgumentException("Empty query item")
|
||||
val separator = item.indexOf('=')
|
||||
val rawName = if (separator >= 0) item.substring(0, separator) else item
|
||||
val rawValue = if (separator >= 0) item.substring(separator + 1) else ""
|
||||
val name = strictFormDecode(rawName)
|
||||
val value = strictFormDecode(rawValue)
|
||||
result.apply { put(name, get(name).orEmpty() + value) }
|
||||
}
|
||||
} catch (_: IllegalArgumentException) {
|
||||
null
|
||||
} catch (_: CharacterCodingException) {
|
||||
null
|
||||
}
|
||||
}
|
||||
|
||||
private fun strictFormDecode(value: String): String {
|
||||
val bytes = ByteArrayOutputStream(value.length)
|
||||
val literal = StringBuilder()
|
||||
|
||||
fun flushLiteral() {
|
||||
if (literal.isEmpty()) return
|
||||
val encoder = StandardCharsets.UTF_8.newEncoder()
|
||||
.onMalformedInput(CodingErrorAction.REPORT)
|
||||
.onUnmappableCharacter(CodingErrorAction.REPORT)
|
||||
val encoded = encoder.encode(CharBuffer.wrap(literal))
|
||||
while (encoded.hasRemaining()) bytes.write(encoded.get().toInt())
|
||||
literal.clear()
|
||||
}
|
||||
|
||||
var index = 0
|
||||
while (index < value.length) {
|
||||
when (value[index]) {
|
||||
'%' -> {
|
||||
flushLiteral()
|
||||
require(index + 2 < value.length) { "Incomplete percent encoding" }
|
||||
val high = value[index + 1].digitToIntOrNull(16)
|
||||
val low = value[index + 2].digitToIntOrNull(16)
|
||||
require(high != null && low != null) { "Invalid percent encoding" }
|
||||
bytes.write((high shl 4) or low)
|
||||
index += 3
|
||||
}
|
||||
|
||||
'+' -> {
|
||||
flushLiteral()
|
||||
bytes.write(' '.code)
|
||||
index++
|
||||
}
|
||||
|
||||
else -> {
|
||||
literal.append(value[index])
|
||||
index++
|
||||
}
|
||||
}
|
||||
}
|
||||
flushLiteral()
|
||||
|
||||
val decoder = StandardCharsets.UTF_8.newDecoder()
|
||||
.onMalformedInput(CodingErrorAction.REPORT)
|
||||
.onUnmappableCharacter(CodingErrorAction.REPORT)
|
||||
return decoder.decode(java.nio.ByteBuffer.wrap(bytes.toByteArray())).toString()
|
||||
}
|
||||
|
||||
private fun Long.saturatingAdd(value: Long): Long =
|
||||
if (this > Long.MAX_VALUE - value) Long.MAX_VALUE else this + value
|
||||
|
||||
private fun Long.saturatingSubtract(value: Long): Long =
|
||||
if (this < Long.MIN_VALUE + value) Long.MIN_VALUE else this - value
|
||||
|
||||
private fun constantTimeEquals(first: String, second: String): Boolean =
|
||||
MessageDigest.isEqual(
|
||||
first.toByteArray(StandardCharsets.UTF_8),
|
||||
second.toByteArray(StandardCharsets.UTF_8),
|
||||
)
|
||||
}
|
||||
@ -0,0 +1,59 @@
|
||||
package com.ubisam.ubgitea.core.auth.protocol
|
||||
|
||||
import java.net.URI
|
||||
|
||||
data class OAuthClientConfig(
|
||||
val baseUri: URI,
|
||||
val clientId: String,
|
||||
val redirectUri: URI,
|
||||
val scopes: Set<OAuthScope>,
|
||||
) {
|
||||
init {
|
||||
require(baseUri.isSecureOrigin()) { "OAuth base URI must be an HTTPS origin" }
|
||||
require(clientId.isNotBlank()) { "OAuth client ID cannot be blank" }
|
||||
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.all { it.isReadOnly }) { "Only read-only OAuth scopes are allowed" }
|
||||
}
|
||||
|
||||
val authorizationEndpoint: URI
|
||||
get() = baseUri.resolve("/login/oauth/authorize")
|
||||
|
||||
private fun URI.isSecureOrigin(): Boolean =
|
||||
scheme.equals("https", ignoreCase = true) &&
|
||||
!host.isNullOrBlank() &&
|
||||
userInfo == null &&
|
||||
(path.isNullOrEmpty() || path == "/") &&
|
||||
query == null &&
|
||||
fragment == null
|
||||
|
||||
private fun URI.isSecureRedirect(): Boolean =
|
||||
scheme.equals("https", ignoreCase = true) &&
|
||||
!host.isNullOrBlank() &&
|
||||
userInfo == null &&
|
||||
query == null &&
|
||||
fragment == null
|
||||
}
|
||||
|
||||
enum class OAuthScope(val wireValue: String, val isReadOnly: Boolean) {
|
||||
READ_USER("read:user", true),
|
||||
READ_REPOSITORY("read:repository", true),
|
||||
READ_ORGANIZATION("read:organization", true),
|
||||
READ_ISSUE("read:issue", true),
|
||||
READ_NOTIFICATION("read:notification", true),
|
||||
}
|
||||
|
||||
object UBGiteaOAuthDefaults {
|
||||
val config = OAuthClientConfig(
|
||||
baseUri = URI("https://gitea.ubisampaju.synology.me"),
|
||||
clientId = "225589df-b992-4676-aae0-b2534ba7452d",
|
||||
redirectUri = URI("https://gitea.ubisampaju.synology.me/ubisampaju/oauth/callback"),
|
||||
scopes = setOf(
|
||||
OAuthScope.READ_USER,
|
||||
OAuthScope.READ_REPOSITORY,
|
||||
OAuthScope.READ_ORGANIZATION,
|
||||
OAuthScope.READ_ISSUE,
|
||||
OAuthScope.READ_NOTIFICATION,
|
||||
),
|
||||
)
|
||||
}
|
||||
@ -0,0 +1,44 @@
|
||||
package com.ubisam.ubgitea.core.auth.protocol
|
||||
|
||||
import java.security.MessageDigest
|
||||
import java.security.SecureRandom
|
||||
import java.util.Base64
|
||||
|
||||
class OAuthTransaction(
|
||||
val verifier: String,
|
||||
val state: String,
|
||||
val createdAtEpochMillis: Long,
|
||||
) {
|
||||
override fun toString(): String = "OAuthTransaction(REDACTED)"
|
||||
}
|
||||
|
||||
class PkceGenerator(
|
||||
private val secureRandom: SecureRandom = SecureRandom(),
|
||||
) {
|
||||
fun createTransaction(nowEpochMillis: Long): OAuthTransaction {
|
||||
return OAuthTransaction(
|
||||
verifier = randomBase64Url(BYTE_LENGTH),
|
||||
state = randomBase64Url(BYTE_LENGTH),
|
||||
createdAtEpochMillis = nowEpochMillis,
|
||||
)
|
||||
}
|
||||
|
||||
private fun randomBase64Url(byteLength: Int): String {
|
||||
val bytes = ByteArray(byteLength)
|
||||
secureRandom.nextBytes(bytes)
|
||||
return BASE64_URL_ENCODER.encodeToString(bytes)
|
||||
}
|
||||
|
||||
companion object {
|
||||
private const val BYTE_LENGTH = 32
|
||||
private val BASE64_URL_ENCODER = Base64.getUrlEncoder().withoutPadding()
|
||||
private val VERIFIER_PATTERN = Regex("[A-Za-z0-9._~-]{43,128}")
|
||||
|
||||
fun challenge(verifier: String): String {
|
||||
require(VERIFIER_PATTERN.matches(verifier)) { "PKCE verifier is invalid" }
|
||||
val digest = MessageDigest.getInstance("SHA-256")
|
||||
.digest(verifier.toByteArray(Charsets.US_ASCII))
|
||||
return BASE64_URL_ENCODER.encodeToString(digest)
|
||||
}
|
||||
}
|
||||
}
|
||||
@ -0,0 +1,89 @@
|
||||
package com.ubisam.ubgitea.core.auth.protocol
|
||||
|
||||
import java.net.URI
|
||||
import java.net.URLDecoder
|
||||
import java.nio.charset.StandardCharsets
|
||||
import java.security.SecureRandom
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertFalse
|
||||
import org.junit.Assert.assertNotEquals
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Test
|
||||
|
||||
class OAuthAuthorizationRequestTest {
|
||||
@Test
|
||||
fun buildsAuthorizationRequestWithApprovedReadScopesAndPkce() {
|
||||
val request = OAuthAuthorizationRequestFactory(
|
||||
config = UBGiteaOAuthDefaults.config,
|
||||
pkceGenerator = PkceGenerator(ChangingSecureRandom()),
|
||||
).create(nowEpochMillis = 123L)
|
||||
val query = request.uri.queryParameters()
|
||||
|
||||
assertEquals("https", request.uri.scheme)
|
||||
assertEquals("gitea.ubisampaju.synology.me", request.uri.host)
|
||||
assertEquals("/login/oauth/authorize", request.uri.path)
|
||||
assertEquals(UBGiteaOAuthDefaults.config.clientId, query.getValue("client_id"))
|
||||
assertEquals(UBGiteaOAuthDefaults.config.redirectUri.toString(), query.getValue("redirect_uri"))
|
||||
assertEquals("code", query.getValue("response_type"))
|
||||
assertEquals("S256", query.getValue("code_challenge_method"))
|
||||
assertEquals(PkceGenerator.challenge(request.transaction.verifier), query.getValue("code_challenge"))
|
||||
assertEquals(request.transaction.state, query.getValue("state"))
|
||||
assertEquals(
|
||||
UBGiteaOAuthDefaults.config.scopes.map(OAuthScope::wireValue).sorted(),
|
||||
query.getValue("scope").split(' ').sorted(),
|
||||
)
|
||||
assertFalse(request.uri.toString().contains("client_secret", ignoreCase = true))
|
||||
assertEquals("OAuthAuthorizationRequest(REDACTED)", request.toString())
|
||||
assertFalse(request.toString().contains(request.transaction.state))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun createsFreshTransactionForEachRequest() {
|
||||
val factory = OAuthAuthorizationRequestFactory(
|
||||
UBGiteaOAuthDefaults.config,
|
||||
PkceGenerator(ChangingSecureRandom()),
|
||||
)
|
||||
|
||||
val first = factory.create(1L)
|
||||
val second = factory.create(2L)
|
||||
|
||||
assertNotEquals(first.transaction.verifier, second.transaction.verifier)
|
||||
assertNotEquals(first.transaction.state, second.transaction.state)
|
||||
}
|
||||
|
||||
@Test(expected = IllegalArgumentException::class)
|
||||
fun rejectsHttpBaseUri() {
|
||||
OAuthClientConfig(
|
||||
baseUri = URI("http://gitea.example.com"),
|
||||
clientId = "public-client",
|
||||
redirectUri = URI("https://gitea.example.com/callback"),
|
||||
scopes = setOf(OAuthScope.READ_USER),
|
||||
)
|
||||
}
|
||||
|
||||
@Test(expected = IllegalArgumentException::class)
|
||||
fun rejectsBaseUriWithPath() {
|
||||
OAuthClientConfig(
|
||||
baseUri = URI("https://gitea.example.com/unexpected"),
|
||||
clientId = "public-client",
|
||||
redirectUri = URI("https://gitea.example.com/callback"),
|
||||
scopes = setOf(OAuthScope.READ_USER),
|
||||
)
|
||||
}
|
||||
|
||||
private fun URI.queryParameters(): Map<String, String> =
|
||||
checkNotNull(rawQuery).split('&').associate { item ->
|
||||
val (name, value) = item.split('=', limit = 2)
|
||||
URLDecoder.decode(name, StandardCharsets.UTF_8) to
|
||||
URLDecoder.decode(value, StandardCharsets.UTF_8)
|
||||
}
|
||||
|
||||
private class ChangingSecureRandom : SecureRandom() {
|
||||
private var seed = 5
|
||||
|
||||
override fun nextBytes(bytes: ByteArray) {
|
||||
bytes.indices.forEach { index -> bytes[index] = (seed + index).toByte() }
|
||||
seed++
|
||||
}
|
||||
}
|
||||
}
|
||||
@ -0,0 +1,163 @@
|
||||
package com.ubisam.ubgitea.core.auth.protocol
|
||||
|
||||
import java.net.URI
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Test
|
||||
|
||||
class OAuthCallbackValidatorTest {
|
||||
private val config = UBGiteaOAuthDefaults.config
|
||||
private val validator = OAuthCallbackValidator(config)
|
||||
private val transaction = OAuthTransaction(
|
||||
verifier = "v".repeat(43),
|
||||
state = "expected-state",
|
||||
createdAtEpochMillis = 1_000L,
|
||||
)
|
||||
|
||||
@Test
|
||||
fun acceptsExactRedirectWithMatchingStateAndSingleCode() {
|
||||
val result = validate("?state=expected-state&code=authorization-code")
|
||||
|
||||
assertTrue(result is OAuthCallbackResult.Success)
|
||||
assertEquals("authorization-code", (result as OAuthCallbackResult.Success).authorizationCode)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun representsDenialSeparatelyAfterValidatingState() {
|
||||
val result = validate("?state=expected-state&error=access_denied&error_description=Cancelled")
|
||||
|
||||
assertTrue(result is OAuthCallbackResult.Denied)
|
||||
result as OAuthCallbackResult.Denied
|
||||
assertEquals("access_denied", result.error)
|
||||
assertEquals("Cancelled", result.description)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun rejectsUnexpectedSchemeHostPortAndPath() {
|
||||
val unexpected = listOf(
|
||||
"http://gitea.ubisampaju.synology.me/ubisampaju/oauth/callback",
|
||||
"https://evil.example/ubisampaju/oauth/callback",
|
||||
"https://gitea.ubisampaju.synology.me:8443/ubisampaju/oauth/callback",
|
||||
"https://gitea.ubisampaju.synology.me/other/callback",
|
||||
)
|
||||
|
||||
unexpected.forEach { uri ->
|
||||
assertInvalid(
|
||||
validator.validate(URI("$uri?state=expected-state&code=code"), transaction, 2_000L),
|
||||
InvalidCallbackReason.UNEXPECTED_REDIRECT,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun rejectsMissingDuplicateAndMismatchedState() {
|
||||
assertInvalid(validate("?code=code"), InvalidCallbackReason.MISSING_STATE)
|
||||
assertInvalid(
|
||||
validate("?state=expected-state&state=other&code=code"),
|
||||
InvalidCallbackReason.DUPLICATE_STATE,
|
||||
)
|
||||
assertInvalid(validate("?state=wrong&code=code"), InvalidCallbackReason.MISMATCHED_STATE)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun rejectsExpiredAndFutureTransactions() {
|
||||
assertInvalid(
|
||||
validator.validate(callback("?state=expected-state&code=code"), transaction, 601_001L),
|
||||
InvalidCallbackReason.EXPIRED_TRANSACTION,
|
||||
)
|
||||
assertInvalid(
|
||||
validator.validate(callback("?state=expected-state&code=code"), transaction, -29_001L),
|
||||
InvalidCallbackReason.FUTURE_TRANSACTION,
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun rejectsMissingBlankAndDuplicateCode() {
|
||||
assertInvalid(validate("?state=expected-state"), InvalidCallbackReason.MISSING_CODE)
|
||||
assertInvalid(validate("?state=expected-state&code="), InvalidCallbackReason.MISSING_CODE)
|
||||
assertInvalid(
|
||||
validate("?state=expected-state&code=first&code=second"),
|
||||
InvalidCallbackReason.DUPLICATE_CODE,
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun validatesStateBeforeReturningDenial() {
|
||||
assertInvalid(validate("?state=wrong&error=access_denied"), InvalidCallbackReason.MISMATCHED_STATE)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun rejectsDuplicateError() {
|
||||
assertInvalid(
|
||||
validate("?state=expected-state&error=first&error=second"),
|
||||
InvalidCallbackReason.DUPLICATE_ERROR,
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun rejectsResponseContainingBothErrorAndCode() {
|
||||
assertInvalid(
|
||||
validate("?state=expected-state&error=access_denied&code=unexpected"),
|
||||
InvalidCallbackReason.AMBIGUOUS_RESPONSE,
|
||||
)
|
||||
assertInvalid(
|
||||
validate("?state=expected-state&error=&code=unexpected"),
|
||||
InvalidCallbackReason.AMBIGUOUS_RESPONSE,
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun rejectsDuplicateErrorDescriptionAndMalformedUtf8() {
|
||||
assertInvalid(
|
||||
validate("?state=expected-state&error=access_denied&error_description=one&error_description=two"),
|
||||
InvalidCallbackReason.MALFORMED_QUERY,
|
||||
)
|
||||
assertInvalid(
|
||||
validate("?state=expected-state&code=%C3%28"),
|
||||
InvalidCallbackReason.MALFORMED_QUERY,
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun sensitiveValuesAreRedactedFromStringRepresentations() {
|
||||
val success = validate("?state=expected-state&code=authorization-code")
|
||||
|
||||
assertEquals("OAuthTransaction(REDACTED)", transaction.toString())
|
||||
assertEquals("OAuthCallbackResult.Success(REDACTED)", success.toString())
|
||||
assertTrue(!success.toString().contains("authorization-code"))
|
||||
val denied = validate("?state=expected-state&error=access_denied&error_description=Private")
|
||||
assertEquals("OAuthCallbackResult.Denied(REDACTED)", denied.toString())
|
||||
assertTrue(!denied.toString().contains("Private"))
|
||||
}
|
||||
|
||||
@Test(expected = java.net.URISyntaxException::class)
|
||||
fun malformedPercentEncodingIsRejectedBeforeCallbackValidation() {
|
||||
URI("${config.redirectUri}?state=expected-state&code=%ZZ")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun rejectsExtremeFutureAndExpiredTransactionWithoutOverflow() {
|
||||
val callback = callback("?state=expected-state&code=code")
|
||||
val extremeFuture = OAuthTransaction("v".repeat(43), "expected-state", Long.MAX_VALUE)
|
||||
val extremePast = OAuthTransaction("v".repeat(43), "expected-state", Long.MIN_VALUE)
|
||||
|
||||
assertInvalid(
|
||||
validator.validate(callback, extremeFuture, Long.MIN_VALUE),
|
||||
InvalidCallbackReason.FUTURE_TRANSACTION,
|
||||
)
|
||||
assertInvalid(
|
||||
validator.validate(callback, extremePast, Long.MAX_VALUE),
|
||||
InvalidCallbackReason.EXPIRED_TRANSACTION,
|
||||
)
|
||||
}
|
||||
|
||||
private fun validate(query: String): OAuthCallbackResult =
|
||||
validator.validate(callback(query), transaction, 2_000L)
|
||||
|
||||
private fun callback(query: String): URI = URI("${config.redirectUri}$query")
|
||||
|
||||
private fun assertInvalid(result: OAuthCallbackResult, reason: InvalidCallbackReason) {
|
||||
assertTrue(result is OAuthCallbackResult.Invalid)
|
||||
assertEquals(reason, (result as OAuthCallbackResult.Invalid).reason)
|
||||
}
|
||||
}
|
||||
@ -0,0 +1,66 @@
|
||||
package com.ubisam.ubgitea.core.auth.protocol
|
||||
|
||||
import java.security.SecureRandom
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertNotEquals
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Test
|
||||
|
||||
class PkceTest {
|
||||
@Test
|
||||
fun matchesRfc7636S256Example() {
|
||||
val verifier = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk"
|
||||
|
||||
assertEquals("E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", PkceGenerator.challenge(verifier))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun generatesFreshUrlSafeVerifierAndState() {
|
||||
val generator = PkceGenerator(ChangingSecureRandom())
|
||||
|
||||
val first = generator.createTransaction(100L)
|
||||
val second = generator.createTransaction(200L)
|
||||
|
||||
assertEquals(43, first.verifier.length)
|
||||
assertEquals(43, first.state.length)
|
||||
assertTrue(first.verifier.matches(Regex("[A-Za-z0-9_-]+")))
|
||||
assertTrue(first.state.matches(Regex("[A-Za-z0-9_-]+")))
|
||||
assertNotEquals(first.verifier, second.verifier)
|
||||
assertNotEquals(first.state, second.state)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun rejectsInvalidVerifierLengthAndCharacters() {
|
||||
val invalid = listOf(
|
||||
"a".repeat(42),
|
||||
"a".repeat(129),
|
||||
"a".repeat(42) + "한",
|
||||
"a".repeat(42) + " ",
|
||||
"a".repeat(42) + "+",
|
||||
"a".repeat(42) + "/",
|
||||
"a".repeat(42) + "=",
|
||||
)
|
||||
|
||||
invalid.forEach { verifier ->
|
||||
org.junit.Assert.assertThrows(IllegalArgumentException::class.java) {
|
||||
PkceGenerator.challenge(verifier)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun acceptsEveryRfc7636UnreservedVerifierCharacter() {
|
||||
val verifier = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~"
|
||||
|
||||
assertTrue(PkceGenerator.challenge(verifier).isNotBlank())
|
||||
}
|
||||
|
||||
private class ChangingSecureRandom : SecureRandom() {
|
||||
private var seed = 0
|
||||
|
||||
override fun nextBytes(bytes: ByteArray) {
|
||||
bytes.indices.forEach { index -> bytes[index] = (seed + index).toByte() }
|
||||
seed++
|
||||
}
|
||||
}
|
||||
}
|
||||
38
docs/decisions/002-oauth-security.md
Normal file
38
docs/decisions/002-oauth-security.md
Normal file
@ -0,0 +1,38 @@
|
||||
# ADR-002: OAuth security and read-only scopes
|
||||
|
||||
## Status
|
||||
|
||||
Accepted — 2026-08-25
|
||||
|
||||
## Context
|
||||
|
||||
UBGitea is a public Android client for the company Gitea instance. The instance reports Gitea version 1.23.5, which supports granular OAuth scopes. The application must browse private resources visible to the signed-in account without receiving write access.
|
||||
|
||||
## Decision
|
||||
|
||||
- Use OAuth 2.0 Authorization Code with a fresh PKCE S256 verifier and random state per attempt.
|
||||
- Do not use, request, store, transmit, or log a client secret.
|
||||
- Request only `read:user`, `read:repository`, `read:organization`, `read:issue`, and `read:notification`.
|
||||
- Submit access tokens only as an HTTP Bearer authorization header.
|
||||
- Use the system browser and the registered verified HTTPS App Link.
|
||||
- Require exact callback origin and path, matching state, a valid unexpired transaction, and a single authorization code.
|
||||
- Store tokens only through an Android Keystore-backed vault and exclude authentication data from Android backup.
|
||||
- Treat 401 as expired authentication and 403 as insufficient permission; never broaden scopes automatically.
|
||||
|
||||
## Alternatives considered
|
||||
|
||||
- A broad or write scope was rejected because the current product is read-only.
|
||||
- A client secret was rejected because an installed Android application cannot keep one confidential and the registered flow uses PKCE.
|
||||
- A WebView was rejected because credentials must remain in the system browser security context.
|
||||
- A custom URI scheme was rejected in favor of the registered verified HTTPS App Link.
|
||||
|
||||
## Consequences
|
||||
|
||||
- Adding a future feature that requires a new scope needs product approval, ADR revision, and explicit reauthorization UX.
|
||||
- End-to-end verification remains blocked until signing fingerprints and `assetlinks.json` are configured.
|
||||
- Private repository data must not be persisted until a separate retention and cache decision is approved.
|
||||
|
||||
## Evidence
|
||||
|
||||
- `GET /api/v1/version` on 2026-08-25 returned `1.23.5` without authentication.
|
||||
- Gitea's OAuth provider documentation states that v1.23 and later support granular read/write resource scopes and PKCE.
|
||||
@ -21,6 +21,11 @@ Reviewer: Claude
|
||||
|
||||
A user can authorize UBGitea through the system browser, return through the verified App Link, exchange the code using PKCE, and view their Gitea identity.
|
||||
|
||||
## Delivery split
|
||||
|
||||
- TASK-002A implements and verifies the pure OAuth protocol core without external infrastructure.
|
||||
- TASK-002B integrates the system browser, App Link, Keystore-backed token vault, token exchange, identity call, and real-server verification.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [ ] Uses a system browser, not WebView.
|
||||
|
||||
62
tasks/002a-oauth-protocol-core.md
Normal file
62
tasks/002a-oauth-protocol-core.md
Normal file
@ -0,0 +1,62 @@
|
||||
# TASK-002A: Implement the OAuth PKCE protocol core
|
||||
|
||||
Status: complete
|
||||
Owner: Codex
|
||||
Reviewer: Claude
|
||||
|
||||
## Outcome
|
||||
|
||||
Provide a deterministic, Android-independent OAuth authorization contract that generates fresh PKCE/state values, requests only approved read scopes, and strictly validates callbacks before any authorization code can be exchanged.
|
||||
|
||||
## Writable paths
|
||||
|
||||
- `app/src/main/java/com/ubisam/ubgitea/core/auth/protocol/`
|
||||
- `app/src/test/java/com/ubisam/ubgitea/core/auth/protocol/`
|
||||
- `docs/decisions/002-oauth-security.md`
|
||||
- `tasks/002-oauth-spike.md`
|
||||
- `tasks/002a-oauth-protocol-core.md`
|
||||
- `tasks/002b-oauth-android-integration.md`
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [x] A fresh cryptographically random verifier and state are generated for every authorization attempt.
|
||||
- [x] PKCE uses SHA-256 and unpadded Base64 URL encoding with method `S256`.
|
||||
- [x] Authorization requests use HTTPS, the registered client ID and redirect URI, and only approved read scopes.
|
||||
- [x] Callback validation requires an exact redirect scheme, host, effective port, and path.
|
||||
- [x] Callback validation rejects missing, duplicated, mismatched, or expired state before accepting code or error parameters.
|
||||
- [x] Missing, blank, or duplicated codes and malformed query encoding are rejected.
|
||||
- [x] OAuth denial is represented separately from malformed callbacks.
|
||||
- [x] No client secret, token, Android type, or logging appears in the protocol package.
|
||||
- [x] Focused tests cover URL construction, freshness, PKCE vectors, scope, redirect validation, state, expiry, denial, code, and malformed input.
|
||||
|
||||
## Security and privacy
|
||||
|
||||
This package must remain pure and must never persist or log authorization codes, tokens, verifier values, or state values.
|
||||
|
||||
## Required checks
|
||||
|
||||
```text
|
||||
./gradlew assembleDebug
|
||||
./gradlew testDebugUnitTest
|
||||
./gradlew lintDebug
|
||||
```
|
||||
|
||||
## Evidence
|
||||
|
||||
Recorded on 2026-08-25 using JDK 17 and Android SDK 36:
|
||||
|
||||
- Focused OAuth protocol tests — 21 tests, 0 failures, 0 errors.
|
||||
- `./gradlew assembleDebug` — passed.
|
||||
- `./gradlew testDebugUnitTest` — passed; 43 tests across 9 suites, 0 failures, 0 errors.
|
||||
- `./gradlew lintDebug` — passed.
|
||||
- Sensitive-pattern scan excluding generated and Git metadata — no assigned client secret, token, password, or private-key value found.
|
||||
- `git diff --check` — passed.
|
||||
- Automatic string representations for transaction, request, authorization code, and denial results redact sensitive values.
|
||||
- Actual browser, App Link, token exchange, Keystore, and identity verification remain in TASK-002B.
|
||||
|
||||
## Review findings
|
||||
|
||||
- P2 verifier grammar — fixed by enforcing the RFC 7636 unreserved ASCII character set and 43–128 character length; adversarial tests added.
|
||||
- P2 callback pollution/encoding — fixed by rejecting conflicting response keys and duplicate recognized parameters, and using strict UTF-8 percent decoding; adversarial tests added.
|
||||
- P3 time arithmetic overflow — fixed with saturating comparison boundaries and `Long.MIN_VALUE`/`Long.MAX_VALUE` tests.
|
||||
- Independent re-review confirmed all findings closed with no remaining P0-P3 findings.
|
||||
44
tasks/002b-oauth-android-integration.md
Normal file
44
tasks/002b-oauth-android-integration.md
Normal file
@ -0,0 +1,44 @@
|
||||
# TASK-002B: Integrate OAuth with Android and verify App Links
|
||||
|
||||
Status: planned — implementation depends on TASK-002A; E2E verification depends on external App Link configuration
|
||||
Owner: Codex
|
||||
Reviewer: Claude
|
||||
|
||||
## Outcome
|
||||
|
||||
Launch authorization in the system browser, receive only the verified HTTPS App Link, exchange the code, protect tokens with Android Keystore, show the authenticated identity, and remove all authentication data on logout.
|
||||
|
||||
## Writable paths
|
||||
|
||||
- `app/src/main/java/com/ubisam/ubgitea/core/auth/android/`
|
||||
- `app/src/main/java/com/ubisam/ubgitea/data/auth/`
|
||||
- `app/src/main/java/com/ubisam/ubgitea/feature/auth/`
|
||||
- Matching test paths
|
||||
- `app/src/main/AndroidManifest.xml`
|
||||
- `app/src/main/res/values/strings_auth.xml`
|
||||
- `app/src/main/res/xml/data_extraction_rules.xml`
|
||||
- `app/build.gradle.kts`
|
||||
- `gradle/libs.versions.toml`
|
||||
- `tasks/002-oauth-spike.md`
|
||||
- `tasks/002b-oauth-android-integration.md`
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [ ] Credentials are entered only through an external system browser flow; no WebView exists.
|
||||
- [ ] Only the registered HTTPS App Link scheme, host, and path reach callback handling.
|
||||
- [ ] Transaction state is short-lived and cleared on success, denial, cancellation, expiry, and error.
|
||||
- [ ] Token exchange sends the original verifier and never sends or requests a client secret.
|
||||
- [ ] Access and refresh tokens are encrypted using an Android Keystore-backed key and excluded from backup.
|
||||
- [ ] Tokens, authorization codes, cookies, identity data, verifier, and state are never logged.
|
||||
- [ ] 401 causes authentication-expired state while 403 remains permission denied.
|
||||
- [ ] Logout clears tokens, transaction state, and private cached authentication data.
|
||||
- [ ] Debug/release fingerprints and hosted `assetlinks.json` are verified before E2E completion.
|
||||
- [ ] Tests cover storage round trip/clear, browser intent boundary, callback dispatch, token exchange mapping, identity, and logout.
|
||||
|
||||
## Required checks
|
||||
|
||||
```text
|
||||
./gradlew assembleDebug
|
||||
./gradlew testDebugUnitTest
|
||||
./gradlew lintDebug
|
||||
```
|
||||
Loading…
x
Reference in New Issue
Block a user