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

Adding a mechanism for disabling new OAuth signups. #9112

Merged
merged 6 commits into from
Nov 3, 2022

Conversation

landreev
Copy link
Contributor

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:

curl -X PUT -d '{"default":"false"}' http://localhost:8080/api/admin/settings/:AllowRemoteAuthSignUp

disables all remote signups.

curl -X PUT -d '{"default":"true","google":"false"}' http://localhost:8080/api/admin/settings/:AllowRemoteAuthSignUp

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:

…ps for

remote auth. of specific type (without blocking all the existing accounts of
the type). #9111
Copy link
Contributor

@scolapasta scolapasta left a 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.

@landreev
Copy link
Contributor Author

landreev commented Nov 2, 2022

Did the cleanup/added the doc. section.
I couldn't think of how to add tests for this. It may be possible; just like I think it should be possible to implement this in some more elegant way. But I would handle any such improvements as a separate issue in a normal release cycle. This patch I'm just itching to throw in prod. asap.

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Review 🔎 to QA ✅ Nov 2, 2022
@scolapasta scolapasta removed their assignment Nov 2, 2022
@mreekie mreekie added this to the 5.12.1 milestone Nov 3, 2022
@pdurbin pdurbin self-assigned this Nov 3, 2022
@pdurbin
Copy link
Member

pdurbin commented Nov 3, 2022

I'm not sure why this test is failing:

Screen Shot 2022-11-03 at 2 22 14 PM

302 is this line (the second line). Seem very unrelated to this PR. And "develop" is fine.

    UtilIT.publishDatasetViaNativeApi(datasetPid, "major", apiToken)
            .then().assertThat().statusCode(OK.getStatusCode());

@qqmyers
Copy link
Member

qqmyers commented Nov 3, 2022

I think this one has been seen before and may be timing related - another case where a sleepForLocks/sleepForReindex might help?

@pdurbin
Copy link
Member

pdurbin commented Nov 3, 2022

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 test

I 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:

Screen Shot 2022-11-03 at 2 29 48 PM

Screen Shot 2022-11-03 at 2 30 00 PM

Screen Shot 2022-11-03 at 2 30 21 PM

With signup disabled, can I still log in with Google?

Yes, works fine.

Nice feature!


I think this one has been seen before and may be timing related - another case where a sleepForLocks/sleepForReindex might help?

@qqmyers yes, I suspect so too.

@pdurbin pdurbin merged commit a38585d into develop Nov 3, 2022
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA ✅ to Done 🚀 Nov 3, 2022
@pdurbin pdurbin changed the title Adding a mechanism for disabling new remote auth signups. Adding a mechanism for disabling new OAuth signups. Nov 3, 2022
@pdurbin
Copy link
Member

pdurbin commented Nov 3, 2022

may be timing related

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.

@pdurbin pdurbin removed their assignment Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Add a mechanism for turning off OAuth account signups without disabling existing OAuth accounts
5 participants