[asterisk-dev] [Code Review] named_acls: Named ACLs - a system for creating and applying ACLs with named profiles which can be shared

[Kevin Fleming]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1978/#review6427
-----------------------------------------------------------



/trunk/CHANGES
<https://reviewboard.asterisk.org/r/1978/#comment12099>

    This is 'reload acl' now, right?



/trunk/include/asterisk/acl.h
<https://reviewboard.asterisk.org/r/1978/#comment12104>

    The function as written does not actually require the destination list to exist. Either change the function to work the way the documentation indicates, or change the documentation so that users won't unnecessarily create an empty list just to pass into the function.
    
    Also, you did not document the function's return value.



/trunk/include/asterisk/acl.h
<https://reviewboard.asterisk.org/r/1978/#comment12106>

    intact is one word, not two.



/trunk/include/asterisk/acl.h
<https://reviewboard.asterisk.org/r/1978/#comment12105>

    The requested ACL can't be 'invalid'; it could be 'not found', though, as the last sentence in this comment indicates.
    
    Also, the usage of 'it' in the first sentence is unclear, as there are two nouns in the previous part of the sentence (and 'it' doesn't actually refer to either of them in this context).
    
    Please consistently capitalize NULL (and acl, as others have pointed out already).



/trunk/include/asterisk/acl.h
<https://reviewboard.asterisk.org/r/1978/#comment12107>

    s/with anything that needs it//
    
    There is no reason to initialize it other than for things that need it :-)



/trunk/include/asterisk/event_defs.h
<https://reviewboard.asterisk.org/r/1978/#comment12100>

    This is much too generic. If you are going to use the event subsystem to notify consumers of named ACL changes, then the event should be specific to that change.
    
    I think it would be reasonable for a subscriber to also be able to subscribe to this event for a specific named ACL (by name), so that it can optimize its processing of updates to only the entries that use that named ACL.



/trunk/main/acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12101>

    'into this list'? This could be more easily described as 'If there is no destination structure to populate, just return a copy of the source'.



/trunk/main/acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12102>

    'Find the last entry in the destination list'



/trunk/main/acl.c
<https://reviewboard.asterisk.org/r/1978/#comment12103>

    Since the API call uses the verb 'duplicate', your comment should as well, although that is repetitive. A better comment would be 'Attach copies of the entries in the source list to the destination list'.



/trunk/main/asterisk.c
<https://reviewboard.asterisk.org/r/1978/#comment12108>

    This should be occur before *anything* that accepts network connections is initialized (at least, before ast_http_init()). While the HTTP subsystem does not currently support ACLs, if someone was to add support for them in the future, the resulting behavior would be non-intuitive since normal ACLs would function as expected but named ACLs would not.


- Kevin


On June 8, 2012, 2:04 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1978/
> -----------------------------------------------------------
> 
> (Updated June 8, 2012, 2:04 p.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson, Terry Wilson, and Olle E Johansson.
> 
> 
> Summary
> -------
> 
> This feature is based on oej's deluxepine (or something like that) branch with a similarly named feature.  ACLs are defined in acl.conf and can be used by pretty much anything that has ACL options permit/deny (acl='aclname').  acl= works similarly to permit= and deny= in that it simply appends to the working ACL, so they can be combined with other uses of permit/deny/acl.
> 
> Also in use in this patch are twilson's new config options.
> 
> Since named acls are duplicated when used in another configuration, configurations that use named acls need to be updated if acl.conf is reloaded. This is accomplished with a new event type and the consumption of that event is demonstrated currently only in manager.conf
> If this seems like a proper approach to this problem, that will be replicated across other consumers of named acls.
> 
> NOTE: This code is very much WIP and not intended for merging.
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 368662 
>   /trunk/channels/chan_h323.c 368662 
>   /trunk/channels/chan_iax2.c 368662 
>   /trunk/channels/chan_mgcp.c 368662 
>   /trunk/channels/chan_sip.c 368662 
>   /trunk/channels/chan_skinny.c 368662 
>   /trunk/channels/chan_unistim.c 368662 
>   /trunk/configs/acl.conf.sample PRE-CREATION 
>   /trunk/configs/iax.conf.sample 368662 
>   /trunk/configs/manager.conf.sample 368662 
>   /trunk/configs/sip.conf.sample 368662 
>   /trunk/configs/skinny.conf.sample 368662 
>   /trunk/include/asterisk/acl.h 368662 
>   /trunk/include/asterisk/event_defs.h 368662 
>   /trunk/main/acl.c 368662 
>   /trunk/main/asterisk.c 368662 
>   /trunk/main/manager.c 368662 
>   /trunk/main/named_acl.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/1978/diff
> 
> 
> Testing
> -------
> 
> Various tests for configuring and using named acls were performed, and a task for writing comprehensive testsuite tests is in the queue.  Additionally, various means of reloading the configuration have been performed, and so far they pan out aside from a bug with an unchanged acl.conf which is a generic problem against config options accidentally introduced a little while back.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120609/108e8e47/attachment-0001.htm>