Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[18.0][IMP] auth_jwt: allow more authorization options over aud #753

Open
wants to merge 53 commits into
base: 18.0
Choose a base branch
from

Conversation

dnplkndll
Copy link

@dnplkndll dnplkndll commented Jan 17, 2025

@kobros-tech can you add some tests?

probably want to require one of the possible types too. so maybe a type selection [aud,scope,group] then the aud_text to use to parse the match value?

need to rebase after: #752 merges

image or image

sbidoul and others added 30 commits January 16, 2025 17:55
Because it allows validating with a list of audiences.
Make it work with pyjwt.
This method is useful for public endpoints that need
to work for anonymous user, but can be enhanced when
an authenticated user is know.

A typical use case is a "add to cart" enpoint that can
work for anonymous users, but can be enhanced by
binding the cart to a known customer when the authenticated
user is known.
The unit tests were broken for non-functional reasons (interaction with
the mock) and is easier to implement as integration test.
Distinguish errors that lead to a 401
from internal configuration errors.
Extract _parse_bearer_authorization function for easier reuse by fastapi_auth_jwt
OCA-git-bot and others added 17 commits January 16, 2025 17:55
Currently translated at 100.0% (64 of 64 strings)

Translation: server-auth-16.0/server-auth-16.0-auth_jwt
Translate-URL: https://translation.odoo-community.org/projects/server-auth-16-0/server-auth-16-0-auth_jwt/es/
Currently translated at 89.0% (57 of 64 strings)

Translation: server-auth-16.0/server-auth-16.0-auth_jwt
Translate-URL: https://translation.odoo-community.org/projects/server-auth-16-0/server-auth-16-0-auth_jwt/it/
Currently translated at 100.0% (64 of 64 strings)

Translation: server-auth-16.0/server-auth-16.0-auth_jwt
Translate-URL: https://translation.odoo-community.org/projects/server-auth-16-0/server-auth-16-0-auth_jwt/it/
Currently translated at 100.0% (64 of 64 strings)

Translation: server-auth-16.0/server-auth-16.0-auth_jwt
Translate-URL: https://translation.odoo-community.org/projects/server-auth-16-0/server-auth-16-0-auth_jwt/it/
@dnplkndll dnplkndll force-pushed the 18.0-imp-auth_jwt-drop-aud branch 4 times, most recently from b3e8a9b to 538beb2 Compare January 18, 2025 01:27
@dnplkndll dnplkndll force-pushed the 18.0-imp-auth_jwt-drop-aud branch from 538beb2 to 5da46e0 Compare January 18, 2025 01:46
@dnplkndll
Copy link
Author

@sbidoul working aws cognito as a token provider, there are no aud. but we do have scopes and user groups. would it be useful to replace the aud with these new tests to validate a server to server account has proper scope or a user has a group? the tests are pretty sloppy ( any intersection of the sets) can refine if useful. another option might be a simple disable but really do need the scope test in our case.
The next issue is we would have tokens that have sub and map user/partner via oath provider data? I guess that can be done in partner_id_strategy extension.

validation["user_id"] = validation["sub"]

@dnplkndll dnplkndll force-pushed the 18.0-imp-auth_jwt-drop-aud branch from 7342133 to 5da46e0 Compare January 18, 2025 13:04
@sbidoul
Copy link
Member

sbidoul commented Jan 18, 2025

I'm ok to make audience optional. May I suggest doing that in an independent PR to facilitate review?

Then adding validation on additional claim sounds ok too. I would not override the meaning of the audience field, though. How about an expected_claim field containing a literal dictionary (to be parsed with ast.literal_eval) or a json field.

@dnplkndll dnplkndll force-pushed the 18.0-imp-auth_jwt-drop-aud branch from fabb83e to f5e43c3 Compare January 18, 2025 14:49
@dnplkndll
Copy link
Author

optional aud only
#755

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.