[PATCH] trac-admin import/export permissions

[anatoly techtonik <techtonik@…>]

trac-admin should allow to export permissions from old environment, edit them and import back

[lkraav <leho@…>]

yes, just had a case where i needed this. following.

[anatoly techtonik <techtonik@…>]

There is also invisible upvote arrow at the top of the page. I suppose it is like a typical Google star except that it probably does not subscribe.

[cboos]

PatchWelcome.

[thijstriemstra]

Duplicate of #8489 it seems.

[anatoly techtonik <techtonik@…>]

Not really a duplicate. #8489 is about transparent copy of permissions from one env to another. This proposal is to export to file/edit/then import from file.

[cboos]

… back to unscheduled then, please provide a patch.

[psuter <petsuter@…>]

CSV permission import and export trac-admin commands

[psuter <petsuter@…>]

The above patch adds commands trac-admin permission import <filename> and trac-admin permission export <filename> to import and export simple CSV files of user, action pair rows.

• Is CSV a good idea?
• The ​''csv'' library documenteation notes some problems with Unicode input. I'm not sure what needs o be done here.
• Importing a user, action pair that already exists in the DB results in a IntegrityError: columns username, action are not unique. I'm not sure how to best avoid this. Should this exception just be swallowed?
• Command completion is not implemented. One could maybe suggest a list of existing files for import?

[rblank]

Replying to psuter <petsuter@…>:

The above patch adds commands trac-admin permission import <filename> and trac-admin permission export <filename> to import and export simple CSV files of user, action pair rows.

Excellent, thanks for working on this!

My two cents:

• Is CSV a good idea?

Reasonable, yes. I was going to suggest space-separated, but user names can have spaces IIRC, so that's a no go.

However, instead of using (user, perm) pairs on each line, I would write one line per user, structured as (user, perm1, perm2, perm3, …).

• Importing a user, action pair that already exists in the DB results in a IntegrityError: columns username, action are not unique. I'm not sure how to best avoid this. Should this exception just be swallowed?

The command should succeed, but not by swallowing the exception, as it's DB backend-specific, but by checking if the permission is already granted before trying to insert it.

• Command completion is not implemented. One could maybe suggest a list of existing files for import?

Yes, absolutely. Look at the wiki import command in trac.wiki.admin for an example how to do that (basically, use get_dir_list()).

[psuter <petsuter@…>]

one user per line; check existing perms; file completion

[psuter <petsuter@…>]

Thanks for the review & hints. I attached an updated patch that incorporates your suggestions.

Look at the wiki import command in trac.wiki.admin for an example how to do that (basically, use get_dir_list()).

I added this as well, but I can't seem to test it successfully. (On Windows, Python 2.7 with pyreadline 1.7) Command completion itself works; however path completion seems broken, even for e.g. wiki import.

[rblank]

Replying to psuter <petsuter@…>:

Thanks for the review & hints. I attached an updated patch that incorporates your suggestions.

Very good! I'll apply the patch shortly. If you want to get bonus points, you could add unit tests for the new commands (see trac/admin/tests/console.py​ and trac/admin/tests/console-tests.txt​ for inspiration).

I added this as well, but I can't seem to test it successfully. (On Windows, Python 2.7 with pyreadline 1.7) Command completion itself works; however path completion seems broken, even for e.g. wiki import.

Let me try… Yes, pyreadline seems to behave quite strangely indeed. I have a feeling that it's due to an incompatibility of pyreadline, but I may be wrong.

[psuter <petsuter@…>]

The problem seems to be that shlex.split (called in trac.admin.console.TracAdmin arg_tokenize) doesn't like Windows paths. Trailing backslashes like shlex.split('C:\\') raise a ValueError: No escaped character; others get lost: shlex.split('C:\\Code\\Trac') evaluates to ['C:CodeTrac'] and matching fails.

