-
Notifications
You must be signed in to change notification settings - Fork 492
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
Adding a mechanism for disabling new OAuth signups. #9112
Conversation
…ps for remote auth. of specific type (without blocking all the existing accounts of the type). #9111
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. A few comments:
• I think you noted that you would move the texts into bundles.
• This setting should be added to the docs
• Are there any tests we can add? There really isn't a concept of logging in through the API, so not sure what exactly we could add.
Did the cleanup/added the doc. section. |
I think this one has been seen before and may be timing related - another case where a sleepForLocks/sleepForReindex might help? |
I tested this on dataverse-internal and I'm satisfied that it's working properly. Will merge shortly. I tweaked the docs in cf00e61 to emphasize that even though "remote" is in the name of the new database setting, for now this only affects OAuth accounts. I also did a little reorg, cross-linking, clean up, etc. Here's how I tested. Regression testI checked that I could still create a fresh Google account with nothing configured. Then I deleted it. Am I prevented from creating a Google account?Yes, by following the example in the guides, I get this error when I try: With signup disabled, can I still log in with Google?Yes, works fine. Nice feature!
@qqmyers yes, I suspect so too. |
I just checked the tests on develop after I merged this pull request and they all passed: https://jenkins.dataverse.org/job/IQSS-dataverse-develop/1192/testReport/ So, yes, probably timing related. It looks like we fixed DownloadFilesIT.downloadAllFilesRestricted here but it wasn't timing related then: Anyway, we can create a new issue if it comes up again. |
What this PR does / why we need it:
We all know why we need it.
This implementation allows to disable new signups from specific remote/oauth type, without blocking all the existing accounts authenticated by this method.
This is controlled by a new compound setting
:AllowRemoteAuthSignUp
. By default, if the setting is not present, all remote signups are open. If the setting is present, but the value for this specific method is not specified, it is assumed that the signups are allowed.Examples:
disables all remote signups.
keeps signups open for all the methods except google. (but note that the
"default":"true"
part in this example is redundant, since it would default to true anyway for all the methods other than google).I am sure there are prettier ways to implement this. But, seeing how this is a bit of an emergency, I'm not super concerned about prettiness at this point. (I will move the error messages into the bundle though).
I am concerned about whether there are still ways to go around this somehow. So it would definitely help to know how those users were able to create their google accounts after I had quote-unquote "disabled" it.
Which issue(s) this PR closes:
Closes #9111
Special notes for your reviewer:
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: