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

Add waffle flag to turn submissions on or off #1830

Open
diox opened this issue May 26, 2020 · 19 comments · Fixed by mozilla/addons-server#22729 · May be fixed by mozilla/addons-server#22774
Open

Add waffle flag to turn submissions on or off #1830

diox opened this issue May 26, 2020 · 19 comments · Fixed by mozilla/addons-server#22729 · May be fixed by mozilla/addons-server#22774
Assignees
Labels
Milestone

Comments

@diox
Copy link
Member

diox commented May 26, 2020

We want to add something admins could easily toggle to turn submissions on or off (devhub + old and new APIs) in case of emergencies - we already have solutions, but they require ops, and we want something more granular.

Ideally, this would be some kind of custom Waffle flag, enabling us to have an option allowing submissions of add-ons matching some criteria (like recommended) to still go through if we so wish.

It should also return a custom error message and HTTP status code that clearly states why submissions are turned off temporarily (also something that waffle Flag model supports, through the note attribute - it would be English only, but that should be acceptable for this feature). We should make sure that error message is clearly visible in devhub and web-ext.

┆Issue is synchronized with this Jira Task

@stale

This comment was marked as outdated.

@stale stale bot added the state:stale Issues marked as stale. These can be re-opened should there be plans to fix them. label Nov 22, 2020
@diox

This comment was marked as outdated.

@stale stale bot removed the state:stale Issues marked as stale. These can be re-opened should there be plans to fix them. label Nov 30, 2020
@stale

This comment was marked as outdated.

@stale stale bot added the state:stale Issues marked as stale. These can be re-opened should there be plans to fix them. label Jun 2, 2021
@stale stale bot closed this as completed Jun 16, 2021
@willdurand willdurand reopened this Jun 17, 2021
@stale stale bot removed the state:stale Issues marked as stale. These can be re-opened should there be plans to fix them. label Jun 17, 2021
@diox diox added the neverstale Use this to tell stalebot to not touch this issue. Should be used infrequently. label Aug 12, 2021
@KevinMind
Copy link
Contributor

@KevinMind KevinMind transferred this issue from mozilla/addons-server May 3, 2024
@KevinMind KevinMind added repository:addons-server Issue relating to addons-server migration:2024 labels May 3, 2024
@diox diox added the needs:info label Sep 9, 2024
@diox
Copy link
Member Author

diox commented Sep 9, 2024

Need scope clarification on "have an option allowing submissions of add-ons matching some criteria (like recommended) to still go through if we so wish."

@diox diox removed the neverstale Use this to tell stalebot to not touch this issue. Should be used infrequently. label Sep 9, 2024
@diox
Copy link
Member Author

diox commented Sep 10, 2024

@wagnerand what do you think about #1830 (comment) ? If it's not needed, it obviously makes our life much easier. If it is, we need the exact criteria/mechanism wanted for bypass (which could be optional, but we still need to know what it is to build it).

@wagnerand
Copy link
Member

I could see an override option for admins to submit a new version for any add-on, in case there is a critical request from a developer (similar to admins being allowed to override a failed validation). I am not sure we should have exceptions for certain sets of add-ons.

@abyrne-moz might want to weigh in on this as well.

@wagnerand wagnerand removed their assignment Sep 11, 2024
@abyrne-moz
Copy link

Can you provide an example scenario of when we might need to use this? "In case of emergencies" isn't that clear to me, and I assume an emergency lasts a short period of time and therefore limits the need for an override option.

@diox
Copy link
Member Author

diox commented Sep 17, 2024

A bypass for admins is trivial for us to implement through a waffle Flag (we'd essentially be adding a Flag that defaults to everyone=True, and when we want to turn submissions off, we'd flip that everyone bit to False, and we could still have superusers left at True to allow admins to submit stuff)

@diox
Copy link
Member Author

diox commented Oct 8, 2024

@wagnerand @abyrne-moz

