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

Add onAllow callback support #20

Merged
merged 1 commit into from
Apr 21, 2021
Merged

Conversation

kibertoad
Copy link
Contributor

We intend to use this callback for audit purposes. We already can log failures using onDeny, but successful access is tricky currently.

@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2021

Codecov Report

Merging #20 (9888395) into master (79eedbf) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #20   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           37        39    +2     
=========================================
+ Hits            37        39    +2     
Impacted Files Coverage Δ
plugin.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79eedbf...9888395. Read the comment docs.

return await options.onDeny(reply, { sub, dom, obj, act })
}

if (options.onAllow) {
Copy link
Contributor Author

@kibertoad kibertoad Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that executing this conditionally should be faster for noop cases than invoking an empty function always, but I'm happy to change to always executing an empty function by default if you prefer that approach.

Copy link
Member

@simoneb simoneb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@simoneb simoneb merged commit 026195d into nearform:master Apr 21, 2021
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.

3 participants