-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add an Admin API to temporarily grant the ability to update an existing cross-signing key without UIA #16634
Conversation
This endpoint is not intended for server administrator usage; | ||
we describe it here for completeness. |
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.
Wasn't sure if I should omit this. But I thought it might be best to err on the side of transparency.
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.
I'm happy to keep this documented, it's clear enough that as an admin you shouldn't use that
Returns: a 2-tuple of booleans | ||
- whether the user has cross-signing set up, and | ||
- whether the user's master cross-signing key may be replaced without UIA. |
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.
A bit naughty, since there aren't really 4 states here. But I think this makes things clearer at the call site.
""" | ||
|
||
PATTERNS = admin_patterns( | ||
"/users/(?P<user_id>[^/]*)/_allow_cross_signing_replacement_without_uia" |
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.
@sandhose and I wondered if this should come with warnings e.g. "This is an internal endpoint, we reserve the right to break this". That's what the _
in _allow
means.
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
ALTER TABLE e2e_cross_signing_keys ADD COLUMN updatable_without_uia_before_ms bigint DEFAULT NULL; |
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.
I did think about adding a check constraint to enforce that only master keys may have this column set to a non-null value. But then I'd have to think about adding a background update to validate the check in the background; it didn't seem worth it.
synapse/storage/schema/main/delta/83/05_cross_signing_key_update_grant.sql
Show resolved
Hide resolved
}, | ||
} | ||
) | ||
def test_master_cross_signing_key_replacement_msc3861(self) -> None: |
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.
That is way more testing that I'd have expected, nice!
This endpoint is not intended for server administrator usage; | ||
we describe it here for completeness. |
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.
I'm happy to keep this documented, it's clear enough that as an admin you shouldn't use that
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.
Everything looks good, thanks!
Fixes #16632.