-
Notifications
You must be signed in to change notification settings - Fork 486
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
Local account creation URL ignores :AllowSignup #2838
Comments
I just tested this on my laptop on the 4.2.3 branch (24c7a4e) and can confirm that this is a bug. The setting only seems to hide the "Sign Up" link in the header. You can still create "builtin" accounts if you know the right URL (for me it's http://localhost:8080/dataverseuser.xhtml?editMode=CREATE ). This feature/setting seems to have been last worked on in #988 ("My understanding of the purpose this is to prevent users from creating new accounts, until an admin is ready."). There I learned that the setting also hides the "Sign Up" link from a popup when you click Add Data when you aren't logged in. This setting was asked about https://groups.google.com/d/msg/dataverse-community/JTZTEjKCaFE/zf02BOEkBgAJ |
Note that builtin users can also be created via API. I just left a warning about this in my pull request at #2895 which can be previewed here: http://guides.dataverse.org/en/2884-install-guide-reorg/installation/config.html#blocking-api-endpoints I assume that installations that want to disallow sign up (builtin accounts) wouldn't want local accounts to be created via API. The point of the docs above is that it's possible to block the API endpoint separately or change the key required frequently. |
@majorseitan ideally you would improve the messaging to make sense but please don't kill yourself on this because we'll want to review it with @eaquigley and @mheppler . If you're asking what the user should see when they try to go to http://localhost:8080/dataverseuser.xhtml?editMode=CREATE directly, I think it should say something like this: "Creation of local accounts has been disabled. Please click "Log In" to log in via your institution." (For end users, I prefer the term "local account" over "builtin account" despite the terms we use in the code.) We could also check with @ajirnyi @ecr2c and @shlake on what they think the message should say. I think @akio-sone might want to run in this |
Note to self to review some of the ideas from @majorseitan at develop...majorseitan:2838-account-creation-ignores-AllowSignup |
Yes, when AllowSignup up is turned off (i.e, what we would want for users logging in with Shibboleth SSO), the (above) message: sounds good. |
@shlake ah, yes, that makes a lot of sense. Heads up to @eaquigley and @mheppler about this. Thanks! |
Leaving myself a reminder in this ticket to check and make sure Sign Up isn't referenced in places besides the Request Access and Add Data pop ups. @eaquigley Also, @mheppler, I've just noticed the same alert in both those pop ups above are formatted differently. We should make those consistent with each other. |
@eaquigley @mheppler @scolapasta @kcondon @mcrosas I move we re-name and re-scope this issu to be about making what is now the ":AllowSignup" boolean a real feature with a real name. That is to say, the features purpose is to support one of the modes listed in the business requirements. Below it's "2) only remote authentication": "An installation should be able to configure to use: 1) only local authentication, 2) only remote authentication (support both Federated Login mode - all approved institutions from InCommon - and Specific Identity Provider(s)), 3) both 1 and 2" That is the say, the various modes should be given names, documented, etc. Please note also my comment about how local users can be created via API: #2838 (comment) . Should the "mode" control this? Should it remain an independent control for more flexibility? This is still an open question in my mind. |
Redirect to 403 (unauthorized) if you try to hack the URL. Also, only show "user.signup.tip" when you are really about to create.
As of f328110 the original security problem reported by @ajirnyi has been addressed. (This is still the 2939-shib branch.) While I was making that commit I went ahead and fixed the login tip problem @mheppler pointed out: #2838 (comment) Next I'll see if I can find mentions of signups per @shlake : #2838 (comment) . @eaquigley and @mheppler I deployed the latest to https://shibtest.dataverse.org and put it in "signups not allowed" mode with @majorseitan thanks for your suggestions. I didn't incorporate all of them but they helped. |
@shlake from what I can tell, the I'm saying I don't think there's anything to fix in this area. That said, @eaquigley you had some notes for @mheppler at #2838 (comment) ... I'm not sure if you want to get anything else in for this issue for phase 1. For phase 2 there's #2974 stubbed out that's related to this issue. I know that at #2838 (comment) I made noise about changing the behavior of how we block the creation of builtin users but I'm planning on deferring that decision until #2974 is talked about. If anyone feels strongly about this for phase 1, please leave a comment. Mental note that I still need to change the docs at http://guides.dataverse.org/en/4.2.4/installation/config.html#allowsignup Otherwise, I believe I'm done with this issue. |
Fixed styling for the warning msg in the Request Access popup on the dataset pg. Also was able to replicate what @pdurbin reported seeing, that the proper "Log In"-only msg was displaying in the two "Add Data" and "Request Access" popups after changing the settings for |
@mheppler 901e804 looks like a good fix. I see red triangles everywhere, which seems to be what you and @eaquigley are going for. Thanks. I think we're done code-wise. I still need to add a summary for QA. |
I'm passing this issue to QA. To me, the scope of this issue is a security fix for the problem originally reported above by @ajirnyi wherein even if you set I'm aware that http://guides.dataverse.org/en/2939-shib/installation/config.html#allowsignup still says "Set to false to disallow local accounts to be created if you are using Shibboleth but not for production use until #2838 has been fixed" and I'm happy to remove that warning after this issue has passed QA. I'd probably re-write that sentence, actually, because it's still technically possible to create accounts via API even when Please note that setting |
OK, links are removed, hacking results in 403, not allowed. Closing. |
I'm setting the milestone of this issue to 4.4 because it was just asked about at http://irclog.iq.harvard.edu/dataverse/2016-04-01 even though I'm not sure if pull request #3025 will land in 4.4 or 4.5. I just want to make it clear that the fix is not in 4.3 or earlier. |
Just a note, tested in 4.4, still allowed a user to signup.
|
If I set
:AllowSignup=no
by doingcurl -X PUT -d no "$SERVER/admin/settings/:AllowSignUp"
, it removes the "Sign Up" link at the top of the page; however, the actual URL "https://our.dataverse.server.edu/dataverseuser.xhtml?editMode=CREATE" remains functional, and allows creation of new local user accounts.It would be nice if the
:AllowSignup=no
setting actually disabled creation of new local accounts, not just removed a link from the navigation bar, while still allowing creation of new accounts for new users logging in with Shibboleth SSO.The text was updated successfully, but these errors were encountered: