-
Notifications
You must be signed in to change notification settings - Fork 67
Add permissions check in josh-filter #503
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
src/filter/mod.rs
Outdated
return warnings; | ||
} | ||
|
||
fn make_chain(ops: &[Op]) -> Op { |
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 would be less code to use the function in line 686 and split the chaining over multiple lines rather than introducing a new helper.
src/bin/josh-filter.rs
Outdated
&transaction, | ||
josh::filter::parse(&i)?, | ||
&[(input_ref.to_string(), "refs/JOSH_TMP".to_string())], | ||
josh::filter::nop(), |
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 result of the permissions filter will be the paths that have missing permissions. So it should probably be josh::filter::empty() and not josh::filter::nop().
src/bin/josh-filter.rs
Outdated
require_literal_leading_dot: true, | ||
let check_permissions = args.is_present("check-permission"); | ||
let whitelist = josh::filter::nop(); | ||
let blacklist = josh::filter::nop(); |
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.
blacklist should be josh::filter::empty() by default or?
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
src/lib.rs
Outdated
let oid = original_commit.id(); | ||
|
||
if permissions != filter::nop() { | ||
let perms_commit = if let Some(s) = transaction.get_ref(permissions, oid) { |
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.
Not sure why this is needed 🤔
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.
My idea was to use the nop() filter as a hint that we are not checking permissions. Thus the nop() above also
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.
But actually you're right, if I just give empty() then it's easier
3cf0da0
to
3b7edf6
Compare
tests/filter/permissions.t
Outdated
|
||
# default same as this | ||
$ josh-filter -s :/ master --check-permission -b :empty -w :nop --update refs/josh/filtered | ||
Warning: reference refs/josh/filtered wasn't updated |
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.
oops...
3feceac
to
d262f64
Compare
5fd93fe
to
0401ee1
Compare
No description provided.