notion-sdk-py
notion-sdk-py copied to clipboard
ramnes
Add OAuth revoke and introspect endpoints
OAuth revoke and introspect endpoints got added to the JS SDK: https://github.com/makenotion/notion-sdk-js/pull/552
Let's add them here as well. :)
I'd love to work on this! Are there any additional requirements I should be aware of?
Go ahead!
The only requirements are that the DX/library API be identical to the JS SDK and that our Python code remain consistent with what we already have. :)
Great, thanks :-) It looks like the tests I would write on this would be dependent on the access token's being generated. Is there anyway to generate these without a public integration?
I don't think so, but we only need to burn one token to register the VCR.py cassette. Subsequent tests won't revoke the token as long as this cassette is being used, meaning that we only need to generate a token manually when we update this cassette in particular.
Sorry if I am not understanding 😅 ! From my understanding of the Read me "To create new tests or run them without cassettes, you need to set up the environment variables NOTION_TOKEN and NOTION_TEST_PAGE_ID", this means I would have to generate my own token to develop any additional tests. The pytest-vcr docs are a bit minimal, so I am not totally sure how I would add new tests in the cassette.
Yes, you need to create an integration so that you can generate a new token manually and burn it while generating the cassette for this test.
I have the endpoints working with tests for each endpoint, using the method of burning a token to generate the cassette for the test. I did notice that in the client.py, the authorization is in the form Bearer {auth}, but from the docs, the authorization looks to be Basic '"$BASE64_ENCODED_ID_AND_SECRET"'. Will this be a problem for users?
I can push any changes for now if that would help :-)
Oh, good catch! It looks like we missed this commit from two years ago: https://github.com/makenotion/notion-sdk-js/commit/0877d3478c680bf6b5860cfc21e7ab012084aad4
We should implement this "Get token" endpoint as well, and add the same changes for authorization, i.e. allow either the token or the client id and secret combo, which should be only usable for the OAuth-related endpoints, as in the JS SDK.
We can handle this in a separate issue and PR, or address everything at once, whichever you prefer.
Also, if we implement the client id and secret authorization as well as the "Get token" endpoint, we should update the tests so that they run entirely with these environment variables and create their own token, rather than requiring the user to generate a token manually that will end up being revoked. :)
We can handle that all here if that works for you!
I think we still will have to burn a token because I had to intercept the code from the 2nd step after visiting the authorization URL for my public integration. Ie, after visiting that auth URL, and being redirected to the redirect URI, I had to grab the code from here http://localhost:3000/callback?code=<GENERATED_CODE>&state=. I could then use that code to generate a token via a Curl or Postman request, then introspect and revoke in the SDK. I think if we implement the create token endpoint from the docs, a public integration would have to be set up and a code would have to be generated. If I am misunderstanding this, or you have a more streamlined approach, please let me know :-)!
Also from my testing, it looks like a the token can be "revoked" multiple times. Even though it isn't active after the first revocation, the API still returns a 200 response for revoking that token.
Alright, let's keep it simple for now and say that we need both the token and the id / secret pair to run the tests. I'll see if we can work around this later. :)
Sorry for the long hiatus, I have been incredibly busy with school. I can finish this in the coming weeks, or someone can help out if they so desire :-)! Thanks!
Hi @ramnes! I'd like to work on this since PR #261 was closed.
I've been looking through the JS SDK and the discussion on the previous PR. From what I can see, this needs three OAuth endpoints: token for exchanging authorization codes, introspect for checking token status, and revoke for revoking tokens.They should use Basic auth with the client_id and client_secret base64 encoded.
I also saw your comment on PR #595 about adding refresh_token to the token endpoint, so I'll make sure that's included along with the other parameters.
For the tests, I noticed you suggested using pytest-mock instead of VCR cassettes since the OAuth flow is pretty cumbersome with the browser and manual code grabbing. Should I go with mocked tests for all three OAuth endpoints?
Let me know if this approach sounds good and I'll get started on it!
Hi @ramnes! I'd like to work on this since PR #261 was closed.
I've been looking through the JS SDK and the discussion on the previous PR. From what I can see, this needs three OAuth endpoints: token for exchanging authorization codes, introspect for checking token status, and revoke for revoking tokens.They should use Basic auth with the client_id and client_secret base64 encoded.
I also saw your comment on PR #595 about adding refresh_token to the token endpoint, so I'll make sure that's included along with the other parameters.
For the tests, I noticed you suggested using pytest-mock instead of VCR cassettes since the OAuth flow is pretty cumbersome with the browser and manual code grabbing. Should I go with mocked tests for all three OAuth endpoints?
Let me know if this approach sounds good and I'll get started on it!
Hi @Sumeet213 , I implemented the authorization and revoke, but never had the chance to do any of the future work because of school obligations. If you could build off the endpoints I already made that would be awesome. If @ramnes doesn't want this, or you would rather start from scratch, no worries :-)
Hey @Sumeet213, awesome!
For the tests, I noticed you suggested using pytest-mock instead of VCR cassettes since the OAuth flow is pretty cumbersome with the browser and manual code grabbing. Should I go with mocked tests for all three OAuth endpoints?
I think so! Unless Notion made it a easier to work with this without a browser in the meantime. But just to be clear, I really want the as little mocks as possible; we should still test as much as possible against the real system with cassettes.