One could use forward slashes instead, but os.path.join called in get_dir_list inserts and appends backslashes, so matching again fails. Matching with normalized strings in trac.admin.api.PathList using os.path.normcase helps marginally - the completion result still contains backslashes preventing further completion.

​http://bugs.python.org/issue1724822

[psuter <petsuter@…>]

Improve trac-admin path completion on Windows.

[psuter <petsuter@…>]

Sorry if this is getting somewhat off-topic. The above patch seems to improve trac-admin's path tab completion on Windows for me.

[psuter <petsuter@…>]

Replying to psuter <petsuter@…>:

The above patch seems to improve trac-admin's path tab completion on Windows for me.

Several unit tests fail with this patch applied, so it probably breaks some other things.

[psuter <petsuter@…>]

Unit tests fixed and added for permission import / export (based on permission-import-export2.patch)

[psuter <petsuter@…>]

Replying to rblank:

If you want to get bonus points, you could add unit tests for the new commands (see trac/admin/tests/console.py​ and trac/admin/tests/console-tests.txt​ for inspiration).

Thanks again for the pointers. The above patch fixes the existing test_help_ok patch to include the help text for the two new commands, and adds a new test each for importing / exporting. Due to the nature of these commands I had to supply a filename, which I hardcoded as 'permissions.csv'.

(I noticed again that on Windows the file must be in the current directory, as for example shlex.split('C:\\Temp\\permissions.csv') evaluates to ['C:Temppermissions.csv'].)

[rblank]

permission-import-export2.patch​ and permission-import-export-tests.patch​ applied in [10714], with the following minor changes:

• Added from __future__ import with_statement in trac/admin/tests/console.py.
• Set the CSV writer to output with a platform-specific line terminator.
• Sorted the export by user and each line by permission for test repeatability.
• Used a single try: except: block in _do_import().
• Used read_file() in the export test.

As you can tell, the changes are on the level of details, so great work!

Two possible extensions:

• Allow using a single dash (-) as a file name for exporting to / importing from stdout / stdin (the same as for wiki export and wiki import). This should also allow getting rid of the temporary permissions.csv in the unit tests.
• Allow specifying for which users to export permissions, by passing a list of users after the file name in permission export.

I'll have a look at the completion issue, but reading that ticket, it seems we will have to have our own copy of shlex.split(), at least for 2.5 (and the fix should go into 0.12.3).

[rblank]

trac-admin-completion-on-windows.patch​ indeed solves part of the issue with auto-completion, but it opens a can of worms. The failing tests are due to parsing in non-POSIX mode, where quoting doesn't operate the same, and the DB connection string when initializing functional tests is passed as 'sqlite:db/trac.db' (including the quotes).

I have the feeling that we should rather leave the parsing in POSIX mode, but simply disable escaping on Windows. Something like the following (in addition to your changes in api.py, which are fine):

trac/admin/console.py

@@ -16 +16 @@
 import locale
 import os.path
 import pkg_resources
-import shlex
+from shlex import shlex
 import StringIO
 import sys
 import traceback
@@ -71 +71 @@
         try:
             import readline
             delims = readline.get_completer_delims()
-            for c in '-/:()':
+            for c in '-/:()\\':
                 delims = delims.replace(c, '')
             readline.set_completer_delims(delims)
             
