Refactor and new features #25
Conversation
samedii
force-pushed
the
interface
branch
5 times, most recently
from
ec72af7 to
392eccb
Compare
|
Hey @samedii. Your code is good. It's better than what I wrote in most ways. But you have a 600+ line PR in repo whose source code was a third that size. And your changes are breaking the modules API. So I have to ask, why use this package at all? Why not just publish your own? EDIT: It probably is worth getting into this library so there are not two options floating around. We'll just have to cut a 1.0.0 release for all the breaking changes. The smaller we can keep the PRs the easier it's going to be for me to be able to make time to review them. |
|
Yes if you don't want to merge this then I will have to publish another. There are already a lot of alternative packages on pypi (that don't have working docs) Breaking this up into many small PRs will be extremely time consuming. I'll probably temporarily publish and then you can have a look at the total. If you want to merge then we can do cleanup/rewrites however you prefer |
|
Well, let me know when the branch is ready for review and I'll go over it when I can. I'm not at to ReadTheDocs btw if you've got a preference for something else. |
|
I'll probably make minor cleanups and doc changes but it's largely done now. Do you have a use case for having another audience than client id? This is from the specs:
I removed |
There was a problem hiding this comment.
It looks really good @samedii! I left some comments but I hope they serve as a conversation starter more than explicit change requests. They fall into a couple of broad categories:
- Subclassing FastAPI errors.
- Type annotations (can we make them dynamic? Could be tricky).
- Security vs. Depends.
- Authorship and ownership. I hope you'll consider adding your name to the authors and becoming a maintainer.
Thanks for your patience while I got to this.
|
|
||
| except (ExpiredSignatureError, JWTError, JWTClaimsError) as err: | ||
| raise HTTPException(status_code=401, detail=f"Unauthorized: {err}") |
There was a problem hiding this comment.
What do you think about subclassing HTTPException? It would make it easier and more explicit to catch exceptions from this library.
There was a problem hiding this comment.
Sounds like a good idea
There was a problem hiding this comment.
Can you have a look at this since you seem to know what you would like to have? @HarryMWinters
|
I tried to get everything in here before publishing |
|
@samedii I think there's a few unresolved comments but nothing that should take very long. I'm happy to take on some of the work or I'm sure you could wrap it up quickly if you have time. Once this is merged I'm going to play around a bit with the tagging workflow but I imagine we should be able to publish quickly. Anyways, let me know what you think. Cheers |
|
Bump @samedii Are you still interested in merging this? Or would it be better if I took it over? |
|
Sorry I've had too many other things that are higher priority since we are using the other published repo at the moment and it's working. It would be nicer to migrate to fastapi-oidc and have a common codebase instead The merge is a bit messy since master is another 5 commits ahead. I think maybe taking a look at one small thing at a time might be the most viable. |
idtoken_model was removed as an argument as it is enforced by pydantic with type hinting in the depends statement BREAKING CHANGE
base_authorization_server_uri is never used on its own and openid_connect_url is already used by fastapi and says more about what is going on BREAKING CHANGE
chose between schemes, optional auth, reintroduce idtoken_model, define scopes, and verify scope BREAKING CHANGE
|
Only have a few commits left to cherry pick from. Will finish it tonight. |
|
Hi @HarryMWinters and @samedii, I'm using fastapi-third-party-auth but I'm pretty much happy to switch to Also, one additional question: is the |
auth.requiredandauth.optionalSecurityinstead ofDepends