We need copy for this feature. Currently in the PR we have:

  • "Submissions have been temporarily disabled. Please check back later". That's the default if no message is set in the waffle Flag.
    On top of that, we have a special error page if somehow the developers bypasses disabled buttons or has unlucky timing and submits an add-on right as the submissions are being disabled. That page has a title and a header:
  • "Submissions Unavailable" (page title)
  • "Submissions are not currently available" (page header)

I'm thinking "Submissions" is too generic here. Could you help come up with something?

@abyrne-moz
Copy link

Would "Add-on uploads" be more appropriate?

  • "Add-on uploads have been temporarily disabled. Please check back later". That's the default if no message is set in the waffle Flag.
    

On top of that, we have a special error page if somehow the developers bypasses disabled buttons or has unlucky timing and submits an add-on right as the submissions are being disabled. That page has a title and a header:

  • "Add-on uploads temporarily disabled" (page title)
    
  • "Add-on uploads are temporarily unavailable" (page header)
    

@diox
Copy link
Member Author

diox commented Oct 8, 2024

Yeah I think that's good. cc @chrstinalin ^

@ioanarusiczki
Copy link

I did not change anything to the enable-submissions only switched between yes/no/unknown (so I left Superusers checked)

With switch off (no/unknown)

  • on dev hub add-on uploads are not available for a regular user or an admin (and I think I should be able to do that as an admin)
  • with web-ext I get different results. A first version submitted as a regular user or admin is not possible, I get an error message
    "Service Unavailable". But I can submit new versions (as a developer or admin) which I don't think is expected.

I must continue testing around but I thought I should leave an update so far (I hope I am using the correct settings for the flag)

@diox
Copy link
Member Author

diox commented Oct 16, 2024

We don't have the decorator on FileUploadViewSet.create() and we probably should, but that shouldn't be necessary to prevent web-ext submissions, since we still need to use the upload, so we're probably missing it somewhere else ? Need to investigate.

The admin not being able to bypass the flag if superusers is True is also weird.

@diox diox reopened this Oct 16, 2024
@ioanarusiczki
Copy link

ioanarusiczki commented Oct 16, 2024

I'll add my latest test results:

  • I tried new addon uploads and new versions from Postman with a regular and then an admin account with "*" permission. Here I hit the 503 every time (I could not make new version uploads with those I tried for web-ext 633391 or 633393)

503

themewizard zip

  • PATCH requests on addon or version endpoints are allowed.

@chrstinalin
Copy link

chrstinalin commented Oct 16, 2024

We don't have the decorator on FileUploadViewSet.create() and we probably should, but that shouldn't be necessary to prevent web-ext submissions, since we still need to use the upload, so we're probably missing it somewhere else ? Need to investigate.

I checked the web-ext repo, and it looks like they directly use /addons/upload in the submission process, i.e FileUploadViewSet.create(), which is possibly why it's behaving differently. I did have a decorator on that endpoint originally, but removed it for a reason that's... lost on me now. 🙃

The admin not being able to bypass the flag if superusers is True is also weird.

This I'm stumped on. Does flag_is_active not handle this? I'll look into it some more.

@diox
Copy link
Member Author

diox commented Oct 16, 2024

This I'm stumped on. Does flag_is_active not handle this? I'll look into it some more.

Also stumped on this. <flag>.is_active(request) should definitely handle this. There is also a <flag>.is_active_for_user(user) method and when creating a flag with everyone=False and superusers=True it seemed to work fine for me. Maybe some caching involved? Very weird.

@chrstinalin
Copy link

Submissions seem to disable correctly for me locally when everyone: None and superusers: True.

on dev hub add-on uploads are not available for a regular user or an admin (and I think I should be able to do that as an admin)

@ioanarusiczki could I ask what the exact steps you took here were?

@chrstinalin
Copy link

(Figured it out over Slack -- Everyone was set to No and overrode all other permissions).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment