Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Fix bug in client with no scopes #830

Merged
merged 11 commits into from
Jul 8, 2022
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ The types of changes are:
* Fix publish_docs CI action [#818](https://github.com/ethyca/fidesops/pull/818)
* Bump fideslib to handle base64 encoded password [#820](https://github.com/ethyca/fidesops/pull/820)
* Stop masking uvicorn logs by default [#831](https://github.com/ethyca/fidesops/pull/831)
* Fix error when there are no scopes in `ClientDetail` [#830](https://github.com/ethyca/fidesops/pull/830)

## [1.6.1](https://github.com/ethyca/fidesops/compare/1.6.0...1.6.1)

Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ fastapi-caching[redis]
fastapi-pagination[sqlalchemy]~= 0.8.3
fastapi[all]==0.78.0
fideslang==1.0.0
fideslib==2.2.1
fideslib==2.2.2
fideslog==1.2.1
multidimensional_urlencode==0.0.4
pandas==1.3.3
Expand Down
5 changes: 4 additions & 1 deletion src/fidesops/api/v1/endpoints/oauth_endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,10 @@ async def acquire_access_token(
else:
raise AuthenticationFailure(detail="Authentication Failure")

client_detail = ClientDetail.get(db, object_id=client_id, config=config)
# scopes param is only used if client is root client, otherwise we use the client's associated scopes
client_detail = ClientDetail.get(
db, object_id=client_id, config=config, scopes=SCOPE_REGISTRY
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an organizational perspective, this is confusing, in that these scopes are only used if the client is the root client, otherwise, we use their actual associated scopes, but just looking at this line, that is not necessarily obvious. I'd at least add a code comment here explaining that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, I'd love to rename scopes to root_scopes or all_scopes or something in future, though that'll require more fideslib changes. For now, let's get this working, and I'll update with a comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for adding!

)

if client_detail is None:
raise AuthenticationFailure(detail="Authentication Failure")
Expand Down
4 changes: 3 additions & 1 deletion src/fidesops/util/oauth_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from starlette.status import HTTP_404_NOT_FOUND

from fidesops.api.deps import get_db
from fidesops.api.v1.scope_registry import SCOPE_REGISTRY
from fidesops.api.v1.urn_registry import TOKEN, V1_URL_PREFIX
from fidesops.core.config import config
from fidesops.models.policy import PolicyPreWebhook
Expand Down Expand Up @@ -138,8 +139,9 @@ async def verify_oauth_client(
if not client_id:
raise AuthorizationError(detail="Not Authorized for this action")

# scopes param is only used if client is root client, otherwise we use the client's associated scopes
client = ClientDetail.get(
db, object_id=client_id, config=config, scopes=security_scopes.scopes
db, object_id=client_id, config=config, scopes=SCOPE_REGISTRY
Comment on lines -142 to +144
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there already a ticket to cut over to using the fideslib version?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean fideslib scops? We weren't able to put the scopes in fideslib and had to do it this way because the different libraries have different scopes.

Copy link
Contributor

@pattisdr pattisdr Jul 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant using the fideslib verify_oauth_client!

)

if not client:
Expand Down
22 changes: 22 additions & 0 deletions tests/api/v1/endpoints/test_oauth_endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,28 @@ def test_invalid_client_secret(self, db, url, api_client):

new_client.delete(db)

def test_get_access_token_root_client(self, url, api_client):
data = {
"client_id": config.security.OAUTH_ROOT_CLIENT_ID,
"client_secret": config.security.OAUTH_ROOT_CLIENT_SECRET,
}

response = api_client.post(url, data=data)
jwt = json.loads(response.text).get("access_token")
assert 200 == response.status_code
assert (
data["client_id"]
== json.loads(extract_payload(jwt, config.security.APP_ENCRYPTION_KEY))[
JWE_PAYLOAD_CLIENT_ID
]
)
assert (
json.loads(extract_payload(jwt, config.security.APP_ENCRYPTION_KEY))[
JWE_PAYLOAD_SCOPES
]
== SCOPE_REGISTRY
)

pattisdr marked this conversation as resolved.
Show resolved Hide resolved
def test_get_access_token(self, db, url, api_client):
new_client, secret = ClientDetail.create_client_and_secret(
db,
Expand Down