Skip to content
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

respect expose_user_email configuration in datalibraries permissions list #4611

Merged
merged 5 commits into from Sep 15, 2017

Conversation

4 participants
@martenson
Copy link
Member

commented Sep 13, 2017

part of #1177

ping @erasche

@martenson martenson force-pushed the martenson:respect-email-exposure branch from 313c36e to 0d6a71d Sep 13, 2017

@martenson martenson added this to Done in Data Libraries Sep 13, 2017

@galaxybot galaxybot added this to the 17.09 milestone Sep 13, 2017

martenson added some commits Sep 12, 2017

@martenson martenson force-pushed the martenson:respect-email-exposure branch from 0d6a71d to 1b2221c Sep 14, 2017

@martenson

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

rebased, rebuilt and fixed indent

@erasche

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

Ok, tested this today, I'm happy with the changes. This is a much, much better sharing experience. 👍

@martenson

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

Thanks for testing @erasche and sorry the sharing is still ... limited. We should do better.

@@ -1076,26 +964,11 @@ var LibraryDatasetView = Backbone.View.extend({
'<div class="alert alert-info roles-selection">User with <strong>any</strong> of these roles can modify name, metadata, and other information about this library item.</div>',
'<hr/>',
'<h2>Dataset-related permissions</h2>',
'<div class="alert alert-warning">Changes made below will affect <strong>every</strong> library item that was created from this dataset and also every history this dataset is part of.</div>',
'<divclass="alert alert-warning">Changes made below will affect <strong>every</strong> library item that was created from this dataset and also every history this dataset is part of.</div>',

This comment has been minimized.

Copy link
@dannon

dannon Sep 14, 2017

Member

s/divclass/div\ class/ ?

@dannon

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

I'm probably doing it wrong, but I tested the following.

  1. create a new data library as an admin (foo@foo.com).
  2. granted "access" and "manage" permissions to a non-admin (bar@bar.com)
  3. (as bar@bar.com user, in a separate browser) I go to the permission management page, and I can't actually add more roles that can access the page. "No matches found" won't go away, preventing me from entering any additional roles here.

Note: with default expose_user_name/expose_user_email in galaxy.ini, so, False

@martenson

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

@dannon did you flip the expose_user_email config?

fix typo
thanks @dannon
@dannon

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

@martenson No, I left it false on purpose to test. I should still be able to add emails that I know, without the assisted autocomplete, right?

@martenson

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

@dannon No, you cannot search for specific emails/users nor can you add arbitrary string that Galaxy will check against the roles DB table.

@dannon

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

Ok, so without 'expose' set, these are just completely disabled for non-admins? Can we add a hint that that's the case on the interface? (and not allow people to try)

@martenson

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

@dannon They are not disabled, you only see your role, your sharing roles and other roles/groups you are part of. The logic is in the get_valid_roles method.

edit: The reason why you cannot see anything is that your only valid option is your own private role. But if you were part of an e.g. group you would see its role there.

@dannon

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

I get that I can see my own role. This user is supposed to be able to administer this library though, and cannot, and the UI doesn't provide any sort of a clue why -- it just appears broken, which is bad UX.

(screenshot snipped, wasn't useful)

If we just add some text saying "Note that this instance is configured to hide usernames and email addresses, so you will not be able to add users that are not <blah>", it'd be fine.

@martenson

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

@dannon That does not have much to do with this PR though. Feel free to contribute to the discussion here.

@dannon

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

I took that issue to be about much higher level overall security design, which will of course eventually change the UX here, but I'm talking about my experience with this particular interface that this PR touches. It appears broken, to me, and a little additional text would explain that.

@martenson

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

@dannon Similar interface is in history on every dataset's permissions and in fact on sharing of every object - workflows, pages, history etc.

I suggest opening a PR that alters labeling on all these, if you think we can do a better job explaining the current configuration and options.

This PR specifically addresses the following part of #1177 :

When modifying permissions as non-admin user, expose_user_name and expose_user_email config options are not respected

and thus, unless there are bugs related to that, I deem it finished.

@dannon

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

You specifically mention workflows, pages, and histories, but they all have a simple "email address of user to share with", which allows me to (as expected) type in whatever address I want. Anyway, this was an open PR, I gave feedback, you're welcome to ignore it. I'm moving on.

@martenson

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

@dannon Could you please stop holding my PR as hostage for what you deem is a label changing issue?

@dannon

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

@martenson What am I holding hostage? I spent time to review this, reported one typo and a confusing interface. At first, I literally did not know if the interface was fixed or not because it's so confusing. I did not -1 this, and am not 'holding anything hostage'.

@erasche

This comment has been minimized.

Copy link
Member

commented Sep 15, 2017

I see where @dannon is coming from, but that can be kicked down the road slightly. I don't think we're regressing any here. I've added an item to #1177 relating to his comment.

@erasche erasche merged commit c5ff555 into galaxyproject:dev Sep 15, 2017

6 checks passed

api test Build finished. 292 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 161 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 45 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details

@martenson martenson moved this from Done to Closed in Data Libraries Sep 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.