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

Provide better feedback when a remote file can't be accessed. #132

Closed
1 task done
tshead2 opened this issue Aug 14, 2014 · 8 comments
Closed
1 task done

Provide better feedback when a remote file can't be accessed. #132

tshead2 opened this issue Aug 14, 2014 · 8 comments
Assignees

Comments

@tshead2
Copy link
Member

tshead2 commented Aug 14, 2014

  • See if we can show the permissions to the end-user so they know what went wrong.
@tshead2 tshead2 self-assigned this Aug 14, 2014
@pullmonkey pullmonkey assigned pullmonkey and unassigned tshead2 Aug 20, 2014
@pullmonkey
Copy link
Contributor

Added the permissions as they exist for an ls -l at the time of failure.
When the user hovers over a data point, and the associated image is not readable to the session's user, the app used to just tell the user there was a permission issue. Now it provides the output of ls -l to give the user a greater sense of what may be wrong.

Somethings to consider before I merge this into master:

  1. Is it safe to assume that the remote servers are all Unix-variant?

  2. From a user perspective, it was nicer to see the username and group name, rather than the uid and gid provided by sftp. Hence the use of ssh.exec_command. There is an implied security concern when using exec_command, so the code checks that "path" (user provided input) is an existing file, otherwise, a new exception of "No such file" is raised.

  3. Less code, but less friendly - there exists a way via sftp to get the same data, but with uid and gid in place of the username and group name.

@tshead2
Copy link
Member Author

tshead2 commented Aug 27, 2014

Charlie:

I'm not in love with using ls for this ... we have functionality to lookup information via LDAP, could we use that instead? Take a look at what we've got in packages/slycat/web/server/directory.py and see what you think ... we could add some functionality to do lookups by uid / gid (note that the user() method misleadingly looks like it takes a uid, but it actually does a lookup by username).

Cheers,
Tim

@pullmonkey
Copy link
Contributor

Nice. That sounds better.
Added uid_to_username() and gid_to_username().
Added ldap tests and config file vars and switched to the sftp stat with uid to gid conversions via ldap.

@pullmonkey
Copy link
Contributor

Updated test to not require configuring.
Updated config to have an expression for the ldap_query.
Ready for review.

To test:

  1. Start without the changes (RGR-style)
  2. Load up an image param model
  3. Hover over a data point, with a typical setup, you'll be prompted for user credentials and will likely see the image.
  4. manipulate the image on the backend (e.g., remove read permissions, change owner and group, remove file, etc)
  5. Repeat (3) - you'll be shown an alert - if it is permission related, you'll see a permission issue (no additional note regarding current permissions.
  6. get the new code (update the config.ini with the proper ldap conf)
  7. run tests (4-5) and at step (5) you'll see additional permission data

@pullmonkey pullmonkey assigned tshead2 and unassigned pullmonkey Aug 29, 2014
@pullmonkey
Copy link
Contributor

@tshead2
Copy link
Member Author

tshead2 commented Sep 2, 2014

Charlie:

Looking good, just have a couple of requests:

  • I like the addition of directory.ldap.ldap_query() to cut down on boilerplate. Let's make it a private function and use it in the implementation of directory.ldap.user() too.
  • Let's put the new uid_to_username() and gid_to_username() functionality in directory.prototype and directory.identity. That way we don't have to use ldap in our default config.ini, and we can test the client without needing an ldap server.

Back over to you :)

@tshead2 tshead2 assigned pullmonkey and unassigned tshead2 Sep 2, 2014
@pullmonkey
Copy link
Contributor

Now leveraging ldap_query() (privately) for the user() method as well.
Also, filled in the prototype and identity with the new methods: uid_ and gid_to_username().

@pullmonkey pullmonkey assigned tshead2 and unassigned pullmonkey Sep 2, 2014
@pullmonkey
Copy link
Contributor

and added tests for user() since it had to be refactored.

@tshead2 tshead2 closed this as completed in 70910bb Sep 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants