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
Merged

Fix bug in client with no scopes #830

merged 11 commits into from
Jul 8, 2022

Conversation

sanders41
Copy link
Contributor

@sanders41 sanders41 commented Jul 7, 2022

Purpose

Bumps fideslib to fix bug when there are no scopes in ClientDetail

Changes

  • Bump fideslib to 2.2.2

Checklist

  • Update CHANGELOG.md file
    • Merge in main so the most recent CHANGELOG.md file is being appended to
    • Add description within the Unreleased section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.
    • Add a link to this PR at the end of the description with the PR number as the text. example: #1
  • Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
  • If docs updated (select one):
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Good unit test/integration test coverage
  • This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
  • The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Fixes #829

Comment on lines 408 to 412
assert (
json.loads(extract_payload(jwt, config.security.APP_ENCRYPTION_KEY))[
JWE_PAYLOAD_SCOPES
]
is None
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry i'm probably just missing something, why are there no scopes here? I thought the root client got all the scopes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see anywhere in the original fidesops root client where that happed. What I saw it looked like it got set to None. Maybe I missed it somewhere?

@eastandwestwind
Copy link
Contributor

eastandwestwind commented Jul 7, 2022

Pushing out what I have, but using Postman, I get 403 for POST {{host}}/oauth/client, after receiving auth token for root client.

steps:

  1. make server,
  2. POST {{host}}/oauth/token,
  3. Copy returned access_token and save it to postman env variables,
  4. hit POST {{host}}/oauth/client

I've confirmed the jwt token contains all scopes, so unsure what could be wrong.

@eastandwestwind
Copy link
Contributor

eastandwestwind commented Jul 8, 2022

@pattisdr this is ready for you again.

We realized that previously, we were hard-coding scopes (scopes=SCOPE_REGISTRY) to be returned for root client:

def _get_root_client_detail() -> Optional[ClientDetail]:
    root_secret = config.security.OAUTH_ROOT_CLIENT_SECRET_HASH
    assert root_secret is not None
    return ClientDetail(
        id=config.security.OAUTH_ROOT_CLIENT_ID,
        hashed_secret=root_secret[0],
        salt=root_secret[1].decode(config.security.ENCODING),
        scopes=SCOPE_REGISTRY,
    )

Currently, that code has moved to fideslib, and since fideslib has no knowledge of SCOPE_REGISTRY, we have to explicitly pass in SCOPE_REGISTRY whenever we call ClientDetail.get()

Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

Looks great, just looking for a comment to add clarity.

@@ -76,7 +76,9 @@ async def acquire_access_token(
else:
raise AuthenticationFailure(detail="Authentication Failure")

client_detail = ClientDetail.get(db, object_id=client_id, config=config)
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!

Comment on lines -142 to +143
db, object_id=client_id, config=config, scopes=security_scopes.scopes
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.

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!

@pattisdr pattisdr merged commit 2ef3cf1 into main Jul 8, 2022
@pattisdr pattisdr deleted the client-detail branch July 8, 2022 16:43
sanders41 added a commit that referenced this pull request Sep 22, 2022
Co-authored-by: Paul Sanders <[email protected]>
Co-authored-by: eastandwestwind <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

500 error code for POST /oauth/token
3 participants