Disallow adding ADMIN contacts via console

[gbrodman]

this is a potential security issue if we give users who only have EDIT_REGISTRAR_DETAILS access to become admin PoCs, which can have a different set of permissions.

this wasn't allowed by policy or by the web UI anyway, this is just tightening up the backend

---

This change is 

[CydeWeys]

What are the effects of making this change? Right now registrars are used to self-managing these admin contacts; what happens if they can no longer do so? How big of a load on our customer support does that become? Does our customer support have processes for managing these? (And presumably some sort of process to authenticate the incoming requests?)

@CydeWeys made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.

[CydeWeys]

And the PR description says "users who only have EDIT_REGISTRAR_DETAILS access to become admin PoCs", but my read on this is that it's preventing anyone from adding admin PoCs -- is that wrong? (Or at least anyone who's not a system-level admin, e.g. a registry employee.)

@CydeWeys made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.

[gbrodman]

nah they self-manage the users, but admin RegisrarPocs are limited (either by restrictions or by practice) to one per registrar and they're much more locked down in general. We do allow users with EDIT_REGISTRAR_DETAILS to change other types of RegistrarPocs though since those aren't as restricted

this is preventing anyone from adding admin RegistrarPocs via the console, yeah. We should only be doing that via the CLI.

@gbrodman made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.

[CydeWeys]

So it sounds like we need to communicate this change with registrars, and come up with a new process for tech support, as they'll now need to do these changes?

Note -- I'm not saying we have to wait 30 days after communication to deploy this change, just that we should communicate it, ideally in advance of it rolling out.

@CydeWeys made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.

[gbrodman]

No, we don't. You can't add or remove admin contacts via the console anyway already.

@gbrodman made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.

[CydeWeys]

Well, apparently you can, or this PR wouldn't be needed ...

Maybe clarify in the PR description something like "The web interface already does not permit this action, but the server-side code does not enforce the restriction".

Also more generally if you spent just a little more time writing up a little bit more context/explanation on your PR descriptions, it would save a lot of code review time / round-trips. It has been a recurring thing.

@CydeWeys made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.

[gbrodman]

Well, apparently you can, or this PR wouldn't be needed ...

No, as i specified "via the console" -- just saying that this is not changing existing console behavior.

description updated

i generally do include more description when i believe it's something that wouldn't be decently common knowledge or reasonable (such as #3148), but sure I can add more. This one seemed straightforward, since this is a policy we've talked about a lot, even before this round of bugs.

@gbrodman made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.

[CydeWeys]

Here is a better wording for the commit description that resolves all ambiguity, makes no assumptions, and will be significantly clearer in posterity years down the line:

Do not allow adding ADMIN contacts via server-side contact action

We already did not allow it through the registrar console web UI, but this prevents it on the server-side action as well, to prevent potential client-side bypasses.

@CydeWeys made 1 comment.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.