Skip to content

Conversation

@loganbertram
Copy link
Contributor

@loganbertram loganbertram commented Oct 7, 2024

JIRA Ticket:
BB2-3321

What Does This PR Do?

Adds /revoke endpoint which behaves and is named in accordance with expectations set by the OAuth standard.

What Should Reviewers Watch For?

Play around in postman to make sure DAGs and access tokens are deleted as expected.

If you're reviewing this PR, please check for these things in particular:

Validation

The unit test provides pretty explicit testing, but Postman should be used to validate manually. The test illustrates how this should be done.

What Security Implications Does This PR Have?

Please indicate if this PR does any of the following:

  • Adds any new software dependencies
  • Modifies any security controls
  • Adds new transmission or storage of data
  • Any other changes that could possibly affect security?
  • Yes, one or more of the above security implications apply. This PR must not be merged without the ISSO or team
    security engineer's approval.

Any Migrations?

  • Yes, there are migrations
    • The migrations should be run PRIOR to the code being deployed
    • The migrations should be run AFTER the code is deployed
    • There is a more complicated migration plan (downtime,
      etc)
  • No migrations

@loganbertram loganbertram marked this pull request as ready for review October 8, 2024 15:22
Copy link
Contributor

@JFU-NAVA-PBC JFU-NAVA-PBC left a comment

Choose a reason for hiding this comment

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

testing it out locally, when using curl to revoke:

curl -X POST \
--url 'http://localhost:8000/v1/o/revoke/'
--header 'content-type: application/json'
--data '{ "client_id": "TsRcxeqRy9zMHXOq3AI0Fsk148YeA4vaQDo9tK7E", "client_secret": "5BIFRcq4saiXqRv89UC5XU94JZKMmjVF02T4IrMG5O94ItVOqZfIO23OAZUjo0SC7UnjytZ0eHrjnsMWKcAlgGfUDzCZ8r2ys5UKBIdLZgwAYRmBjIICTUUPH74lGAMt", "token": "gPKWDDGq9Xh4Ikr3zM2CwNzizUeIpj" }'

