-
Notifications
You must be signed in to change notification settings - Fork 77
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
refactor(jans-cedarling): handle recoverable errors gracefully in authorization #10721
Draft
rmarinn
wants to merge
9
commits into
main
Choose a base branch
from
jans-cedarling-10589
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
9e4eee7
refactor(jans-cedarling): error message when creatig workload and user
rmarinn 369fd1a
feat(jans-cedarling): gracefully handle most authorize errors
rmarinn e37b740
refactor(jans-cedarling): move logging authz result to a separate fun…
rmarinn 6dac85a
refactor(jans-cedarling): restore workload & user in AuthorizeResult
rmarinn d46ad36
refactor(jans-cedarling): remove input errors from AuthorizeError
rmarinn 02c127a
refactor(jans-cedarling): update python bindings with new implementation
rmarinn 4df20d6
refactor(jans-cedarling): update wasm bindings with new implementation
rmarinn 3eae8dc
Merge branch 'main' into jans-cedarling-10589
rmarinn 7599ac7
chore(jans-cedarling): fix typo
rmarinn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 think It should be
Vec
because it can be more that onerecoverable errors
.And in the issue is said that for
"decision": "DENY"
will be key with"errors"
and for"decision": "DENY"
will be key with"warnings"
.It is planning?
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 return immediately instead of going through the request the in the first recoverable error since there's no point continuing if for example the access token is malformed. In that context, is does
Vec
still make sense since i think it will always only have 1 item.Yeah, i just changed it from
"warnings"
to"reason_input"
after looking at authzen. It looks like they prefer using"reason_*"
for adding additional context.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.
Hmm, the task
handle recoverable errors gracefully in authorization
. As I understand it means that if we have error on buildrole
(for example) or other entity, and we can continue to execute authorize request we should continue. And this kind of errors should be collected in the vector (and be logged).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.
@nynymike what do you think?
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 agree with your statement, but I think the scope of the issue was beyond authz. I don't want the Cedarling to crash ever... it should keep running. Your web server never crashes... A crash should only happen if something really catastrophic happens. Right now it seems like there are too many reasons the PDP might just crash.
That was my goal here...
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.
@SafinWasi does
flask-sidecar
crash on bad input? Do you usetry ... expect ...
statement like here?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.
The sidecar does not crash on invalid input. It can catch cedarling exceptions and puts them in the response.
jans/jans-cedarling/flask-sidecar/main/base/cedarling/cedarling.py
Line 121 in 2c0cc62