-
Notifications
You must be signed in to change notification settings - Fork 13
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
Changes to make compatible with OAM #64
Conversation
djay
commented
Dec 2, 2024
•
edited
Loading
edited
- set client.allow["issuer_mismatch"] = True to make more compatible
- this is tested. decided to use for all since it shouldn't make a difference to non OAM?
- add in changes to make work with OAM
- new advanced setting for identity_domain_name which is used as a param and header which OAM needs apparently
- add header
- add extra params
- test against live OAM
Ok, this has now been tested and works against OAM |
) | ||
else: | ||
client = Client(client_authn_method=CLIENT_AUTHN_METHOD) | ||
client.allow["issuer_mismatch"] = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I noticed the same issue with the Microsoft implementation. However, in my opinion, it might be better to have this in a separate property, even though this plugin already seems to have too many configurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can put a toggle for it. It's not exactly clear what the implications are if allow mismatch. I guess its less secure? Could have a toggle that defaults mismatch allowed so it's less likely to trip up first timers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no way to test the modification, but I don't see anything that could interfere with the current implementation. So, it's fine by me. Thanks!
@mamico ideally I'd test with more than one OAM but I only have access to one :( But at least I can say that works. |
@djay do you need a pypi release ? |
@mamico no rush |