-
Notifications
You must be signed in to change notification settings - Fork 663
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
Implement the security endpoints check for identification #783
base: master
Are you sure you want to change the base?
Conversation
No more need of PR #620 |
sys.path.insert(0, "..") | ||
|
||
|
||
def simple_certificate_manager(isession, certificate, signature): |
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 not looked in detail at this. But is it enough with a function? Should'nt we use a class with a few methods? just asking
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.
if it is just a function, then using the name manager is a bit wrong. A manager is an object. a function would be called something like set_certificate_validation_function or whatever
|
||
# FIXME: Get the server available UserIdentityToken policy's ids. I don't how to get it in other way. | ||
# iserver_policy_ids = ["anonymous", "certificate_basic256sha256", "username"] | ||
# Note that it is different from server._policyIDs = ["Anonymous", "Basic256Sha256", "Username"] |
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 that seems to be the correct way to get policy IDs . Just write a comment that we assume that the same polcies are used n all endpoints.. maybe add a FIXME. I am not sure what spec says but in theory it should be possible to have different policies per endpoints
|
||
# Route to the correct manager | ||
if isinstance(params_id_token, ua.AnonymousIdentityToken): | ||
pass # TODO: Call AnonymousIdentityToken Manager |
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.
we should at least check that "anonymous" is in the list of Policies and return an UaError if not
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.
Can't we use the user_manager for that? maybe just have a bool member called something like "allow_anonymous"? not sure I have not looked at that code for very long...
thanks for the patch anyway. this looks good |
@okyame what is the status? |
close #782