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

Magpie not able to handle users with slash (/) #171

Closed
tlvu opened this issue Apr 4, 2019 · 8 comments
Closed

Magpie not able to handle users with slash (/) #171

tlvu opened this issue Apr 4, 2019 · 8 comments
Labels
bug Problem, error, or invalid behaviour feature New feature to be developed
Milestone

Comments

@tlvu
Copy link
Contributor

tlvu commented Apr 4, 2019

@moulab88 and I are working on issue Ouranosinc/pavics-sdi#58 to provide single sign-on between Magpie and JupyterHub and we found out that Magpie is not able to handle user name containing a slash (/).

We tried the External Signin Provider "PCMDI" with Magpie. We were able to authenticate but when we want to "edit" (as admin user) the newly added user, we got this link "https://lvu.ouranos.ca/magpie/ui/users/https://esgf-node.llnl.gov/esgf-idp/openid/DavidHuard_pcmdi/default" and the "Internal Server Error", see the 2 attached screenshots.
Internal-server-error
User-list-with-ESGF-OpenId-user

It is not because of the length of the username but because there is a slash (/) in the username. We have retried deleting that OpenId user and adding a new user called "abc/def" and same behavior again.

Worst, once there is at least one user with a slash, the page add user is also broken with the same "Internal Server Error" preventing new users to be added.

Reproduced with Magpie docker image pavics/magpie:0.7.3 with this patch to fix the wrong ESGF OpenId provider url:

$ git diff
diff --git a/magpie/api/login/esgfopenid.py b/magpie/api/login/esgfopenid.py
index 67789e1..dcf6346 100644
--- a/magpie/api/login/esgfopenid.py
+++ b/magpie/api/login/esgfopenid.py
@@ -46,7 +46,7 @@ class ESGFOpenID(OpenID):
         super(ESGFOpenID, self).__init__(*args, **kwargs)
 
         self.hostname = self._kwarg(kwargs, 'hostname', 'localhost')
-        self.provider_url = self._kwarg(kwargs, 'provider_url', 'https://{hostname}/providers-idp/openid/{username}')
+        self.provider_url = self._kwarg(kwargs, 'provider_url', 'https://esgf-node.llnl.gov/esgf-idp/openid/{username}')
 
         # if username is given set provider identifier using the provider url
         if 'username' in self.params:
@fmigneault
Copy link
Collaborator

@tlvu I would suggest that you upgrade Magpie to version 0.7.11 (at least).
With this version on another server, such cases are handled.

image

@tlvu
Copy link
Contributor Author

tlvu commented Apr 4, 2019

@fmigneault can you show a screenshot of the "edit" view of your ESGF user where the admin can set permissions? It's the "edit" view that was not working for me, not these screens.

@tlvu
Copy link
Contributor Author

tlvu commented Apr 4, 2019

Upgraded to docker image pavics/magpie:0.7.11 as you suggested @fmigneault

magpie-version-0 7 11

Created a user 'abc/def'

magpie-user-list-with-slash

Click on "Edit" button next to that "abc/def" user, that "edit" view as I reported earlier is still broken

magpie-404-on-user-with-slash

@fmigneault
Copy link
Collaborator

@tlvu
/ are especially disallowed for the above reason. It makes the API routes not work as expected because we cannot differentiate between the real / and the ones of the username.
For / to be supported, we would need to call the route with the escape %2F.
I have tested quickly. It seems %2F are still misinterpreted as /, so these users won't work for now.

@fmigneault fmigneault added bug Problem, error, or invalid behaviour feature New feature to be developed labels Apr 4, 2019
@fmigneault
Copy link
Collaborator

fmigneault commented Apr 4, 2019

@tlvu
As a side note, there is no plan to support / in the username.
The idea of external provider is to give an alternative login method of an existing "internal" user.
The form has to be updated to indicate which "internal" user the external one has to be associated with, but there should never be a case where /users/<external-user> is called.

see #84, #118 for details

@tlvu
Copy link
Contributor Author

tlvu commented Apr 8, 2019

@fmigneault , @dbyrns since all the various OpenID external signin providers will store the username as url, meaning with /, meaning they do not work, should we label them with "(coming soon)" so Magpie users knows they do not work yet to not waste time on them? As it stands, users will have the impression they are full working, which is not the case.

I just want us to better communicate what features are still "work in progress" and what features are fully tested and operational.

@fmigneault
Copy link
Collaborator

@tlvu
The thing is, these "users" should not even exist according to #118
Even in the db, they are stored in the ExternalUsers table, and a dummy "user" is created just to pad the user-id reference requirement.

When #118 will be completed, there will not be a need of this dummy user as the external user entry will point to a real user. The dummy user is a patched operation carried over from the beginning of this repo.

You could manually had "coming soon" to such users for informative purposes, but I think adding any name-label in the code is both a fragile check (as it could be renamed), and quickly irrelevant when #118 is implemented, to be worth changing the code.

@fmigneault fmigneault added this to the 0.10.x milestone Apr 8, 2019
@tlvu
Copy link
Contributor Author

tlvu commented Apr 8, 2019

@fmigneault, by user I meant the human that will try to use Magpie, like myself, @huard and @moulab88. We were excited to see Magpie supporting so many external signin providers but turns out they do work but not completely, but just enough to give us false hope so we invested more time in them. Had we knew they are not completely finished and tested, we would probably have picked another option to try integrating the signin of JupyterHub with Magpie or contacted you earlier for help.

I will ping you earlier next time we need any integration with Magpie.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Problem, error, or invalid behaviour feature New feature to be developed
Projects
None yet
Development

No branches or pull requests

2 participants