Edgewall Software
Modify

Opened 9 years ago

Closed 8 years ago

Last modified 2 years ago

#9336 closed enhancement (fixed)

[PATCH] trac-admin import/export permissions

Reported by: anatoly techtonik <techtonik@…> Owned by: Peter Suter
Priority: high Milestone: 1.0
Component: admin/console Version: 0.12dev
Severity: normal Keywords:
Cc: leho@…, Thijs Triemstra Branch:
Release Notes:

Import and export Trac permissions with trac-admin $ENV permission <import|export>

API Changes:

Description

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

Attachments (7)

permission-import-export.patch (3.7 KB ) - added by psuter <petsuter@…> 8 years ago.
CSV permission import and export trac-admin commands
permission-import-export2.patch (4.2 KB ) - added by psuter <petsuter@…> 8 years ago.
one user per line; check existing perms; file completion
trac-admin-completion-on-windows.patch (2.6 KB ) - added by psuter <petsuter@…> 8 years ago.
Improve trac-admin path completion on Windows.
permission-import-export-tests.patch (4.6 KB ) - added by psuter <petsuter@…> 8 years ago.
Unit tests fixed and added for permission import / export (based on permission-import-export2.patch)
permission-import-export-stdin-stdout.patch (8.4 KB ) - added by psuter <petsuter@…> 8 years ago.
Allow importing from stdin and exporting to stdout
permission-import-export-unicode.patch (3.2 KB ) - added by psuter <petsuter@…> 8 years ago.
Workaround csv library unicode issues (incomplete)
permission-import-export-unicode2.patch (5.3 KB ) - added by psuter <petsuter@…> 8 years ago.
Fix unicode issues by using the stream's encoding.

Download all attachments as: .zip

Change History (40)

comment:1 by lkraav <leho@…>, 9 years ago

Cc: leho@… added

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

comment:2 by anatoly techtonik <techtonik@…>, 9 years ago

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.

comment:3 by Christian Boos, 9 years ago

Milestone: unscheduled

comment:4 by Thijs Triemstra, 9 years ago

Cc: Thijs Triemstra added

comment:5 by Thijs Triemstra, 9 years ago

Resolution: duplicate
Status: newclosed

Duplicate of #8489 it seems.

comment:6 by Remy Blank, 9 years ago

Milestone: unscheduled

comment:7 by anatoly techtonik <techtonik@…>, 9 years ago

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.

comment:8 by Thijs Triemstra, 9 years ago

Resolution: duplicate
Status: closedreopened

comment:9 by Christian Boos, 8 years ago

Milestone: unscheduled

… back to unscheduled then, please provide a patch.

by psuter <petsuter@…>, 8 years ago

CSV permission import and export trac-admin commands

comment:10 by psuter <petsuter@…>, 8 years ago

Summary: trac-admin import/export permissions[PATCH] trac-admin import/export permissions

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?

in reply to:  10 comment:11 by Remy Blank, 8 years ago

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()).

by psuter <petsuter@…>, 8 years ago

one user per line; check existing perms; file completion

comment:12 by psuter <petsuter@…>, 8 years ago

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.

in reply to:  12 ; comment:13 by Remy Blank, 8 years ago

Milestone: unscheduled0.13
Owner: set to Remy Blank
Priority: normalhigh
Status: reopenednew

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.

in reply to:  13 comment:14 by psuter <petsuter@…>, 8 years ago

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

by psuter <petsuter@…>, 8 years ago

Improve trac-admin path completion on Windows.

comment:15 by psuter <petsuter@…>, 8 years ago

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

in reply to:  15 comment:16 by psuter <petsuter@…>, 8 years ago

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.

by psuter <petsuter@…>, 8 years ago

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

in reply to:  13 comment:17 by psuter <petsuter@…>, 8 years ago

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'].)

comment:18 by Remy Blank, 8 years ago

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).

comment:19 by Remy Blank, 8 years ago

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

    diff --git a/trac/admin/console.py b/trac/admin/console.py
    a b import cmd  
    1616import locale
    1717import os.path
    1818import pkg_resources
    19 import shlex
     19from shlex import shlex
    2020import StringIO
    2121import sys
    2222import traceback
    class TracAdmin(cmd.Cmd):  
    7171        try:
    7272            import readline
    7373            delims = readline.get_completer_delims()
    74             for c in '-/:()':
     74            for c in '-/:()\\':
    7575                delims = delims.replace(c, '')
    7676            readline.set_completer_delims(delims)
    7777           
    Type: '?' or 'help' for help on command  
    182182
    183183        ... but shlex is not unicode friendly.
    184184        """
    185         return [unicode(token, 'utf-8')
    186                 for token in shlex.split(argstr.encode('utf-8'))] or ['']
     185        lex = shlex(argstr.encode('utf-8'), posix=True)
     186        lex.whitespace_split = True
     187        lex.commenters = ''
     188        if os.name == 'nt':
     189            lex.escape = ''
     190        return [unicode(token, 'utf-8') for token in lex] or ['']
    187191
    188192    def word_complete(self, text, words):
    189193        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.

in reply to:  19 ; comment:20 by psuter <petsuter@…>, 8 years ago

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")

in reply to:  18 ; comment:21 by psuter <petsuter@…>, 8 years ago

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?)

in reply to:  21 ; comment:22 by Remy Blank, 8 years ago

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).

by psuter <petsuter@…>, 8 years ago

Allow importing from stdin and exporting to stdout

by psuter <petsuter@…>, 8 years ago

Workaround csv library unicode issues (incomplete)

in reply to:  22 comment:23 by psuter <petsuter@…>, 8 years ago

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.)

comment:24 by Remy Blank, 8 years ago

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().

in reply to:  20 comment:25 by Remy Blank, 8 years ago

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].

by psuter <petsuter@…>, 8 years ago

Fix unicode issues by using the stream's encoding.

in reply to:  24 ; comment:26 by psuter <petsuter@…>, 8 years ago

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ɑ.

comment:27 by Remy Blank, 8 years ago

Resolution: fixed
Status: newclosed

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

comment:28 by Remy Blank, 8 years ago

Owner: changed from Remy Blank to psuter <petsuter@…>

in reply to:  26 ; comment:29 by Christian Boos, 8 years ago

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).

in reply to:  29 comment:30 by Remy Blank, 8 years ago

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 ;)

comment:31 by Peter Suter, 8 years ago

Owner: changed from psuter <petsuter@…> to Peter Suter

comment:32 by al.willmer@…, 8 years ago

Release Notes: modified (diff)

comment:33 by Ryan J Ollos, 2 years ago

Support for pyreadline documented in 1.3/TracInstall@11 and TracAdmin@60.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Peter Suter.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Peter Suter to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.