feat(stac): policy enforcement point proof of concept#569
feat(stac): policy enforcement point proof of concept#569
Conversation
Co-authored-by: sandrahoang686 <sandrahoang686@gmail.com>
| return None | ||
|
|
||
|
|
||
| async def extract_ingest_resource_id(request: Request) -> Optional[str]: |
There was a problem hiding this comment.
I should have asked this in the resource extractor PR, sorry...do we need to consider the ingest api /ingestions/* endpoints POST, PATCH and DELETE?
There was a problem hiding this comment.
No worries @smohiudd. Yes, eventually to cover the ingestions endpoints but not all of them. We need to cover POST, but I think PATCH only updates the status and message right?
ividito
left a comment
There was a problem hiding this comment.
A couple questions and very minor suggestions. It all works as intended AFAIK. Ran most tests locally and also tried a few requests to SIT.
| See https://www.keycloak.org/docs/latest/authorization_services/#_service_rpt_overview | ||
| """ | ||
| for permission in permissions: | ||
| rsname = permission.get("rsname") or permission.get("resource_id") |
There was a problem hiding this comment.
I'm unclear on why rsid isn't relevant to check anymore
There was a problem hiding this comment.
Yeah that's a good question! Basically we're not using rsid because the RPT puts the name in rsname (e.g. "stac:item:faketenant2:*") and the UUID in rsid (and sometimes in resource_id). You can check this yourself by requesting an RPT and introspecting it but basically it will show something like
{
"scopes": [
"read"
],
"rsid": "some-uuid",
"rsname": "stac:item:faketenant2:*",
"resource_id": "some-uuid",
"resource_scopes": [
"read"
]
}
We need the name to match and to parse type/tenant, so we use rsname (and resource_id when it’s the name). rsid only ever gives us the UUID which we don't use. I guess really we don't need resource_id but I had that as a fallback because I wasn't sure where keycloak would put the resource name.
| "Access token is expired or invalid. Please re-authenticate." | ||
| ) from error | ||
| if error.response.status_code == 403: | ||
| return False |
There was a problem hiding this comment.
What do you think about raising another error (PermissionDeniedError?) here and catching it in the middleware? I think the return False/raise Error pattern is a bit confusing, this way we could replace https://github.com/NASA-IMPACT/veda-backend/pull/569/changes#diff-4857746bfd90942dbb37adfab0448bb3a8c3683230e3fadf002e8e89b615445cR187-R202 with another except block
There was a problem hiding this comment.
I think your suggestion makes sense! The bool/error return is confusing, sorry about that.
2176ff9 What do you think of something like this?
stac_api/runtime/src/app.py
Outdated
| ) | ||
| from src.extension import TiTilerExtension | ||
| from stac_auth_proxy import configure_app | ||
| from veda_auth.pep_middleware import PEPMiddleware |
There was a problem hiding this comment.
nit: should we put this import in the same conditional that instantiates it?
stac_api/runtime/src/app.py
Outdated
| ) | ||
| else: | ||
| logger.info( | ||
| "PEP middleware disabled, openid_url=%s, secret_name=%s", |
There was a problem hiding this comment.
| "PEP middleware disabled, openid_url=%s, secret_name=%s", | |
| "PEP middleware disabled, openid_url=%s, secret_name=%s", |
| resource_id=resource_id, | ||
| scope=scope, | ||
| ) | ||
| except TokenError as e: |
There was a problem hiding this comment.
Appreciate the structured and comprehensive error responses!
Issue
#557
#558
What?
Testing?