From 8d1377dea536ec59933141268fe54e433d112699 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 16 Sep 2026 10:28:10 +0200 Subject: [PATCH 01/29] Changelog. --- ...ngle-oauth-scopes-_write-access-should-not-imply-read-access_ | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/1-api-changes/WPB-28193-disentangle-oauth-scopes-_write-access-should-not-imply-read-access_ diff --git a/changelog.d/1-api-changes/WPB-28193-disentangle-oauth-scopes-_write-access-should-not-imply-read-access_ b/changelog.d/1-api-changes/WPB-28193-disentangle-oauth-scopes-_write-access-should-not-imply-read-access_ new file mode 100644 index 0000000000..fe8f312730 --- /dev/null +++ b/changelog.d/1-api-changes/WPB-28193-disentangle-oauth-scopes-_write-access-should-not-imply-read-access_ @@ -0,0 +1 @@ +Disentangle oauth scopes (write access should not imply read access). From a752ca4954188d1da9089bbc3688c35ab4e24b06 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Thu, 17 Sep 2026 18:57:12 +0200 Subject: [PATCH 02/29] Better OpenApi rendering of oauth scopes. --- libs/wire-api/src/Wire/API/Routes/Public.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/wire-api/src/Wire/API/Routes/Public.hs b/libs/wire-api/src/Wire/API/Routes/Public.hs index cacd78420d..969f43bee3 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public.hs @@ -363,7 +363,7 @@ addScopeDescription = -- that test only.) renderOAuthScope :: OAuth.OAuthScope -> Text renderOAuthScope scope = - "\nOAuth scope: `" + "
OAuth scope: `" <> (decodeUtf8With lenientDecode . toStrict . toByteString $ scope) <> "`" From 2bca04ebc751dd3a5f4bcd260c4886bd193f138d Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Thu, 17 Sep 2026 14:47:33 +0200 Subject: [PATCH 03/29] Refactor: split up `data OAuthScope` into base and tier. --- libs/types-common/src/Data/GenericEnum.hs | 102 ++++++++++++++++++ libs/types-common/types-common.cabal | 1 + libs/wire-api/src/Wire/API/OAuth.hs | 80 +++++++++----- .../src/Wire/API/Routes/Public/Brig.hs | 2 +- .../API/Routes/Public/Galley/Conversation.hs | 14 +-- .../Wire/API/Routes/Public/Galley/Feature.hs | 2 +- .../Wire/API/Routes/Public/Galley/Meetings.hs | 2 +- services/brig/test/integration/API/OAuth.hs | 52 ++++----- 8 files changed, 190 insertions(+), 65 deletions(-) create mode 100644 libs/types-common/src/Data/GenericEnum.hs diff --git a/libs/types-common/src/Data/GenericEnum.hs b/libs/types-common/src/Data/GenericEnum.hs new file mode 100644 index 0000000000..428d43f907 --- /dev/null +++ b/libs/types-common/src/Data/GenericEnum.hs @@ -0,0 +1,102 @@ +-- This file is part of the Wire Server implementation. +-- +-- Copyright (C) 2026 Wire Swiss GmbH +-- +-- This program is free software: you can redistribute it and/or modify it under +-- the terms of the GNU Affero General Public License as published by the Free +-- Software Foundation, either version 3 of the License, or (at your option) any +-- later version. +-- +-- This program is distributed in the hope that it will be useful, but WITHOUT +-- ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS +-- FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +-- details. +-- +-- You should have received a copy of the GNU Affero General Public License along +-- with this program. If not, see . + +module Data.GenericEnum + ( GenericEnum (..), + GEnum (..), + ) +where + +import Data.Kind (Type) +import GHC.Generics (Generic (..), K1 (..), M1 (..), U1 (..), V1, type (:*:) (..), type (:+:) (..)) +import Imports + +-------------------------------------------------------------------------------- +-- Generic Bounded / Enum + +-- | 'Bounded' and 'Enum' read off a type's 'Generic' representation, for use +-- with @DerivingVia@: +-- +-- > data OAuthScope = FeatureConfigs OAuthTier | {- ... -} | Meetings OAuthTier +-- > deriving (Bounded, Enum) via (GenericEnum OAuthScope) +-- +-- @deriving Enum@ only covers types whose constructors are all +-- nullary, which @OAuthScope@ is not. `GenericEnum` covers products +-- of 'Bounded' + 'Enum' fields as well, numbering sums in constructor +-- order and products lexicographically. This is the same order +-- @deriving Ord@ uses, so @[minBound .. maxBound]@ comes out sorted. +newtype GenericEnum a = GenericEnum a + +class GEnum (f :: Type -> Type) where + -- | How many values @f@ has. + gCard :: Int + + gToEnum :: Int -> f a + gFromEnum :: f a -> Int + +instance GEnum V1 where + gCard = 0 + gToEnum i = error $ "GenericEnum: uninhabited type, toEnum " <> show i + gFromEnum v = case v of {} + +instance GEnum U1 where + gCard = 1 + gToEnum _ = U1 + gFromEnum _ = 0 + +instance (GEnum f) => GEnum (M1 i c f) where + gCard = gCard @f + gToEnum = M1 . gToEnum + gFromEnum = gFromEnum . unM1 + +instance (Bounded a, Enum a) => GEnum (K1 i a) where + gCard = fromEnum (maxBound @a) - fromEnum (minBound @a) + 1 + gToEnum i = K1 (toEnum (i + fromEnum (minBound @a))) + gFromEnum (K1 x) = fromEnum x - fromEnum (minBound @a) + +instance (GEnum f, GEnum g) => GEnum (f :+: g) where + gCard = gCard @f + gCard @g + gToEnum i + | i < gCard @f = L1 (gToEnum i) + | otherwise = R1 (gToEnum (i - gCard @f)) + gFromEnum = \case + L1 x -> gFromEnum x + R1 y -> gCard @f + gFromEnum y + +instance (GEnum f, GEnum g) => GEnum (f :*: g) where + gCard = gCard @f * gCard @g + gToEnum i = case i `divMod` gCard @g of + (q, r) -> gToEnum q :*: gToEnum r + gFromEnum (x :*: y) = gFromEnum x * gCard @g + gFromEnum y + +instance (Generic a, GEnum (Rep a)) => Bounded (GenericEnum a) where + minBound = GenericEnum . to $ gToEnum 0 + maxBound = GenericEnum . to $ gToEnum (gCard @(Rep a) - 1) + +instance (Generic a, GEnum (Rep a)) => Enum (GenericEnum a) where + fromEnum (GenericEnum x) = gFromEnum (from x) + + toEnum i + | 0 <= i && i < gCard @(Rep a) = GenericEnum . to $ gToEnum i + | otherwise = error $ "GenericEnum: toEnum out of range: " <> show i + + -- the class defaults for these two run off past 'maxBound' + enumFrom x = enumFromTo x maxBound + + enumFromThen x y = + enumFromThenTo x y $ + if fromEnum y >= fromEnum x then maxBound else minBound diff --git a/libs/types-common/types-common.cabal b/libs/types-common/types-common.cabal index 7d1590c057..9396f1d2c2 100644 --- a/libs/types-common/types-common.cabal +++ b/libs/types-common/types-common.cabal @@ -19,6 +19,7 @@ library Data.Credentials Data.Domain Data.ETag + Data.GenericEnum Data.Handle Data.HavePendingInvitations Data.Id diff --git a/libs/wire-api/src/Wire/API/OAuth.hs b/libs/wire-api/src/Wire/API/OAuth.hs index 9392b7f909..611928997f 100644 --- a/libs/wire-api/src/Wire/API/OAuth.hs +++ b/libs/wire-api/src/Wire/API/OAuth.hs @@ -26,6 +26,7 @@ import Data.Aeson.Types qualified as A import Data.ByteArray (convert) import Data.ByteString.Conversion import Data.ByteString.Lazy (fromStrict, toStrict) +import Data.GenericEnum import Data.HashMap.Strict qualified as HM import Data.Id as Id import Data.Json.Util @@ -196,49 +197,70 @@ instance ToSchema OAuthResponseType where -- However, having this typed makes it easier to handle scopes in the backend, -- and e.g. provide more meaningful error messages when the scope is invalid. data OAuthScope - = ReadFeatureConfigs - | ReadSelf - | WriteConversations - | WriteConversationsCode - | WriteConversationsName - | WriteMeetings - | AdminMeetings - deriving (Eq, Show, Generic, Ord, Bounded, Enum) + = FeatureConfigs OAuthTier + | Self OAuthTier + | Conversations OAuthTier + | ConversationsCode OAuthTier + | ConversationsName OAuthTier + | Meetings OAuthTier + deriving (Eq, Show, Generic, Ord) deriving (Arbitrary) via (GenericUniform OAuthScope) + deriving (Bounded, Enum) via (GenericEnum OAuthScope) + +-- `Write` implies `Read`, `Admin` implies `Write`. +data OAuthTier = Read | Write | Admin + deriving (Eq, Show, Generic, Ord, Bounded, Enum) + deriving (Arbitrary) via (GenericUniform OAuthTier) + +-- | Reflect a type-level 'OAuthTier' (as used in the routing tables via +-- 'Wire.API.Routes.Public.DescriptionOAuthScope') down to the value level. +class IsOAuthTier (t :: OAuthTier) where + toOAuthTier :: OAuthTier + +instance IsOAuthTier 'Read where + toOAuthTier = Read + +instance IsOAuthTier 'Write where + toOAuthTier = Write + +instance IsOAuthTier 'Admin where + toOAuthTier = Admin class IsOAuthScope scope where toOAuthScope :: OAuthScope -instance IsOAuthScope 'WriteConversations where - toOAuthScope = WriteConversations +instance (IsOAuthTier t) => IsOAuthScope ('Conversations t) where + toOAuthScope = Conversations (toOAuthTier @t) -instance IsOAuthScope 'WriteConversationsCode where - toOAuthScope = WriteConversationsCode +instance (IsOAuthTier t) => IsOAuthScope ('ConversationsCode t) where + toOAuthScope = ConversationsCode (toOAuthTier @t) -instance IsOAuthScope 'ReadSelf where - toOAuthScope = ReadSelf +instance (IsOAuthTier t) => IsOAuthScope ('Self t) where + toOAuthScope = Self (toOAuthTier @t) -instance IsOAuthScope 'ReadFeatureConfigs where - toOAuthScope = ReadFeatureConfigs +instance (IsOAuthTier t) => IsOAuthScope ('FeatureConfigs t) where + toOAuthScope = FeatureConfigs (toOAuthTier @t) -instance IsOAuthScope 'WriteConversationsName where - toOAuthScope = WriteConversationsName +instance (IsOAuthTier t) => IsOAuthScope ('ConversationsName t) where + toOAuthScope = ConversationsName (toOAuthTier @t) -instance IsOAuthScope 'WriteMeetings where - toOAuthScope = WriteMeetings +instance (IsOAuthTier t) => IsOAuthScope ('Meetings t) where + toOAuthScope = Meetings (toOAuthTier @t) -instance IsOAuthScope 'AdminMeetings where - toOAuthScope = AdminMeetings +instance ToByteString OAuthTier where + builder = \case + Read -> "read" + Write -> "write" + Admin -> "admin" instance ToByteString OAuthScope where builder = \case - WriteConversations -> "write:conversations" - WriteConversationsCode -> "write:conversations_code" - WriteConversationsName -> "write:conversations_name" - WriteMeetings -> "write:meetings" - AdminMeetings -> "admin:meetings" - ReadSelf -> "read:self" - ReadFeatureConfigs -> "read:feature_configs" + FeatureConfigs t -> builder t <> ":feature_configs" + Self t -> builder t <> ":self" + Conversations t -> builder t <> ":conversations" + ConversationsCode t -> builder t <> ":conversations_code" + ConversationsName t -> builder t <> ":conversations_name" + Meetings t -> builder t <> ":meetings" instance FromByteString OAuthScope where parser = do diff --git a/libs/wire-api/src/Wire/API/Routes/Public/Brig.hs b/libs/wire-api/src/Wire/API/Routes/Public/Brig.hs index fd3c390173..56dfacfacd 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Brig.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Brig.hs @@ -449,7 +449,7 @@ type SelfAPI = Named "get-self" ( Summary "Get your own profile" - :> DescriptionOAuthScope 'ReadSelf + :> DescriptionOAuthScope ('Self 'Read) :> ZLocalUser :> "self" :> Get '[JSON] SelfProfile diff --git a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Conversation.hs b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Conversation.hs index 68bb60a9d3..7862731910 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Conversation.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Conversation.hs @@ -407,7 +407,7 @@ type ConversationAPI = :<|> Named "create-group-conversation@v2" ( Summary "Create a new conversation" - :> DescriptionOAuthScope 'WriteConversations + :> DescriptionOAuthScope ('Conversations 'Write) :> Until 'V3 :> CanThrow 'ConvAccessDenied :> CanThrow 'MLSNonEmptyMemberList @@ -430,7 +430,7 @@ type ConversationAPI = :<|> Named "create-group-conversation@v3" ( Summary "Create a new conversation" - :> DescriptionOAuthScope 'WriteConversations + :> DescriptionOAuthScope ('Conversations 'Write) :> From 'V3 :> Until 'V4 :> CanThrow 'ConvAccessDenied @@ -534,7 +534,7 @@ type ConversationAPI = :<|> Named "create-group-conversation" ( Summary "Create a new conversation" - :> DescriptionOAuthScope 'WriteConversations + :> DescriptionOAuthScope ('Conversations 'Write) :> From 'V16 :> CanThrow 'ConvAccessDenied :> CanThrow 'MLSNonEmptyMemberList @@ -1085,7 +1085,7 @@ type ConversationAPI = "create-conversation-code-unqualified@v3" ( Summary "Create or recreate a conversation code" :> Until 'V4 - :> DescriptionOAuthScope 'WriteConversationsCode + :> DescriptionOAuthScope ('ConversationsCode 'Write) :> CanThrow 'ConvAccessDenied :> CanThrow 'ConvNotFound :> CanThrow 'GuestLinksDisabled @@ -1104,7 +1104,7 @@ type ConversationAPI = "create-conversation-code-unqualified" ( Summary "Create or recreate a conversation code" :> From 'V4 - :> DescriptionOAuthScope 'WriteConversationsCode + :> DescriptionOAuthScope ('ConversationsCode 'Write) :> CanThrow 'ConvAccessDenied :> CanThrow 'ConvNotFound :> CanThrow 'GuestLinksDisabled @@ -1151,7 +1151,7 @@ type ConversationAPI = :<|> Named "get-code" ( Summary "Get existing conversation code" - :> DescriptionOAuthScope 'WriteConversationsCode + :> DescriptionOAuthScope ('ConversationsCode 'Read) :> CanThrow 'CodeNotFound :> CanThrow 'ConvAccessDenied :> CanThrow 'ConvNotFound @@ -1340,7 +1340,7 @@ type ConversationAPI = :<|> Named "update-conversation-name" ( Summary "Update conversation name" - :> DescriptionOAuthScope 'WriteConversationsName + :> DescriptionOAuthScope ('ConversationsName 'Write) :> CanThrow ('ActionDenied 'ModifyConversationName) :> CanThrow 'ConvNotFound :> CanThrow 'InvalidOperation diff --git a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Feature.hs b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Feature.hs index 923ecc7d4a..5e49ba598b 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Feature.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Feature.hs @@ -272,7 +272,7 @@ type AllTeamFeaturesUserGet = :> Description "Gets feature configs for a user. If the user is a member of a team and has the required permissions, this will return the team's feature configs.\ \If the user is not a member of a team, this will return the personal feature configs (the server defaults)." - :> DescriptionOAuthScope 'ReadFeatureConfigs + :> DescriptionOAuthScope ('FeatureConfigs 'Read) :> ZUser :> CanThrow 'NotATeamMember :> CanThrow OperationDenied diff --git a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Meetings.hs b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Meetings.hs index db65df7147..536e39b508 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Meetings.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Meetings.hs @@ -50,7 +50,7 @@ type MeetingsAPI = :<|> Named "create-meeting" ( Summary "Create a new meeting" - :> DescriptionOAuthScope 'WriteMeetings + :> DescriptionOAuthScope ('Meetings 'Write) :> From 'V17 :> ZLocalUser :> ZConn diff --git a/services/brig/test/integration/API/OAuth.hs b/services/brig/test/integration/API/OAuth.hs index 7aa0f85d14..862420dcb7 100644 --- a/services/brig/test/integration/API/OAuth.hs +++ b/services/brig/test/integration/API/OAuth.hs @@ -159,7 +159,7 @@ testCreateOAuthCodeSuccess brig = do let newOAuthClient@(OAuthClientConfig _ redirectUrl) = newOAuthClientRequestBody "E Corp" "https://example.com" c <- registerNewOAuthClient brig newOAuthClient uid <- randomId - let scope = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scope = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] state <- UUID.toText <$> liftIO nextRandom createOAuthCode brig uid (CreateOAuthAuthorizationCodeRequest c.clientId scope OAuthResponseTypeCode redirectUrl state S256 challenge) !!! do @@ -202,7 +202,7 @@ testCreateAccessTokenSuccess opts brig = do now <- liftIO getCurrentTime user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.singleton ReadSelf + let scopes = OAuthScopes $ Set.singleton (Self Read) (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -231,7 +231,7 @@ testCreateAccessTokenWrongClientId :: Brig -> Http () testCreateAccessTokenWrongClientId brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scopes = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] (_, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl cid <- randomId let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl @@ -243,7 +243,7 @@ testCreateAccessTokenWrongAuthorizationCode :: Brig -> Http () testCreateAccessTokenWrongAuthorizationCode brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scopes = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] (cid, _) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let code = OAuthAuthorizationCode $ encodeBase16 "eb32eb9e2aa36c081c89067dddf81bce83c1c57e0b74cfb14c9f026f145f2b1f" let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl @@ -255,7 +255,7 @@ testCreateAccessTokenWrongUrl :: Brig -> Http () testCreateAccessTokenWrongUrl brig = do uid <- randomId let redirectUrl = mkUrl "https://wire.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scopes = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let wrongUrl = mkUrl "https://example.com" let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code wrongUrl @@ -268,7 +268,7 @@ testCreateAccessTokenExpiredCode opts brig = withSettingsOverrides (opts & Opt.settingsLens . Opt.oAuthAuthorizationCodeExpirationTimeSecsInternalLens ?~ 1) $ do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scopes = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl liftIO $ threadDelay (1 * 1200 * 1000) let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl @@ -280,7 +280,7 @@ testCreateAccessTokenWrongGrantType :: Brig -> Http () testCreateAccessTokenWrongGrantType brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scopes = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeRefreshToken cid verifier code redirectUrl createOAuthAccessToken' brig accessTokenRequest !!! assertAccessDenied @@ -289,7 +289,7 @@ testCreateAccessTokenWrongCodeChallenge :: Brig -> Http () testCreateAccessTokenWrongCodeChallenge brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scopes = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] (cid, code) <- generateOAuthClientAndAuthorizationCode' wrongCodeChallenge brig uid scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl createOAuthAccessToken' brig accessTokenRequest !!! do @@ -303,7 +303,7 @@ testCreateAccessTokenWrongCodeVerifier :: Brig -> Http () testCreateAccessTokenWrongCodeVerifier brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scopes = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid wrongCodeVerifier code redirectUrl createOAuthAccessToken' brig accessTokenRequest !!! do @@ -347,7 +347,7 @@ testRefreshAccessTokenAccessDeniedWhenDisabled :: Opt.Opts -> Brig -> Http () testRefreshAccessTokenAccessDeniedWhenDisabled opts brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [ReadSelf] + let scopes = OAuthScopes $ Set.fromList [Self Read] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -378,7 +378,7 @@ testAccessResourceSuccessNginz brig nginz = do -- with Authorization header containing an OAuth bearer token let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [ReadSelf] + let scopes = OAuthScopes $ Set.fromList [Self Read] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -389,7 +389,7 @@ testAccessResourceInsufficientScope :: Brig -> Nginz -> Http () testAccessResourceInsufficientScope brig nginz = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations] + let scopes = OAuthScopes $ Set.fromList [Conversations Write] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -401,7 +401,7 @@ testAccessResourceExpiredToken :: Brig -> Nginz -> Http () testAccessResourceExpiredToken brig nginz = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [ReadSelf] + let scopes = OAuthScopes $ Set.fromList [Self Read] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -426,7 +426,7 @@ testAccessResourceInvalidSignature :: Opt.Opts -> Brig -> Nginz -> Http () testAccessResourceInvalidSignature opts brig nginz = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [ReadSelf] + let scopes = OAuthScopes $ Set.fromList [Self Read] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -443,7 +443,7 @@ testRefreshTokenMaxActiveTokens opts db brig = uid <- randomId jwk <- liftIO $ readJwk (fromMaybe "path to jwk not set" opts.settings.oAuthJwkKeyPair) <&> fromMaybe (error "invalid key") let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [WriteConversations, WriteConversationsCode] + let scopes = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] let delayOneSec = -- we have to wait ~1 sec before we create the next token, to make sure it is created with a different timestamp -- this is due to the interpreter of the `Now` effect which auto-updates every second @@ -500,7 +500,7 @@ testRefreshTokenRetrieveAccessToken :: Brig -> Nginz -> Http () testRefreshTokenRetrieveAccessToken brig nginz = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [ReadSelf] + let scopes = OAuthScopes $ Set.fromList [Self Read] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -515,7 +515,7 @@ testRefreshTokenWrongSignature :: Opts -> Brig -> Http () testRefreshTokenWrongSignature opts brig = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [ReadSelf] + let scopes = OAuthScopes $ Set.fromList [Self Read] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -532,7 +532,7 @@ testRefreshTokenNoTokenId :: Opts -> Brig -> Http () testRefreshTokenNoTokenId opts brig = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [ReadSelf] + let scopes = OAuthScopes $ Set.fromList [Self Read] (cid, _) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl key <- liftIO $ readJwk (fromMaybe "path to jwk not set" opts.settings.oAuthJwkKeyPair) <&> fromMaybe (error "invalid key") badRefreshToken <- liftIO $ OAuthToken <$> signRefreshToken key emptyClaimsSet @@ -545,7 +545,7 @@ testRefreshTokenNonExistingId :: Opts -> Brig -> Http () testRefreshTokenNonExistingId opts brig = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [ReadSelf] + let scopes = OAuthScopes $ Set.fromList [Self Read] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -567,7 +567,7 @@ testRefreshTokenWrongClientId :: Brig -> Http () testRefreshTokenWrongClientId brig = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [ReadSelf] + let scopes = OAuthScopes $ Set.fromList [Self Read] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -581,7 +581,7 @@ testRefreshTokenWrongGrantType :: Brig -> Http () testRefreshTokenWrongGrantType brig = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [ReadSelf] + let scopes = OAuthScopes $ Set.fromList [Self Read] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -596,7 +596,7 @@ testRefreshTokenExpiredToken opts brig = withSettingsOverrides (opts & Opt.settingsLens . Opt.oAuthRefreshTokenExpirationTimeSecsInternalLens ?~ 2) $ do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [ReadSelf] + let scopes = OAuthScopes $ Set.fromList [Self Read] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -610,7 +610,7 @@ testRefreshTokenRevokedToken :: Brig -> Http () testRefreshTokenRevokedToken brig = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [ReadSelf] + let scopes = OAuthScopes $ Set.fromList [Self Read] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -645,21 +645,21 @@ testListApplicationsWithAccountAccess brig = do testWriteConversationsSuccessNginz :: Brig -> Nginz -> Http () testWriteConversationsSuccessNginz brig nginz = do (uid, tid) <- Team.createUserWithTeam brig - resp <- getAccessTokenForScope brig uid [WriteConversations] + resp <- getAccessTokenForScope brig uid [Conversations Write] createTeamConv nginz authHeader resp.accessToken tid "oauth test group" !!! do const 201 === statusCode testReadFeatureConfigsSuccessNginz :: Brig -> Nginz -> Http () testReadFeatureConfigsSuccessNginz brig nginz = do (uid, _) <- Team.createUserWithTeam brig - resp <- getAccessTokenForScope brig uid [ReadFeatureConfigs] + resp <- getAccessTokenForScope brig uid [FeatureConfigs Read] getFeatureConfigs nginz authHeader resp.accessToken !!! do const 200 === statusCode testWriteConversationsCodeSuccessNginz :: Brig -> Nginz -> Http () testWriteConversationsCodeSuccessNginz brig nginz = do (uid, tid) <- Team.createUserWithTeam brig - resp <- getAccessTokenForScope brig uid [WriteConversations, WriteConversationsCode] + resp <- getAccessTokenForScope brig uid [Conversations Write, ConversationsCode Write] conv <- responseJsonError @_ @(Conversation GroupConvType) =<< createTeamConv nginz authHeader resp.accessToken tid "oauth test group" Date: Thu, 17 Sep 2026 15:29:16 +0200 Subject: [PATCH 04/29] Unit tests for matching swagger with nginz config: support `oauth_scopes`. --- .../unit/Test/Wire/API/Routes/OAuthScopes.hs | 180 +++++++++--------- 1 file changed, 89 insertions(+), 91 deletions(-) diff --git a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs index 0400bc3a9a..d723a8b14d 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs @@ -1,4 +1,5 @@ {-# LANGUAGE TemplateHaskell #-} +{-# OPTIONS_GHC -Wno-incomplete-patterns #-} -- This file is part of the Wire Server implementation. -- @@ -49,7 +50,6 @@ import Servant.API (toUrlPiece) import Test.Tasty import Test.Tasty.HUnit import Text.Regex.TDFA ((=~)) -import Wire.API.OAuth (OAuthScope) import Wire.API.Routes.Public (renderOAuthScope) import Wire.API.Routes.Public.Swagger (devVersion, devVersionSwagger) import Wire.API.Routes.Version @@ -59,7 +59,6 @@ tests = testGroup "OAuth scopes (charts/nginz/values.yaml vs. swagger docs)" [ testCase "nginz path patterns avoid PCRE-only constructs" testPatternVocabulary, - testCase "every nginz oauth_scope names a real scope" testScopeNamesAreReal, testCase "enforced scopes and documented scopes agree" testScopesAgree ] @@ -78,29 +77,47 @@ newtype NginzLocations = NginzLocations [Location] data Location = Location { locPattern :: Text, - locScope :: Maybe Text + locOldScope :: Maybe Text, + locNewScopes :: [Text] } --- | The scope an OAuth token needs to get past nginz to this endpoint. +-- | The scopes that get an OAuth token past nginz to this endpoint. -- --- libzauth accepts a whole tier range per method, so a token holding --- @admin:meetings@ may also @POST@. Documenting every accepted scope would be --- noise, and would force @POST@ to be annotated with both @write:@ and --- @admin:@; what the docs should name is the /least/ privilege that suffices, --- so we take the lowest tier that is actually grantable. --- --- Empty when nginz requires no scope, and also when no tier it would accept is --- in 'Wire.API.OAuth.OAuthScope' -- then the endpoint cannot be reached with an --- OAuth token at all and there is nothing to document. Mistyped scope names --- are caught by 'testScopeNamesAreReal'. --- --- FUTUREWORK(fisx): https://wearezeta.atlassian.net/browse/WPB-28193 -enforcedScopes :: Text -> Text -> Set Text -enforcedScopes method path = - maybe Set.empty Set.singleton $ do - loc <- find (`locationMatches` path) nginzLocations - base <- locScope loc - find (`Set.member` grantableScopes) [tier <> ":" <> base | tier <- methodScopeTiers method] +-- Empty when nginz requires no scope, and also when it requires one +-- not in 'Wire.API.OAuth.OAuthScopes'. Mistyped scope names are +-- caught by 'testScopeNamesAreReal'. +enforcedScope :: Text -> Text -> Maybe Text +enforcedScope method path = do + loc <- find locationMatches nginzLocations + case loc.locOldScope of + Just s -> Just (methodScopeTier method <> ":" <> s) + Nothing -> find (methodScopeTier method `T.isPrefixOf`) loc.locNewScopes + where + -- Does this location capture that path? nginx anchors regex locations at the + -- start of the URI but not at the end, so a pattern without a trailing @$@ + -- matches every path with that prefix. + -- + -- The patterns are PCRE (that is what nginx uses) and we match them with + -- regex-tdfa, which is POSIX ERE. The two agree on the handful of constructs + -- values.yaml actually uses; 'testPatternVocabulary' keeps it that way. + locationMatches :: Location -> Bool + locationMatches loc = + T.unpack (probePath path) =~ T.unpack ("^" <> locPattern loc) + + -- @/conversations/{cnv}/code@ becomes @/conversations/PARAM/code@: the literal + -- segments still have to match, the captures must not. + probePath :: Text -> Text + probePath t = + let (before, rest) = T.breakOn "{" t + in if T.null rest + then before + else before <> "PARAM" <> probePath (T.drop 1 (T.dropWhile (/= '}') rest)) + + methodScopeTier :: Text -> Text + methodScopeTier "GET" = "read" + methodScopeTier "POST" = "write" + methodScopeTier "PUT" = "write" + methodScopeTier "DELETE" = "admin" nginzLocations :: [Location] nginzLocations = @@ -141,58 +158,60 @@ instance A.FromJSON NginzLocations where <> Map.restrictKeys extra (Set.fromList (enabled :: [Text])) instance A.FromJSON Location where - parseJSON = A.withObject "nginz upstream entry" $ \o -> - Location <$> o A..: "path" <*> o A..:? "oauth_scope" + parseJSON = A.withObject "nginz upstream entry" $ \o -> do + path <- o A..: "path" + oldScope :: Maybe Text <- do + s <- o A..:? "oauth_scope" + forM s validateOldScope + newScopes :: [Text] <- do + s <- o A..:? "oauth_scopes" A..!= [] + forM s validateNewScope + pure (Location path oldScope newScopes) --- | Does this location capture that path? nginx anchors regex locations at the --- start of the URI but not at the end, so a pattern without a trailing @$@ --- matches every path with that prefix. --- --- The patterns are PCRE (that is what nginx uses) and we match them with --- regex-tdfa, which is POSIX ERE. The two agree on the handful of constructs --- values.yaml actually uses; 'testPatternVocabulary' keeps it that way. -locationMatches :: Location -> Text -> Bool -locationMatches loc path = - T.unpack (probePath path) =~ T.unpack ("^" <> locPattern loc) +validateOldScope :: (MonadFail m) => Text -> m Text +validateOldScope s = if allowed then pure s else fail ("unknown scope: " <> show s) + where + allowed = + s + `elem` [ "feature_configs", + "self", + "conversations", + "conversations_code", + "conversations_name", + "meetings" + ] --- | @/conversations/{cnv}/code@ becomes @/conversations/PARAM/code@: the literal --- segments still have to match, the captures must not. -probePath :: Text -> Text -probePath t = - let (before, rest) = T.breakOn "{" t - in if T.null rest - then before - else before <> "PARAM" <> probePath (T.drop 1 (T.dropWhile (/= '}') rest)) +-- | NB: this could be re-written in terms of `data OAuth{Scope,Tier}` +-- to auto-re-align it with changes, but at the time of writing, the +-- changes to the data type had not been implemented yet. +validateNewScope :: (MonadFail m) => Text -> m Text +validateNewScope s = if allowed then pure s else fail ("unknown scope: " <> show s) + where + allowed = t && n + where + t = + T.takeWhile (/= ':') s `elem` ["read", "write", "admin"] + n = + T.dropWhile (/= ':') s + `elem` [ ":feature_configs", + ":self", + ":conversations", + ":conversations_code", + ":conversations_name", + ":meetings" + ] pcreOnlyConstructs :: [Text] pcreOnlyConstructs = ["(?", "\\", "{", "*?", "+?"] --- | @oauth_scope: foo@ in values.yaml names a scope without a tier; libzauth --- decides which tiers satisfy it from the request method. See @verify_scope@ in --- @libs/libzauth/libzauth/src/oauth.rs@. Listed in increasing order of --- privilege: 'enforcedScopes' takes the first grantable one, so this order --- decides which scope an endpoint gets documented with. -methodScopeTiers :: Text -> [Text] -methodScopeTiers = \case - "GET" -> ["read", "write", "admin"] - "POST" -> ["write", "admin"] - "PUT" -> ["write", "admin"] - "DELETE" -> ["admin"] - _ -> [] - -------------------------------------------------------------------------------- -- what the swagger docs claim -documentedScopes :: Text -> Set Text -documentedScopes descr = - Set.fromList - [ T.decodeUtf8 (toByteString' scope) - | scope <- [minBound .. maxBound] :: [OAuthScope], - -- Recognise a documented scope by the very string - -- 'renderOAuthScope' produces, so that the two cannot drift - -- apart. - renderOAuthScope scope `T.isInfixOf` descr - ] +documentedScope :: Text -> Maybe Text +documentedScope descr = enc <$> find prop [minBound ..] + where + enc = T.decodeUtf8 . toByteString' + prop = (`T.isInfixOf` descr) . renderOAuthScope httpMethods :: [Text] httpMethods = ["GET", "PUT", "POST", "DELETE", "OPTIONS", "HEAD", "PATCH", "TRACE"] @@ -219,21 +238,14 @@ data Finding = Finding { fVersion :: Version, fMethod :: Text, fPath :: Text, - fEnforced :: Set Text, - fDocumented :: Set Text + fEnforced :: Maybe Text, + fDocumented :: Maybe Text } --- | The scopes brig can actually issue. Anything else is not a scope at all: --- 'Wire.API.OAuth.OAuthScopes' fails to parse it, and yields the empty scope set --- for the whole request. -grantableScopes :: Set Text -grantableScopes = - Set.fromList [T.decodeUtf8 (toByteString' s) | s <- [minBound .. maxBound] :: [OAuthScope]] - renderFinding :: Finding -> Text renderFinding f = T.intercalate - "\t" + " " [ toUrlPiece (fVersion f), fMethod f, fPath f, @@ -245,8 +257,8 @@ findings :: [Finding] findings = [ Finding devVersion method path enforced documented | (path, method, descr) <- operations (A.toJSON devVersionSwagger), - let enforced = enforcedScopes method path, - let documented = documentedScopes descr, + let enforced = enforcedScope method path, + let documented = documentedScope descr, enforced /= documented ] @@ -266,20 +278,6 @@ testPatternVocabulary = <> "', which nginx reads as PCRE but this test matches with regex-tdfa, " <> "i.e. POSIX ERE. The two may disagree, which would be bad." --- | 'enforcedScopes' ignores scopes brig cannot issue, so a typo in an --- @oauth_scope:@ would otherwise make every endpoint under it drop silently out --- of the comparison. Require that each name is usable at some tier. -testScopeNamesAreReal :: Assertion -testScopeNamesAreReal = - for_ (nub (mapMaybe locScope nginzLocations)) $ \base -> - unless (any (\tier -> (tier <> ":" <> base) `Set.member` grantableScopes) ["read", "write", "admin"]) $ - assertFailure . T.unpack $ - "charts/nginz/values.yaml: 'oauth_scope: " - <> base - <> "' matches no scope in Wire.API.OAuth.OAuthScope at any tier, so no " - <> "OAuth token can ever satisfy it and every endpoint under that " - <> "location is closed to OAuth.\nEither fix the name, or add the scope." - testScopesAgree :: Assertion testScopesAgree = do unless (Set.null actual) . assertFailure . T.unpack . T.unlines $ From 32872a5d11462a6997ef0891f334f863abe14020 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Thu, 17 Sep 2026 09:38:19 +0200 Subject: [PATCH 05/29] Update nginz chart with new `oauth_scopes` attribute. The change to the values.yaml schema is fully backwards compatible, but since the new behavior should ignore `oauth_scope` if `oauth_scopes` is present, this will make wire-api tests fail. good. --- charts/nginz/values.yaml | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/charts/nginz/values.yaml b/charts/nginz/values.yaml index d114b3c7d3..a9ec9d0b45 100644 --- a/charts/nginz/values.yaml +++ b/charts/nginz/values.yaml @@ -228,7 +228,8 @@ nginx_conf: envs: - staging - path: /self$ # Matches exactly /self - oauth_scope: self + oauth_scope: self # deprecated, will be ignored if 'oauth_scopes' is present. + oauth_scopes: ["read:self", "write-only:self", "delete-only:self"] envs: - all - path: /self/name @@ -643,7 +644,8 @@ nginx_conf: - path: /conversations/([^/]*)/([^/]*)/name envs: - all - oauth_scope: conversations_name + oauth_scope: conversations_name # deprecated, will be ignored if 'oauth_scopes' is present. + oauth_scopes: ["write-only:conversations_name"] - path: /broadcast envs: - all @@ -660,11 +662,13 @@ nginx_conf: - path: /conversations$ envs: - all - oauth_scope: conversations + oauth_scope: conversations # deprecated, will be ignored if 'oauth_scopes' is present. + oauth_scopes: ["write-only:conversations"] - path: /conversations/([^/]*)/code envs: - all - oauth_scope: conversations_code + oauth_scope: conversations_code # deprecated, will be ignored if 'oauth_scopes' is present. + oauth_scopes: ["read:conversations_code", "write-only:conversations_code", "delete-only:conversations_code"] - path: /conversations/join envs: - all @@ -758,7 +762,8 @@ nginx_conf: - path: /feature-configs(.*) envs: - all - oauth_scope: feature_configs + oauth_scope: feature_configs # deprecated, will be ignored if 'oauth_scopes' is present. + oauth_scopes: ["read:feature_configs"] - path: /mls/welcome envs: - all @@ -785,12 +790,12 @@ nginx_conf: - path: /meetings$ envs: - all - oauth_scope: meetings - ## this rule can't be expressed yet: https://wearezeta.atlassian.net/browse/WPB-28193 - #- path: /meetings/([^/]*)/([^/]*)$ - # envs: - # - all - # oauth_scopes: [write:meetings, admin:meetings] + oauth_scope: meetings # deprecated, will be ignored if 'oauth_scopes' is present. + oauth_scopes: ["write-only:meetings"] + - path: /meetings/([^/]*)/([^/]*)$ + envs: + - all + oauth_scopes: ["write-only:meetings", "delete-only:meetings"] - path: /meetings/(.*) envs: - all From 1d9f5736c88c437221b7ac50f1ef6514056d2be2 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Fri, 18 Sep 2026 13:28:53 +0200 Subject: [PATCH 06/29] fixup tests. --- .../unit/Test/Wire/API/Routes/OAuthScopes.hs | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs index d723a8b14d..9582942c3f 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs @@ -90,8 +90,8 @@ enforcedScope :: Text -> Text -> Maybe Text enforcedScope method path = do loc <- find locationMatches nginzLocations case loc.locOldScope of - Just s -> Just (methodScopeTier method <> ":" <> s) - Nothing -> find (methodScopeTier method `T.isPrefixOf`) loc.locNewScopes + Just s -> Just (oldMethodScopeTier method <> ":" <> s) + Nothing -> find (newMethodScopeTier method `T.isPrefixOf`) loc.locNewScopes where -- Does this location capture that path? nginx anchors regex locations at the -- start of the URI but not at the end, so a pattern without a trailing @$@ @@ -113,11 +113,17 @@ enforcedScope method path = do then before else before <> "PARAM" <> probePath (T.drop 1 (T.dropWhile (/= '}') rest)) - methodScopeTier :: Text -> Text - methodScopeTier "GET" = "read" - methodScopeTier "POST" = "write" - methodScopeTier "PUT" = "write" - methodScopeTier "DELETE" = "admin" + oldMethodScopeTier :: Text -> Text + oldMethodScopeTier "GET" = "read" + oldMethodScopeTier "POST" = "write" + oldMethodScopeTier "PUT" = "write" + oldMethodScopeTier "DELETE" = "admin" + + newMethodScopeTier :: Text -> Text + newMethodScopeTier "GET" = "read" + newMethodScopeTier "POST" = "write-only" + newMethodScopeTier "PUT" = "write-only" + newMethodScopeTier "DELETE" = "delete-only" nginzLocations :: [Location] nginzLocations = @@ -169,7 +175,7 @@ instance A.FromJSON Location where pure (Location path oldScope newScopes) validateOldScope :: (MonadFail m) => Text -> m Text -validateOldScope s = if allowed then pure s else fail ("unknown scope: " <> show s) +validateOldScope s = if allowed then pure s else fail ("unknown old scope: " <> show s) where allowed = s @@ -185,12 +191,12 @@ validateOldScope s = if allowed then pure s else fail ("unknown scope: " <> show -- to auto-re-align it with changes, but at the time of writing, the -- changes to the data type had not been implemented yet. validateNewScope :: (MonadFail m) => Text -> m Text -validateNewScope s = if allowed then pure s else fail ("unknown scope: " <> show s) +validateNewScope s = if allowed then pure s else fail ("unknown new scope: " <> show s) where allowed = t && n where t = - T.takeWhile (/= ':') s `elem` ["read", "write", "admin"] + T.takeWhile (/= ':') s `elem` ["read", "write-only", "delete-only"] n = T.dropWhile (/= ':') s `elem` [ ":feature_configs", From c48d166e2b67ead89505cd94aefbeeb8d29c9175 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Fri, 18 Sep 2026 13:29:27 +0200 Subject: [PATCH 07/29] fixup values.yaml --- charts/nginz/values.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/charts/nginz/values.yaml b/charts/nginz/values.yaml index a9ec9d0b45..7c30e27a3d 100644 --- a/charts/nginz/values.yaml +++ b/charts/nginz/values.yaml @@ -795,7 +795,7 @@ nginx_conf: - path: /meetings/([^/]*)/([^/]*)$ envs: - all - oauth_scopes: ["write-only:meetings", "delete-only:meetings"] + oauth_scopes: [] # TODO: this will be `["write-only:meetings", "delete-only:meetings"]` soon, according to https://wearezeta.atlassian.net/browse/WPB-28194 - path: /meetings/(.*) envs: - all From dc1ee3ff94578cdfc4ee62e0f924db00e1049547 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 21 Sep 2026 14:13:09 +0200 Subject: [PATCH 08/29] Fix wire-api unit test to test new behavior. Now values.yaml must match docs according to new oauth scope semantics. The actual policy control happens in libzauth and is changed in the next commit. --- libs/wire-api/src/Wire/API/OAuth.hs | 66 +++- .../API/Routes/Public/Galley/Conversation.hs | 12 +- .../Wire/API/Routes/Public/Galley/Meetings.hs | 2 +- .../unit/Test/Wire/API/Routes/OAuthScopes.hs | 284 ++++++++++-------- services/brig/test/integration/API/OAuth.hs | 24 +- 5 files changed, 233 insertions(+), 155 deletions(-) diff --git a/libs/wire-api/src/Wire/API/OAuth.hs b/libs/wire-api/src/Wire/API/OAuth.hs index 611928997f..865e3c4508 100644 --- a/libs/wire-api/src/Wire/API/OAuth.hs +++ b/libs/wire-api/src/Wire/API/OAuth.hs @@ -196,6 +196,8 @@ instance ToSchema OAuthResponseType where -- with the supported scopes defined in the nginx configs. -- However, having this typed makes it easier to handle scopes in the backend, -- and e.g. provide more meaningful error messages when the scope is invalid. +-- +-- TODO: refactor this again: newtype `OAuthScopeReally = OAuthScopeReally { fromOAuthScopeReally :: (OAuthTier, OAuthScope) }` data OAuthScope = FeatureConfigs OAuthTier | Self OAuthTier @@ -207,11 +209,22 @@ data OAuthScope deriving (Arbitrary) via (GenericUniform OAuthScope) deriving (Bounded, Enum) via (GenericEnum OAuthScope) --- `Write` implies `Read`, `Admin` implies `Write`. -data OAuthTier = Read | Write | Admin +-- | Unlike 'OldOAuthScope', these tiers are disjoint. +-- TODO: s/Read/ReadOnly/g +data OAuthTier = Read | WriteOnly | DeleteOnly deriving (Eq, Show, Generic, Ord, Bounded, Enum) deriving (Arbitrary) via (GenericUniform OAuthTier) +-- | (Once the TODO on 'OAuthScope' is done this is just @fst@.) +oAuthScopeTier :: OAuthScope -> OAuthTier +oAuthScopeTier = \case + FeatureConfigs t -> t + Self t -> t + Conversations t -> t + ConversationsCode t -> t + ConversationsName t -> t + Meetings t -> t + -- | Reflect a type-level 'OAuthTier' (as used in the routing tables via -- 'Wire.API.Routes.Public.DescriptionOAuthScope') down to the value level. class IsOAuthTier (t :: OAuthTier) where @@ -220,11 +233,11 @@ class IsOAuthTier (t :: OAuthTier) where instance IsOAuthTier 'Read where toOAuthTier = Read -instance IsOAuthTier 'Write where - toOAuthTier = Write +instance IsOAuthTier 'WriteOnly where + toOAuthTier = WriteOnly -instance IsOAuthTier 'Admin where - toOAuthTier = Admin +instance IsOAuthTier 'DeleteOnly where + toOAuthTier = DeleteOnly class IsOAuthScope scope where toOAuthScope :: OAuthScope @@ -250,8 +263,8 @@ instance (IsOAuthTier t) => IsOAuthScope ('Meetings t) where instance ToByteString OAuthTier where builder = \case Read -> "read" - Write -> "write" - Admin -> "admin" + WriteOnly -> "write-only" + DeleteOnly -> "delete-only" instance ToByteString OAuthScope where builder = \case @@ -291,6 +304,43 @@ instance ToSchema OAuthScopes where & maybe Set.empty Set.fromList . mapM (fromByteString' . fromStrict . TE.encodeUtf8) +-- | The deprecated, cumulative scopes: @write:*@ implies @read:*@, +-- @admin:*@ implies @write:*@ (see @verify_scope@ in +-- @libs/libzauth/libzauth/src/oauth.rs@). +-- +-- NB: not every @:@ combination is a scope accepted by +-- the servant handler: if not listed here, the parser in the route +-- will reject it. +data OldOAuthScope + = ReadFeatureConfigs + | ReadSelf + | WriteConversations + | WriteConversationsCode + | WriteConversationsName + | WriteMeetings + | AdminMeetings + deriving (Eq, Show, Generic, Ord) + deriving (Arbitrary) via (GenericUniform OldOAuthScope) + deriving (Bounded, Enum) via (GenericEnum OldOAuthScope) + +instance ToByteString OldOAuthScope where + builder = \case + WriteConversations -> "write:conversations" + WriteConversationsCode -> "write:conversations_code" + WriteConversationsName -> "write:conversations_name" + WriteMeetings -> "write:meetings" + AdminMeetings -> "admin:meetings" + ReadSelf -> "read:self" + ReadFeatureConfigs -> "read:feature_configs" + +instance FromByteString OldOAuthScope where + parser = do + s <- (toByteString' . T.toLower) <$> parser + let table = Map.fromList [(toByteString' c, c) | c <- [(minBound :: OldOAuthScope) ..]] + case Map.lookup s table of + Just c -> pure c + Nothing -> fail $ "invalid legacy scope: " <> show s + data CodeChallengeMethod = S256 deriving (Eq, Show, Generic) deriving (Arbitrary) via (GenericUniform CodeChallengeMethod) diff --git a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Conversation.hs b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Conversation.hs index 7862731910..77eeca7909 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Conversation.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Conversation.hs @@ -407,7 +407,7 @@ type ConversationAPI = :<|> Named "create-group-conversation@v2" ( Summary "Create a new conversation" - :> DescriptionOAuthScope ('Conversations 'Write) + :> DescriptionOAuthScope ('Conversations 'WriteOnly) -- TODO: use servant to run the policy check, not just for openapi! :> Until 'V3 :> CanThrow 'ConvAccessDenied :> CanThrow 'MLSNonEmptyMemberList @@ -430,7 +430,7 @@ type ConversationAPI = :<|> Named "create-group-conversation@v3" ( Summary "Create a new conversation" - :> DescriptionOAuthScope ('Conversations 'Write) + :> DescriptionOAuthScope ('Conversations 'WriteOnly) :> From 'V3 :> Until 'V4 :> CanThrow 'ConvAccessDenied @@ -534,7 +534,7 @@ type ConversationAPI = :<|> Named "create-group-conversation" ( Summary "Create a new conversation" - :> DescriptionOAuthScope ('Conversations 'Write) + :> DescriptionOAuthScope ('Conversations 'WriteOnly) :> From 'V16 :> CanThrow 'ConvAccessDenied :> CanThrow 'MLSNonEmptyMemberList @@ -1085,7 +1085,7 @@ type ConversationAPI = "create-conversation-code-unqualified@v3" ( Summary "Create or recreate a conversation code" :> Until 'V4 - :> DescriptionOAuthScope ('ConversationsCode 'Write) + :> DescriptionOAuthScope ('ConversationsCode 'WriteOnly) :> CanThrow 'ConvAccessDenied :> CanThrow 'ConvNotFound :> CanThrow 'GuestLinksDisabled @@ -1104,7 +1104,7 @@ type ConversationAPI = "create-conversation-code-unqualified" ( Summary "Create or recreate a conversation code" :> From 'V4 - :> DescriptionOAuthScope ('ConversationsCode 'Write) + :> DescriptionOAuthScope ('ConversationsCode 'WriteOnly) :> CanThrow 'ConvAccessDenied :> CanThrow 'ConvNotFound :> CanThrow 'GuestLinksDisabled @@ -1340,7 +1340,7 @@ type ConversationAPI = :<|> Named "update-conversation-name" ( Summary "Update conversation name" - :> DescriptionOAuthScope ('ConversationsName 'Write) + :> DescriptionOAuthScope ('ConversationsName 'WriteOnly) :> CanThrow ('ActionDenied 'ModifyConversationName) :> CanThrow 'ConvNotFound :> CanThrow 'InvalidOperation diff --git a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Meetings.hs b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Meetings.hs index 536e39b508..380045db02 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Meetings.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Meetings.hs @@ -50,7 +50,7 @@ type MeetingsAPI = :<|> Named "create-meeting" ( Summary "Create a new meeting" - :> DescriptionOAuthScope ('Meetings 'Write) + :> DescriptionOAuthScope ('Meetings 'WriteOnly) :> From 'V17 :> ZLocalUser :> ZConn diff --git a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs index 9582942c3f..f0fecdacd6 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs @@ -1,5 +1,4 @@ {-# LANGUAGE TemplateHaskell #-} -{-# OPTIONS_GHC -Wno-incomplete-patterns #-} -- This file is part of the Wire Server implementation. -- @@ -18,26 +17,14 @@ -- You should have received a copy of the GNU Affero General Public License along -- with this program. If not, see . --- | Two independent places declare which OAuth scope an endpoint needs, and --- nothing keeps them in sync: --- --- 1. @charts/nginz/values.yaml@ -- @oauth_scope:@ on an upstream entry. This is --- what is actually /enforced/: nginz rejects OAuth tokens without the scope. --- 2. The servant routing tables -- 'Wire.API.Routes.Public.DescriptionOAuthScope'. --- This is only /documentation/: it appends a line to the endpoint description --- in the swagger docs and has no effect on request handling. --- --- Forgetting (2) while doing (1) -- or, more commonly, adding a new version of an --- endpoint that is already covered by (1) and not carrying the annotation over -- --- silently produces endpoints that reject OAuth tokens for a scope documented --- nowhere. This module compares the two for the development version, which is --- the only one still assembled from the routing tables. module Test.Wire.API.Routes.OAuthScopes (tests) where +-- TODO: test backwards compatibility (copy old values.yaml to wire-api tests and run them against the same swagger. + import Data.Aeson qualified as A import Data.Aeson.Key qualified as Key import Data.Aeson.KeyMap qualified as KeyMap -import Data.ByteString.Conversion (toByteString') +import Data.ByteString.Conversion import Data.FileEmbed (embedFile, makeRelativeToProject) import Data.Map qualified as Map import Data.Set qualified as Set @@ -50,6 +37,7 @@ import Servant.API (toUrlPiece) import Test.Tasty import Test.Tasty.HUnit import Text.Regex.TDFA ((=~)) +import Wire.API.OAuth import Wire.API.Routes.Public (renderOAuthScope) import Wire.API.Routes.Public.Swagger (devVersion, devVersionSwagger) import Wire.API.Routes.Version @@ -58,10 +46,65 @@ tests :: TestTree tests = testGroup "OAuth scopes (charts/nginz/values.yaml vs. swagger docs)" - [ testCase "nginz path patterns avoid PCRE-only constructs" testPatternVocabulary, - testCase "enforced scopes and documented scopes agree" testScopesAgree + [ testCase "enforced scopes and documented scopes agree" testScopesAgree, + testCase "nginz path patterns avoid PCRE-only constructs" testPatternVocabulary ] +-- | Two independent places declare which OAuth scope an endpoint needs, and +-- nothing keeps them in sync: +-- +-- 1. @charts/nginz/values.yaml@: @oauth_scope:@ (deprecated) or @oauth_scopes:@ +-- on an upstream entry. This is what is actually /enforced/: nginz rejects +-- OAuth tokens without a matching scope, and rejects all of them +-- where no scope is configured at all. +-- 2. The servant routing tables: 'Wire.API.Routes.Public.DescriptionOAuthScope'. +-- This is only documentation: it appends a line to the endpoint description +-- in the swagger docs and has no effect on request handling. +-- +-- This test matches the openapi docs generated from servant routes +-- against what nginz enforces. The behavior of nginz is emulated by +-- this test. This is not testing any properties of libzauth. +testScopesAgree :: Assertion +testScopesAgree = do + unless (Set.null actual) . assertFailure . T.unpack . T.unlines $ + [ "OAuth scope declarations are out of sync.", + "", + "Columns: version, method, path, accepted by nginz, documented in swagger.", + "The nginz column lists every scope that gets an OAuth token through to that", + "verb; '[]' means none does, i.e. OAuth is not usable there at all (the route", + "may still be reachable with a zauth cookie or token). A finding means", + "swagger.json does not match values.yaml:", + "", + " accepted but not documented charts/nginz/values.yaml lets a scope through", + " that the swagger docs do not mention -- most", + " likely a missing DescriptionOAuthScope in the", + " routing table, e.g. on a newly added version of", + " an endpoint that already had one.", + " documented but not accepted the swagger docs promise a scope that does not", + " get anybody in -- a stale annotation, or a scope", + " missing from charts/nginz/values.yaml.", + "" + ] + <> section "deviations:" actual + where + actual = Set.fromList (renderFinding <$> findings) + section title xs + | Set.null xs = [] + | otherwise = [" " <> title] <> ((" " <>) <$> Set.toAscList xs) <> [""] + +testPatternVocabulary :: Assertion +testPatternVocabulary = + for_ nginzLocations $ \loc -> + for_ pcreOnlyConstructs $ \bad -> + when (bad `T.isInfixOf` locPattern loc) $ + assertFailure . T.unpack $ + "charts/nginz/values.yaml: the path pattern " + <> locPattern loc + <> " uses '" + <> bad + <> "', which nginx reads as PCRE but this test matches with regex-tdfa, " + <> "i.e. POSIX ERE. The two may disagree, which would be bad." + -------------------------------------------------------------------------------- -- what nginz enforces @@ -77,21 +120,41 @@ newtype NginzLocations = NginzLocations [Location] data Location = Location { locPattern :: Text, - locOldScope :: Maybe Text, - locNewScopes :: [Text] + locOldScope :: Maybe Text, -- can't use OldOAuthScope because we don't know the HTTP verb yet. + locNewScopes :: Maybe [OAuthScope] } --- | The scopes that get an OAuth token past nginz to this endpoint. +-- | Which scopes let an OAuth token through to this method and path? The +-- answer is always given in new scopes, also where values.yaml still uses old +-- ones. +-- +-- If the matching location has @oauth_scopes@, the answer is those of the +-- listed scopes that have the tier this verb needs. +-- +-- If it only has the deprecated @oauth_scope@, we first ask which old scopes +-- nginz lets through for this verb. Old scopes are cumulative, so a @GET@ gets +-- through with @read:@, @write:@, or @admin:@, but only some of those exist as +-- scopes a client can be granted. Then we translate the ones that do into new +-- scopes. Example: for a @GET@ under @oauth_scope: conversations_code@, the +-- only old scope that exists and passes is @write:conversations_code@, and that +-- one is @read:conversations_code@ plus @write-only:conversations_code@ in new +-- scopes: if wire-api requires new read *or* new write, old write is acceptable. -- --- Empty when nginz requires no scope, and also when it requires one --- not in 'Wire.API.OAuth.OAuthScopes'. Mistyped scope names are --- caught by 'testScopeNamesAreReal'. -enforcedScope :: Text -> Text -> Maybe Text -enforcedScope method path = do - loc <- find locationMatches nginzLocations - case loc.locOldScope of - Just s -> Just (oldMethodScopeTier method <> ":" <> s) - Nothing -> find (newMethodScopeTier method `T.isPrefixOf`) loc.locNewScopes +-- NB: We do not need to distinguish between "nginz asks for no scope +-- at all" and "asks for one that nobody can have": a @DELETE@ under a +-- deprecated @oauth_scope@ needs @admin:\@, and for most bases +-- that scope does not exist, in which case no OAuth token can be +-- accepted. +enforcedScopes :: Text -> Text -> Set OAuthScope +enforcedScopes method path = case find locationMatches nginzLocations of + Nothing -> Set.empty + Just loc -> case (loc.locOldScope, loc.locNewScopes) of + (_, Just newScopes) -> + -- Filter scopes listed in values.yaml by matching method/tier. + Set.fromList (filter ((newTier method ==) . Just . oAuthScopeTier) newScopes) + (Just base, Nothing) -> + Set.fromList (oldOAuthScopeToNewScopes =<< oldScopeBaseAccepted base) + (Nothing, Nothing) -> Set.empty where -- Does this location capture that path? nginx anchors regex locations at the -- start of the URI but not at the end, so a pattern without a trailing @$@ @@ -113,17 +176,43 @@ enforcedScope method path = do then before else before <> "PARAM" <> probePath (T.drop 1 (T.dropWhile (/= '}') rest)) - oldMethodScopeTier :: Text -> Text - oldMethodScopeTier "GET" = "read" - oldMethodScopeTier "POST" = "write" - oldMethodScopeTier "PUT" = "write" - oldMethodScopeTier "DELETE" = "admin" + -- Which tier is strictly required for which verb? 'Nothing' for the verbs + -- nginz has no rule for; nothing gets those past an oauth_scope directive. + newTier :: Text -> Maybe OAuthTier + newTier = \case + "GET" -> Just Read + "POST" -> Just WriteOnly + "PUT" -> Just WriteOnly + "DELETE" -> Just DeleteOnly + _ -> Nothing + + -- The deprecated @oauth_scope@ names only the base; which tiers + -- of it are accepted follows from the verb in the request, + -- cumulatively. Of those @:@ combinations only the + -- ones that parse exist as grantable scopes. + oldScopeBaseAccepted :: Text -> [OldOAuthScope] + oldScopeBaseAccepted base = + mapMaybe (\tier -> fromByteString (T.encodeUtf8 (tier <> ":" <> base))) (oldTiers method) + + -- Mirrors @verify_scope@ in @libs/libzauth/libzauth/src/oauth.rs@, which is + -- what nginz calls: `write:*` implies `read:*`, `admin:*` implies `write:*`. + oldTiers :: Text -> [Text] + oldTiers = \case + "GET" -> ["read", "write", "admin"] + "POST" -> ["write", "admin"] + "PUT" -> ["write", "admin"] + "DELETE" -> ["admin"] + _ -> [] - newMethodScopeTier :: Text -> Text - newMethodScopeTier "GET" = "read" - newMethodScopeTier "POST" = "write-only" - newMethodScopeTier "PUT" = "write-only" - newMethodScopeTier "DELETE" = "delete-only" +oldOAuthScopeToNewScopes :: OldOAuthScope -> [OAuthScope] +oldOAuthScopeToNewScopes = \case + ReadFeatureConfigs -> [FeatureConfigs Read] + ReadSelf -> [Self Read] + WriteConversations -> [Conversations Read, Conversations WriteOnly] + WriteConversationsCode -> [ConversationsCode Read, ConversationsCode WriteOnly] + WriteConversationsName -> [ConversationsName Read, ConversationsName WriteOnly] + WriteMeetings -> [Meetings Read, Meetings WriteOnly] + AdminMeetings -> [Meetings Read, Meetings WriteOnly, Meetings DeleteOnly] nginzLocations :: [Location] nginzLocations = @@ -167,45 +256,18 @@ instance A.FromJSON Location where parseJSON = A.withObject "nginz upstream entry" $ \o -> do path <- o A..: "path" oldScope :: Maybe Text <- do - s <- o A..:? "oauth_scope" - forM s validateOldScope - newScopes :: [Text] <- do - s <- o A..:? "oauth_scopes" A..!= [] - forM s validateNewScope + o A..:? "oauth_scope" + newScopes :: Maybe [OAuthScope] <- do + mbs :: Maybe [Text] <- o A..:? "oauth_scopes" + mapM (mapM validateNewScope) mbs pure (Location path oldScope newScopes) -validateOldScope :: (MonadFail m) => Text -> m Text -validateOldScope s = if allowed then pure s else fail ("unknown old scope: " <> show s) - where - allowed = - s - `elem` [ "feature_configs", - "self", - "conversations", - "conversations_code", - "conversations_name", - "meetings" - ] - --- | NB: this could be re-written in terms of `data OAuth{Scope,Tier}` --- to auto-re-align it with changes, but at the time of writing, the --- changes to the data type had not been implemented yet. -validateNewScope :: (MonadFail m) => Text -> m Text -validateNewScope s = if allowed then pure s else fail ("unknown new scope: " <> show s) - where - allowed = t && n - where - t = - T.takeWhile (/= ':') s `elem` ["read", "write-only", "delete-only"] - n = - T.dropWhile (/= ':') s - `elem` [ ":feature_configs", - ":self", - ":conversations", - ":conversations_code", - ":conversations_name", - ":meetings" - ] +validateNewScope :: (MonadFail m) => Text -> m OAuthScope +validateNewScope s = + fromByteString @OAuthScope (T.encodeUtf8 s) + & maybe + (fail ("unknown new scope: " <> show s)) + pure pcreOnlyConstructs :: [Text] pcreOnlyConstructs = ["(?", "\\", "{", "*?", "+?"] @@ -213,11 +275,8 @@ pcreOnlyConstructs = ["(?", "\\", "{", "*?", "+?"] -------------------------------------------------------------------------------- -- what the swagger docs claim -documentedScope :: Text -> Maybe Text -documentedScope descr = enc <$> find prop [minBound ..] - where - enc = T.decodeUtf8 . toByteString' - prop = (`T.isInfixOf` descr) . renderOAuthScope +documentedScope :: Text -> Maybe OAuthScope +documentedScope descr = find ((`T.isInfixOf` descr) . renderOAuthScope) [minBound ..] httpMethods :: [Text] httpMethods = ["GET", "PUT", "POST", "DELETE", "OPTIONS", "HEAD", "PATCH", "TRACE"] @@ -239,13 +298,13 @@ operations doc = do -------------------------------------------------------------------------------- -- the comparison --- | 'Finding's are interesting iff @fEnforced /= fDocumented@. +-- | 'Finding's are interesting iff 'fDocumented' is not one of 'fEnforced'. data Finding = Finding { fVersion :: Version, fMethod :: Text, fPath :: Text, - fEnforced :: Maybe Text, - fDocumented :: Maybe Text + fEnforced :: Set OAuthScope, + fDocumented :: Maybe OAuthScope } renderFinding :: Finding -> Text @@ -255,7 +314,7 @@ renderFinding f = [ toUrlPiece (fVersion f), fMethod f, fPath f, - T.pack . show . toList $ fEnforced f, + T.pack . show . Set.toList $ fEnforced f, T.pack . show . toList $ fDocumented f ] @@ -263,48 +322,17 @@ findings :: [Finding] findings = [ Finding devVersion method path enforced documented | (path, method, descr) <- operations (A.toJSON devVersionSwagger), - let enforced = enforcedScope method path, + let enforced = enforcedScopes method path, let documented = documentedScope descr, - enforced /= documented + not (scopesMatch enforced documented) ] --------------------------------------------------------------------------------- --- the actual tests - -testPatternVocabulary :: Assertion -testPatternVocabulary = - for_ nginzLocations $ \loc -> - for_ pcreOnlyConstructs $ \bad -> - when (bad `T.isInfixOf` locPattern loc) $ - assertFailure . T.unpack $ - "charts/nginz/values.yaml: the path pattern " - <> locPattern loc - <> " uses '" - <> bad - <> "', which nginx reads as PCRE but this test matches with regex-tdfa, " - <> "i.e. POSIX ERE. The two may disagree, which would be bad." - -testScopesAgree :: Assertion -testScopesAgree = do - unless (Set.null actual) . assertFailure . T.unpack . T.unlines $ - [ "OAuth scope declarations are out of sync.", - "", - "Columns: version, method, path, accepted by nginz, documented in swagger.", - "'-' means no scope. A finding means those last two disagree:", - "", - " enforced but not documented charts/nginz/values.yaml requires a scope the", - " swagger docs do not mention -- most likely a", - " missing DescriptionOAuthScope in the routing", - " table, e.g. on a newly added version of an", - " endpoint that already had one.", - " documented but not enforced the swagger docs promise a scope nginz does not", - " require -- a stale annotation, or a missing", - " oauth_scope: in charts/nginz/values.yaml.", - "" - ] - <> section "deviations:" actual - where - actual = Set.fromList (renderFinding <$> findings) - section title xs - | Set.null xs = [] - | otherwise = [" " <> title] <> ((" " <>) <$> Set.toAscList xs) <> [""] +-- | The documented scope has to be one of the scopes that actually get a token +-- through. If no token gets through at all, there is nothing to document. +scopesMatch :: + -- | required + Set OAuthScope -> + -- | documented + Maybe OAuthScope -> + Bool +scopesMatch enforced = maybe (Set.null enforced) (`Set.member` enforced) diff --git a/services/brig/test/integration/API/OAuth.hs b/services/brig/test/integration/API/OAuth.hs index 862420dcb7..d3da7f8b0e 100644 --- a/services/brig/test/integration/API/OAuth.hs +++ b/services/brig/test/integration/API/OAuth.hs @@ -159,7 +159,7 @@ testCreateOAuthCodeSuccess brig = do let newOAuthClient@(OAuthClientConfig _ redirectUrl) = newOAuthClientRequestBody "E Corp" "https://example.com" c <- registerNewOAuthClient brig newOAuthClient uid <- randomId - let scope = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] + let scope = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] state <- UUID.toText <$> liftIO nextRandom createOAuthCode brig uid (CreateOAuthAuthorizationCodeRequest c.clientId scope OAuthResponseTypeCode redirectUrl state S256 challenge) !!! do @@ -231,7 +231,7 @@ testCreateAccessTokenWrongClientId :: Brig -> Http () testCreateAccessTokenWrongClientId brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] + let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] (_, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl cid <- randomId let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl @@ -243,7 +243,7 @@ testCreateAccessTokenWrongAuthorizationCode :: Brig -> Http () testCreateAccessTokenWrongAuthorizationCode brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] + let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] (cid, _) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let code = OAuthAuthorizationCode $ encodeBase16 "eb32eb9e2aa36c081c89067dddf81bce83c1c57e0b74cfb14c9f026f145f2b1f" let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl @@ -255,7 +255,7 @@ testCreateAccessTokenWrongUrl :: Brig -> Http () testCreateAccessTokenWrongUrl brig = do uid <- randomId let redirectUrl = mkUrl "https://wire.com" - let scopes = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] + let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let wrongUrl = mkUrl "https://example.com" let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code wrongUrl @@ -268,7 +268,7 @@ testCreateAccessTokenExpiredCode opts brig = withSettingsOverrides (opts & Opt.settingsLens . Opt.oAuthAuthorizationCodeExpirationTimeSecsInternalLens ?~ 1) $ do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] + let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl liftIO $ threadDelay (1 * 1200 * 1000) let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl @@ -280,7 +280,7 @@ testCreateAccessTokenWrongGrantType :: Brig -> Http () testCreateAccessTokenWrongGrantType brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] + let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeRefreshToken cid verifier code redirectUrl createOAuthAccessToken' brig accessTokenRequest !!! assertAccessDenied @@ -289,7 +289,7 @@ testCreateAccessTokenWrongCodeChallenge :: Brig -> Http () testCreateAccessTokenWrongCodeChallenge brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] + let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] (cid, code) <- generateOAuthClientAndAuthorizationCode' wrongCodeChallenge brig uid scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl createOAuthAccessToken' brig accessTokenRequest !!! do @@ -303,7 +303,7 @@ testCreateAccessTokenWrongCodeVerifier :: Brig -> Http () testCreateAccessTokenWrongCodeVerifier brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] + let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid wrongCodeVerifier code redirectUrl createOAuthAccessToken' brig accessTokenRequest !!! do @@ -389,7 +389,7 @@ testAccessResourceInsufficientScope :: Brig -> Nginz -> Http () testAccessResourceInsufficientScope brig nginz = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Conversations Write] + let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -443,7 +443,7 @@ testRefreshTokenMaxActiveTokens opts db brig = uid <- randomId jwk <- liftIO $ readJwk (fromMaybe "path to jwk not set" opts.settings.oAuthJwkKeyPair) <&> fromMaybe (error "invalid key") let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Conversations Write, ConversationsCode Write] + let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] let delayOneSec = -- we have to wait ~1 sec before we create the next token, to make sure it is created with a different timestamp -- this is due to the interpreter of the `Now` effect which auto-updates every second @@ -645,7 +645,7 @@ testListApplicationsWithAccountAccess brig = do testWriteConversationsSuccessNginz :: Brig -> Nginz -> Http () testWriteConversationsSuccessNginz brig nginz = do (uid, tid) <- Team.createUserWithTeam brig - resp <- getAccessTokenForScope brig uid [Conversations Write] + resp <- getAccessTokenForScope brig uid [Conversations WriteOnly] createTeamConv nginz authHeader resp.accessToken tid "oauth test group" !!! do const 201 === statusCode @@ -659,7 +659,7 @@ testReadFeatureConfigsSuccessNginz brig nginz = do testWriteConversationsCodeSuccessNginz :: Brig -> Nginz -> Http () testWriteConversationsCodeSuccessNginz brig nginz = do (uid, tid) <- Team.createUserWithTeam brig - resp <- getAccessTokenForScope brig uid [Conversations Write, ConversationsCode Write] + resp <- getAccessTokenForScope brig uid [Conversations WriteOnly, ConversationsCode WriteOnly] conv <- responseJsonError @_ @(Conversation GroupConvType) =<< createTeamConv nginz authHeader resp.accessToken tid "oauth test group" Date: Mon, 21 Sep 2026 15:19:26 +0200 Subject: [PATCH 09/29] Update existing integration tests to new scope syntax. --- integration/test/Test/OAuth.hs | 6 +++--- services/brig/test/integration/API/OAuth.hs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/integration/test/Test/OAuth.hs b/integration/test/Test/OAuth.hs index 5cd899885e..dd88ce2940 100644 --- a/integration/test/Test/OAuth.hs +++ b/integration/test/Test/OAuth.hs @@ -31,7 +31,7 @@ testOAuthRevokeSession = do user <- randomUser OwnDomain def let uri = "https://example.com" cid <- createOAuthClient user "foobar" uri >>= getJSON 200 >>= flip (%.) "client_id" - let scopes = ["write:conversations"] + let scopes = ["write-only:conversations"] -- create a session that will be revoked later (tokenToBeRevoked, sessionToBeRevoked) <- do @@ -87,7 +87,7 @@ testRevokeApplicationAccountAccessV6 = do apps <- resp.json & asList length apps `shouldMatchInt` 0 let uri = "https://example.com" - let scopes = ["write:conversations"] + let scopes = ["write-only:conversations"] replicateM_ 3 $ do cid <- createOAuthClient user "foobar" uri >>= getJSON 200 >>= flip (%.) "client_id" generateAccessToken user cid scopes uri @@ -120,7 +120,7 @@ testRevokeApplicationAccountAccess = do apps <- resp.json & asList length apps `shouldMatchInt` 0 let uri = "https://example.com" - let scopes = ["write:conversations"] + let scopes = ["write-only:conversations"] replicateM_ 3 $ do cid <- createOAuthClient user "foobar" uri >>= getJSON 200 >>= flip (%.) "client_id" generateAccessToken user cid scopes uri diff --git a/services/brig/test/integration/API/OAuth.hs b/services/brig/test/integration/API/OAuth.hs index d3da7f8b0e..33d2f7fb4c 100644 --- a/services/brig/test/integration/API/OAuth.hs +++ b/services/brig/test/integration/API/OAuth.hs @@ -122,9 +122,9 @@ tests m db b n o = do ], testGroup "accessing resources (only testing happy path to ensure scopes are valid)" - [ test m "write:conversations" $ testWriteConversationsSuccessNginz b n, + [ test m "write-only:conversations" $ testWriteConversationsSuccessNginz b n, test m "read:feature_configs" $ testReadFeatureConfigsSuccessNginz b n, - test m "write:conversations_code" $ testWriteConversationsCodeSuccessNginz b n + test m "write-only:conversations_code" $ testWriteConversationsCodeSuccessNginz b n ], testGroup "refresh tokens" From 1087c04a595abd3bcfaa0a004eda16fd3581cf69 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 21 Sep 2026 15:23:38 +0200 Subject: [PATCH 10/29] Fix: requesting tokens with non-existent scope fail. Until now, it succeeded with a scope-less, and thus useless, token issued, setting the user up for a disappointment. --- libs/wire-api/src/Wire/API/OAuth.hs | 15 +++++++------ .../wire-api/test/unit/Test/Wire/API/OAuth.hs | 21 +++++++++++++++++++ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/libs/wire-api/src/Wire/API/OAuth.hs b/libs/wire-api/src/Wire/API/OAuth.hs index 865e3c4508..0c9340eb56 100644 --- a/libs/wire-api/src/Wire/API/OAuth.hs +++ b/libs/wire-api/src/Wire/API/OAuth.hs @@ -296,13 +296,16 @@ instance ToSchema OAuthScopes where . fmap (TE.decodeUtf8With lenientDecode . toByteString') . Set.toList + -- A scope we do not know is an error. Silently dropping it, or silently + -- returning no scopes at all, would hand out a token that does not do + -- what the client asked for. oauthScopeParser :: Text -> A.Parser (Set OAuthScope) - oauthScopeParser scope = - pure $ - (not . T.null) - `filter` T.splitOn " " scope - & maybe Set.empty Set.fromList - . mapM (fromByteString' . fromStrict . TE.encodeUtf8) + oauthScopeParser scope = Set.fromList <$> mapM parseScope (T.splitOn " " scope) + + parseScope :: Text -> A.Parser OAuthScope + parseScope = + maybe (fail ("invalid scope: " <> show s)) pure + . (fromByteString' . fromStrict . TE.encodeUtf8) -- | The deprecated, cumulative scopes: @write:*@ implies @read:*@, -- @admin:*@ implies @write:*@ (see @verify_scope@ in diff --git a/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs b/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs index a7775f8af1..7df3aa39da 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs @@ -20,6 +20,7 @@ module Test.Wire.API.OAuth where import Data.Aeson +import Data.Set qualified as Set import Imports import Test.Tasty import Test.Tasty.HUnit @@ -30,9 +31,29 @@ tests = testGroup "Oauth" $ [ testGroup "code challenge verification should succeed" $ [ testCase "should" testCodeChallengeVerification + ], + testGroup "scopes" $ + [ testCase "only known scopes parse" testScopesParseOnlyKnown ] ] +-- | A scope nobody can be granted has to be an error. If it were dropped, or +-- turned the whole set into no scopes at all, the client would get a token that +-- does not do what it asked for. +testScopesParseOnlyKnown :: Assertion +testScopesParseOnlyKnown = do + (eitherDecode "\"read:self write-only:conversations\"" :: Either String OAuthScopes) + @?= Right (OAuthScopes (Set.fromList [Self Read, Conversations WriteOnly])) + for_ + [ "\"read:pizza\"", -- no such scope + "\"write:conversations\"", -- deprecated tier + "\"read:self read:pizza\"" -- one bad scope spoils the request + ] + $ \bad -> case eitherDecode bad :: Either String OAuthScopes of + Left _ -> pure () + Right scopes -> + assertFailure $ "expected a parse error for " <> show bad <> ", got " <> show scopes + testCodeChallengeVerification :: Assertion testCodeChallengeVerification = do mkChallenge codeVerifier @?= codeChallenge From 5cd96fbf6d9c3b14b14eeed32be6cf57e5eed620 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 21 Sep 2026 15:39:34 +0200 Subject: [PATCH 11/29] Fixup dc1ee3ff9 --- .../unit/Test/Wire/API/Routes/OAuthScopes.hs | 66 +++++++------------ 1 file changed, 25 insertions(+), 41 deletions(-) diff --git a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs index f0fecdacd6..c7412f5644 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs @@ -63,7 +63,10 @@ tests = -- -- This test matches the openapi docs generated from servant routes -- against what nginz enforces. The behavior of nginz is emulated by --- this test. This is not testing any properties of libzauth. +-- this test. Actual behavior of libzauth is tested in the rust code; +-- those tests and these here need to be kept in sync manually +-- (compare `enforcedScopes` below with +-- `/libs/libzauth/libzauth/src/oauth.rs` (search for `mod tests`)). testScopesAgree :: Assertion testScopesAgree = do unless (Set.null actual) . assertFailure . T.unpack . T.unlines $ @@ -131,20 +134,20 @@ data Location = Location -- If the matching location has @oauth_scopes@, the answer is those of the -- listed scopes that have the tier this verb needs. -- --- If it only has the deprecated @oauth_scope@, we first ask which old scopes --- nginz lets through for this verb. Old scopes are cumulative, so a @GET@ gets --- through with @read:@, @write:@, or @admin:@, but only some of those exist as --- scopes a client can be granted. Then we translate the ones that do into new --- scopes. Example: for a @GET@ under @oauth_scope: conversations_code@, the --- only old scope that exists and passes is @write:conversations_code@, and that --- one is @read:conversations_code@ plus @write-only:conversations_code@ in new --- scopes: if wire-api requires new read *or* new write, old write is acceptable. +-- TODO: the following paragraph is less than clear, rephrase! -- --- NB: We do not need to distinguish between "nginz asks for no scope --- at all" and "asks for one that nobody can have": a @DELETE@ under a --- deprecated @oauth_scope@ needs @admin:\@, and for most bases --- that scope does not exist, in which case no OAuth token can be --- accepted. +-- If it only has the deprecated @oauth_scope@, the answer is the one scope made +-- of that base and the tier this verb needs: under @oauth_scope: +-- conversations_code@, a @GET@ wants @read:conversations_code@ and nothing +-- else. Old scopes are cumulative, so a token carrying +-- @write:conversations_code@ passes that @GET@ as well, but nginz reads that off +-- the token rather than off the configuration (@granted_scopes@ in +-- @libs/libzauth/libzauth/src/oauth.rs@), and it does not change which scope the +-- docs should name. +-- +-- NB: an empty answer means no OAuth token gets in at all. That happens if +-- the location has no @oauth_scope[s]@, if its @oauth_scopes@ list has nothing +-- of the tier the verb needs, or if the verb is one nginz has no rule for. enforcedScopes :: Text -> Text -> Set OAuthScope enforcedScopes method path = case find locationMatches nginzLocations of Nothing -> Set.empty @@ -153,7 +156,8 @@ enforcedScopes method path = case find locationMatches nginzLocations of -- Filter scopes listed in values.yaml by matching method/tier. Set.fromList (filter ((newTier method ==) . Just . oAuthScopeTier) newScopes) (Just base, Nothing) -> - Set.fromList (oldOAuthScopeToNewScopes =<< oldScopeBaseAccepted base) + -- The deprecated attribute gives the base; the tier comes from the method. + maybe Set.empty Set.singleton (oldScopeBase base) (Nothing, Nothing) -> Set.empty where -- Does this location capture that path? nginx anchors regex locations at the @@ -186,33 +190,13 @@ enforcedScopes method path = case find locationMatches nginzLocations of "DELETE" -> Just DeleteOnly _ -> Nothing - -- The deprecated @oauth_scope@ names only the base; which tiers - -- of it are accepted follows from the verb in the request, - -- cumulatively. Of those @:@ combinations only the - -- ones that parse exist as grantable scopes. - oldScopeBaseAccepted :: Text -> [OldOAuthScope] - oldScopeBaseAccepted base = - mapMaybe (\tier -> fromByteString (T.encodeUtf8 (tier <> ":" <> base))) (oldTiers method) - -- Mirrors @verify_scope@ in @libs/libzauth/libzauth/src/oauth.rs@, which is - -- what nginz calls: `write:*` implies `read:*`, `admin:*` implies `write:*`. - oldTiers :: Text -> [Text] - oldTiers = \case - "GET" -> ["read", "write", "admin"] - "POST" -> ["write", "admin"] - "PUT" -> ["write", "admin"] - "DELETE" -> ["admin"] - _ -> [] - -oldOAuthScopeToNewScopes :: OldOAuthScope -> [OAuthScope] -oldOAuthScopeToNewScopes = \case - ReadFeatureConfigs -> [FeatureConfigs Read] - ReadSelf -> [Self Read] - WriteConversations -> [Conversations Read, Conversations WriteOnly] - WriteConversationsCode -> [ConversationsCode Read, ConversationsCode WriteOnly] - WriteConversationsName -> [ConversationsName Read, ConversationsName WriteOnly] - WriteMeetings -> [Meetings Read, Meetings WriteOnly] - AdminMeetings -> [Meetings Read, Meetings WriteOnly, Meetings DeleteOnly] + -- what nginz calls for a location with the deprecated attribute. 'Nothing' + -- for a base that is no scope of ours, e.g. a typo in values.yaml. + oldScopeBase :: Text -> Maybe OAuthScope + oldScopeBase base = do + tier <- newTier method + fromByteString (toByteString' tier <> ":" <> T.encodeUtf8 base) nginzLocations :: [Location] nginzLocations = From f38d3c83bc3d9218f35d4e8111a981f5bf1cc687 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 21 Sep 2026 15:42:54 +0200 Subject: [PATCH 12/29] Fix actual access control in libzauth, update charts. --- charts/nginz/templates/conf/_nginx.conf.tpl | 9 +- libs/libzauth/libzauth-c/src/lib.rs | 56 ++++- libs/libzauth/libzauth-c/src/zauth.h | 3 + libs/libzauth/libzauth/src/lib.rs | 2 +- libs/libzauth/libzauth/src/oauth.rs | 197 ++++++++++++++++-- .../nginx-zauth-module/zauth_module.c | 69 +++++- 6 files changed, 308 insertions(+), 28 deletions(-) diff --git a/charts/nginz/templates/conf/_nginx.conf.tpl b/charts/nginz/templates/conf/_nginx.conf.tpl index 671acd3886..db4d5a1204 100644 --- a/charts/nginz/templates/conf/_nginx.conf.tpl +++ b/charts/nginz/templates/conf/_nginx.conf.tpl @@ -297,7 +297,14 @@ http { {{- end }} {{- end }} - {{- if ($location.oauth_scope) }} + {{- if hasKey $location "oauth_scopes" }} + {{- if $location.oauth_scopes }} + oauth_scopes {{ join " " $location.oauth_scopes }}; + {{- else }} + # 'oauth_scopes: []': no OAuth token gets in here. (Without any + # 'oauth_scope[s]' directive libzauth rejects them all.) + {{- end }} + {{- else if ($location.oauth_scope) }} oauth_scope {{ $location.oauth_scope }}; {{- end }} diff --git a/libs/libzauth/libzauth-c/src/lib.rs b/libs/libzauth/libzauth-c/src/lib.rs index 91eb28062f..097df12459 100644 --- a/libs/libzauth/libzauth-c/src/lib.rs +++ b/libs/libzauth/libzauth-c/src/lib.rs @@ -30,7 +30,8 @@ use std::slice; use std::str; use zauth::acl; use zauth::{ - verify_oauth_token, Acl, Error, Keystore, OauthError, Token, TokenType, TokenVerification, + verify_oauth_token, verify_oauth_token_scopes, Acl, Error, Keystore, OauthError, Token, + TokenType, TokenVerification, }; /// Variant of std::try! that returns the unwrapped error. @@ -440,6 +441,8 @@ pub extern "C" fn oauth_key_delete(a: *mut OAuthPubJwk) { ); } +/// Verify against the deprecated `oauth_scope` directive, which holds the base +/// of a scope and leaves the tier to the request method. #[no_mangle] pub extern "C" fn oauth_verify_token( jwk: &OAuthPubJwk, @@ -449,6 +452,55 @@ pub extern "C" fn oauth_verify_token( scope_len: size_t, method: *const u8, method_len: size_t, +) -> OAuthResult { + oauth_verify( + jwk, + token, + token_len, + scope, + scope_len, + method, + method_len, + verify_oauth_token, + ) +} + +/// Verify against the `oauth_scopes` directive, which holds whole scopes, +/// separated by spaces. +#[no_mangle] +pub extern "C" fn oauth_verify_token_scopes( + jwk: &OAuthPubJwk, + token: *const u8, + token_len: size_t, + scopes: *const u8, + scopes_len: size_t, + method: *const u8, + method_len: size_t, +) -> OAuthResult { + oauth_verify( + jwk, + token, + token_len, + scopes, + scopes_len, + method, + method_len, + verify_oauth_token_scopes, + ) +} + +/// A NULL `scope` is how a location with nothing configured arrives here, and +/// there is nothing we could let the token do: no scope, no access. +#[allow(clippy::too_many_arguments)] +fn oauth_verify( + jwk: &OAuthPubJwk, + token: *const u8, + token_len: size_t, + scope: *const u8, + scope_len: size_t, + method: *const u8, + method_len: size_t, + verify: fn(&str, &str, &str, &str) -> Result, ) -> OAuthResult { match panic::catch_unwind(|| { if token.is_null() { @@ -475,7 +527,7 @@ pub extern "C" fn oauth_verify_token( let scope = try_unwrap!(str::from_utf8(bytes)); let bytes = unsafe { slice::from_raw_parts(method, method_len) }; let method = str::from_utf8(bytes).unwrap(); - let subject = try_unwrap!(verify_oauth_token(&jwk.0, token, scope, method)); + let subject = try_unwrap!(verify(&jwk.0, token, scope, method)); let c_str = try_unwrap!(CString::new(subject)); OAuthResult { uid: c_str.into_raw(), diff --git a/libs/libzauth/libzauth-c/src/zauth.h b/libs/libzauth/libzauth-c/src/zauth.h index 33878c8820..3fc3d70775 100644 --- a/libs/libzauth/libzauth-c/src/zauth.h +++ b/libs/libzauth/libzauth-c/src/zauth.h @@ -83,7 +83,10 @@ uint8_t zauth_token_version(ZauthToken const *); Range zauth_token_lookup(ZauthToken const *, uint8_t); ZauthResult zauth_token_allowed(ZauthToken const *, ZauthAcl const *, uint8_t const * path, size_t len, uint8_t * result); void zauth_token_delete(ZauthToken *); +// 's' is the base of a scope, e.g. "conversations_code" (deprecated). OAuthResult oauth_verify_token(OAuthPubJwk const *, uint8_t const * t, size_t t_len, uint8_t const * s, size_t s_len, uint8_t const * m, size_t m_len); +// 's' is a space separated list of whole scopes, e.g. "read:conversations_code write-only:conversations_code". +OAuthResult oauth_verify_token_scopes(OAuthPubJwk const *, uint8_t const * t, size_t t_len, uint8_t const * s, size_t s_len, uint8_t const * m, size_t m_len); OAuthResultStatus oauth_result_uid_delete(char *); #ifdef __cplusplus diff --git a/libs/libzauth/libzauth/src/lib.rs b/libs/libzauth/libzauth/src/lib.rs index 195d06138b..a89dd0274f 100644 --- a/libs/libzauth/libzauth/src/lib.rs +++ b/libs/libzauth/libzauth/src/lib.rs @@ -36,4 +36,4 @@ mod matcher; pub use acl::Acl; pub use error::Error; pub use zauth::{Keystore, Token, TokenType, TokenVerification}; -pub use oauth::{verify_oauth_token, OauthError}; +pub use oauth::{verify_oauth_token, verify_oauth_token_scopes, OauthError}; diff --git a/libs/libzauth/libzauth/src/oauth.rs b/libs/libzauth/libzauth/src/oauth.rs index 058e0b7cda..a01b38b3c4 100644 --- a/libs/libzauth/libzauth/src/oauth.rs +++ b/libs/libzauth/libzauth/src/oauth.rs @@ -23,12 +23,36 @@ pub struct OAuthToken { pub scope: String, } +/// Verify a token against the deprecated `oauth_scope` directive, which names +/// only the base of a scope (`conversations_code`) and leaves the tier to the +/// request method. pub fn verify_oauth_token( jwk: &str, token: &str, required_scope: &str, method: &str, ) -> Result { + let (subject, scopes) = verify_token(jwk, token)?; + verify_scope(&scopes, required_scope, method)?; + Ok(subject) +} + +/// Verify a token against the `oauth_scopes` directive, which lists whole +/// scopes (`read:conversations_code write-only:conversations_code`), tier +/// included. +pub fn verify_oauth_token_scopes( + jwk: &str, + token: &str, + required_scopes: &str, + method: &str, +) -> Result { + let (subject, scopes) = verify_token(jwk, token)?; + verify_scopes(&scopes, required_scopes, method)?; + Ok(subject) +} + +/// Check the signature and return `(subject, scope claim)`. +fn verify_token(jwk: &str, token: &str) -> Result<(String, String), OauthError> { let jwk = serde_json::from_str::(jwk)?; let key = try_from_jwk(&jwk)?; let options = VerificationOptions { @@ -37,35 +61,66 @@ pub fn verify_oauth_token( }; let claims = key.verify_token::(token, Some(options))?; let subject = claims.subject.ok_or(OauthError::InvalidJwtNoSubject)?; - verify_scope(&claims.custom.scope, required_scope, method)?; - Ok(subject) + Ok((subject, claims.custom.scope)) +} + +/// Compatibility: `authorized_scopes` must be new syntax, +/// `required_scopes` can be old or new. +fn verify_scopes( + authorized_scopes: &str, + required_scopes: &str, + method: &str, +) -> Result<(), OauthError> { + let tier = required_tier(method)?; + let required = required_scopes + .split_whitespace() + .filter(|s| scope_tier(s) == Some(tier)) + .map(|s| s.to_string()) + .collect::>(); + verify_any(authorized_scopes, &required) +} + +fn scope_tier(scope: &str) -> Option<&str> { + scope.split_once(':').map(|(tier, _)| tier) } -// if method is GET, authorized scopes must contain either read:_, write:_, or admin:_ -// if method is POST, authorized scopes must contain either write:_ or admin:_ -// if method is PUT, authorized scopes must contain either write:_ or admin:_ -// if method is DELETE, authorized scopes must contain admin:_ -// FUTUREWORK: this works for now, but maybe we should consider using a more flexible scope system in the future -// e.g. by using a configuration file with a mapping of scopes to methods and paths +/// This is deprecated, use tiers "read", "write-only", "delete-only" +/// instead. See `verify_scopes` below. +/// +/// Deprecated behavior: +/// +/// if method is GET, authorized scopes must contain either read:_, write:_, or admin:_ +/// if method is POST, authorized scopes must contain either write:_ or admin:_ +/// if method is PUT, authorized scopes must contain either write:_ or admin:_ +/// if method is DELETE, authorized scopes must contain admin:_ +/// +/// Compatibility: `authorized_scopes` may be old or new syntax; +/// `required_scopes` must be a bare base name (old syntax), the tier +/// is added implicitly. fn verify_scope( authorized_scopes: &str, required_scope: &str, method: &str, ) -> Result<(), OauthError> { - let valid_scopes = match method.to_uppercase().as_str() { - "GET" => Ok(vec!["read", "write", "admin"]), - "POST" => Ok(vec!["write", "admin"]), - "PUT" => Ok(vec!["write", "admin"]), - "DELETE" => Ok(vec!["admin"]), + let required = vec![format!("{}:{}", required_tier(method)?, required_scope)]; + verify_any(authorized_scopes, &required) +} + +fn required_tier(method: &str) -> Result<&'static str, OauthError> { + match method.to_uppercase().as_str() { + "GET" => Ok("read"), + "POST" => Ok("write-only"), + "PUT" => Ok("write-only"), + "DELETE" => Ok("delete-only"), _ => Err(OauthError::InvalidScope), - }? - .iter() - .map(|s| format!("{}:{}", s, required_scope)) - .collect::>(); + } +} +fn verify_any(authorized_scopes: &str, required: &[String]) -> Result<(), OauthError> { let valid = authorized_scopes .split_whitespace() - .any(|s| valid_scopes.contains(&s.to_string())); + .flat_map(granted_scopes) + .any(|granted| required.contains(&granted)); if !valid { return Err(OauthError::InvalidScope); @@ -73,6 +128,23 @@ fn verify_scope( Ok(()) } +/// Which scopes does a scope carried by a token (old or new syntax) +/// grant, in the vocabulary [`required_tier`] speaks? +fn granted_scopes(scope: &str) -> Vec { + let Some((tier, base)) = scope.split_once(':') else { + return Vec::new(); + }; + let tiers: &[&str] = match tier { + "read" => &["read"], + "write" => &["read", "write-only"], + "admin" => &["read", "write-only", "delete-only"], + "write-only" => &["write-only"], + "delete-only" => &["delete-only"], + _ => &[], + }; + tiers.iter().map(|t| format!("{}:{}", t, base)).collect() +} + fn try_from_jwk(jwk: &Jwk) -> Result { Ok(match &jwk.algorithm { AlgorithmParameters::OctetKeyPair(p) => { @@ -141,6 +213,73 @@ mod tests { assert!(verify_scope("foo bar", "self", "DELETE").is_err()); } + #[test] + fn should_grant_scopes() { + assert_eq!(granted_scopes("read:self"), vec!["read:self"]); + assert_eq!( + granted_scopes("write:self"), + vec!["read:self", "write-only:self"] + ); + assert_eq!( + granted_scopes("admin:self"), + vec!["read:self", "write-only:self", "delete-only:self"] + ); + assert_eq!(granted_scopes("write-only:self"), vec!["write-only:self"]); + assert_eq!(granted_scopes("delete-only:self"), vec!["delete-only:self"]); + assert!(granted_scopes("self").is_empty()); + assert!(granted_scopes("nonsense:self").is_empty()); + } + + // The deprecated directive with scopes as they are handed out now. + #[test] + fn should_verify_scope_with_new_scopes() { + assert!(verify_scope("read:self foo bar", "self", "GET").is_ok()); + assert!(verify_scope("write-only:self", "self", "GET").is_err()); // this is the interesting case. + assert!(verify_scope("write-only:self", "self", "POST").is_ok()); + assert!(verify_scope("write-only:self", "self", "PUT").is_ok()); + assert!(verify_scope("write-only:self", "self", "DELETE").is_err()); + assert!(verify_scope("delete-only:self", "self", "DELETE").is_ok()); + assert!(verify_scope("delete-only:self", "self", "POST").is_err()); + assert!(verify_scope("read:other write-only:other", "self", "GET").is_err()); + } + + #[test] + fn should_verify_scopes() { + let cfg = "read:self write-only:self"; + assert!(verify_scopes("read:self", cfg, "GET").is_ok()); + assert!(verify_scopes("write-only:self", cfg, "POST").is_ok()); + assert!(verify_scopes("write-only:self", cfg, "PUT").is_ok()); + assert!(verify_scopes("read:self", cfg, "POST").is_err()); + assert!(verify_scopes("write-only:self", cfg, "GET").is_err()); + assert!(verify_scopes("read:self write-only:self", cfg, "DELETE").is_err()); + assert!(verify_scopes("read:other", cfg, "GET").is_err()); + assert!(verify_scopes("foo bar", cfg, "GET").is_err()); + } + + // A list without a scope of the tier the method needs lets nobody in, and + // neither does a location with no scope configured at all. + #[test] + fn should_verify_scopes_closed() { + assert!(verify_scopes("read:self write-only:self admin:self", "", "GET").is_err()); + assert!(verify_scopes("delete-only:self", "read:self write-only:self", "DELETE").is_err()); + assert!(verify_scopes("read:self", "read:self", "PATCH").is_err()); + } + + // Tokens handed out before the migration carry cumulative scopes and have + // to keep working against a migrated location. + #[test] + fn should_verify_scopes_with_old_scopes() { + let cfg = "read:conversations_code write-only:conversations_code"; + assert!(verify_scopes("write:conversations_code", cfg, "GET").is_ok()); + assert!(verify_scopes("write:conversations_code", cfg, "POST").is_ok()); + assert!(verify_scopes("read:conversations_code", cfg, "GET").is_ok()); + assert!(verify_scopes("read:conversations_code", cfg, "POST").is_err()); + assert!(verify_scopes("admin:conversations_code", cfg, "GET").is_ok()); + assert!(verify_scopes("admin:conversations_code", cfg, "POST").is_ok()); + // ... but only as far as the location goes: nothing here is deletable. + assert!(verify_scopes("admin:conversations_code", cfg, "DELETE").is_err()); + } + #[test] fn should_verify_oauth_token() { let uid = "842ddbc8-56ec-408d-9fa8-7a8c37ad22a7"; @@ -158,6 +297,28 @@ mod tests { verify_oauth_token(&serde_json::to_string(&jwk).unwrap(), &jwt, "test", "GET").unwrap(); assert_eq!(&subject, uid); } + + #[test] + fn should_verify_oauth_token_scopes() { + let uid = "842ddbc8-56ec-408d-9fa8-7a8c37ad22a7"; + let key = Ed25519KeyPair::generate(); + let jwk = mk_jwk(key.public_key()); + let token = Claims::with_custom_claims( + OAuthToken { + scope: "write-only:foo read:test".to_string(), + }, + Duration::from_secs(3600), + ) + .with_subject(uid); + let jwt = key.sign::(token).unwrap(); + let jwk = serde_json::to_string(&jwk).unwrap(); + let subject = + verify_oauth_token_scopes(&jwk, &jwt, "read:test write-only:test", "GET").unwrap(); + assert_eq!(&subject, uid); + assert!(verify_oauth_token_scopes(&jwk, &jwt, "read:test write-only:test", "POST").is_err()); + assert!(verify_oauth_token_scopes(&jwk, &jwt, "write-only:foo", "POST").is_ok()); + } + fn mk_jwk(key: Ed25519PublicKey) -> Jwk { let x = base64::prelude::BASE64_URL_SAFE_NO_PAD.encode(&key.to_bytes()); Jwk { diff --git a/services/nginz/third_party/nginx-zauth-module/zauth_module.c b/services/nginz/third_party/nginx-zauth-module/zauth_module.c index 27aeb02246..98ac8ddb2c 100644 --- a/services/nginz/third_party/nginx-zauth-module/zauth_module.c +++ b/services/nginz/third_party/nginx-zauth-module/zauth_module.c @@ -13,8 +13,9 @@ typedef struct { } ZauthServerConf; typedef struct { - ngx_flag_t zauth; // 1=on, 0=off - ngx_str_t oauth_scope; + ngx_flag_t zauth; // 1=on, 0=off + ngx_str_t oauth_scope; // scope base, tier implied by the method (deprecated) + ngx_str_t oauth_scopes; // whole scopes, separated by spaces; supersedes oauth_scope } ZauthLocationConf; enum { @@ -40,6 +41,7 @@ static char * merge_srv_conf (ngx_conf_t *, void *, void *); static char * load_keystore (ngx_conf_t *, ngx_command_t *, void *); static char * load_acl (ngx_conf_t *, ngx_command_t *, void *); static char * load_oauth_key (ngx_conf_t *, ngx_command_t *, void *); +static char * set_oauth_scopes(ngx_conf_t *, ngx_command_t *, void *); static void delete_srv_conf (void *); // Module setup @@ -69,7 +71,7 @@ static void zauth_empty_val (ngx_http_variable_value_t *); // Utility functions static ngx_int_t zauth_handle_request (ngx_http_request_t *, const ZauthServerConf *, ZauthToken const *); -static ngx_int_t oauth_handle_request(ngx_http_request_t *, OAuthPubJwk const *, ngx_str_t const); +static ngx_int_t oauth_handle_request(ngx_http_request_t *, OAuthPubJwk const *, ZauthLocationConf const *); static ngx_http_module_t zauth_module_ctx = { zauth_variables // pre-configuration @@ -99,6 +101,14 @@ static ngx_command_t zauth_commands [] = { , NULL } + , { ngx_string ("oauth_scopes") + , NGX_HTTP_LOC_CONF | NGX_CONF_1MORE + , set_oauth_scopes + , NGX_HTTP_LOC_CONF_OFFSET + , offsetof (ZauthLocationConf, oauth_scopes) + , NULL + } + , { ngx_string ("zauth_keystore") , NGX_HTTP_SRV_CONF | NGX_CONF_TAKE1 , load_keystore @@ -231,6 +241,46 @@ static char * merge_loc_conf (ngx_conf_t * _, void * pc, void * cc) { ZauthLocationConf * child = cc; ngx_conf_merge_off_value(child->zauth, parent->zauth, 1); ngx_conf_merge_str_value(child->oauth_scope, parent->oauth_scope, NULL); + if (child->oauth_scopes.data == NULL) { + child->oauth_scopes = parent->oauth_scopes; + } + return NGX_CONF_OK; +} + +// Join the arguments into one space separated string, which is how the scope +// claim of an OAuth token spells a list as well. +static char * set_oauth_scopes (ngx_conf_t * conf, ngx_command_t * cmd, void * data) { + ZauthLocationConf * lc = data; + + if (lc->oauth_scopes.data != NULL) { + return "is duplicate"; + } + + ngx_str_t * const args = conf->args->elts; + size_t len = conf->args->nelts - 2; // separators + + for (ngx_uint_t i = 1; i < conf->args->nelts; ++i) { + len += args[i].len; + } + + u_char * const buf = ngx_pnalloc(conf->pool, len); + + if (buf == NULL) { + return NGX_CONF_ERROR; + } + + u_char * p = buf; + + for (ngx_uint_t i = 1; i < conf->args->nelts; ++i) { + if (i > 1) { + *p++ = ' '; + } + p = ngx_cpymem(p, args[i].data, args[i].len); + } + + lc->oauth_scopes.data = buf; + lc->oauth_scopes.len = len; + return NGX_CONF_OK; } @@ -346,7 +396,7 @@ static ngx_int_t zauth_and_oauth_handle_request (ngx_http_request_t * r) { if (ctx != NULL && ctx->tag == CONTEXT_ZAUTH) { return zauth_handle_request(r, sc, ctx->token); } else if (ctx == NULL) { - return oauth_handle_request(r, sc->oauth_pub_key, lc->oauth_scope); + return oauth_handle_request(r, sc->oauth_pub_key, lc); } else { return NGX_HTTP_UNAUTHORIZED; } @@ -375,7 +425,7 @@ static ngx_int_t zauth_handle_request (ngx_http_request_t * r, const ZauthServer return NGX_OK; } -ngx_int_t oauth_handle_request(ngx_http_request_t *r, OAuthPubJwk const * key, ngx_str_t const scope) { +ngx_int_t oauth_handle_request(ngx_http_request_t *r, OAuthPubJwk const * key, ZauthLocationConf const * lc) { if (r->headers_in.authorization == NULL) { return NGX_HTTP_UNAUTHORIZED; } @@ -383,7 +433,14 @@ ngx_int_t oauth_handle_request(ngx_http_request_t *r, OAuthPubJwk const * key, n ngx_str_t hdr = r->headers_in.authorization->value; if (strncmp((char const *) hdr.data, "Bearer ", 7) == 0) { - OAuthResult res = oauth_verify_token(key, &hdr.data[7], hdr.len - 7, scope.data, scope.len, r->method_name.data, r->method_name.len); + // 'oauth_scopes' supersedes 'oauth_scope' where both are given. + // Where neither is, libzauth is handed a NULL scope and lets + // nobody in: no scope configured, no access through OAuth. + ngx_str_t const scopes = lc->oauth_scopes; + ngx_str_t const scope = lc->oauth_scope; + OAuthResult res = scopes.data != NULL + ? oauth_verify_token_scopes(key, &hdr.data[7], hdr.len - 7, scopes.data, scopes.len, r->method_name.data, r->method_name.len) + : oauth_verify_token(key, &hdr.data[7], hdr.len - 7, scope.data, scope.len, r->method_name.data, r->method_name.len); if (res.status == OAUTH_OK) { ZauthContext * ctx = alloc_oauth_context(r, res.uid); if (ctx == NULL) return NGX_HTTP_INTERNAL_SERVER_ERROR; // for OOM-safety From c6de490e8f23eaca50d2e2f379fc862030f33a66 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 21 Sep 2026 15:47:41 +0200 Subject: [PATCH 13/29] Update docs (local part, wire-docs repo bump coming up). --- .../src/developer/reference/config-options.md | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/docs/src/developer/reference/config-options.md b/docs/src/developer/reference/config-options.md index 9154c97ea7..2a08c9711c 100644 --- a/docs/src/developer/reference/config-options.md +++ b/docs/src/developer/reference/config-options.md @@ -1102,6 +1102,63 @@ optSettings: setOAuthMaxActiveRefreshTokens: 10 ``` +#### Scopes + +Which scope an OAuth token needs is configured per route in `nginz`'s Helm +chart, and enforced by *nginz*: + +```yaml +# [nginz/values.yaml] +nginx_conf: + upstreams: + galley: + - path: /conversations/([^/]*)/code + envs: + - all + oauth_scopes: ["read:conversations_code", "write-only:conversations_code"] +``` + +A scope has a tier and a base, e.g. the tier `read` and the base +`conversations_code`. The method of the request decides which tier it needs: + +| method | tier | +| --------------- | ------------- | +| `GET` | `read` | +| `POST`, `PUT` | `write-only` | +| `DELETE` | `delete-only` | + +Only the listed scopes of that tier let a token in, and the tiers are +independent of each other: `write-only:` does not include `read:`. So in the +example above, a `GET` needs `read:conversations_code`, and a `DELETE` gets +nowhere, because the list has no `delete-only:conversations_code`. + +A route with no `oauth_scopes` (or with an empty list) (and with no +`oauth_scope`, see next section) accepts no OAuth token at all. It +may still be reachable with a cookie or a zauth token; scopes are +about OAuth only. + +Every scope should also be named in the swagger docs, which is a separate +annotation in the routing tables. A unit test in `wire-api` compares the two +and fails if they disagree. + +##### Deprecated: `oauth_scope` + +Older configurations name only the base and leave the tier to *nginz*: + +```yaml +# [nginz/values.yaml] + oauth_scope: conversations_code +``` + +Here the tiers build on one another: `write:` includes `read:`, and `admin:` +includes `write:`. A `GET` therefore passes with `read:`, `write:`, or +`admin:conversations_code`, and a `DELETE` needs `admin:conversations_code`. + +`oauth_scopes` replaces this, and wins over it where a route has both. Tokens +work across the change in either direction: a token with old scopes gets into a +route that has moved to `oauth_scopes`, and a token with new scopes gets into a +route that has not. + #### Password hashing options Since release 5.6.0, wire-server can hash passwords with From db19b9666a53ed3ab09cff11232b108826815ddc Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 21 Sep 2026 15:48:13 +0200 Subject: [PATCH 14/29] Mess with values.yaml (not sure how and why?) --- charts/nginz/values.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/charts/nginz/values.yaml b/charts/nginz/values.yaml index 7c30e27a3d..65a5c74756 100644 --- a/charts/nginz/values.yaml +++ b/charts/nginz/values.yaml @@ -229,7 +229,7 @@ nginx_conf: - staging - path: /self$ # Matches exactly /self oauth_scope: self # deprecated, will be ignored if 'oauth_scopes' is present. - oauth_scopes: ["read:self", "write-only:self", "delete-only:self"] + oauth_scopes: ["read:self"] envs: - all - path: /self/name @@ -668,7 +668,7 @@ nginx_conf: envs: - all oauth_scope: conversations_code # deprecated, will be ignored if 'oauth_scopes' is present. - oauth_scopes: ["read:conversations_code", "write-only:conversations_code", "delete-only:conversations_code"] + # oauth_scopes: ["read:conversations_code", "write-only:conversations_code"] - path: /conversations/join envs: - all From 06735e0562c3b15be9c6081232193096aee4746e Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 21 Sep 2026 16:04:50 +0200 Subject: [PATCH 15/29] Revert ealier refactoring of OAuthScope back into simple sum type. This morally reverts 2bca04ebc751dd3a5f4bcd260c4886bd193f138d "Refactor: split up `data OAuthScope` into base and tier." --- libs/types-common/src/Data/GenericEnum.hs | 102 ------------ libs/types-common/types-common.cabal | 1 - libs/wire-api/src/Wire/API/OAuth.hs | 147 +++++++----------- .../src/Wire/API/Routes/Public/Brig.hs | 2 +- .../API/Routes/Public/Galley/Conversation.hs | 14 +- .../Wire/API/Routes/Public/Galley/Feature.hs | 2 +- .../Wire/API/Routes/Public/Galley/Meetings.hs | 2 +- .../wire-api/test/unit/Test/Wire/API/OAuth.hs | 2 +- .../unit/Test/Wire/API/Routes/OAuthScopes.hs | 2 +- services/brig/test/integration/API/OAuth.hs | 52 +++---- 10 files changed, 94 insertions(+), 232 deletions(-) delete mode 100644 libs/types-common/src/Data/GenericEnum.hs diff --git a/libs/types-common/src/Data/GenericEnum.hs b/libs/types-common/src/Data/GenericEnum.hs deleted file mode 100644 index 428d43f907..0000000000 --- a/libs/types-common/src/Data/GenericEnum.hs +++ /dev/null @@ -1,102 +0,0 @@ --- This file is part of the Wire Server implementation. --- --- Copyright (C) 2026 Wire Swiss GmbH --- --- This program is free software: you can redistribute it and/or modify it under --- the terms of the GNU Affero General Public License as published by the Free --- Software Foundation, either version 3 of the License, or (at your option) any --- later version. --- --- This program is distributed in the hope that it will be useful, but WITHOUT --- ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS --- FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more --- details. --- --- You should have received a copy of the GNU Affero General Public License along --- with this program. If not, see . - -module Data.GenericEnum - ( GenericEnum (..), - GEnum (..), - ) -where - -import Data.Kind (Type) -import GHC.Generics (Generic (..), K1 (..), M1 (..), U1 (..), V1, type (:*:) (..), type (:+:) (..)) -import Imports - --------------------------------------------------------------------------------- --- Generic Bounded / Enum - --- | 'Bounded' and 'Enum' read off a type's 'Generic' representation, for use --- with @DerivingVia@: --- --- > data OAuthScope = FeatureConfigs OAuthTier | {- ... -} | Meetings OAuthTier --- > deriving (Bounded, Enum) via (GenericEnum OAuthScope) --- --- @deriving Enum@ only covers types whose constructors are all --- nullary, which @OAuthScope@ is not. `GenericEnum` covers products --- of 'Bounded' + 'Enum' fields as well, numbering sums in constructor --- order and products lexicographically. This is the same order --- @deriving Ord@ uses, so @[minBound .. maxBound]@ comes out sorted. -newtype GenericEnum a = GenericEnum a - -class GEnum (f :: Type -> Type) where - -- | How many values @f@ has. - gCard :: Int - - gToEnum :: Int -> f a - gFromEnum :: f a -> Int - -instance GEnum V1 where - gCard = 0 - gToEnum i = error $ "GenericEnum: uninhabited type, toEnum " <> show i - gFromEnum v = case v of {} - -instance GEnum U1 where - gCard = 1 - gToEnum _ = U1 - gFromEnum _ = 0 - -instance (GEnum f) => GEnum (M1 i c f) where - gCard = gCard @f - gToEnum = M1 . gToEnum - gFromEnum = gFromEnum . unM1 - -instance (Bounded a, Enum a) => GEnum (K1 i a) where - gCard = fromEnum (maxBound @a) - fromEnum (minBound @a) + 1 - gToEnum i = K1 (toEnum (i + fromEnum (minBound @a))) - gFromEnum (K1 x) = fromEnum x - fromEnum (minBound @a) - -instance (GEnum f, GEnum g) => GEnum (f :+: g) where - gCard = gCard @f + gCard @g - gToEnum i - | i < gCard @f = L1 (gToEnum i) - | otherwise = R1 (gToEnum (i - gCard @f)) - gFromEnum = \case - L1 x -> gFromEnum x - R1 y -> gCard @f + gFromEnum y - -instance (GEnum f, GEnum g) => GEnum (f :*: g) where - gCard = gCard @f * gCard @g - gToEnum i = case i `divMod` gCard @g of - (q, r) -> gToEnum q :*: gToEnum r - gFromEnum (x :*: y) = gFromEnum x * gCard @g + gFromEnum y - -instance (Generic a, GEnum (Rep a)) => Bounded (GenericEnum a) where - minBound = GenericEnum . to $ gToEnum 0 - maxBound = GenericEnum . to $ gToEnum (gCard @(Rep a) - 1) - -instance (Generic a, GEnum (Rep a)) => Enum (GenericEnum a) where - fromEnum (GenericEnum x) = gFromEnum (from x) - - toEnum i - | 0 <= i && i < gCard @(Rep a) = GenericEnum . to $ gToEnum i - | otherwise = error $ "GenericEnum: toEnum out of range: " <> show i - - -- the class defaults for these two run off past 'maxBound' - enumFrom x = enumFromTo x maxBound - - enumFromThen x y = - enumFromThenTo x y $ - if fromEnum y >= fromEnum x then maxBound else minBound diff --git a/libs/types-common/types-common.cabal b/libs/types-common/types-common.cabal index 9396f1d2c2..7d1590c057 100644 --- a/libs/types-common/types-common.cabal +++ b/libs/types-common/types-common.cabal @@ -19,7 +19,6 @@ library Data.Credentials Data.Domain Data.ETag - Data.GenericEnum Data.Handle Data.HavePendingInvitations Data.Id diff --git a/libs/wire-api/src/Wire/API/OAuth.hs b/libs/wire-api/src/Wire/API/OAuth.hs index 0c9340eb56..5ccc65b667 100644 --- a/libs/wire-api/src/Wire/API/OAuth.hs +++ b/libs/wire-api/src/Wire/API/OAuth.hs @@ -26,7 +26,6 @@ import Data.Aeson.Types qualified as A import Data.ByteArray (convert) import Data.ByteString.Conversion import Data.ByteString.Lazy (fromStrict, toStrict) -import Data.GenericEnum import Data.HashMap.Strict qualified as HM import Data.Id as Id import Data.Json.Util @@ -197,68 +196,70 @@ instance ToSchema OAuthResponseType where -- However, having this typed makes it easier to handle scopes in the backend, -- and e.g. provide more meaningful error messages when the scope is invalid. -- --- TODO: refactor this again: newtype `OAuthScopeReally = OAuthScopeReally { fromOAuthScopeReally :: (OAuthTier, OAuthScope) }` +-- Furthermore, since tokens are only issued if the scopes listed in +-- the request can be parsed, so this type also guarantees that no +-- unusable tokens can be issued. +-- +-- A scope is a tier and a base, but not every combination of the two is a +-- scope: only the ones listed here exist, and they are the ones the routing +-- tables document and @charts/nginz/values.yaml@ enforces. Anything else is +-- rejected by 'FromByteString', so nobody can be granted e.g. a +-- @delete-only:conversations_code@ that no endpoint honours. data OAuthScope - = FeatureConfigs OAuthTier - | Self OAuthTier - | Conversations OAuthTier - | ConversationsCode OAuthTier - | ConversationsName OAuthTier - | Meetings OAuthTier - deriving (Eq, Show, Generic, Ord) + = ReadFeatureConfigs + | ReadSelf + | ReadConversationsCode + | WriteOnlyConversations + | WriteOnlyConversationsCode + | WriteOnlyConversationsName + | WriteOnlyMeetings + deriving (Eq, Show, Generic, Ord, Bounded, Enum) deriving (Arbitrary) via (GenericUniform OAuthScope) - deriving (Bounded, Enum) via (GenericEnum OAuthScope) --- | Unlike 'OldOAuthScope', these tiers are disjoint. +-- | The tiers are disjoint: 'WriteOnly' does not include 'Read'. (The +-- deprecated scopes were cumulative; see @granted_scopes@ in +-- @libs/libzauth/libzauth/src/oauth.rs@, which still honours tokens carrying +-- them.) -- TODO: s/Read/ReadOnly/g data OAuthTier = Read | WriteOnly | DeleteOnly deriving (Eq, Show, Generic, Ord, Bounded, Enum) deriving (Arbitrary) via (GenericUniform OAuthTier) --- | (Once the TODO on 'OAuthScope' is done this is just @fst@.) +-- | Which tier a scope grants. The HTTP method of a request decides which +-- tier it needs, see @required_tier@ in @libs/libzauth/libzauth/src/oauth.rs@. oAuthScopeTier :: OAuthScope -> OAuthTier oAuthScopeTier = \case - FeatureConfigs t -> t - Self t -> t - Conversations t -> t - ConversationsCode t -> t - ConversationsName t -> t - Meetings t -> t - --- | Reflect a type-level 'OAuthTier' (as used in the routing tables via --- 'Wire.API.Routes.Public.DescriptionOAuthScope') down to the value level. -class IsOAuthTier (t :: OAuthTier) where - toOAuthTier :: OAuthTier - -instance IsOAuthTier 'Read where - toOAuthTier = Read - -instance IsOAuthTier 'WriteOnly where - toOAuthTier = WriteOnly - -instance IsOAuthTier 'DeleteOnly where - toOAuthTier = DeleteOnly + ReadFeatureConfigs -> Read + ReadSelf -> Read + ReadConversationsCode -> Read + WriteOnlyConversations -> WriteOnly + WriteOnlyConversationsCode -> WriteOnly + WriteOnlyConversationsName -> WriteOnly + WriteOnlyMeetings -> WriteOnly class IsOAuthScope scope where toOAuthScope :: OAuthScope -instance (IsOAuthTier t) => IsOAuthScope ('Conversations t) where - toOAuthScope = Conversations (toOAuthTier @t) +instance IsOAuthScope 'ReadFeatureConfigs where + toOAuthScope = ReadFeatureConfigs + +instance IsOAuthScope 'ReadSelf where + toOAuthScope = ReadSelf -instance (IsOAuthTier t) => IsOAuthScope ('ConversationsCode t) where - toOAuthScope = ConversationsCode (toOAuthTier @t) +instance IsOAuthScope 'ReadConversationsCode where + toOAuthScope = ReadConversationsCode -instance (IsOAuthTier t) => IsOAuthScope ('Self t) where - toOAuthScope = Self (toOAuthTier @t) +instance IsOAuthScope 'WriteOnlyConversations where + toOAuthScope = WriteOnlyConversations -instance (IsOAuthTier t) => IsOAuthScope ('FeatureConfigs t) where - toOAuthScope = FeatureConfigs (toOAuthTier @t) +instance IsOAuthScope 'WriteOnlyConversationsCode where + toOAuthScope = WriteOnlyConversationsCode -instance (IsOAuthTier t) => IsOAuthScope ('ConversationsName t) where - toOAuthScope = ConversationsName (toOAuthTier @t) +instance IsOAuthScope 'WriteOnlyConversationsName where + toOAuthScope = WriteOnlyConversationsName -instance (IsOAuthTier t) => IsOAuthScope ('Meetings t) where - toOAuthScope = Meetings (toOAuthTier @t) +instance IsOAuthScope 'WriteOnlyMeetings where + toOAuthScope = WriteOnlyMeetings instance ToByteString OAuthTier where builder = \case @@ -268,12 +269,13 @@ instance ToByteString OAuthTier where instance ToByteString OAuthScope where builder = \case - FeatureConfigs t -> builder t <> ":feature_configs" - Self t -> builder t <> ":self" - Conversations t -> builder t <> ":conversations" - ConversationsCode t -> builder t <> ":conversations_code" - ConversationsName t -> builder t <> ":conversations_name" - Meetings t -> builder t <> ":meetings" + ReadFeatureConfigs -> "read:feature_configs" + ReadSelf -> "read:self" + ReadConversationsCode -> "read:conversations_code" + WriteOnlyConversations -> "write-only:conversations" + WriteOnlyConversationsCode -> "write-only:conversations_code" + WriteOnlyConversationsName -> "write-only:conversations_name" + WriteOnlyMeetings -> "write-only:meetings" instance FromByteString OAuthScope where parser = do @@ -300,49 +302,12 @@ instance ToSchema OAuthScopes where -- returning no scopes at all, would hand out a token that does not do -- what the client asked for. oauthScopeParser :: Text -> A.Parser (Set OAuthScope) - oauthScopeParser scope = Set.fromList <$> mapM parseScope (T.splitOn " " scope) + oauthScopeParser scope = Set.fromList <$> mapM parseScope (T.words scope) parseScope :: Text -> A.Parser OAuthScope - parseScope = - maybe (fail ("invalid scope: " <> show s)) pure - . (fromByteString' . fromStrict . TE.encodeUtf8) - --- | The deprecated, cumulative scopes: @write:*@ implies @read:*@, --- @admin:*@ implies @write:*@ (see @verify_scope@ in --- @libs/libzauth/libzauth/src/oauth.rs@). --- --- NB: not every @:@ combination is a scope accepted by --- the servant handler: if not listed here, the parser in the route --- will reject it. -data OldOAuthScope - = ReadFeatureConfigs - | ReadSelf - | WriteConversations - | WriteConversationsCode - | WriteConversationsName - | WriteMeetings - | AdminMeetings - deriving (Eq, Show, Generic, Ord) - deriving (Arbitrary) via (GenericUniform OldOAuthScope) - deriving (Bounded, Enum) via (GenericEnum OldOAuthScope) - -instance ToByteString OldOAuthScope where - builder = \case - WriteConversations -> "write:conversations" - WriteConversationsCode -> "write:conversations_code" - WriteConversationsName -> "write:conversations_name" - WriteMeetings -> "write:meetings" - AdminMeetings -> "admin:meetings" - ReadSelf -> "read:self" - ReadFeatureConfigs -> "read:feature_configs" - -instance FromByteString OldOAuthScope where - parser = do - s <- (toByteString' . T.toLower) <$> parser - let table = Map.fromList [(toByteString' c, c) | c <- [(minBound :: OldOAuthScope) ..]] - case Map.lookup s table of - Just c -> pure c - Nothing -> fail $ "invalid legacy scope: " <> show s + parseScope s = + (fromByteString' . fromStrict . TE.encodeUtf8) s + & maybe (fail ("invalid scope: " <> show s)) pure data CodeChallengeMethod = S256 deriving (Eq, Show, Generic) diff --git a/libs/wire-api/src/Wire/API/Routes/Public/Brig.hs b/libs/wire-api/src/Wire/API/Routes/Public/Brig.hs index 56dfacfacd..fd3c390173 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Brig.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Brig.hs @@ -449,7 +449,7 @@ type SelfAPI = Named "get-self" ( Summary "Get your own profile" - :> DescriptionOAuthScope ('Self 'Read) + :> DescriptionOAuthScope 'ReadSelf :> ZLocalUser :> "self" :> Get '[JSON] SelfProfile diff --git a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Conversation.hs b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Conversation.hs index 77eeca7909..77bc1549d1 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Conversation.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Conversation.hs @@ -407,7 +407,7 @@ type ConversationAPI = :<|> Named "create-group-conversation@v2" ( Summary "Create a new conversation" - :> DescriptionOAuthScope ('Conversations 'WriteOnly) -- TODO: use servant to run the policy check, not just for openapi! + :> DescriptionOAuthScope 'WriteOnlyConversations :> Until 'V3 :> CanThrow 'ConvAccessDenied :> CanThrow 'MLSNonEmptyMemberList @@ -430,7 +430,7 @@ type ConversationAPI = :<|> Named "create-group-conversation@v3" ( Summary "Create a new conversation" - :> DescriptionOAuthScope ('Conversations 'WriteOnly) + :> DescriptionOAuthScope 'WriteOnlyConversations :> From 'V3 :> Until 'V4 :> CanThrow 'ConvAccessDenied @@ -534,7 +534,7 @@ type ConversationAPI = :<|> Named "create-group-conversation" ( Summary "Create a new conversation" - :> DescriptionOAuthScope ('Conversations 'WriteOnly) + :> DescriptionOAuthScope 'WriteOnlyConversations :> From 'V16 :> CanThrow 'ConvAccessDenied :> CanThrow 'MLSNonEmptyMemberList @@ -1085,7 +1085,7 @@ type ConversationAPI = "create-conversation-code-unqualified@v3" ( Summary "Create or recreate a conversation code" :> Until 'V4 - :> DescriptionOAuthScope ('ConversationsCode 'WriteOnly) + :> DescriptionOAuthScope 'WriteOnlyConversationsCode :> CanThrow 'ConvAccessDenied :> CanThrow 'ConvNotFound :> CanThrow 'GuestLinksDisabled @@ -1104,7 +1104,7 @@ type ConversationAPI = "create-conversation-code-unqualified" ( Summary "Create or recreate a conversation code" :> From 'V4 - :> DescriptionOAuthScope ('ConversationsCode 'WriteOnly) + :> DescriptionOAuthScope 'WriteOnlyConversationsCode :> CanThrow 'ConvAccessDenied :> CanThrow 'ConvNotFound :> CanThrow 'GuestLinksDisabled @@ -1151,7 +1151,7 @@ type ConversationAPI = :<|> Named "get-code" ( Summary "Get existing conversation code" - :> DescriptionOAuthScope ('ConversationsCode 'Read) + :> DescriptionOAuthScope 'ReadConversationsCode :> CanThrow 'CodeNotFound :> CanThrow 'ConvAccessDenied :> CanThrow 'ConvNotFound @@ -1340,7 +1340,7 @@ type ConversationAPI = :<|> Named "update-conversation-name" ( Summary "Update conversation name" - :> DescriptionOAuthScope ('ConversationsName 'WriteOnly) + :> DescriptionOAuthScope 'WriteOnlyConversationsName :> CanThrow ('ActionDenied 'ModifyConversationName) :> CanThrow 'ConvNotFound :> CanThrow 'InvalidOperation diff --git a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Feature.hs b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Feature.hs index 5e49ba598b..923ecc7d4a 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Feature.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Feature.hs @@ -272,7 +272,7 @@ type AllTeamFeaturesUserGet = :> Description "Gets feature configs for a user. If the user is a member of a team and has the required permissions, this will return the team's feature configs.\ \If the user is not a member of a team, this will return the personal feature configs (the server defaults)." - :> DescriptionOAuthScope ('FeatureConfigs 'Read) + :> DescriptionOAuthScope 'ReadFeatureConfigs :> ZUser :> CanThrow 'NotATeamMember :> CanThrow OperationDenied diff --git a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Meetings.hs b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Meetings.hs index 380045db02..5cf22e5c41 100644 --- a/libs/wire-api/src/Wire/API/Routes/Public/Galley/Meetings.hs +++ b/libs/wire-api/src/Wire/API/Routes/Public/Galley/Meetings.hs @@ -50,7 +50,7 @@ type MeetingsAPI = :<|> Named "create-meeting" ( Summary "Create a new meeting" - :> DescriptionOAuthScope ('Meetings 'WriteOnly) + :> DescriptionOAuthScope 'WriteOnlyMeetings :> From 'V17 :> ZLocalUser :> ZConn diff --git a/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs b/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs index 7df3aa39da..7fba8f4780 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs @@ -43,7 +43,7 @@ tests = testScopesParseOnlyKnown :: Assertion testScopesParseOnlyKnown = do (eitherDecode "\"read:self write-only:conversations\"" :: Either String OAuthScopes) - @?= Right (OAuthScopes (Set.fromList [Self Read, Conversations WriteOnly])) + @?= Right (OAuthScopes (Set.fromList [ReadSelf, WriteOnlyConversations])) for_ [ "\"read:pizza\"", -- no such scope "\"write:conversations\"", -- deprecated tier diff --git a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs index c7412f5644..117db8ae3e 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs @@ -123,7 +123,7 @@ newtype NginzLocations = NginzLocations [Location] data Location = Location { locPattern :: Text, - locOldScope :: Maybe Text, -- can't use OldOAuthScope because we don't know the HTTP verb yet. + locOldScope :: Maybe Text, -- only the base, e.g. "conversations_code": no tier without the verb. locNewScopes :: Maybe [OAuthScope] } diff --git a/services/brig/test/integration/API/OAuth.hs b/services/brig/test/integration/API/OAuth.hs index 33d2f7fb4c..ccffc87412 100644 --- a/services/brig/test/integration/API/OAuth.hs +++ b/services/brig/test/integration/API/OAuth.hs @@ -159,7 +159,7 @@ testCreateOAuthCodeSuccess brig = do let newOAuthClient@(OAuthClientConfig _ redirectUrl) = newOAuthClientRequestBody "E Corp" "https://example.com" c <- registerNewOAuthClient brig newOAuthClient uid <- randomId - let scope = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] + let scope = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] state <- UUID.toText <$> liftIO nextRandom createOAuthCode brig uid (CreateOAuthAuthorizationCodeRequest c.clientId scope OAuthResponseTypeCode redirectUrl state S256 challenge) !!! do @@ -202,7 +202,7 @@ testCreateAccessTokenSuccess opts brig = do now <- liftIO getCurrentTime user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.singleton (Self Read) + let scopes = OAuthScopes $ Set.singleton (ReadSelf) (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -231,7 +231,7 @@ testCreateAccessTokenWrongClientId :: Brig -> Http () testCreateAccessTokenWrongClientId brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] (_, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl cid <- randomId let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl @@ -243,7 +243,7 @@ testCreateAccessTokenWrongAuthorizationCode :: Brig -> Http () testCreateAccessTokenWrongAuthorizationCode brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] (cid, _) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let code = OAuthAuthorizationCode $ encodeBase16 "eb32eb9e2aa36c081c89067dddf81bce83c1c57e0b74cfb14c9f026f145f2b1f" let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl @@ -255,7 +255,7 @@ testCreateAccessTokenWrongUrl :: Brig -> Http () testCreateAccessTokenWrongUrl brig = do uid <- randomId let redirectUrl = mkUrl "https://wire.com" - let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let wrongUrl = mkUrl "https://example.com" let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code wrongUrl @@ -268,7 +268,7 @@ testCreateAccessTokenExpiredCode opts brig = withSettingsOverrides (opts & Opt.settingsLens . Opt.oAuthAuthorizationCodeExpirationTimeSecsInternalLens ?~ 1) $ do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl liftIO $ threadDelay (1 * 1200 * 1000) let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl @@ -280,7 +280,7 @@ testCreateAccessTokenWrongGrantType :: Brig -> Http () testCreateAccessTokenWrongGrantType brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeRefreshToken cid verifier code redirectUrl createOAuthAccessToken' brig accessTokenRequest !!! assertAccessDenied @@ -289,7 +289,7 @@ testCreateAccessTokenWrongCodeChallenge :: Brig -> Http () testCreateAccessTokenWrongCodeChallenge brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] (cid, code) <- generateOAuthClientAndAuthorizationCode' wrongCodeChallenge brig uid scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl createOAuthAccessToken' brig accessTokenRequest !!! do @@ -303,7 +303,7 @@ testCreateAccessTokenWrongCodeVerifier :: Brig -> Http () testCreateAccessTokenWrongCodeVerifier brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid wrongCodeVerifier code redirectUrl createOAuthAccessToken' brig accessTokenRequest !!! do @@ -347,7 +347,7 @@ testRefreshAccessTokenAccessDeniedWhenDisabled :: Opt.Opts -> Brig -> Http () testRefreshAccessTokenAccessDeniedWhenDisabled opts brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Self Read] + let scopes = OAuthScopes $ Set.fromList [ReadSelf] (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -378,7 +378,7 @@ testAccessResourceSuccessNginz brig nginz = do -- with Authorization header containing an OAuth bearer token let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Self Read] + let scopes = OAuthScopes $ Set.fromList [ReadSelf] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -389,7 +389,7 @@ testAccessResourceInsufficientScope :: Brig -> Nginz -> Http () testAccessResourceInsufficientScope brig nginz = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -401,7 +401,7 @@ testAccessResourceExpiredToken :: Brig -> Nginz -> Http () testAccessResourceExpiredToken brig nginz = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Self Read] + let scopes = OAuthScopes $ Set.fromList [ReadSelf] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -426,7 +426,7 @@ testAccessResourceInvalidSignature :: Opt.Opts -> Brig -> Nginz -> Http () testAccessResourceInvalidSignature opts brig nginz = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Self Read] + let scopes = OAuthScopes $ Set.fromList [ReadSelf] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -443,7 +443,7 @@ testRefreshTokenMaxActiveTokens opts db brig = uid <- randomId jwk <- liftIO $ readJwk (fromMaybe "path to jwk not set" opts.settings.oAuthJwkKeyPair) <&> fromMaybe (error "invalid key") let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Conversations WriteOnly, ConversationsCode WriteOnly] + let scopes = OAuthScopes $ Set.fromList [WriteOnlyConversations, WriteOnlyConversationsCode] let delayOneSec = -- we have to wait ~1 sec before we create the next token, to make sure it is created with a different timestamp -- this is due to the interpreter of the `Now` effect which auto-updates every second @@ -500,7 +500,7 @@ testRefreshTokenRetrieveAccessToken :: Brig -> Nginz -> Http () testRefreshTokenRetrieveAccessToken brig nginz = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Self Read] + let scopes = OAuthScopes $ Set.fromList [ReadSelf] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -515,7 +515,7 @@ testRefreshTokenWrongSignature :: Opts -> Brig -> Http () testRefreshTokenWrongSignature opts brig = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Self Read] + let scopes = OAuthScopes $ Set.fromList [ReadSelf] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -532,7 +532,7 @@ testRefreshTokenNoTokenId :: Opts -> Brig -> Http () testRefreshTokenNoTokenId opts brig = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Self Read] + let scopes = OAuthScopes $ Set.fromList [ReadSelf] (cid, _) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl key <- liftIO $ readJwk (fromMaybe "path to jwk not set" opts.settings.oAuthJwkKeyPair) <&> fromMaybe (error "invalid key") badRefreshToken <- liftIO $ OAuthToken <$> signRefreshToken key emptyClaimsSet @@ -545,7 +545,7 @@ testRefreshTokenNonExistingId :: Opts -> Brig -> Http () testRefreshTokenNonExistingId opts brig = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Self Read] + let scopes = OAuthScopes $ Set.fromList [ReadSelf] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -567,7 +567,7 @@ testRefreshTokenWrongClientId :: Brig -> Http () testRefreshTokenWrongClientId brig = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Self Read] + let scopes = OAuthScopes $ Set.fromList [ReadSelf] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -581,7 +581,7 @@ testRefreshTokenWrongGrantType :: Brig -> Http () testRefreshTokenWrongGrantType brig = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Self Read] + let scopes = OAuthScopes $ Set.fromList [ReadSelf] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -596,7 +596,7 @@ testRefreshTokenExpiredToken opts brig = withSettingsOverrides (opts & Opt.settingsLens . Opt.oAuthRefreshTokenExpirationTimeSecsInternalLens ?~ 2) $ do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Self Read] + let scopes = OAuthScopes $ Set.fromList [ReadSelf] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -610,7 +610,7 @@ testRefreshTokenRevokedToken :: Brig -> Http () testRefreshTokenRevokedToken brig = do user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.fromList [Self Read] + let scopes = OAuthScopes $ Set.fromList [ReadSelf] (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest @@ -645,21 +645,21 @@ testListApplicationsWithAccountAccess brig = do testWriteConversationsSuccessNginz :: Brig -> Nginz -> Http () testWriteConversationsSuccessNginz brig nginz = do (uid, tid) <- Team.createUserWithTeam brig - resp <- getAccessTokenForScope brig uid [Conversations WriteOnly] + resp <- getAccessTokenForScope brig uid [WriteOnlyConversations] createTeamConv nginz authHeader resp.accessToken tid "oauth test group" !!! do const 201 === statusCode testReadFeatureConfigsSuccessNginz :: Brig -> Nginz -> Http () testReadFeatureConfigsSuccessNginz brig nginz = do (uid, _) <- Team.createUserWithTeam brig - resp <- getAccessTokenForScope brig uid [FeatureConfigs Read] + resp <- getAccessTokenForScope brig uid [ReadFeatureConfigs] getFeatureConfigs nginz authHeader resp.accessToken !!! do const 200 === statusCode testWriteConversationsCodeSuccessNginz :: Brig -> Nginz -> Http () testWriteConversationsCodeSuccessNginz brig nginz = do (uid, tid) <- Team.createUserWithTeam brig - resp <- getAccessTokenForScope brig uid [Conversations WriteOnly, ConversationsCode WriteOnly] + resp <- getAccessTokenForScope brig uid [WriteOnlyConversations, WriteOnlyConversationsCode] conv <- responseJsonError @_ @(Conversation GroupConvType) =<< createTeamConv nginz authHeader resp.accessToken tid "oauth test group" Date: Mon, 21 Sep 2026 16:55:21 +0200 Subject: [PATCH 16/29] Fix cql instance(s) to accomodate old tokens. --- libs/cassandra-util/src/Cassandra.hs | 4 +- libs/cassandra-util/src/Cassandra/CQL.hs | 4 +- libs/wire-api/src/Wire/API/OAuth.hs | 56 +++++++++++++++---- .../wire-api/test/unit/Test/Wire/API/OAuth.hs | 21 ++++++- services/brig/src/Brig/API/OAuth.hs | 20 +++---- 5 files changed, 77 insertions(+), 28 deletions(-) diff --git a/libs/cassandra-util/src/Cassandra.hs b/libs/cassandra-util/src/Cassandra.hs index 2440633481..1713ca9767 100644 --- a/libs/cassandra-util/src/Cassandra.hs +++ b/libs/cassandra-util/src/Cassandra.hs @@ -26,7 +26,7 @@ import Cassandra.CQL as C ( Ascii (Ascii), BatchType (BatchLogged, BatchUnLogged), Blob (Blob), - ColumnType (AsciiColumn, BigIntColumn, BlobColumn, BooleanColumn, DoubleColumn, IntColumn, ListColumn, MaybeColumn, TextColumn, TimestampColumn, UdtColumn, UuidColumn, VarCharColumn), + ColumnType (AsciiColumn, BigIntColumn, BlobColumn, BooleanColumn, DoubleColumn, IntColumn, ListColumn, MaybeColumn, SetColumn, TextColumn, TimestampColumn, UdtColumn, UuidColumn, VarCharColumn), Consistency (All, LocalQuorum, One), -- DO NOT EXPORT 'Quorum' here (until a DC migration is complete) Cql, Keyspace (Keyspace), @@ -39,7 +39,7 @@ import Cassandra.CQL as C Tagged (Tagged), TimeUuid (TimeUuid), Tuple (), - Value (CqlAscii, CqlBigInt, CqlBlob, CqlBoolean, CqlDouble, CqlInt, CqlList, CqlText, CqlUdt), + Value (CqlAscii, CqlBigInt, CqlBlob, CqlBoolean, CqlDouble, CqlInt, CqlList, CqlSet, CqlText, CqlUdt), Version (V4), W, ctype, diff --git a/libs/cassandra-util/src/Cassandra/CQL.hs b/libs/cassandra-util/src/Cassandra/CQL.hs index d1e1e7c8c7..e66333a4ff 100644 --- a/libs/cassandra-util/src/Cassandra/CQL.hs +++ b/libs/cassandra-util/src/Cassandra/CQL.hs @@ -25,7 +25,7 @@ import Database.CQL.Protocol as C ( Ascii (Ascii), BatchType (BatchLogged, BatchUnLogged), Blob (Blob), - ColumnType (AsciiColumn, BigIntColumn, BlobColumn, BooleanColumn, DoubleColumn, IntColumn, ListColumn, MaybeColumn, TextColumn, TimestampColumn, UdtColumn, UuidColumn, VarCharColumn), + ColumnType (AsciiColumn, BigIntColumn, BlobColumn, BooleanColumn, DoubleColumn, IntColumn, ListColumn, MaybeColumn, SetColumn, TextColumn, TimestampColumn, UdtColumn, UuidColumn, VarCharColumn), Consistency (All, LocalQuorum, One), -- DO NOT EXPORT 'Quorum' here (until a DC migration is complete) Cql, Keyspace (Keyspace), @@ -38,7 +38,7 @@ import Database.CQL.Protocol as C Tagged (Tagged), TimeUuid (TimeUuid), Tuple (), - Value (CqlAscii, CqlBigInt, CqlBlob, CqlBoolean, CqlDouble, CqlInt, CqlList, CqlText, CqlTimestamp, CqlUdt), + Value (CqlAscii, CqlBigInt, CqlBlob, CqlBoolean, CqlDouble, CqlInt, CqlList, CqlSet, CqlText, CqlTimestamp, CqlUdt), Version (V4), W, ctype, diff --git a/libs/wire-api/src/Wire/API/OAuth.hs b/libs/wire-api/src/Wire/API/OAuth.hs index 5ccc65b667..75b525df2e 100644 --- a/libs/wire-api/src/Wire/API/OAuth.hs +++ b/libs/wire-api/src/Wire/API/OAuth.hs @@ -264,7 +264,7 @@ instance IsOAuthScope 'WriteOnlyMeetings where instance ToByteString OAuthTier where builder = \case Read -> "read" - WriteOnly -> "write-only" + WriteOnly -> "write-only" -- TODO: make this "write_only" for consistency? (also needs adjusting in OAuthScope instance.) DeleteOnly -> "delete-only" instance ToByteString OAuthScope where @@ -309,6 +309,34 @@ instance ToSchema OAuthScopes where (fromByteString' . fromStrict . TE.encodeUtf8) s & maybe (fail ("invalid scope: " <> show s)) pure +-- | A scope as it can appear in the database, in terms of the scopes we have +-- now. +-- +-- We write what 'ToByteString' gives us, but rows outlive renamings: a refresh +-- token handed out before the tiers were split up carries a deprecated, +-- cumulative scope. Reading one is not one-to-one, because @write:x@ implies +-- @read:x@, hence the 'Set'. Scopes are stored in a set column +-- anyway, so the extra elements simply join it. +-- +-- Anything we cannot make sense of yields no scopes at all. That can only +-- shrink what a token may do, never grow it. +storedScope :: Text -> Set OAuthScope +storedScope t = + (fromByteString' . fromStrict . TE.encodeUtf8) t + & maybe (deprecatedScope t) Set.singleton + +-- | The cumulative scopes we used to hand out. Only scopes that exist now can +-- come out of this, so @admin:meetings@ shrinks to @write-only:meetings@: there +-- is no @delete-only:meetings@ to grant, and no endpoint that would honour it. +deprecatedScope :: Text -> Set OAuthScope +deprecatedScope = \case + "write:conversations" -> Set.fromList [WriteOnlyConversations] + "write:conversations_code" -> Set.fromList [ReadConversationsCode, WriteOnlyConversationsCode] + "write:conversations_name" -> Set.fromList [WriteOnlyConversationsName] + "write:meetings" -> Set.fromList [WriteOnlyMeetings] + "admin:meetings" -> Set.fromList [WriteOnlyMeetings] + _ -> Set.empty + data CodeChallengeMethod = S256 deriving (Eq, Show, Generic) deriving (Arbitrary) via (GenericUniform CodeChallengeMethod) @@ -802,16 +830,22 @@ instance Cql OAuthAuthorizationCode where fromCql (CqlAscii t) = OAuthAuthorizationCode <$> validateBase16 t fromCql _ = Left "OAuthAuthorizationCode: Ascii expected" -instance Cql OAuthScope where - ctype = Tagged TextColumn - toCql = CqlText . TE.decodeUtf8With lenientDecode . toByteString' - fromCql (CqlText t) = - maybe (Left "invalid oauth scope") Right - $ fromByteString' - . fromStrict - . TE.encodeUtf8 - $ t - fromCql _ = Left "OAuthScope: Text expected" +-- | Scopes are read and written as a whole set, not one at a time: a single +-- stored scope can stand for several of ours, see 'storedScope'. +instance Cql OAuthScopes where + ctype = Tagged (SetColumn TextColumn) + + toCql = + CqlSet + . fmap (CqlText . TE.decodeUtf8With lenientDecode . toByteString') + . Set.toList + . unOAuthScopes + + fromCql (CqlSet scopes) = OAuthScopes . Set.unions <$> mapM element scopes + where + element (CqlText t) = Right (storedScope t) + element _ = Left "OAuthScopes: Text expected" + fromCql _ = Left "OAuthScopes: Set expected" instance Cql OAuthCodeChallenge where ctype = Tagged BlobColumn diff --git a/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs b/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs index 7fba8f4780..58cefbf83f 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs @@ -33,10 +33,29 @@ tests = [ testCase "should" testCodeChallengeVerification ], testGroup "scopes" $ - [ testCase "only known scopes parse" testScopesParseOnlyKnown + [ testCase "only known scopes parse" testScopesParseOnlyKnown, + testCase "stored scopes cover the deprecated ones" testStoredScopes ] ] +-- | Rows outlive renamings, so what we read from the database is not what a +-- client may ask for. A deprecated scope was cumulative and can be worth more +-- than one of ours. +testStoredScopes :: Assertion +testStoredScopes = do + -- what we write now + storedScope "read:self" @?= Set.singleton ReadSelf + storedScope "write-only:conversations" @?= Set.singleton WriteOnlyConversations + -- the deprecated write tier was a read tier as well + storedScope "write:conversations_code" + @?= Set.fromList [ReadConversationsCode, WriteOnlyConversationsCode] + -- the deprecated admin tier had a delete tier on top, but there is no + -- delete-only:meetings to grant + storedScope "admin:meetings" @?= Set.singleton WriteOnlyMeetings + -- nothing we could honour + storedScope "read:pizza" @?= Set.empty + storedScope "smell:meetings" @?= Set.empty + -- | A scope nobody can be granted has to be an error. If it were dropped, or -- turned the whole set into no scopes at all, the client would get a token that -- does not do what it asked for. diff --git a/services/brig/src/Brig/API/OAuth.hs b/services/brig/src/Brig/API/OAuth.hs index b84df3d0a6..0e1c001329 100644 --- a/services/brig/src/Brig/API/OAuth.hs +++ b/services/brig/src/Brig/API/OAuth.hs @@ -28,7 +28,6 @@ import Brig.API.Handler (Handler) import Brig.App import Brig.Options qualified as Opt import Cassandra hiding (Set) -import Cassandra qualified as C import Control.Error import Control.Lens ((?~), (^?)) import Crypto.JWT hiding (params, uri) @@ -39,7 +38,6 @@ import Data.Json.Util (toUTCTimeMillis) import Data.Map qualified as Map import Data.Misc import Data.Qualified -import Data.Set qualified as Set import Data.Text.Ascii import Data.Text.Encoding qualified as T import Data.Time @@ -425,10 +423,9 @@ lookupOauthClient cid = do insertOAuthAuthorizationCode :: (MonadClient m) => Word64 -> OAuthAuthorizationCode -> OAuthClientId -> UserId -> OAuthScopes -> RedirectUrl -> OAuthCodeChallenge -> m () insertOAuthAuthorizationCode ttl code cid uid scope uri chal = do - let cqlScope = C.Set (Set.toList (unOAuthScopes scope)) - retry x5 . write q $ params LocalQuorum (code, cid, uid, cqlScope, uri, chal, fromIntegral ttl) + retry x5 . write q $ params LocalQuorum (code, cid, uid, scope, uri, chal, fromIntegral ttl) where - q :: PrepQuery W (OAuthAuthorizationCode, OAuthClientId, UserId, C.Set OAuthScope, RedirectUrl, OAuthCodeChallenge, Int32) () + q :: PrepQuery W (OAuthAuthorizationCode, OAuthClientId, UserId, OAuthScopes, RedirectUrl, OAuthCodeChallenge, Int32) () q = fromString $ "INSERT INTO oauth_auth_code (code, client, user, scope, redirect_uri, code_challenge) VALUES (?, ?, ?, ?, ?, ?) USING TTL ?" lookupAndDeleteByOAuthAuthorizationCode :: (MonadClient m) => OAuthAuthorizationCode -> m (Maybe (OAuthClientId, UserId, OAuthScopes, RedirectUrl, Maybe OAuthCodeChallenge)) @@ -436,10 +433,9 @@ lookupAndDeleteByOAuthAuthorizationCode code = lookupOAuthAuthorizationCode <* d where lookupOAuthAuthorizationCode :: (MonadClient m) => m (Maybe (OAuthClientId, UserId, OAuthScopes, RedirectUrl, Maybe OAuthCodeChallenge)) lookupOAuthAuthorizationCode = do - mTuple <- retry x5 . query1 q $ params LocalQuorum (Identity code) - pure $ mTuple <&> \(cid, uid, C.Set scope, uri, mChal) -> (cid, uid, OAuthScopes (Set.fromList scope), uri, mChal) + retry x5 . query1 q $ params LocalQuorum (Identity code) where - q :: PrepQuery R (Identity OAuthAuthorizationCode) (OAuthClientId, UserId, C.Set OAuthScope, RedirectUrl, Maybe OAuthCodeChallenge) + q :: PrepQuery R (Identity OAuthAuthorizationCode) (OAuthClientId, UserId, OAuthScopes, RedirectUrl, Maybe OAuthCodeChallenge) q = "SELECT client, user, scope, redirect_uri, code_challenge FROM oauth_auth_code WHERE code = ?" deleteOAuthAuthorizationCode :: (MonadClient m) => m () @@ -454,9 +450,9 @@ insertOAuthRefreshToken maxActiveTokens ttl info = do oldTokes <- determineOldestTokensToBeDeleted <$> lookupOAuthRefreshTokens info.userId for_ oldTokes (\t -> deleteOAuthRefreshToken t.userId t.refreshTokenId) retry x5 . write qInsertId $ params LocalQuorum (info.userId, rid, fromIntegral ttl) - retry x5 . write qInsertInfo $ params LocalQuorum (rid, info.clientId, info.userId, C.Set (Set.toList (unOAuthScopes info.scopes)), info.createdAt, fromIntegral ttl) + retry x5 . write qInsertInfo $ params LocalQuorum (rid, info.clientId, info.userId, info.scopes, info.createdAt, fromIntegral ttl) where - qInsertInfo :: PrepQuery W (OAuthRefreshTokenId, OAuthClientId, UserId, C.Set OAuthScope, UTCTime, Int32) () + qInsertInfo :: PrepQuery W (OAuthRefreshTokenId, OAuthClientId, UserId, OAuthScopes, UTCTime, Int32) () qInsertInfo = fromString $ "INSERT INTO oauth_refresh_token (id, client, user, scope, created_at) VALUES (?, ?, ?, ?, ?) USING TTL ?" qInsertId :: PrepQuery W (UserId, OAuthRefreshTokenId, Int32) () @@ -479,9 +475,9 @@ lookupOAuthRefreshTokens uid = do lookupOAuthRefreshTokenInfo :: (MonadClient m) => OAuthRefreshTokenId -> m (Maybe OAuthRefreshTokenInfo) lookupOAuthRefreshTokenInfo rid = do mTuple <- retry x5 . query1 q $ params LocalQuorum (Identity rid) - pure $ mTuple <&> \(cid, uid, C.Set scope, createdAt) -> OAuthRefreshTokenInfo rid cid uid (OAuthScopes (Set.fromList scope)) createdAt + pure $ mTuple <&> \(cid, uid, scopes, createdAt) -> OAuthRefreshTokenInfo rid cid uid scopes createdAt where - q :: PrepQuery R (Identity OAuthRefreshTokenId) (OAuthClientId, UserId, C.Set OAuthScope, UTCTime) + q :: PrepQuery R (Identity OAuthRefreshTokenId) (OAuthClientId, UserId, OAuthScopes, UTCTime) q = "SELECT client, user, scope, created_at FROM oauth_refresh_token WHERE id = ?" deleteOAuthRefreshToken :: (MonadClient m) => UserId -> OAuthRefreshTokenId -> m () From ac17e166159638c7637990c57bc71319e7b986d4 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 21 Sep 2026 17:11:34 +0200 Subject: [PATCH 17/29] TODOs. --- libs/wire-api/src/Wire/API/OAuth.hs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/libs/wire-api/src/Wire/API/OAuth.hs b/libs/wire-api/src/Wire/API/OAuth.hs index 75b525df2e..e40d2928e1 100644 --- a/libs/wire-api/src/Wire/API/OAuth.hs +++ b/libs/wire-api/src/Wire/API/OAuth.hs @@ -216,6 +216,14 @@ data OAuthScope deriving (Eq, Show, Generic, Ord, Bounded, Enum) deriving (Arbitrary) via (GenericUniform OAuthScope) +-- TODO: copy old values.yaml and new values.yaml to tests, and run test 3 times. it's fast. + +-- TODO: if OAuthTier is only needed in tests, move it there! + +-- TODO: bump wire-docs + +-- TODO: error when requesting non-existent scopes should show list of legit scopes in message field. + -- | The tiers are disjoint: 'WriteOnly' does not include 'Read'. (The -- deprecated scopes were cumulative; see @granted_scopes@ in -- @libs/libzauth/libzauth/src/oauth.rs@, which still honours tokens carrying From 578e4905edadd3a4d5a9d76cf95e7eca9220c3d5 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Mon, 21 Sep 2026 17:14:21 +0200 Subject: [PATCH 18/29] Integration tests. --- integration/test/API/Nginz.hs | 49 ++++++- integration/test/Test/OAuth.hs | 132 ++++++++++++++++++ integration/test/Testlib/HTTP.hs | 8 ++ libs/wire-api/src/Wire/API/OAuth.hs | 6 +- .../integration-test/conf/nginz/nginx.conf | 8 +- 5 files changed, 193 insertions(+), 10 deletions(-) diff --git a/integration/test/API/Nginz.hs b/integration/test/API/Nginz.hs index 3649bf1d8c..0232cd00ed 100644 --- a/integration/test/API/Nginz.hs +++ b/integration/test/API/Nginz.hs @@ -31,14 +31,14 @@ getSystemSettingsUnAuthorized domain = do login :: (HasCallStack, MakesValue domain, MakesValue email, MakesValue password) => domain -> email -> password -> App Response login domain email pw = do - req <- rawBaseRequest domain Nginz Unversioned "/login" + req <- rawBaseNginzRequest domain Unversioned "/login" emailStr <- make email >>= asString pwStr <- make pw >>= asString submit "POST" (req & addJSONObject ["email" .= emailStr, "password" .= pwStr, "label" .= "auth"]) loginWith2ndFactor :: (HasCallStack, MakesValue domain, MakesValue email, MakesValue password, MakesValue sndFactor) => domain -> email -> password -> sndFactor -> App Response loginWith2ndFactor domain email pw sf = do - req <- rawBaseRequest domain Nginz Unversioned "/login" + req <- rawBaseNginzRequest domain Unversioned "/login" emailStr <- make email >>= asString pwStr <- make pw >>= asString sfStr <- make sf >>= asString @@ -46,13 +46,13 @@ loginWith2ndFactor domain email pw sf = do access :: (HasCallStack, MakesValue domain, MakesValue cookie) => domain -> cookie -> App Response access domain cookie = do - req <- rawBaseRequest domain Nginz Unversioned "/access" + req <- rawBaseNginzRequest domain Unversioned "/access" cookieStr <- make cookie >>= asString submit "POST" (req & setCookie cookieStr) logout :: (HasCallStack, MakesValue domain, MakesValue cookie, MakesValue token) => domain -> cookie -> token -> App Response logout d c t = do - req <- rawBaseRequest d Nginz Unversioned "/access/logout" + req <- rawBaseNginzRequest d Unversioned "/access/logout" cookie <- make c & asString token <- make t & asString submit "POST" (req & setCookie cookie & addHeader "Authorization" ("Bearer " <> token)) @@ -61,12 +61,49 @@ getConversation :: (HasCallStack, MakesValue user, MakesValue qcnv, MakesValue t getConversation user qcnv t = do (domain, cnv) <- objQid qcnv token <- make t & asString - req <- rawBaseRequest user Nginz Versioned (joinHttpPath ["conversations", domain, cnv]) + req <- rawBaseNginzRequest user Versioned (joinHttpPath ["conversations", domain, cnv]) submit "GET" (req & addHeader "Authorization" ("Bearer " <> token)) +-- | The endpoints below take an OAuth access token, which is what nginz checks +-- scopes against. A request with a zauth cookie or token is not affected by +-- any of that; see 'Test.OAuth'. +getSelf :: (HasCallStack, MakesValue user, MakesValue token) => user -> token -> App Response +getSelf user t = do + token <- make t & asString + req <- rawBaseNginzRequest user Versioned "/self" + submit "GET" (req & addHeader "Authorization" ("Bearer " <> token)) + +postConversation :: (HasCallStack, MakesValue user, MakesValue conv, MakesValue token) => user -> conv -> token -> App Response +postConversation user conv t = do + token <- make t & asString + body <- make conv + req <- rawBaseNginzRequest user Versioned "/conversations" + submit "POST" (req & addJSON body & addHeader "Authorization" ("Bearer " <> token)) + +getConversationCode :: (HasCallStack, MakesValue user, MakesValue conv, MakesValue token) => user -> conv -> token -> App Response +getConversationCode user conv t = do + convId <- objQidObject conv & objId + token <- make t & asString + req <- rawBaseNginzRequest user Versioned (joinHttpPath ["conversations", convId, "code"]) + submit "GET" (req & addHeader "Authorization" ("Bearer " <> token)) + +postConversationCode :: (HasCallStack, MakesValue user, MakesValue conv, MakesValue token) => user -> conv -> token -> App Response +postConversationCode user conv t = do + convId <- objQidObject conv & objId + token <- make t & asString + req <- rawBaseNginzRequest user Versioned (joinHttpPath ["conversations", convId, "code"]) + submit "POST" (req & addJSONObject [] & addHeader "Authorization" ("Bearer " <> token)) + +deleteConversationCode :: (HasCallStack, MakesValue user, MakesValue conv, MakesValue token) => user -> conv -> token -> App Response +deleteConversationCode user conv t = do + convId <- objQidObject conv & objId + token <- make t & asString + req <- rawBaseNginzRequest user Versioned (joinHttpPath ["conversations", convId, "code"]) + submit "DELETE" (req & addHeader "Authorization" ("Bearer " <> token)) + uploadProviderAsset :: (HasCallStack, MakesValue domain) => domain -> String -> String -> App Response uploadProviderAsset domain cookie payload = do - req <- rawBaseRequest domain Nginz Versioned $ joinHttpPath ["provider", "assets"] + req <- rawBaseNginzRequest domain Versioned $ joinHttpPath ["provider", "assets"] bdy <- txtAsset payload submit "POST" $ req diff --git a/integration/test/Test/OAuth.hs b/integration/test/Test/OAuth.hs index dd88ce2940..62d3ece2ff 100644 --- a/integration/test/Test/OAuth.hs +++ b/integration/test/Test/OAuth.hs @@ -20,7 +20,15 @@ module Test.OAuth where import API.Brig import API.BrigInternal import API.Common (defPassword) +import API.Galley +import qualified API.Nginz as Nginz +import qualified Data.Aeson as Aeson +import qualified Data.ByteString.Base64.URL as B64U import Data.String.Conversions +import qualified Data.Text as T +import Data.UUID (UUID) +import qualified Data.UUID as UUID +import Database.CQL.IO import Network.HTTP.Types import Network.URI import SetupHelpers @@ -146,6 +154,130 @@ testRevokeApplicationAccountAccess = do apps <- resp.json & asList length apps `shouldMatchInt` 0 +-- | The tiers of a scope are separate: a token that may write may not read, +-- and the other way round. This is about @/conversations/:cnv/code@, the one +-- location in the integration nginx.conf that uses 'oauth_scopes'. +testOAuthScopeTiersAreSeparate :: (HasCallStack) => App () +testOAuthScopeTiersAreSeparate = do + (user, _, _) <- createTeam OwnDomain 1 + conv <- postConversation user (allowGuests defProteus) >>= getJSON 201 + -- with a zauth token, so that there is a code to read later on + postConversationCode user conv Nothing Nothing >>= assertSuccess + + cid <- oauthClient user + readToken <- accessTokenFor user cid ["read:conversations_code"] + writeToken <- accessTokenFor user cid ["write-only:conversations_code"] + + Nginz.getConversationCode user conv readToken >>= assertStatus 200 + Nginz.postConversationCode user conv readToken >>= assertStatus 403 + + Nginz.postConversationCode user conv writeToken >>= assertSuccess + Nginz.getConversationCode user conv writeToken >>= assertStatus 403 + +testOAuthRejectUnusefulTokenRequests :: (HasCallStack) => App () +testOAuthRejectUnusefulTokenRequests = do + (user, _, _) <- createTeam OwnDomain 1 + cid <- oauthClient user + + generateOAuthAuthorizationCode user cid [] redirectUri >>= assertStatus 400 + generateOAuthAuthorizationCode user cid ["pizza"] redirectUri >>= assertStatus 400 + generateOAuthAuthorizationCode user cid ["delete-only:conversations_code"] redirectUri >>= assertStatus 400 + +testOAuthNewScopesOnDeprecatedAttribute :: (HasCallStack) => App () +testOAuthNewScopesOnDeprecatedAttribute = do + user <- randomUser OwnDomain def + cid <- oauthClient user + + selfToken <- accessTokenFor user cid ["read:self"] + Nginz.getSelf user selfToken >>= assertStatus 200 + + convToken <- accessTokenFor user cid ["write-only:conversations"] + Nginz.postConversation user defProteus convToken >>= assertStatus 201 + + -- ... and the deprecated attribute still tells the scopes apart + Nginz.getSelf user convToken >>= assertStatus 403 + +testOAuthDeprecatedScopesInCassandra :: (HasCallStack) => TaggedBool "old scope syntax" -> App () +testOAuthDeprecatedScopesInCassandra (TaggedBool oldScopeSyntax) = do + (user, _, _) <- createTeam OwnDomain 1 + conv <- postConversation user (allowGuests defProteus) >>= getJSON 201 + postConversationCode user conv Nothing Nothing >>= assertSuccess + + cid <- oauthClient user + session <- generateAccessToken user cid ["write-only:conversations_code"] redirectUri + + when oldScopeSyntax (hackCassandra user) + + refreshToken <- session %. "refresh_token" & asString + refreshed <- createOAuthAccessTokenWithRefreshToken user cid refreshToken >>= getJSON 200 + + if oldScopeSyntax + then do + token <- refreshed %. "access_token" & asString + hasScopes token ["read:conversations_code", "write-only:conversations_code"] + Nginz.getConversationCode user conv token >>= assertSuccess + Nginz.postConversationCode user conv token >>= assertSuccess + else do + token <- refreshed %. "access_token" & asString + hasScopes token ["write-only:conversations_code"] + Nginz.getConversationCode user conv token >>= assertStatus 403 + Nginz.postConversationCode user conv token >>= assertSuccess + where + -- pretend the session was created before the split + hackCassandra :: Value -> App () + hackCassandra user = do + keyspace <- readServiceConfig Brig & (%. "cassandra.keyspace") & asString + let setScope :: PrepQuery W (Identity UUID) () = + fromString + $ "UPDATE " + <> keyspace + <> ".oauth_refresh_token SET scope = {'write:conversations_code', 'read:pizza'} WHERE id = ?" + rid <- refreshTokenId user + write setScope (defQueryParams LocalQuorum (Identity rid)) + + hasScopes :: String -> [String] -> App () + hasScopes token expectedScopes = do + claims <- accessTokenClaims token + scopes <- claims %. "scope" & asString + words scopes `shouldMatchSet` expectedScopes + +-------------------------------------------------------------------------------- +-- helpers + +redirectUri :: String +redirectUri = "https://example.com" + +oauthClient :: (HasCallStack, MakesValue user) => user -> App Value +oauthClient user = + createOAuthClient user "foobar" redirectUri >>= getJSON 200 >>= (%. "client_id") + +-- | The access token, which is the part nginz gets to see. +accessTokenFor :: (HasCallStack, MakesValue user, MakesValue cid) => user -> cid -> [String] -> App String +accessTokenFor user cid scopes = + generateAccessToken user cid scopes redirectUri >>= (%. "access_token") >>= asString + +-- | The id of the one session the user has. +refreshTokenId :: (HasCallStack, MakesValue user) => user -> App UUID +refreshTokenId user = do + [app] <- getOAuthApplications user >>= getJSON 200 >>= asList + [session] <- app %. "sessions" >>= asList + rid <- session %. "refresh_token_id" & asString + maybe (assertFailure ("not a uuid: " <> rid)) pure (UUID.fromString rid) + +-- | The claims of an access token, read without verifying anything: we only +-- want to see what brig put in. +accessTokenClaims :: (HasCallStack) => String -> App Value +accessTokenClaims token = do + payload <- case T.splitOn (cs ".") (cs token) of + (_ : p : _) -> pure p + _ -> assertFailure ("not a JWT: " <> token) + claims <- case B64U.decodeUnpadded (cs payload) of + Left e -> assertFailure ("not base64url: " <> token <> ": " <> e) + Right bs -> pure bs + case Aeson.eitherDecode (cs claims) of + Left e -> assertFailure ("not json: " <> token <> ": " <> e) + Right v -> pure v + generateAccessToken :: (MakesValue cid, MakesValue user) => user -> cid -> [String] -> String -> App Value generateAccessToken user cid scopes uri = do authCodeResponse <- generateOAuthAuthorizationCode user cid scopes uri diff --git a/integration/test/Testlib/HTTP.hs b/integration/test/Testlib/HTTP.hs index ad48ed444d..0745958886 100644 --- a/integration/test/Testlib/HTTP.hs +++ b/integration/test/Testlib/HTTP.hs @@ -189,6 +189,14 @@ rawBaseRequest domain service versioned path = do let HostPort h p = serviceHostPort serviceMap service in "http://" <> h <> ":" <> show p <> ("/" <> joinHttpPath (pathSegsPrefix <> splitHttpPath path)) +-- | This is a thin wrapper around rawBaseRequest that adds a non-IP +-- Z-Host header. ('ZHostOpt' parses the HTTP header as a 'Domain', +-- and that doesn't parse if nginz is contacted under its IP address.) +rawBaseNginzRequest :: (HasCallStack, MakesValue domain) => domain -> Versioned -> String -> App HTTP.Request +rawBaseNginzRequest user versioned path = do + domain <- objDomain user + addHeader "Host" domain <$> rawBaseRequest user Nginz versioned path + -- | The bare minimum to ge a `HTTP.Request` given a URL externalRequest :: String -> App HTTP.Request externalRequest = liftIO . HTTP.parseRequest diff --git a/libs/wire-api/src/Wire/API/OAuth.hs b/libs/wire-api/src/Wire/API/OAuth.hs index e40d2928e1..43b98f27c0 100644 --- a/libs/wire-api/src/Wire/API/OAuth.hs +++ b/libs/wire-api/src/Wire/API/OAuth.hs @@ -849,10 +849,10 @@ instance Cql OAuthScopes where . Set.toList . unOAuthScopes - fromCql (CqlSet scopes) = OAuthScopes . Set.unions <$> mapM element scopes + fromCql (CqlSet scopes) = OAuthScopes . Set.unions <$> mapM el scopes where - element (CqlText t) = Right (storedScope t) - element _ = Left "OAuthScopes: Text expected" + el (CqlText t) = Right (storedScope t) + el _ = Left "OAuthScopes: Text expected" fromCql _ = Left "OAuthScopes: Set expected" instance Cql OAuthCodeChallenge where diff --git a/services/nginz/integration-test/conf/nginz/nginx.conf b/services/nginz/integration-test/conf/nginz/nginx.conf index a6e10ecf9e..8ea7e27fa5 100644 --- a/services/nginz/integration-test/conf/nginz/nginx.conf +++ b/services/nginz/integration-test/conf/nginz/nginx.conf @@ -469,7 +469,13 @@ http { location ~* ^(/v[0-9]+)?/conversations/([^/]*)/code { include common_response_with_zauth.conf; - oauth_scope conversations_code; + # The only location where 'oauth_scopes' is used instead of the + # deprecated 'oauth_scope', so that one test run covers both + # paths through the nginz module. See + # 'testOAuthScopeTiersAreSeparate' and + # 'testOAuthNewScopesOnDeprecatedAttribute' in + # integration/test/Test/OAuth.hs. + oauth_scopes read:conversations_code write-only:conversations_code; proxy_pass http://galley; } From b7623c8de48a2fcca596ed1fcd6663f5412f0d9a Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 23 Sep 2026 09:56:32 +0200 Subject: [PATCH 19/29] Make scopes parser reject empty scopes list. --- libs/wire-api/src/Wire/API/OAuth.hs | 17 ++++++++++++++--- libs/wire-api/test/unit/Test/Wire/API/OAuth.hs | 3 ++- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/libs/wire-api/src/Wire/API/OAuth.hs b/libs/wire-api/src/Wire/API/OAuth.hs index 43b98f27c0..acd28d2398 100644 --- a/libs/wire-api/src/Wire/API/OAuth.hs +++ b/libs/wire-api/src/Wire/API/OAuth.hs @@ -45,7 +45,7 @@ import Imports hiding (exp, head) import Prelude.Singletons (Show_) import Servant hiding (Handler, JSON, Tagged, addHeader, respond) import Servant.OpenApi.Internal.Orphans () -import Test.QuickCheck (Arbitrary (..)) +import Test.QuickCheck (Arbitrary (..), listOf1) import URI.ByteString import URI.ByteString.QQ qualified as URI.QQ import Web.FormUrlEncoded (Form (..), FromForm (..), ToForm (..), parseUnique) @@ -294,9 +294,17 @@ instance FromByteString OAuthScope where Nothing -> fail $ "invalid scope: " <> show s newtype OAuthScopes = OAuthScopes {unOAuthScopes :: Set OAuthScope} - deriving (Eq, Show, Generic, Monoid, Semigroup, Arbitrary) + deriving (Eq, Show, Generic, Monoid, Semigroup) deriving (A.ToJSON, A.FromJSON, S.ToSchema) via (Schema OAuthScopes) +instance Arbitrary OAuthScopes where + arbitrary = OAuthScopes . Set.fromList <$> listOf1 arbitrary + shrink (OAuthScopes s) = + [ OAuthScopes (Set.fromList xs) + | xs <- shrink (Set.toList s), + not (null xs) + ] + instance ToSchema OAuthScopes where schema = OAuthScopes <$> (oauthScopesToText . unOAuthScopes) .= withParser schema oauthScopeParser where @@ -310,7 +318,10 @@ instance ToSchema OAuthScopes where -- returning no scopes at all, would hand out a token that does not do -- what the client asked for. oauthScopeParser :: Text -> A.Parser (Set OAuthScope) - oauthScopeParser scope = Set.fromList <$> mapM parseScope (T.words scope) + oauthScopeParser scope = do + let ws = T.words scope + when (null ws) $ fail "empty scope" + Set.fromList <$> mapM parseScope ws parseScope :: Text -> A.Parser OAuthScope parseScope s = diff --git a/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs b/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs index 58cefbf83f..21f70112f1 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/OAuth.hs @@ -64,7 +64,8 @@ testScopesParseOnlyKnown = do (eitherDecode "\"read:self write-only:conversations\"" :: Either String OAuthScopes) @?= Right (OAuthScopes (Set.fromList [ReadSelf, WriteOnlyConversations])) for_ - [ "\"read:pizza\"", -- no such scope + [ "\"\"", -- empty scope + "\"read:pizza\"", -- no such scope "\"write:conversations\"", -- deprecated tier "\"read:self read:pizza\"" -- one bad scope spoils the request ] From 2c5e133aa6fb8231c7d189447d94949678b30cb1 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 23 Sep 2026 11:26:21 +0200 Subject: [PATCH 20/29] Clean up various copies of routes for nginx.conf. --- charts/nginz/values.yaml | 4 ++-- .../dockerephemeral/federation-v0/nginz/conf/nginx.conf | 8 ++++---- .../dockerephemeral/federation-v1/nginz/conf/nginx.conf | 8 ++++---- .../dockerephemeral/federation-v2/nginz/conf/nginx.conf | 8 ++++---- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/charts/nginz/values.yaml b/charts/nginz/values.yaml index 65a5c74756..960c6c43d2 100644 --- a/charts/nginz/values.yaml +++ b/charts/nginz/values.yaml @@ -668,7 +668,7 @@ nginx_conf: envs: - all oauth_scope: conversations_code # deprecated, will be ignored if 'oauth_scopes' is present. - # oauth_scopes: ["read:conversations_code", "write-only:conversations_code"] + oauth_scopes: ["read:conversations_code", "write-only:conversations_code"] - path: /conversations/join envs: - all @@ -795,7 +795,7 @@ nginx_conf: - path: /meetings/([^/]*)/([^/]*)$ envs: - all - oauth_scopes: [] # TODO: this will be `["write-only:meetings", "delete-only:meetings"]` soon, according to https://wearezeta.atlassian.net/browse/WPB-28194 + oauth_scopes: [] # https://wearezeta.atlassian.net/browse/WPB-28194: this will be `["write-only:meetings", "delete-only:meetings"]` soon. - path: /meetings/(.*) envs: - all diff --git a/deploy/dockerephemeral/federation-v0/nginz/conf/nginx.conf b/deploy/dockerephemeral/federation-v0/nginz/conf/nginx.conf index cd4ec97a1a..2f479cff06 100644 --- a/deploy/dockerephemeral/federation-v0/nginz/conf/nginx.conf +++ b/deploy/dockerephemeral/federation-v0/nginz/conf/nginx.conf @@ -232,7 +232,7 @@ http { location ~* ^(/v[0-9]+)?/self$ { include common_response_with_zauth.conf; - oauth_scope self; + oauth_scopes read:self; proxy_pass http://brig; } @@ -357,13 +357,13 @@ http { location ~* ^(/v[0-9]+)?/conversations$ { include common_response_with_zauth.conf; - oauth_scope conversations; + oauth_scopes write-only:conversations; proxy_pass http://galley; } location ~* ^(/v[0-9]+)?/conversations/([^/]*)/code { include common_response_with_zauth.conf; - oauth_scope conversations_code; + oauth_scopes read:conversations_code write-only:conversations_code; proxy_pass http://galley; } @@ -429,7 +429,7 @@ http { location ~* ^(/v[0-9]+)?/feature-configs$ { include common_response_with_zauth.conf; - oauth_scope feature_configs; + oauth_scopes read:feature_configs; proxy_pass http://galley; } diff --git a/deploy/dockerephemeral/federation-v1/nginz/conf/nginx.conf b/deploy/dockerephemeral/federation-v1/nginz/conf/nginx.conf index 43f8c68b30..8346bf1f36 100644 --- a/deploy/dockerephemeral/federation-v1/nginz/conf/nginx.conf +++ b/deploy/dockerephemeral/federation-v1/nginz/conf/nginx.conf @@ -232,7 +232,7 @@ http { location ~* ^(/v[0-9]+)?/self$ { include common_response_with_zauth.conf; - oauth_scope self; + oauth_scopes read:self; proxy_pass http://brig; } @@ -357,13 +357,13 @@ http { location ~* ^(/v[0-9]+)?/conversations$ { include common_response_with_zauth.conf; - oauth_scope conversations; + oauth_scopes write-only:conversations; proxy_pass http://galley; } location ~* ^(/v[0-9]+)?/conversations/([^/]*)/code { include common_response_with_zauth.conf; - oauth_scope conversations_code; + oauth_scopes read:conversations_code write-only:conversations_code; proxy_pass http://galley; } @@ -429,7 +429,7 @@ http { location ~* ^(/v[0-9]+)?/feature-configs$ { include common_response_with_zauth.conf; - oauth_scope feature_configs; + oauth_scopes read:feature_configs; proxy_pass http://galley; } diff --git a/deploy/dockerephemeral/federation-v2/nginz/conf/nginx.conf b/deploy/dockerephemeral/federation-v2/nginz/conf/nginx.conf index bef49f1d04..7d1319ec7e 100644 --- a/deploy/dockerephemeral/federation-v2/nginz/conf/nginx.conf +++ b/deploy/dockerephemeral/federation-v2/nginz/conf/nginx.conf @@ -212,7 +212,7 @@ http { location ~* ^(/v[0-9]+)?/self$ { include common_response_with_zauth.conf; - oauth_scope self; + oauth_scopes read:self; proxy_pass http://brig; } @@ -337,13 +337,13 @@ http { location ~* ^(/v[0-9]+)?/conversations$ { include common_response_with_zauth.conf; - oauth_scope conversations; + oauth_scopes write-only:conversations; proxy_pass http://galley; } location ~* ^(/v[0-9]+)?/conversations/([^/]*)/code { include common_response_with_zauth.conf; - oauth_scope conversations_code; + oauth_scopes read:conversations_code write-only:conversations_code; proxy_pass http://galley; } @@ -409,7 +409,7 @@ http { location ~* ^(/v[0-9]+)?/feature-configs$ { include common_response_with_zauth.conf; - oauth_scope feature_configs; + oauth_scopes read:feature_configs; proxy_pass http://galley; } From 580a3a090e336cea50b8e331794276e9728052cc Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 23 Sep 2026 12:58:28 +0200 Subject: [PATCH 21/29] Advanced rust magic for more types, more inlining. --- libs/libzauth/libzauth-c/src/lib.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/libs/libzauth/libzauth-c/src/lib.rs b/libs/libzauth/libzauth-c/src/lib.rs index 097df12459..23bb636729 100644 --- a/libs/libzauth/libzauth-c/src/lib.rs +++ b/libs/libzauth/libzauth-c/src/lib.rs @@ -491,8 +491,10 @@ pub extern "C" fn oauth_verify_token_scopes( /// A NULL `scope` is how a location with nothing configured arrives here, and /// there is nothing we could let the token do: no scope, no access. +// Monomorphize over `F` with `#[inline]` to avoid indirect call overhead through `fn` pointers. #[allow(clippy::too_many_arguments)] -fn oauth_verify( +#[inline] +fn oauth_verify( jwk: &OAuthPubJwk, token: *const u8, token_len: size_t, @@ -500,8 +502,11 @@ fn oauth_verify( scope_len: size_t, method: *const u8, method_len: size_t, - verify: fn(&str, &str, &str, &str) -> Result, -) -> OAuthResult { + verify: F, +) -> OAuthResult +where + F: Fn(&str, &str, &str, &str) -> Result + std::panic::RefUnwindSafe, +{ match panic::catch_unwind(|| { if token.is_null() { return OAuthResult { From aa33c7243fa6670cfd4a1a692b727ef022fc0fd4 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 23 Sep 2026 13:10:25 +0200 Subject: [PATCH 22/29] Fix old brig integration tests. --- services/brig/test/integration/API/OAuth.hs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/services/brig/test/integration/API/OAuth.hs b/services/brig/test/integration/API/OAuth.hs index ccffc87412..0a988832da 100644 --- a/services/brig/test/integration/API/OAuth.hs +++ b/services/brig/test/integration/API/OAuth.hs @@ -82,7 +82,12 @@ import Wire.API.User.Auth (CookieType (PersistentCookie)) import Wire.Sem.Jwk (readJwk) tests :: Manager -> C.ClientState -> Brig -> Nginz -> Opts -> TestTree -tests m db b n o = do +tests m db b nginz o = do + -- nginz forwards the request's Host as Z-Host and the services parse that as + -- a Domain. The test config contacts nginz by IP, which is not a Domain, so + -- send the local domain as Host. (Cf. 'rawBaseNginzRequest' in the new + -- integration suite.) + let n = nginz . header "Host" (cs (domainText o.settings.federationDomain)) testGroup "oauth" [ test m "register new oauth client" $ testRegisterNewOAuthClient b, @@ -179,7 +184,7 @@ testCreateOAuthCodeRedirectUrlMismatch brig = do uid <- randomId state <- UUID.toText <$> liftIO nextRandom let differentUrl = mkUrl "https://wire.com" - createOAuthCode brig uid (CreateOAuthAuthorizationCodeRequest c.clientId mempty OAuthResponseTypeCode differentUrl state S256 challenge) !!! do + createOAuthCode brig uid (CreateOAuthAuthorizationCodeRequest c.clientId (OAuthScopes $ Set.singleton ReadSelf) OAuthResponseTypeCode differentUrl state S256 challenge) !!! do const 400 === statusCode const Nothing === (fmap getPath . getLocation) const (Just "redirect-url-miss-match") === fmap Error.label . responseJsonMaybe @@ -190,7 +195,7 @@ testCreateOAuthCodeClientNotFound brig = do uid <- randomId let redirectUrl = mkUrl "https://example.com" state <- UUID.toText <$> liftIO nextRandom - createOAuthCode brig uid (CreateOAuthAuthorizationCodeRequest cid mempty OAuthResponseTypeCode redirectUrl state S256 challenge) !!! do + createOAuthCode brig uid (CreateOAuthAuthorizationCodeRequest cid (OAuthScopes $ Set.singleton ReadSelf) OAuthResponseTypeCode redirectUrl state S256 challenge) !!! do const 404 === statusCode const (Just $ "access_denied") === (getLocation >=> getQueryParamValue "error") const (Just $ cs state) === (getLocation >=> getQueryParamValue "state") @@ -327,7 +332,7 @@ testCreateCodeOAuthClientAccessDeniedWhenDisabled opts brig = uid <- randomId state <- UUID.toText <$> liftIO nextRandom let redirectUrl = mkUrl "https://example.com" - createOAuthCode brig uid (CreateOAuthAuthorizationCodeRequest cid mempty OAuthResponseTypeCode redirectUrl state S256 challenge) !!! do + createOAuthCode brig uid (CreateOAuthAuthorizationCodeRequest cid (OAuthScopes $ Set.singleton ReadSelf) OAuthResponseTypeCode redirectUrl state S256 challenge) !!! do const 403 === statusCode const (Just $ "access_denied") === (getLocation >=> getQueryParamValue "error") const (Just $ cs state) === (getLocation >=> getQueryParamValue "state") @@ -732,7 +737,7 @@ getFeatureConfigs svc mkHeader token = do createOAuthApplicationWithAccountAccess :: Brig -> UserId -> Http OAuthAccessTokenResponse createOAuthApplicationWithAccountAccess brig uid = do let redirectUrl = mkUrl "https://example.com" - (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid (OAuthScopes $ mempty) redirectUrl + (cid, code) <- generateOAuthClientAndAuthorizationCode brig uid (OAuthScopes $ Set.singleton ReadSelf) redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl createOAuthAccessToken brig accessTokenRequest From 2e37bfda3e414813cb332a8ccc9bcd17715b24a1 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 23 Sep 2026 13:20:37 +0200 Subject: [PATCH 23/29] Release notes. --- .../WPB-28193-nginx-conf-syntax-changed | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 changelog.d/0-release-notes/WPB-28193-nginx-conf-syntax-changed diff --git a/changelog.d/0-release-notes/WPB-28193-nginx-conf-syntax-changed b/changelog.d/0-release-notes/WPB-28193-nginx-conf-syntax-changed new file mode 100644 index 0000000000..c23ec41570 --- /dev/null +++ b/changelog.d/0-release-notes/WPB-28193-nginx-conf-syntax-changed @@ -0,0 +1,18 @@ +**[Only relevant if you have overwritten `oauth_scope` directives in +your nginx config]** The nginz route configuration syntax changed: +`oauth_scope` (a bare scope base, e.g. `conversations_code`) is +deprecated in favor of `oauth_scopes` (whole scopes, +e.g. `["read:conversations_code", "write-only:conversations_code"]`). + +If you override nginz routes in your own values, migrate each +`oauth_scope` entry to the equivalent `oauth_scopes` list; +`oauth_scopes` wins where a route has both. If you use the bundled +chart, there is nothing to do, it is already updated. + +The old directive and old oauth tokens remain valid with new +wire-server, but we will remove the deprecated functionality in some +future release. + +See docs/src/developer/reference/config-options.md, +https://docs.wire.com/latest/developer/reference/oauth.html for more +context. From 88919626b6df60466d8196df8f62f839f0c876cc Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 23 Sep 2026 13:36:39 +0200 Subject: [PATCH 24/29] Refactor: move code only needed in tests out of prod. --- libs/wire-api/src/Wire/API/OAuth.hs | 29 ------------------- .../unit/Test/Wire/API/Routes/OAuthScopes.hs | 19 ++++++++++-- 2 files changed, 16 insertions(+), 32 deletions(-) diff --git a/libs/wire-api/src/Wire/API/OAuth.hs b/libs/wire-api/src/Wire/API/OAuth.hs index acd28d2398..7b78b705b8 100644 --- a/libs/wire-api/src/Wire/API/OAuth.hs +++ b/libs/wire-api/src/Wire/API/OAuth.hs @@ -218,33 +218,10 @@ data OAuthScope -- TODO: copy old values.yaml and new values.yaml to tests, and run test 3 times. it's fast. --- TODO: if OAuthTier is only needed in tests, move it there! - -- TODO: bump wire-docs -- TODO: error when requesting non-existent scopes should show list of legit scopes in message field. --- | The tiers are disjoint: 'WriteOnly' does not include 'Read'. (The --- deprecated scopes were cumulative; see @granted_scopes@ in --- @libs/libzauth/libzauth/src/oauth.rs@, which still honours tokens carrying --- them.) --- TODO: s/Read/ReadOnly/g -data OAuthTier = Read | WriteOnly | DeleteOnly - deriving (Eq, Show, Generic, Ord, Bounded, Enum) - deriving (Arbitrary) via (GenericUniform OAuthTier) - --- | Which tier a scope grants. The HTTP method of a request decides which --- tier it needs, see @required_tier@ in @libs/libzauth/libzauth/src/oauth.rs@. -oAuthScopeTier :: OAuthScope -> OAuthTier -oAuthScopeTier = \case - ReadFeatureConfigs -> Read - ReadSelf -> Read - ReadConversationsCode -> Read - WriteOnlyConversations -> WriteOnly - WriteOnlyConversationsCode -> WriteOnly - WriteOnlyConversationsName -> WriteOnly - WriteOnlyMeetings -> WriteOnly - class IsOAuthScope scope where toOAuthScope :: OAuthScope @@ -269,12 +246,6 @@ instance IsOAuthScope 'WriteOnlyConversationsName where instance IsOAuthScope 'WriteOnlyMeetings where toOAuthScope = WriteOnlyMeetings -instance ToByteString OAuthTier where - builder = \case - Read -> "read" - WriteOnly -> "write-only" -- TODO: make this "write_only" for consistency? (also needs adjusting in OAuthScope instance.) - DeleteOnly -> "delete-only" - instance ToByteString OAuthScope where builder = \case ReadFeatureConfigs -> "read:feature_configs" diff --git a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs index 117db8ae3e..8e16b336cf 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs @@ -111,6 +111,15 @@ testPatternVocabulary = -------------------------------------------------------------------------------- -- what nginz enforces +data OAuthTier = Read | WriteOnly | DeleteOnly + deriving (Eq, Show) + +instance ToByteString OAuthTier where + builder = \case + Read -> "read" + WriteOnly -> "write-only" + DeleteOnly -> "delete-only" + -- | The locations nginz emits, in the order it emits them. -- -- @charts/nginz/templates/_helpers.tpl@ merges @upstreams@ (minus @@ -154,7 +163,7 @@ enforcedScopes method path = case find locationMatches nginzLocations of Just loc -> case (loc.locOldScope, loc.locNewScopes) of (_, Just newScopes) -> -- Filter scopes listed in values.yaml by matching method/tier. - Set.fromList (filter ((newTier method ==) . Just . oAuthScopeTier) newScopes) + Set.fromList (filter (hasTierFor method newScopes) (Just base, Nothing) -> -- The deprecated attribute gives the base; the tier comes from the method. maybe Set.empty Set.singleton (oldScopeBase base) @@ -180,8 +189,12 @@ enforcedScopes method path = case find locationMatches nginzLocations of then before else before <> "PARAM" <> probePath (T.drop 1 (T.dropWhile (/= '}') rest)) - -- Which tier is strictly required for which verb? 'Nothing' for the verbs - -- nginz has no rule for; nothing gets those past an oauth_scope directive. + hasTierFor :: Text -> OAuthScope -> Bool + hasTierFor method scope = case newTier method of + Nothing _ -> False + Just tier -> T.decodeUtf8 (toByteString' tier <> ":") + `T.isPrefixOf` T.decodeUtf8 (toByteString' scope) + newTier :: Text -> Maybe OAuthTier newTier = \case "GET" -> Just Read From 4cf26efc816b2b446a645204ec26bd109de431d9 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 23 Sep 2026 13:38:03 +0200 Subject: [PATCH 25/29] Postpone un-urgent TODOs to later PR. --- libs/wire-api/src/Wire/API/OAuth.hs | 4 ---- services/brig/test/integration/API/OAuth.hs | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/libs/wire-api/src/Wire/API/OAuth.hs b/libs/wire-api/src/Wire/API/OAuth.hs index 7b78b705b8..2bfec46bd7 100644 --- a/libs/wire-api/src/Wire/API/OAuth.hs +++ b/libs/wire-api/src/Wire/API/OAuth.hs @@ -216,10 +216,6 @@ data OAuthScope deriving (Eq, Show, Generic, Ord, Bounded, Enum) deriving (Arbitrary) via (GenericUniform OAuthScope) --- TODO: copy old values.yaml and new values.yaml to tests, and run test 3 times. it's fast. - --- TODO: bump wire-docs - -- TODO: error when requesting non-existent scopes should show list of legit scopes in message field. class IsOAuthScope scope where diff --git a/services/brig/test/integration/API/OAuth.hs b/services/brig/test/integration/API/OAuth.hs index 0a988832da..97add951ca 100644 --- a/services/brig/test/integration/API/OAuth.hs +++ b/services/brig/test/integration/API/OAuth.hs @@ -207,7 +207,7 @@ testCreateAccessTokenSuccess opts brig = do now <- liftIO getCurrentTime user <- createUser "alice" brig let redirectUrl = mkUrl "https://example.com" - let scopes = OAuthScopes $ Set.singleton (ReadSelf) + let scopes = OAuthScopes $ Set.singleton ReadSelf (cid, code) <- generateOAuthClientAndAuthorizationCode brig (User.userId user) scopes redirectUrl let accessTokenRequest = OAuthAccessTokenRequest OAuthGrantTypeAuthorizationCode cid verifier code redirectUrl resp <- createOAuthAccessToken brig accessTokenRequest From 22daa57985d55b670bce5d6dbb03c338eb350b05 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 23 Sep 2026 13:47:23 +0200 Subject: [PATCH 26/29] Polish oauthscopes test. --- .../test/unit/Test/Wire/API/Routes/OAuthScopes.hs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs index 8e16b336cf..2161269570 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs @@ -163,7 +163,7 @@ enforcedScopes method path = case find locationMatches nginzLocations of Just loc -> case (loc.locOldScope, loc.locNewScopes) of (_, Just newScopes) -> -- Filter scopes listed in values.yaml by matching method/tier. - Set.fromList (filter (hasTierFor method newScopes) + Set.fromList (filter hasTierFor newScopes) (Just base, Nothing) -> -- The deprecated attribute gives the base; the tier comes from the method. maybe Set.empty Set.singleton (oldScopeBase base) @@ -189,11 +189,12 @@ enforcedScopes method path = case find locationMatches nginzLocations of then before else before <> "PARAM" <> probePath (T.drop 1 (T.dropWhile (/= '}') rest)) - hasTierFor :: Text -> OAuthScope -> Bool - hasTierFor method scope = case newTier method of - Nothing _ -> False - Just tier -> T.decodeUtf8 (toByteString' tier <> ":") - `T.isPrefixOf` T.decodeUtf8 (toByteString' scope) + hasTierFor :: OAuthScope -> Bool + hasTierFor scope = case newTier method of + Nothing -> False + Just tier -> + T.decodeUtf8 (toByteString' tier <> ":") + `T.isPrefixOf` T.decodeUtf8 (toByteString' scope) newTier :: Text -> Maybe OAuthTier newTier = \case From 7d3d6def5a8d4d006cc1e071841ef7f97a8a9753 Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 23 Sep 2026 15:06:49 +0200 Subject: [PATCH 27/29] Add valid oauth scope to parse error message. --- integration/test/Test/OAuth.hs | 7 ++++++- libs/wire-api/src/Wire/API/OAuth.hs | 11 +++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/integration/test/Test/OAuth.hs b/integration/test/Test/OAuth.hs index 62d3ece2ff..e5cae68aeb 100644 --- a/integration/test/Test/OAuth.hs +++ b/integration/test/Test/OAuth.hs @@ -180,7 +180,12 @@ testOAuthRejectUnusefulTokenRequests = do cid <- oauthClient user generateOAuthAuthorizationCode user cid [] redirectUri >>= assertStatus 400 - generateOAuthAuthorizationCode user cid ["pizza"] redirectUri >>= assertStatus 400 + -- the 400 has to say which scopes would have worked + bindResponse (generateOAuthAuthorizationCode user cid ["pizza"] redirectUri) $ \resp -> do + resp.status `shouldMatchInt` 400 + msg <- resp.json %. "message" & asString + msg `shouldContainString` "read:self" + msg `shouldContainString` "write-only:conversations" generateOAuthAuthorizationCode user cid ["delete-only:conversations_code"] redirectUri >>= assertStatus 400 testOAuthNewScopesOnDeprecatedAttribute :: (HasCallStack) => App () diff --git a/libs/wire-api/src/Wire/API/OAuth.hs b/libs/wire-api/src/Wire/API/OAuth.hs index 2bfec46bd7..e42effe91f 100644 --- a/libs/wire-api/src/Wire/API/OAuth.hs +++ b/libs/wire-api/src/Wire/API/OAuth.hs @@ -216,8 +216,6 @@ data OAuthScope deriving (Eq, Show, Generic, Ord, Bounded, Enum) deriving (Arbitrary) via (GenericUniform OAuthScope) --- TODO: error when requesting non-existent scopes should show list of legit scopes in message field. - class IsOAuthScope scope where toOAuthScope :: OAuthScope @@ -287,13 +285,18 @@ instance ToSchema OAuthScopes where oauthScopeParser :: Text -> A.Parser (Set OAuthScope) oauthScopeParser scope = do let ws = T.words scope - when (null ws) $ fail "empty scope" + when (null ws) $ fail ("empty scope; " <> validScopes) Set.fromList <$> mapM parseScope ws parseScope :: Text -> A.Parser OAuthScope parseScope s = (fromByteString' . fromStrict . TE.encodeUtf8) s - & maybe (fail ("invalid scope: " <> show s)) pure + & maybe (fail ("invalid scope: " <> show s <> "; " <> validScopes)) pure + + validScopes :: String + validScopes = + "valid scopes are: " + <> T.unpack (oauthScopesToText (Set.fromList [minBound ..])) -- | A scope as it can appear in the database, in terms of the scopes we have -- now. From bcbe8896b4e17e43d7665cdeac11545b60aed4cd Mon Sep 17 00:00:00 2001 From: Matthias Fischmann Date: Wed, 23 Sep 2026 15:11:44 +0200 Subject: [PATCH 28/29] Polish haddocks; remove outdated TODO. --- .../unit/Test/Wire/API/Routes/OAuthScopes.hs | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs index 2161269570..f54d872caa 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs @@ -19,8 +19,6 @@ module Test.Wire.API.Routes.OAuthScopes (tests) where --- TODO: test backwards compatibility (copy old values.yaml to wire-api tests and run them against the same swagger. - import Data.Aeson qualified as A import Data.Aeson.Key qualified as Key import Data.Aeson.KeyMap qualified as KeyMap @@ -74,7 +72,7 @@ testScopesAgree = do "", "Columns: version, method, path, accepted by nginz, documented in swagger.", "The nginz column lists every scope that gets an OAuth token through to that", - "verb; '[]' means none does, i.e. OAuth is not usable there at all (the route", + "method; '[]' means none does, i.e. OAuth is not usable there at all (the route", "may still be reachable with a zauth cookie or token). A finding means", "swagger.json does not match values.yaml:", "", @@ -132,7 +130,7 @@ newtype NginzLocations = NginzLocations [Location] data Location = Location { locPattern :: Text, - locOldScope :: Maybe Text, -- only the base, e.g. "conversations_code": no tier without the verb. + locOldScope :: Maybe Text, -- only the base, e.g. "conversations_code": no tier without knowing the method. locNewScopes :: Maybe [OAuthScope] } @@ -140,23 +138,18 @@ data Location = Location -- answer is always given in new scopes, also where values.yaml still uses old -- ones. -- --- If the matching location has @oauth_scopes@, the answer is those of the --- listed scopes that have the tier this verb needs. --- --- TODO: the following paragraph is less than clear, rephrase! +-- If the matching location has @oauth_scopes@, the answer is the +-- listed scopes filtered by the tier corresponding to the HTTP +-- method. -- -- If it only has the deprecated @oauth_scope@, the answer is the one scope made --- of that base and the tier this verb needs: under @oauth_scope: +-- of that base and the tier this method needs: under @oauth_scope: -- conversations_code@, a @GET@ wants @read:conversations_code@ and nothing --- else. Old scopes are cumulative, so a token carrying --- @write:conversations_code@ passes that @GET@ as well, but nginz reads that off --- the token rather than off the configuration (@granted_scopes@ in --- @libs/libzauth/libzauth/src/oauth.rs@), and it does not change which scope the --- docs should name. +-- else. -- -- NB: an empty answer means no OAuth token gets in at all. That happens if -- the location has no @oauth_scope[s]@, if its @oauth_scopes@ list has nothing --- of the tier the verb needs, or if the verb is one nginz has no rule for. +-- of the tier the method needs, or if the method is one nginz has no rule for. enforcedScopes :: Text -> Text -> Set OAuthScope enforcedScopes method path = case find locationMatches nginzLocations of Nothing -> Set.empty From 6f238cc78cc0e47c682ca9160c26237bcb15b33f Mon Sep 17 00:00:00 2001 From: Leif Battermann Date: Thu, 24 Sep 2026 17:02:24 +0200 Subject: [PATCH 29/29] validate legacy oauth scope bases in nginz config --- .../unit/Test/Wire/API/Routes/OAuthScopes.hs | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs index f54d872caa..eec23324ec 100644 --- a/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs +++ b/libs/wire-api/test/unit/Test/Wire/API/Routes/OAuthScopes.hs @@ -44,7 +44,8 @@ tests :: TestTree tests = testGroup "OAuth scopes (charts/nginz/values.yaml vs. swagger docs)" - [ testCase "enforced scopes and documented scopes agree" testScopesAgree, + [ testCase "legacy scope names are valid" testLegacyScopeNamesAreReal, + testCase "enforced scopes and documented scopes agree" testScopesAgree, testCase "nginz path patterns avoid PCRE-only constructs" testPatternVocabulary ] @@ -106,6 +107,26 @@ testPatternVocabulary = <> "', which nginx reads as PCRE but this test matches with regex-tdfa, " <> "i.e. POSIX ERE. The two may disagree, which would be bad." +-- | A misspelled legacy base would make 'oldScopeBase' return 'Nothing'. +-- That would silently close the route to OAuth, and would not necessarily be +-- caught by the Swagger comparison if the route has no matching annotation. +testLegacyScopeNamesAreReal :: Assertion +testLegacyScopeNamesAreReal = + for_ nginzLocations $ \loc -> + for_ loc.locOldScope $ \base -> + unless (base `Set.member` supportedScopeBases) $ + assertFailure . T.unpack $ + "charts/nginz/values.yaml: unknown legacy oauth_scope base: " <> base + where + supportedScopeBases :: Set Text + supportedScopeBases = + Set.fromList + [ base + | scope <- [(minBound :: OAuthScope) ..], + let (_, baseWithSeparator) = T.breakOn ":" (T.decodeUtf8 (toByteString' scope)), + let base = T.drop 1 baseWithSeparator + ] + -------------------------------------------------------------------------------- -- what nginz enforces