-
Notifications
You must be signed in to change notification settings - Fork 2
Enhance remote flow sources for CAP #201
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
Conversation
add test cases to cover cap event handler registrations that were previously missing add remote flow source type to cover more cases of event handler registration
javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll
Outdated
Show resolved
Hide resolved
javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/RemoteFlowSources.qll
Outdated
Show resolved
Hide resolved
javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/RemoteFlowSources.qll
Outdated
Show resolved
Hide resolved
javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/RemoteFlowSources.qll
Outdated
Show resolved
Hide resolved
javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/RemoteFlowSources.qll
Outdated
Show resolved
Hide resolved
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.
First round of review.
javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/RemoteFlowSources.qll
Outdated
Show resolved
Hide resolved
add more testcases for all entity spec scenarios add extra ability to know name in one case for ServiceinCDSHandlerParameterWithName
refactor remoteflowsources
javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDS.qll
Show resolved
Hide resolved
javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/RemoteFlowSources.qll
Outdated
Show resolved
Hide resolved
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.
Two more minor points to go!
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.
Looks good! Thanks for your work!
New approach
This PR refactors the remoteflowsource types to a simpler type that captures explicit known exposed service handlers OR general event handlers that are registered but not necessarily tied to services
This PR also enhances unit tests for the different ways that handlers can be specified
Old (deprecated) approach
this PR aims to address the following previously missing scenarios:this.on ('UPDATE', '*', req => {...})
)specifically the PR:
the current approach enumerates over service + event handler pairing cases and follows closely with the previous technique of adding remote flow sources as event handler request objects
an alternative potential approach would be to consider making
ServiceInstance
more responsible for its event handler registration (if we can assume that all/base service types may have event handlers) and then make their request object params exposed on the implementing types (and still extendRemoteFlowSource
)this approach may be more assured to cover all ways of specifying services and their handlers/looks more generic
But may not make sense if many implementing types are redundant or do not indeed allow event handler registration