feat(lib): Add matrix auth lib #509
Conversation
| @@ -0,0 +1,516 @@ | |||
| # Copyright 2024 Canonical Ltd. | |||
| # Licensed under the Apache2.0. See LICENSE file in charm source for details. | |||
There was a problem hiding this comment.
This might be shipped without the LICENSE file., so I'd go for
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
There was a problem hiding this comment.
That's an interesting point and you made me have a look at other libs :)
So observability have a reference to a LICENSE file, while data platform do not for instance. We make a reference to a LICENSE file on the nginx route library.
~/dev/canonical
❯ head is-charms/synapse-operator/lib/charms/observability_libs/v0/juju_topology.py
# Copyright 2022 Canonical Ltd.
# See LICENSE file for licensing details.
"""## Overview.
This document explains how to use the `JujuTopology` class to
create and consume topology information from Juju in a consistent manner.
~/dev/canonical
❯ head is-charms/synapse-operator/lib/charms/data_platform_libs/v0/data_interfaces.py
# Copyright 2023 Canonical Ltd.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
~/dev/canonical
❯ head is-charms/synapse-operator/lib/charms/prometheus_k8s/v0/prometheus_scrape.py
# Copyright 2021 Canonical Ltd.
# See LICENSE file for licensing details.
~/dev/canonical
❯ head is-charms/synapse-operator/lib/charms/nginx_ingress_integrator/v0/nginx_route.py
# Copyright 2024 Canonical Ltd.
# Licensed under the Apache2.0. See LICENSE file in charm source for details.I see charmcraft doesn't care, which makes sense, so maybe @erinecon and @gregory-schiano can chip in?
Personally, I'm in favor of removing this and for consistency sake do a cleanup of our libs (like mentioned, at least nginx has that). I checked and bind-operator has that same reference.
Thanks for spotting! 👓
| dumped_model = self.model_dump(exclude_unset=True) | ||
| dumped_data = { | ||
| "shared_secret_id": dumped_model["shared_secret_id"], | ||
| "homeserver": dumped_model["homeserver"], |
There was a problem hiding this comment.
What will be the format?
Maubot, for example, expects something like http://10.1.74.65:8080
There was a problem hiding this comment.
Interesting point, the homeserver.yaml currently has server_name and public_baseurl - I'm saying this because I think all plugins over a plugin library will use the public interface. I'm not assuming they will share a network in any case (like a namespaced one, or an internal one etc.), and at least for the irc charm it will be a cross-model between the irc machine charm and the k8s synapse one. I would recommend the server_name and from there either the provide part prepends an "https://", or the requirer does it. I saw the public_baseurl is optional and requires a SAML integration, whereas you will always need to have a server_name set.
There was a problem hiding this comment.
Using server_name might be challenging in case of different URL (ubuntu.com x chat-server.ubuntu.com)
What about adding a config to set the URL that will be used for the integration? Default to server_name
There was a problem hiding this comment.
That's a fair point, the server name is indeed ubuntu.com and while Element clients look for a .wellknown file under the domain, plugins don't do that (looking at the irc plugin code it will not go that extra length of searching for a .wellknown file, it will connect to the name you give it). That's a great catch! I think the safe bet here is to have it as a config. I don't think we can get the DNS entry for the Synapse homeserver from Synapse, we can extract the homeserver, though clients resolve it again...
Yup I think it should be done - is there anything we have in place that you can point me to, @arturo-seijas (and @amandahla )? I spoke with @jdkandersson , I wasn't sure where to put the tests (do we follow a convention, so I have a matrix_auth.py lib and have a corresponding tests/unit/test_matrix_auth.py, or maybe tests/unit/lib/test_matrix_auth.py or even tests/unit/lib/synapse/v0/test_matrix_auth.py...) and do I wanted to see what to cover in a unit test (pydantic models, mocking with Harness, testing state with Scenario), have we done something in that direction? CCing @gregory-schiano as he mentioned tests for the lib already (several times 😄 ). |
here is how I've been doing it |
|
Test coverage for 65ec799 Static code analysis report |
Overview
Adds a plugins library to manage (charmed) Matrix/Synapse plugins.
Rationale
Decoupling of plugins from the main code to standalone charms.
Juju Events Changes
n/a
Module Changes
n/a
Library Changes
Adds a matrix_auth.py for authentication mechanisms between a Matrix server and a plugin.
Checklist
src-docsurgent,trivial,complex)