[apparmor] [PATCH v2] json support for tools (logprof and genprof)

[Goldwyn Rodrigues]

On 04/02/2017 08:48 AM, Christian Boltz wrote:
> Hello,
> 
> Am Mittwoch, 22. März 2017, 19:24:04 CEST schrieb Goldwyn Rodrigues:
>> From: Goldwyn Rodrigues <rgoldwyn at suse.com>
>>
>> This adds JSON support for tools in order to be able to talk to
>> other utilities such as Yast.
>>
>> The json is one per line, in order to differentiate between multiple
>> records. This is based on work presented by Christian Boltz some time
>> back.
>>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn at suse.com>
>>
>> ---
>> Changes since v1:
>>  - implementation of set_json_mode(), write_json()
>>  - Changed the way output is provided to keep input the same. This
>> would write in either of two formats: text or json, but will keep
>> input the same. This helps in keeping localizations in place. I so
>> wish UI was a class.. - Removed all yast calls.
> 
> A generic notice first: make check complained about some whitespace 
> issues. Please always run "make check" in the utils directory before 
> sending a patch ;-)  (even if your changes are probably not covered by 
> unittests)
> 
> Speaking about tests - I'd still like to see an unittest for the json 
> interface ;-)  (maybe a sample to-be-updated profile, an audit.log sniplet, 
> the expected JSON, predefined answers and finally the expected profile.)
> 
> 
> I tried to use aa-logprof --json, but all I get is:
> 
> python3 aa-logprof --json
> Traceback (most recent call last):
>   File "aa-logprof", line 56, in <module>
>     apparmor.do_logprof_pass(logmark)
>   File "/home/cb/apparmor/HEAD-clean/utils/apparmor/aa.py", line 1905, in do_logprof_pass
>     aaui.UI_Info(_('Reading log entries from %s.') % logfile)
>   File "/home/cb/apparmor/HEAD-clean/utils/apparmor/ui.py", line 61, in UI_Info
>     write_json(jsonout)
>   File "/home/cb/apparmor/HEAD-clean/utils/apparmor/ui.py", line 43, in write_json
>     sys.stdout.write(json.dumps(jsonout, soft_keys=False, separators=(',', ': ')) + '\n')
>   File "/usr/lib64/python3.6/json/__init__.py", line 238, in dumps
>     **kw).encode(obj)
> TypeError: __init__() got an unexpected keyword argument 'soft_keys'
> 
> Is that meant to be sort_keys instead of soft_keys?
> 
> After changing that, it finally worked. I get a prompt like (linebreaks 
> added for readability)
> 
> {
> "title": null,
> "headers": {"Profil": "/usr/bin/konversation","Pfad": "/sys/devices/pci0000:00/0000:00:02.0/vendor","Neuer Modus": "r","Schweregrad": 4},
> "explanation": null,
> "options": ["/sys/devices/pci0000:00/0000:00:02.0/vendor r,","/sys/devices/pci*/*/vendor r,"],
> "menu_items": ["Erl(a)uben","[A(b)lehnen]","(I)gnorieren","(G)lob","Glob with (E)xtension","(N)eu","Audi(t)","Abb(r)echen","En(d)e"]
> }
> 
> I'm missing one thing here - the preselected option.
> In most cases, the first option is preselected - but after using (G)lob,
> Glob with (E)xtension or (N)ew, the newly added option will be 
> selected, and the new option is in most (not all!) cases the last one.
> 
>> diff --git a/utils/apparmor/ui.py b/utils/apparmor/ui.py
>> index bfbde8c..e6fcd1c 100644
>> --- a/utils/apparmor/ui.py
>> +++ b/utils/apparmor/ui.py
> 
>> -from apparmor.yasti import yastLog, SendDataToYast, GetDataFromYast
> 
> It seems ui.py was the only user of yastLog(), so you can (and should)
> drop it from yasti.py.
> 
> SendDataToYast and GetDataFromYastcheck if you need to change something there.
> 
> From a quick look, the most important affected functions are:
> - get_profile() - you'll see this in action with aa-genprof if a profile
>   exists in /usr/share/apparmor/extra-profiles/ and you select 
>   "view profile"
> - yast_select_and_upload_profiles() - dead code because we don't have
>   a profile repo currently
> - save_profiles() - SendDataToYaST is used for "view changes"
> 
> There are some more functions that check for YaST - simply search aa.py
> for "Yast" and "yast".
> 
> Note: Ideally, aa.py should know nothing about the UI used, so it might
> be a good idea to move this code to ui.py unless that makes things 
> terribly difficult.
> 
> I'd guess a (to-be-written) UI_view_in_pager() function or extending 
> UI_LongMessage() to handle text mode (instead of deleting it) could 
> get the job done ;-)
> 
>> @@ -47,18 +56,17 @@ def UI_Info(text):
>>      debug_logger.info(text)
>>      if UI_mode == 'text':
>>          sys.stdout.write(text + '\n')
>> -    else:
>> -        yastLog(text)
>> +    elif UI_mode == 'json':
>> +        jsonout = {'info': text}
>> +        write_json(jsonout)
> 
> What happens if someone accidently sets UI_mode to "foo"? (Hint: no output,
> and you'll have a hard time to find out _why_ there isn't any output.)
> I know this is unlikely, but I'd still prefer to be on the safe side.
> 
> What about:
> 
>     if UI_mode == 'json':
>         ...
>     else:  # text mode
>         ...
> 
> You could also add an else branch that raises an exception("unknown 
> UI_mode") - but that might be too much ;-)
> 
> This comment applies to several sections, so please adjust this in the
> whole file.
> 
>> @@ -138,36 +140,31 @@ def UI_YesNoCancel(text, default):
>> ...
>> +            jsonout = {'dialog': 'yesnocancal', 'text': text, 'default': default}
> 
> As Seth already noted in v1, this should be yesnocanc_e_l ;-)
> 
>> @@ -405,7 +388,10 @@ class PromptQuestion(object):
>>
>>              prompt += ' / '.join(menu_items)
>>
>> -            sys.stdout.write(prompt + '\n')
>> +            if UI_mode == 'json':
>> +                sys.stdout.write(json.dumps(jsonprompt,
>> sort_keys=False,  separators=(',', ': ')) + '\n') 
> 
> Please use write_json() ;-)
> 
> 
> That all said - v2 looks much better than v1 :-)
> 

This is getting more complicated than can be handled. Do you think
converting UI module to an abstract class, and deriving JSONUI and
TextUI will be a better option? Though it would need more work, it would
simplify a lot of things, be more flexible to changes and would be more
robust than the hackish way it is being performed now.

Finally, is it a principle to not "reply to all" on the mailing list?
While the mail does reach me, The ML filters move the mails to respctive
folders.


-- 
Goldwyn