(no worries about the creds - it's all on local server)

It throws up error "oauth2_provider.models.AccessToken.DoesNotExist: AccessToken matching query does not exist" on below line - due to that code expect post data in POST but for a json POST, it is as the body content:

`class RevokeView(DotRevokeTokenView):

@method_decorator(sensitive_post_parameters("password"))
def post(self, request, *args, **kwargs):
    try:
        app = validate_app_is_active(request)
    except (InvalidClientError, InvalidGrantError) as error:
        return json_response_from_oauth2_error(error)

    token = get_access_token_model().objects.get(
        token=request.POST.get("token")) <====== the POST is empty
    try:
        dag = DataAccessGrant.objects.get(
            beneficiary=token.user,
            application=app
        )
        dag.delete()
    except DataAccessGrant.DoesNotExist:
        pass

    return super().post(request, args, kwargs)`

I know that the test code works where the post call with data={k:v, k:v, ...} got parsed into request.POST..., but not from curl.....

@loganbertram
Copy link
Contributor Author

testing it out locally, when using curl to revoke:

Odd. No issues in Postman. The only caveat I'd make is that you should have a token that you're using for making test requests and a separate token that's the target of revocation to avoid issues and it was previously configured to accept form data while you may be generating raw data with curl. I added some guardrails for the way that requests are made/tested, so this is good to test again now, but give me a bit to consolidate the smart-auth updates into this ticket so most of what I need done is in one PR. I'll ping you when that's all in for both tickets.

@JFU-NAVA-PBC
Copy link
Contributor

JFU-NAVA-PBC commented Oct 8, 2024

testing it out locally, when using curl to revoke:

Odd. No issues in Postman. The only caveat I'd make is that you should have a token that you're using for making test requests and a separate token that's the target of revocation to avoid issues and it was previously configured to accept form data while you may be generating raw data with curl. I added some guardrails for the way that requests are made/tested, so this is good to test again now, but give me a bit to consolidate the smart-auth updates into this ticket so most of what I need done is in one PR. I'll ping you when that's all in for both tickets.

the token exists, the issue is that the code expecting the json data in request.POST["token"], but if the POST is from a curl command as shown, the request.POST is empty, so the exception....

image

the endpoint logic expects that the data: creds & token are POST'd as content type of x-www-form-urlencoded, below curl worked (token and DAG removed and archived)

curl -X POST --url 'http://localhost:8000/v1/o/revoke/' --header 'content-type: application/x-www-form-urlencoded' --data 'client_id=dBTo4mDCWZN5eWTaW96e3BLU1wvJKv8lEmjvo0TU&client_secret=1nCeMmdZY3Vytt7YvJ1NMbaHvGy1xQOqyGSaagT7j3FvDL0kbuFA6lUoTAjbXZisa2EYiVqtjwkUgTmbYT1Xt5xsOQI9gn5BByim354QS7ZECvlQspcqZkWNftJvQZgZ&token=jDIm3KBx6qBuO3aQQG2mzS21hxIPP0'

Since CURL command with content type : application + json with --data '{"k1": "v1", "k2": "v2", ......, "kn": "vn"}' are frequently used by sites e.g. Auth0 by Okta:

Revoke token

image

It's good to support both data formats.....

loganbertram and others added 2 commits October 9, 2024 09:43
…pting

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@JFU-NAVA-PBC JFU-NAVA-PBC self-requested a review October 9, 2024 14:41
Copy link
Contributor

@JFU-NAVA-PBC JFU-NAVA-PBC left a comment

Choose a reason for hiding this comment

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

when DAG lookup with no match, the error response function throw up at the location shown:

image

the error stack:

web-1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ web-1 | File "/tmp/venv/lib/python3.11/site-packages/django/views/decorators/debug.py", line 92, in sensitive_post_parameters_wrapper web-1 | return view(request, *args, **kwargs) web-1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ web-1 | File "/code/apps/dot_ext/views/authorization.py", line 388, in post web-1 | return json_response_from_oauth2_error(error) web-1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ web-1 | File "/code/apps/dot_ext/utils.py", line 183, in json_response_from_oauth2_error web-1 | ret_data = {"status_code": error.status_code, "error": error.error} web-1 | ^^^^^^^^^^^^^^^^^ web-1 | AttributeError: 'DoesNotExist' object has no attribute 'status_code'

@JFU-NAVA-PBC
Copy link
Contributor

JFU-NAVA-PBC commented Oct 9, 2024

for curl command using json data, the app returned from validate_app_is_active is none, which caused the DAG lookup throw DoesNotExists error...

Revokeview.post:
.........
at_model = get_access_token_model()
try:
app = validate_app_is_active(request) <==== return None
except (InvalidClientError, InvalidGrantError) as error:
return json_response_from_oauth2_error(error)

    try:
        tkn = json.loads(request.body.decode("UTF-8")).get("token")
    except Exception:

............

the app validate & lookup logic expects client_id to be extracted from Basic authentication header, if the EP needs authentication, then might want to enforce permission checking (the Basic b64encoded creds), and ideally, also do scope check of "Token Management" so that BB2 admin could control the app's revoking DAG/token stuff....

the permission check happens before the find & delete logic and response 403 if the auth header is missing.

And the curl with auth header looks like:

curl --request POST --basic -u 9PnOENT8xoNotAlU83XBSWEft7rjcSIbVw4Y5quO:chMCizGhESFqpkaipwxsQ8NVo9pFUqS1lGQey7eH2XAEXnSOeXXCC7vkriQAbv51sX8v1REIewrKgSAI4ZszIyl3syTQuSikfqAaStVHRQVatSU6l5dlMWpl3Csnaph6 --url 'http://localhost:8000/v1/revoke/‘ --header 'content-type: application/json' --data '{"token": "eDQqZlUyjIVE2setkHMk7ibaKHGf2s" }'

Where the -u value is b64encode(“<client_id>:<client_secret>”)

Copy link
Contributor

@bwang-icf bwang-icf left a comment

Choose a reason for hiding this comment

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

Tested and approved

Copy link
Contributor

@JFU-NAVA-PBC JFU-NAVA-PBC left a comment

Choose a reason for hiding this comment

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

LGTM

checked that RevokeView inherits DotRevokeTokenView - similar to RevokeTokenView, this guaranteed the auth and perm checking behavior as the revoke_token EP.

and also same method decorators w.r.t sensitive post params & csrf behavior.

@loganbertram loganbertram merged commit 101bffb into master Oct 29, 2024
6 checks passed
@loganbertram loganbertram deleted the loganbertram/BB2-3321-oauth-revoke-endpoint branch October 29, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants