Fix OpenApi output for mode follow-privileges

[wolfgangwalther]

This demonstrates a breaking change in the latest pre-release by adding a new test for openapi output of views.

Related to #2356.

[steve-chavez]

@wolfgangwalther As a workaround, if you use PGRST_OPENAPI_MODE="ignore-privileges" then the PK will be detected.

[steve-chavez]

On PGRST_OPENAPI_MODE="follow-privileges" the problem is that accessibleTables is a query that runs everytime the OpenAPI endpoint is hit. While on ignore-privileges we use the actual schema cache.

postgrest/src/PostgREST/App.hs

Lines 487 to 500 in ee906da

	handleOpenApi :: Bool -> Schema -> RequestContext -> DbHandler Wai.Response
	handleOpenApi headersOnly tSchema (RequestContext conf@AppConfig{..} dbStructure apiRequest ctxPgVersion) = do
	body <-
	lift $ case configOpenApiMode of
	OAFollowPriv ->
	OpenAPI.encode conf dbStructure
	<$> SQL.statement [tSchema] (DbStructure.accessibleTables ctxPgVersion configDbPreparedStatements)
	<*> SQL.statement tSchema (DbStructure.accessibleProcs ctxPgVersion configDbPreparedStatements)
	<*> SQL.statement tSchema (DbStructure.schemaDescription configDbPreparedStatements)
	OAIgnorePriv ->
	OpenAPI.encode conf dbStructure
	(HM.filterWithKey (\(QualifiedIdentifier sch _) _ -> sch == tSchema) $ DbStructure.dbTables dbStructure)
	(HM.filterWithKey (\(QualifiedIdentifier sch _) _ -> sch == tSchema) $ DbStructure.dbProcs dbStructure)
	<$> SQL.statement tSchema (DbStructure.schemaDescription configDbPreparedStatements)

The schema cache is augmented with addViewPrimaryKeys, which doesn't occur for accessibleTables.

postgrest/src/PostgREST/DbStructure.hs

Lines 85 to 95 in ee906da

	queryDbStructure :: [Schema] -> [Schema] -> Bool -> SQL.Transaction DbStructure
	queryDbStructure schemas extraSearchPath prepared = do
	SQL.sql "set local schema ''" -- This voids the search path. The following queries need this for getting the fully qualified name(schema.name) of every db object
	pgVer <- SQL.statement mempty pgVersionStatement
	tabs <- SQL.statement schemas $ allTables pgVer prepared
	keyDeps <- SQL.statement (schemas, extraSearchPath) $ allViewsKeyDependencies prepared
	m2oRels <- SQL.statement mempty $ allM2ORels pgVer prepared
	procs <- SQL.statement schemas $ allProcs pgVer prepared
	let tabsWViewsPks = addViewPrimaryKeys tabs keyDeps
	rels = relsToMap $ addO2MRels $ addM2MRels tabsWViewsPks $ addViewM2ORels keyDeps m2oRels

[steve-chavez]

Before #2254

Get PKcols inside tables

The above was not done and the PKCols were a separate element in the schema cache and this was used for the OpenAPI output.

The fix would be to add addViewPrimaryKeys to the accessibleTables in App.hs.

Edit: The above can't be done because addViewPrimaryKeys no longer has [ViewKeyDependency] at the App.hs module.

---

Ideally I think we'd use the schema cache for follow-privileges as well instead of doing a query every time. For this we'd have to cache the privileges of the roles that are members of authenticator(connection role) and then filter the OpenAPI output in Haskell code with the role claim of the JWT. Downside is that our schema cache would have to be reloaded for GRANT/REVOKE as well. For now this is too complicated.

Edit: This could also let us reject unauthorized requests at the postgREST level. There's some value in not touching the db when we know the user isn't able to fulfill the request(similar to #1094).

[steve-chavez]

Another alternative that seems simpler. On accessibleTables/Procs we don't query all the metadata again but just the list of tables/procs that are accessible by the role, then we filter the tables/procs from the cache with that list in Haskell code.

There would be no caching if we go this way, but that could be added later.

[wolfgangwalther]

@wolfgangwalther As a workaround, if you use PGRST_OPENAPI_MODE="ignore-privileges" then the PK will be detected.

I tried that and it seems like it works... but not quite, yet. It looks like PKs are not detected through multiple views recursively anymore. So one view selects from another view which selects a PK from a table.

Does this make sense? Is this expected, because of changes to embedding? Or should I try to put together a test-case?

[wolfgangwalther]

@wolfgangwalther As a workaround, if you use PGRST_OPENAPI_MODE="ignore-privileges" then the PK will be detected.

I tried that and it seems like it works... but not quite, yet. It looks like PKs are not detected through multiple views recursively anymore. So one view selects from another view which selects a PK from a table.

Does this make sense? Is this expected, because of changes to embedding? Or should I try to put together a test-case?

Hm. The few ones I'm still missing might also be related to multi column PKs. Will have to put a test-case together, eventually, I guess.

[steve-chavez]

Does this make sense? Is this expected, because of changes to embedding? Or should I try to put together a test-case?

Hm, no. PK detection on views recursively should work. I didn't expect to break that. A test case would be great, yes.

[wolfgangwalther]

Ideally I think we'd use the schema cache for follow-privileges as well instead of doing a query every time. For this we'd have to cache the privileges of the roles that are members of authenticator(connection role) and then filter the OpenAPI output in Haskell code with the role claim of the JWT. Downside is that our schema cache would have to be reloaded for GRANT/REVOKE as well. For now this is too complicated.

What about running a query to only fetch the current privileges each time the openapi output is requested - and then applying those privileges to the schema cache?

[wolfgangwalther]

What about running a query to only fetch the current privileges each time the openapi output is requested - and then applying those privileges to the schema cache?

I did it this way for the tables query. Doing the same for accessibleProcs would be a bit more effort, because those are not uniquely identified by schema/name alone.

@steve-chavez WDYT?

This is based on #2526, so that commit shows up here, too.

[steve-chavez]

Approach looks good! Could you make the change independent of #2526? I still don't understand that one and it might take me more time but 6b5f916 can be merged