-
Notifications
You must be signed in to change notification settings - Fork 5
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
network mode #589
base: master
Are you sure you want to change the base?
network mode #589
Changes from 1 commit
e08cc4f
758b15f
75ccf61
799ad23
5d22688
4c79d8a
2c2fd34
35090da
79714b2
18ccc4c
7ec1c9b
50aa4e5
3e266c7
38410de
4a6ed0d
d690c41
5f4087d
18ee9dd
a3aa357
1bf5584
3e94161
5ec26f9
c632d08
50a86b7
c32501b
521af09
1029847
9be6121
aa1e9e4
ef2ebd1
b337464
f5de462
72d2bb3
93d177f
fedc5ed
7f6a8a0
249f4a1
df90ba2
9de7fee
e56504e
aa8f43d
d4b01b1
cd9c048
8107ebb
18bc104
add132e
a140783
34a1dd0
7002b2d
336e5c3
704f2bc
5f587de
76944c4
8c03aab
a34a060
ae9caf6
37b9f62
c8385c9
bd2da76
c5057cb
9a839b7
64894e3
3e903fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -422,3 +422,84 @@ Furthermore, as described in the `procedure`_, :envvar:`MAGPIE_USER_REGISTRATION | |
specify whether administrator approval is required or not. This additional step is purely up to the developers and | ||
server managers that use `Magpie` to decide if they desire more control over which individuals can join and access | ||
their services. | ||
|
||
.. _Network Mode: | ||
|
||
Network Mode | ||
------------ | ||
|
||
If the :envvar:`MAGPIE_NETWORK_MODE` is enabled, an additional external authentication provider is added to Magpie which | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
allows networked instances of Magpie to authenticate users for each other. | ||
|
||
Users can then log in to any Magpie instance where they have an account and request a personal network token in the form | ||
of a `_JSON Web Token <https://datatracker.ietf.org/doc/html/rfc7519>`_ which can be used to authenticate this user on | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bad underscore at start |
||
any Magpie instance in the network. | ||
|
||
Managing the Network | ||
~~~~~~~~~~~~~~~~~~~~ | ||
|
||
In order for Magpie instances to authenticate each other's users, each instance must be made aware of the existence of | ||
the others so that it knows where to send authentication verification requests. | ||
|
||
In order to register another Magpie instance as part of the same network, an admin user can create a | ||
:term:`Network Node` with a request to ``POST /network-nodes``. The parameters given to that request include a ``name`` | ||
and a ``url``. The ``name`` is a the name of that other Magpie instance in the network and should correspond to the | ||
same value as the :envvar:`MAGPIE_INSTANCE_NAME` value set by the other Magpie instance. The ``url`` is a the root URL | ||
of the other Magpie instance. | ||
|
||
Once a :term:`Network Node` is registered, Magpie can use that other instance to authenticate users as long as the other | ||
instance also has :envvar:`MAGPIE_NETWORK_MODE` enabled. | ||
|
||
Managing Personal JWTs | ||
~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
A :term:`User` can request a new network token with a request to the ``PATCH /token`` route. This route takes one | ||
optional parameter ``expires`` which is an integer indicating how long (in seconds) until that token expires, the | ||
default expiry for this token is :envvar:`MAGPIE_DEFAULT_TOKEN_EXPIRY`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is usually not up to the user to decide the expiry time for security reasons. |
||
|
||
Every time a :term:`User` makes a request to the ``PATCH /token`` route a new token is generated for them. This | ||
effectively cancels all previously created tokens for that user. If a user wishes to cancel all tokens, they can provide | ||
an ``expires`` value of ``0`` when making the request. | ||
|
||
Authentication | ||
~~~~~~~~~~~~~~ | ||
|
||
Once a :term:`User` gets a personal network token, they can use that token to authenticate with any Magpie instance in | ||
the same network. When a user makes a request, they should set the ``provider_name`` parameter to the value of | ||
:envvar:`MAGPIE_NETWORK_PROVIDER` and provide the network token in the Authorization header in the following format: | ||
|
||
.. code-block:: http | ||
|
||
Authorization: Bearer <network_token> | ||
|
||
When using the :ref:`Magpie Adapter <utilities_adapter>`, the token can also be passed as a parameter to the request, | ||
where the parameter name set by :envvar:`MAGPIE_NETWORK_TOKEN_NAME` and the value is the personal network token. | ||
|
||
Authorization | ||
~~~~~~~~~~~~~ | ||
|
||
Managing authorization for :term:`Users` who authenticate using personal network tokens is complicated by the fact that | ||
a :term:`User` is not required to have a full account on both Magpie instances in order to using this authentication | ||
mechanism. This means that a :term:`User` may be logged in as a node-specific "anonymous" user. | ||
|
||
When another Magpie instance is registered as a :term:`Network Node`, a few additional entities are created: | ||
|
||
#. a group used to manage the permissions of all users who authenticate using the new :term:`Network Node`. | ||
* this group's name will be the :envvar:`MAGPIE_NETWORK_NAME_PREFIX` followed by the :term:`Network Node` name | ||
#. a group used to manage the permissions of all users who authenticate using *any* other instance in the network | ||
* this group's name will be the :envvar:`MAGPIE_NETWORK_GROUP_NAME` | ||
* this group will only be created once, when the first :term:`Network Node` is registered | ||
#. an anonymous user that belongs to the two groups that were just created. | ||
* this user name will be the :envvar:`MAGPIE_NETWORK_NAME_PREFIX` followed by the :term:`Network Node` name | ||
Comment on lines
+524
to
+525
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a valid user name (eg: Or maybe this is just an impression? But it looks like it could be possible to inject the "right values" to cause side effects. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the Now, if you already had a user in the system named If this is easier though, I'm happy to split these users out into different tables as you suggest (#589 (review)) if that will keep things cleaner. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think that would make it easier conceptually. The API endpoints however would still need to consider these conditions to allow user names or not. I think you mostly already did it with the regexes, but tests must be done to make sure it works everywhere (trying to purposely cause conflict, edge cases and such). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I have some tests on the way but they're still in progress :) |
||
|
||
Here is an example to illustrate this point: | ||
|
||
* There are 3 Magpie instances in the network named A, B, and C | ||
* There is a :term:`User` named ``"toto"`` registered on instance A | ||
* There is no :term:`User` named ``"toto"`` who belongs to the ``"anonymous_network_A"`` group registered on instance B | ||
* There is a :term:`User` named ``"toto"`` who belongs to the ``"anonymous_network_A"`` group registered on instance C | ||
* Instance A is registered as a :term:`Network Node` on instances B and C | ||
* when ``"toto"`` gets a personal network token from instance A and uses it to log in on instance B they log in as the | ||
the temporary ``"anonymous_network_A"`` user. | ||
* when ``"toto"`` gets a personal network token from instance A and uses it to log in on instance C they log in as the | ||
``"toto"`` user on instance C. | ||
Comment on lines
+536
to
+537
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if user There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are logged in at the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. Will need to investigate more how conflicting user names are handled. Basically, we shouldn't assume that a name would necessarily be exactly the same on other Magpie instances. Preferably, alternative/remote Magpie instances should allow to "link" users of different names, similarly to how you can have 2 distinct user names on distinct social media platforms, but link accounts together. Retrieving the "resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I see what you mean. I'm feeling strongly from this review that we should be handling these remote users by storing them separately in another db table. That would resolve a lot of the issues that you raise here about conflicting names as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remote users are now stored in a different table: |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -969,6 +969,83 @@ remain available as described at the start of the :ref:`Configuration` section. | |
Name of the :term:`Provider` used for login. This represents the identifier that is set to define how to | ||
differentiate between a local sign-in procedure and a dispatched one some known :ref:`authn_providers`. | ||
|
||
Network Mode Settings | ||
~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
The following configuration parameters are related to Magpie's "Network Mode" which allows networked instances of Magpie | ||
to authenticate users for each other. All variables defined in this section are only used if | ||
:envvar:`MAGPIE_NETWORK_MODE` is enabled. | ||
|
||
.. envvar:: MAGPIE_NETWORK_MODE | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
|
||
[:class:`bool`] | ||
(Default: ``False``) | ||
|
||
.. versionadded:: 3.37 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2 versions ahead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I assumed that this change would be its own version and the current unreleased changes would be made into version 3.36 before then. But I don't know how you decide on your releases for Magpie so I can change this if that's not the case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's fine. It's mostly a reminder for adjusting is as necessary when the feature is completed. |
||
|
||
Enable "Network Mode" which enables all functionality to authenticate users using other Magpie instances as | ||
external authentication providers. | ||
|
||
.. envvar:: MAGPIE_INSTANCE_NAME | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefix with |
||
|
||
[:class:`str`] | ||
|
||
.. versionadded:: 3.37 | ||
|
||
The name of this Magpie instance in the network. This variable is used to determine if an authentication token was | ||
issued by this instance of Magpie, or another instance in the network. | ||
|
||
This variable is required if :envvar:`MAGPIE_NETWORK_MODE` is ``True``. | ||
|
||
.. envvar:: MAGPIE_DEFAULT_TOKEN_EXPIRY | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefix with |
||
|
||
[:class:`int`] | ||
(Default: ``86400``) | ||
|
||
.. versionadded:: 3.37 | ||
|
||
The default expiry time (in seconds) for an authentication token issued for the purpose of network authentication. | ||
|
||
.. envvar:: MAGPIE_NETWORK_TOKEN_NAME | ||
|
||
[|constant|_] | ||
(Value: ``"magpie_token"``) | ||
|
||
.. versionadded:: 3.37 | ||
|
||
The name of the request parameter key whose value is the authentication token issued for the purpose of network | ||
authentication. | ||
|
||
.. envvar:: MAGPIE_NETWORK_PROVIDER | ||
|
||
[|constant|_] | ||
(Value: ``"magpie_network"``) | ||
|
||
.. versionadded:: 3.37 | ||
|
||
The name of the external provider that authenticates users using other Magpie instances as external authentication | ||
providers. | ||
|
||
.. envvar:: MAGPIE_NETWORK_NAME_PREFIX | ||
|
||
[|constant|_] | ||
(Value: ``"anonymous_network_"``) | ||
|
||
.. versionadded:: 3.37 | ||
|
||
A prefix added to the anonymous network user and network group names. These names are constructed by prepending the | ||
remote Magpie instance name with this prefix. For example, a Magpie instance named ``"example123"`` will have a | ||
corresponding user and group named ``"anonymous_network_example123"``. | ||
|
||
.. envvar:: MAGPIE_NETWORK_GROUP_NAME | ||
|
||
[|constant|_] | ||
(Value: ``"magpie_network"``) | ||
|
||
.. versionadded:: 3.37 | ||
|
||
The name of the group created to manage permissions for all users authenticated using Magpie instances as external | ||
authentication providers. | ||
|
||
.. _config_phoenix: | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -269,11 +269,18 @@ def update_request_cookies(self, request): | |
""" | ||
settings = get_settings(request) | ||
token_name = get_constant("MAGPIE_COOKIE_NAME", settings_container=settings) | ||
network_mode = get_constant("MAGPIE_NETWORK_MODE", settings_container=settings, | ||
settings_name="magpie.network_mode") | ||
headers = dict(request.headers) | ||
network_token_name = get_constant("MAGPIE_NETWORK_TOKEN_NAME", settings_container=settings) | ||
if network_mode and "Authorization" not in headers and network_token_name in request.params: | ||
headers["Authorization"] = "Bearer {}".format(request.params[network_token_name]) | ||
Comment on lines
+275
to
+276
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use the It seems the endpoint for providers is used also in the login code, but here a separate request parameter is used instead of reusing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think the idea here was to provide an option for cases when a user wants to access a resource and doesn't have the ability to set the request headers. For example, if they're using a tool that just asks for a URL. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I can understand the logic behind it. If really necessary for tools with limited capabilities, that can be permitted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I completely agree. The short lifespan of these tokens is supposed to mitigate this risk a bit. |
||
request.params["provider_name"] = request.params.get("provider_name", | ||
get_constant("MAGPIE_NETWORK_PROVIDER", settings)) | ||
if "Authorization" in request.headers and token_name not in request.cookies: | ||
magpie_prov = request.params.get("provider_name", get_constant("MAGPIE_DEFAULT_PROVIDER", settings)) | ||
magpie_path = ProviderSigninAPI.path.format(provider_name=magpie_prov) | ||
magpie_auth = "{}{}".format(self.magpie_url, magpie_path) | ||
headers = dict(request.headers) | ||
headers.update({"Homepage-Route": "/session", "Accept": CONTENT_TYPE_JSON}) | ||
session_resp = requests.get(magpie_auth, headers=headers, verify=self.twitcher_ssl_verify) | ||
if session_resp.status_code != HTTPOk.code: | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,54 @@ | ||||||||
""" | ||||||||
Add Network_Tokens Table | ||||||||
|
||||||||
Revision ID: 2cfe144538e8 | ||||||||
Revises: 5e5acc33adce | ||||||||
Create Date: 2023-08-25 13:36:16.930374 | ||||||||
""" | ||||||||
import uuid | ||||||||
|
||||||||
import sqlalchemy as sa | ||||||||
from alembic import op | ||||||||
from sqlalchemy.dialects.postgresql import UUID | ||||||||
from sqlalchemy.orm.session import sessionmaker | ||||||||
from sqlalchemy_utils import URLType | ||||||||
|
||||||||
# Revision identifiers, used by Alembic. | ||||||||
# pylint: disable=C0103,invalid-name # revision control variables not uppercase | ||||||||
revision = "2cfe144538e8" | ||||||||
down_revision = "5e5acc33adce" | ||||||||
branch_labels = None | ||||||||
depends_on = None | ||||||||
|
||||||||
Session = sessionmaker() | ||||||||
|
||||||||
|
||||||||
def upgrade(): | ||||||||
op.create_table("network_tokens", | ||||||||
sa.Column("token", UUID(as_uuid=True), | ||||||||
primary_key=True, default=uuid.uuid4, unique=True), | ||||||||
sa.Column("user_id", sa.Integer, | ||||||||
sa.ForeignKey("users.id", onupdate="CASCADE", ondelete="CASCADE"), unique=True) | ||||||||
) | ||||||||
op.create_table("network_nodes", | ||||||||
sa.Column("id", sa.Integer, primary_key=True, autoincrement=True), | ||||||||
sa.Column("name", sa.Unicode(128), nullable=False, unique=True), | ||||||||
sa.Column("url", URLType(), nullable=False) | ||||||||
) | ||||||||
op.add_column("users", sa.Column("network_node_id", sa.Integer, | ||||||||
sa.ForeignKey("network_nodes.id", onupdate="CASCADE", ondelete="CASCADE"), | ||||||||
nullable=True)) | ||||||||
op.drop_constraint("uq_users_user_name", "users") | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cannot remove this constraint without a lot of validation that all the code still works. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I tried to search through to catch all the cases for the current searches and enforce that the associated network node be null. Another pair of eyes would be great. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general, whenever This is an important one: Line 193 in 1d602ee
There are also some considerations to be taken for Line 210 in 1d602ee
and Line 921 in 1d602ee
which generates a somewhat related definition to a future user, but that is only really "linked" via This code never checks for potential multi-results returned from Similarly, all webhooks events for interactions with Cowbird work using |
||||||||
op.create_unique_constraint("uq_users_user_name_network_node_id", "users", ["user_name", "network_node_id"]) | ||||||||
op.drop_constraint("uq_users_email", "users") | ||||||||
op.create_unique_constraint("uq_users_email_network_node_id", "users", ["email", "network_node_id"]) | ||||||||
|
||||||||
|
||||||||
def downgrade(): | ||||||||
op.drop_constraint("uq_users_user_name_network_node_id", "users") | ||||||||
op.create_unique_constraint("uq_users_user_name", "users", ["user_name"]) | ||||||||
op.drop_constraint("uq_users_email_network_node_id", "users") | ||||||||
op.create_unique_constraint("uq_users_email", "users", ["email"]) | ||||||||
op.drop_table("network_tokens") | ||||||||
op.drop_table("network_nodes") | ||||||||
op.drop_column("users", "network_node_id") | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably need to consider dropping the duplicate user-names that have a |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
from magpie.constants import get_constant | ||
from magpie.utils import get_logger | ||
|
||
LOGGER = get_logger(__name__) | ||
|
@@ -11,4 +12,7 @@ def includeme(config): | |
config.add_route(**s.service_api_route_info(s.SigninAPI)) | ||
config.add_route(**s.service_api_route_info(s.ProvidersAPI)) | ||
config.add_route(**s.service_api_route_info(s.ProviderSigninAPI)) | ||
if get_constant("MAGPIE_NETWORK_MODE", settings_name="magpie.network_mode"): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should pass the config to access settings. |
||
config.add_route(**s.service_api_route_info(s.TokenAPI)) | ||
config.add_route(**s.service_api_route_info(s.TokenValidateAPI)) | ||
config.scan() |
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.
missing instance name setting