@@ -182 +182 @@
 
         ... but shlex is not unicode friendly.
         """
-        return [unicode(token, 'utf-8')
-                for token in shlex.split(argstr.encode('utf-8'))] or ['']
+        lex = shlex(argstr.encode('utf-8'), posix=True)
+        lex.whitespace_split = True
+        lex.commenters = ''
+        if os.name == 'nt':
+            lex.escape = ''
+        return [unicode(token, 'utf-8') for token in lex] or ['']
 
     def word_complete(self, text, words):
         words = list(set(a for a in words if a.startswith(text)))

This makes completion work on Windows for me. It still feels weird, as it completes correctly but doesn't display all choices when hitting tab twice. But this may be due to pyrealine.

[psuter <petsuter@…>]

Replying to rblank:

I have the feeling that we should rather leave the parsing in POSIX mode, but simply disable escaping on Windows. Something like […]
This makes completion work on Windows for me. It still feels weird, as it completes correctly but doesn't display all choices when hitting tab twice. But this may be due to pyrealine.

Sounds sensible. And works fine here, too, up to the same tab-twice issue. However, I could fix that by ​creating %HOMEPATH%\pyreadlineconfig.ini containing: show_all_if_ambiguous("on")

[psuter <petsuter@…>]

Replying to rblank:

applied in [10714], with the following minor changes: […]

Great.

• Allow using a single dash (-) as a file name for exporting to / importing from stdout / stdin (the same as for wiki export and wiki import). This should also allow getting rid of the temporary permissions.csv in the unit tests.

Yes, sounds useful. I assume for the wiki commands this is handled by this code: if not filename: printout(text) and if filename: ... else: data = sys.stdin.read()? I can't seem to trigger that functionality using a dash (again a argument parsing problem on Windows?)

[rblank]

Replying to psuter <petsuter@…>:

Yes, sounds useful. I assume for the wiki commands this is handled by this code: if not filename: printout(text) and if filename: ... else: data = sys.stdin.read()? I can't seem to trigger that functionality using a dash (again a argument parsing problem on Windows?)

Yes, sorry, my bad. For the wiki commands, it is triggered by the absence of a path. We should probably do the same here… except then we cannot export the permissions of only a few users. That's ok, it probably isn't that useful anyway (and easy to implement with grep).

[psuter <petsuter@…>]

Allow importing from stdin and exporting to stdout

[psuter <petsuter@…>]

Workaround csv library unicode issues (incomplete)

[psuter <petsuter@…>]

Replying to rblank:

it is triggered by the absence of a path. We should probably do the same here…

Ah of course, that makes sense. In the two patches above I gave this another try, and also experimented a bit with the suggested wrappers from ​the csv library documentation to work around the Unicode issues. (It seems to help, except when importing from stdin, but I maybe don't yet understand the problem enough to fix it properly.)

[rblank]

Unit test modifications from permission-import-export-stdin-stdout.patch​ were applied in [10715]. In the import and export commands, instead of using a closure to avoid code duplication, I added a context manager that opens a file if the filename is non-empty, or uses the standard stream corresponding to the file mode otherwise. This should be reusable for the wiki commands as well, once the encoding issues are sorted out.

When exporting or importing to / from the standard streams, we should actually use the stream's encoding, instead of forcing UTF-8. The logic should probably be very similar to what's used in console_print()​.

[rblank]

Replying to psuter <petsuter@…>:

However, I could fix that by ​creating %HOMEPATH%\pyreadlineconfig.ini containing: show_all_if_ambiguous("on")

Heh, and it's even ​documented. Oh well, who reads the docs, anyway? :) Installing a proper pyreadlineconfig.ini makes completion work very nicely.

So, fix applied on 0.12-stable in [10716].

[psuter <petsuter@…>]

Fix unicode issues by using the stream's encoding.

[psuter <petsuter@…>]

Replying to rblank:

When exporting or importing to / from the standard streams, we should actually use the stream's encoding, instead of forcing UTF-8. The logic should probably be very similar to what's used in console_print()​.

I gave this another shot in the patch above and extracted the code you indicated into get_console_encoding(). Using that in permission import and permission export makes the unittests succeed for a test_userɑ.

[rblank]

Works great. Applied in [10722], with a few cosmetic changes. So… we're done!

[cboos]

Replying to psuter <petsuter@…>:

… the unittests succeed for a test_userɑ.

Great job, but note that non-ascii usernames in general are not yet fully supported (#6318).

[rblank]

Replying to cboos:

but note that non-ascii usernames in general are not yet fully supported (#6318).

Right, but at least this part of the code will be ready ;)