From 552229110cdc1c123ff3e636c305bca2d026fdea Mon Sep 17 00:00:00 2001 From: Dan Date: Mon, 12 Dec 2022 15:43:19 -0500 Subject: [PATCH] Removing two fields from user table that were not used (uid, name) Request email from open id clients, as this would provide a handy way to uniquely reference users when assigning to groups. During Login do a lookup on email if possible -- so that permissions assignments based on email can be connected when sigining in through openid. Don't use "open_id" for the service name on user accounts, use the iss string provided through open id, this will allow us to support more than one open id platform. Update the KeyCloak configuration so it is able to return email addresses for users -- which will make permission assignment easier in the future. Removed several unused commands in the user_service class. --- .../bin/spiffworkflow-realm.json | 27 ++++++++++ .../{4d75421c0af0_.py => e1d0d593c621_.py} | 10 ++-- .../config/permissions/development.yml | 3 +- .../config/permissions/example.yml | 6 ++- .../config/permissions/testing.yml | 7 +++ .../src/spiffworkflow_backend/models/user.py | 23 +-------- .../src/spiffworkflow_backend/routes/user.py | 10 ++-- .../services/authentication_service.py | 2 +- .../services/authorization_service.py | 22 +++++---- .../services/user_service.py | 49 ------------------- .../integration/test_openid_blueprint.py | 19 +++++-- .../unit/test_authorization_service.py | 2 +- 12 files changed, 79 insertions(+), 101 deletions(-) rename spiffworkflow-backend/migrations/versions/{4d75421c0af0_.py => e1d0d593c621_.py} (98%) diff --git a/spiffworkflow-backend/bin/spiffworkflow-realm.json b/spiffworkflow-backend/bin/spiffworkflow-realm.json index a30f53c1..3181284e 100644 --- a/spiffworkflow-backend/bin/spiffworkflow-realm.json +++ b/spiffworkflow-backend/bin/spiffworkflow-realm.json @@ -424,6 +424,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "admin@status.im", "firstName" : "", "lastName" : "", "credentials" : [ { @@ -446,6 +447,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "alex@sartography.com", "credentials" : [ { "id" : "81a61a3b-228d-42b3-b39a-f62d8e7f57ca", "type" : "password", @@ -465,6 +467,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "amir@status.im", "credentials" : [ { "id" : "e589f3ad-bf7b-4756-89f7-7894c03c2831", "type" : "password", @@ -484,6 +487,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "ciadmin1@status.im", "credentials" : [ { "id" : "111b5ea1-c2ab-470a-a16b-2373bc94de7a", "type" : "password", @@ -506,6 +510,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "ciuser1@status.im", "credentials" : [ { "id" : "762f36e9-47af-44da-8520-cf09d752497a", "type" : "password", @@ -528,6 +533,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "core@status.im", "firstName" : "", "lastName" : "", "credentials" : [ { @@ -550,6 +556,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "dan@sartography.com", "credentials" : [ { "id" : "d517c520-f500-4542-80e5-7144daef1e32", "type" : "password", @@ -569,6 +576,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "daniel@sartography.com", "credentials" : [ { "id" : "f240495c-265b-42fc-99db-46928580d07d", "type" : "password", @@ -588,6 +596,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "elizabeth@sartography.com", "credentials" : [ { "id" : "ae951ec8-9fc9-4f1b-b340-bbbe463ae5c2", "type" : "password", @@ -607,6 +616,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "fin@status.im", "firstName" : "", "lastName" : "", "credentials" : [ { @@ -629,6 +639,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "fin1@status.im", "firstName" : "", "lastName" : "", "credentials" : [ { @@ -651,6 +662,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "finance_user1@status.im", "credentials" : [ { "id" : "f14722ec-13a7-4d35-a4ec-0475d405ae58", "type" : "password", @@ -670,6 +682,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "harmeet@status.im", "credentials" : [ { "id" : "89c26090-9bd3-46ac-b038-883d02e3f125", "type" : "password", @@ -689,6 +702,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "j@status.im", "firstName" : "", "lastName" : "", "credentials" : [ { @@ -711,6 +725,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "jakub@status.im", "credentials" : [ { "id" : "ce141fa5-b8d5-4bbe-93e7-22e7119f97c2", "type" : "password", @@ -730,6 +745,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "jarrad@status.im", "credentials" : [ { "id" : "113e0343-1069-476d-83f9-21d98edb9cfa", "type" : "password", @@ -749,6 +765,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "jason@sartography.com", "credentials" : [ { "id" : "40abf32e-f0cc-4a17-8231-1a69a02c1b0b", "type" : "password", @@ -768,6 +785,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "jon@sartography.com", "credentials" : [ { "id" : "8b520e01-5b9b-44ab-9ee8-505bd0831a45", "type" : "password", @@ -787,6 +805,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "kb@sartography.com", "credentials" : [ { "id" : "2c0be363-038f-48f1-86d6-91fdd28657cf", "type" : "password", @@ -806,6 +825,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "lead@status.im", "firstName" : "", "lastName" : "", "credentials" : [ { @@ -828,6 +848,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "lead1@status.im", "firstName" : "", "lastName" : "", "credentials" : [ { @@ -850,6 +871,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "manuchehr@status.im", "credentials" : [ { "id" : "07dabf55-b5d3-4f98-abba-3334086ecf5e", "type" : "password", @@ -869,6 +891,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "mike@sartography.com", "credentials" : [ { "id" : "1ed375fb-0f1a-4c2a-9243-2477242cf7bd", "type" : "password", @@ -888,6 +911,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "natalia@sartography.com", "credentials" : [ { "id" : "b6aa9936-39cc-4931-bfeb-60e6753de5ba", "type" : "password", @@ -907,6 +931,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "sasha@status.im", "credentials" : [ { "id" : "4a170af4-6f0c-4e7b-b70c-e674edf619df", "type" : "password", @@ -926,6 +951,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "service-account@status.im", "serviceAccountClientId" : "spiffworkflow-backend", "credentials" : [ ], "disableableCredentialTypes" : [ ], @@ -943,6 +969,7 @@ "enabled" : true, "totp" : false, "emailVerified" : false, + "email": "service-account-withauth@status.im", "serviceAccountClientId" : "withAuth", "credentials" : [ ], "disableableCredentialTypes" : [ ], diff --git a/spiffworkflow-backend/migrations/versions/4d75421c0af0_.py b/spiffworkflow-backend/migrations/versions/e1d0d593c621_.py similarity index 98% rename from spiffworkflow-backend/migrations/versions/4d75421c0af0_.py rename to spiffworkflow-backend/migrations/versions/e1d0d593c621_.py index 34fa1e97..cbe697cc 100644 --- a/spiffworkflow-backend/migrations/versions/4d75421c0af0_.py +++ b/spiffworkflow-backend/migrations/versions/e1d0d593c621_.py @@ -1,8 +1,8 @@ """empty message -Revision ID: 4d75421c0af0 +Revision ID: e1d0d593c621 Revises: -Create Date: 2022-12-06 17:42:56.417673 +Create Date: 2022-12-12 14:23:44.643766 """ from alembic import op @@ -10,7 +10,7 @@ import sqlalchemy as sa # revision identifiers, used by Alembic. -revision = '4d75421c0af0' +revision = 'e1d0d593c621' down_revision = None branch_labels = None depends_on = None @@ -72,14 +72,12 @@ def upgrade(): op.create_table('user', sa.Column('id', sa.Integer(), nullable=False), sa.Column('username', sa.String(length=255), nullable=False), - sa.Column('uid', sa.String(length=50), nullable=True), sa.Column('service', sa.String(length=50), nullable=False), sa.Column('service_id', sa.String(length=255), nullable=False), - sa.Column('name', sa.String(length=255), nullable=True), sa.Column('email', sa.String(length=255), nullable=True), sa.PrimaryKeyConstraint('id'), sa.UniqueConstraint('service', 'service_id', name='service_key'), - sa.UniqueConstraint('uid') + sa.UniqueConstraint('username') ) op.create_table('message_correlation_property', sa.Column('id', sa.Integer(), nullable=False), diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/config/permissions/development.yml b/spiffworkflow-backend/src/spiffworkflow_backend/config/permissions/development.yml index 419c925f..a0d030f8 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/config/permissions/development.yml +++ b/spiffworkflow-backend/src/spiffworkflow_backend/config/permissions/development.yml @@ -63,6 +63,7 @@ groups: harmeet, ] +# permission "admin" permissions: admin: groups: [admin] @@ -70,6 +71,7 @@ permissions: allowed_permissions: [create, read, update, delete] uri: /* + # permission: "basic" tasks-crud: groups: [everybody] users: [] @@ -81,7 +83,6 @@ permissions: allowed_permissions: [read] uri: /v1.0/service-tasks - # read all for everybody read-all-process-groups: groups: [everybody] diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/config/permissions/example.yml b/spiffworkflow-backend/src/spiffworkflow_backend/config/permissions/example.yml index 79bfed81..493aab1f 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/config/permissions/example.yml +++ b/spiffworkflow-backend/src/spiffworkflow_backend/config/permissions/example.yml @@ -2,14 +2,17 @@ default_group: everybody users: admin: + service: local_open_id email: admin@spiffworkflow.org password: admin preferred_username: Admin nelson: + service: local_open_id email: nelson@spiffworkflow.org password: nelson preferred_username: Nelson malala: + service: local_open_id email: malala@spiffworkflow.org password: malala preferred_username: Malala @@ -72,8 +75,7 @@ permissions: users: [ ] allowed_permissions: [ read ] uri: /v1.0/processes - - # Members of the Education group can change they processes work. + # Members of the Education group can change the processes under "education". education-admin: groups: ["Education", "President"] users: [] diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/config/permissions/testing.yml b/spiffworkflow-backend/src/spiffworkflow_backend/config/permissions/testing.yml index c678205d..429aacdf 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/config/permissions/testing.yml +++ b/spiffworkflow-backend/src/spiffworkflow_backend/config/permissions/testing.yml @@ -1,5 +1,12 @@ default_group: everybody +users: + testadmin1: + service: https://testing/openid/thing + email: testadmin1@spiffworkflow.org + password: admin + preferred_username: El administrador de la muerte + groups: admin: users: [testadmin1, testadmin2] diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/models/user.py b/spiffworkflow-backend/src/spiffworkflow_backend/models/user.py index b8c83d0f..60d40e9d 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/models/user.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/models/user.py @@ -28,14 +28,10 @@ class UserModel(SpiffworkflowBaseDBModel): __tablename__ = "user" __table_args__ = (db.UniqueConstraint("service", "service_id", name="service_key"),) - id = db.Column(db.Integer, primary_key=True) - # server and service id must be unique, not username. - username = db.Column(db.String(255), nullable=False, unique=False) - uid = db.Column(db.String(50), unique=True) - service = db.Column(db.String(50), nullable=False, unique=False) + username = db.Column(db.String(255), nullable=False, unique=True) # should always be an email address. + service = db.Column(db.String(50), nullable=False, unique=False) # not 'openid' -- google, aws service_id = db.Column(db.String(255), nullable=False, unique=False) - name = db.Column(db.String(255)) email = db.Column(db.String(255)) user_group_assignments = relationship("UserGroupAssignmentModel", cascade="delete") # type: ignore @@ -47,21 +43,6 @@ class UserModel(SpiffworkflowBaseDBModel): ) principal = relationship("PrincipalModel", uselist=False) # type: ignore - @validates("service") - def validate_service(self, key: str, value: Any) -> str: - """Validate_service.""" - try: - ap_type = getattr(AuthenticationProviderTypes, value, None) - except Exception as e: - raise ValueError(f"invalid service type: {value}") from e - if ap_type is not None: - ap_value: str = ap_type.value - return ap_value - raise ApiError( - error_code="invalid_service", - message=f"Could not validate service with value: {value}", - ) - def encode_auth_token(self) -> str: """Generate the Auth Token. diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/routes/user.py b/spiffworkflow-backend/src/spiffworkflow_backend/routes/user.py index 2bbbc137..b1ca7b90 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/routes/user.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/routes/user.py @@ -75,7 +75,7 @@ def verify_token( except ApiError as ae: # API Error is only thrown in the token is outdated. # Try to refresh the token user = UserService.get_user_by_service_and_service_id( - "open_id", decoded_token["sub"] + decoded_token["iss"], decoded_token["sub"] ) if user: refresh_token = AuthenticationService.get_refresh_token(user.id) @@ -107,7 +107,7 @@ def verify_token( user_info is not None and "error" not in user_info ): # not sure what to test yet user_model = ( - UserModel.query.filter(UserModel.service == "open_id") + UserModel.query.filter(UserModel.service == user_info["iss"]) .filter(UserModel.service_id == user_info["sub"]) .first() ) @@ -340,9 +340,5 @@ def get_user_from_decoded_internal_token(decoded_token: dict) -> Optional[UserMo ) if user: return user - user = UserModel( - username=service_id, - service=service, - service_id=service_id, - ) + user = UserService.create_user(service, service_id, username=service_id) return user diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/services/authentication_service.py b/spiffworkflow-backend/src/spiffworkflow_backend/services/authentication_service.py index f4bd357b..42cf6a8e 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/services/authentication_service.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/services/authentication_service.py @@ -89,7 +89,7 @@ class AuthenticationService: + f"?state={state}&" + "response_type=code&" + f"client_id={self.client_id()}&" - + "scope=openid&" + + "scope=openid email&" + f"redirect_uri={return_redirect_url}" ) return login_redirect_url diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py b/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py index 9456f8f1..35c94afe 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/services/authorization_service.py @@ -450,18 +450,23 @@ class AuthorizationService: def create_user_from_sign_in(cls, user_info: dict) -> UserModel: """Create_user_from_sign_in.""" is_new_user = False - user_model = ( - UserModel.query.filter(UserModel.service == "open_id") - .filter(UserModel.service_id == user_info["sub"]) - .first() - ) + if user_info.get('email', None) is not None: + user_model = ( + UserModel.query.filter(UserModel.email == user_info["email"]).first() + ) + else: + user_model = ( + UserModel.query.filter(UserModel.service == user_info["iss"]) + .filter(UserModel.service_id == user_info["sub"]) + .first() + ) if user_model is None: current_app.logger.debug("create_user in login_return") is_new_user = True - name = username = email = "" + username = email = "" if "name" in user_info: - name = user_info["name"] + username = user_info["name"] if "username" in user_info: username = user_info["username"] elif "preferred_username" in user_info: @@ -469,9 +474,8 @@ class AuthorizationService: if "email" in user_info: email = user_info["email"] user_model = UserService().create_user( - service="open_id", + service=user_info["iss"], service_id=user_info["sub"], - name=name, username=username, email=email, ) diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/services/user_service.py b/spiffworkflow-backend/src/spiffworkflow_backend/services/user_service.py index 0e8e65c2..b5898c13 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/services/user_service.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/services/user_service.py @@ -23,7 +23,6 @@ class UserService: cls, service: str, service_id: str, - name: Optional[str] = "", username: Optional[str] = "", email: Optional[str] = "", ) -> UserModel: @@ -41,7 +40,6 @@ class UserService: username=username, service=service, service_id=service_id, - name=name, email=email, ) db.session.add(user_model) @@ -69,45 +67,12 @@ class UserService: ) ) - @classmethod - def find_or_create_user( - cls, - service: str, - service_id: str, - name: Optional[str] = None, - username: Optional[str] = None, - email: Optional[str] = None, - ) -> UserModel: - """Find_or_create_user.""" - user_model: UserModel - try: - user_model = cls.create_user( - service=service, - service_id=service_id, - name=name, - username=username, - email=email, - ) - except ApiError: - user_model = ( - UserModel.query.filter(UserModel.service == service) - .filter(UserModel.service_id == service_id) - .first() - ) - return user_model - # Returns true if the current user is logged in. @staticmethod def has_user() -> bool: """Has_user.""" return "token" in g and bool(g.token) and "user" in g and bool(g.user) - # Returns true if the given user uid is different from the current user's uid. - @staticmethod - def is_different_user(uid: str) -> bool: - """Is_different_user.""" - return UserService.has_user() and uid is not None and uid is not g.user.uid - @staticmethod def current_user() -> Any: """Current_user.""" @@ -117,20 +82,6 @@ class UserService: ) return g.user - @staticmethod - def in_list(uids: list[str]) -> bool: - """Returns true if the current user's id is in the given list of ids. - - False if there is no user, or the user is not in the list. - """ - if ( - UserService.has_user() - ): # If someone is logged in, lock tasks that don't belong to them. - user = UserService.current_user() - if user.uid in uids: - return True - return False - @staticmethod def get_principal_by_user_id(user_id: int) -> PrincipalModel: """Get_principal_by_user_id.""" diff --git a/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_openid_blueprint.py b/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_openid_blueprint.py index 20a0bb67..54130c93 100644 --- a/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_openid_blueprint.py +++ b/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_openid_blueprint.py @@ -1,4 +1,8 @@ """Test_authentication.""" +import base64 +import time + +import jwt from flask import Flask from flask.testing import FlaskClient from tests.spiffworkflow_backend.helpers.base_test import BaseTest @@ -44,13 +48,16 @@ class TestFlaskOpenId(BaseTest): client: FlaskClient, with_db_and_bpmn_file_cleanup: None, ) -> None: + + code = ("testadmin1:1234123412341234") + """It should be possible to get a token.""" - code = ( - "c3BpZmZ3b3JrZmxvdy1iYWNrZW5kOkpYZVFFeG0wSmhRUEx1bWdIdElJcWY1MmJEYWxIejBx" - ) + backend_basic_auth_string = code + backend_basic_auth_bytes = bytes(backend_basic_auth_string, encoding="ascii") + backend_basic_auth = base64.b64encode(backend_basic_auth_bytes) headers = { "Content-Type": "application/x-www-form-urlencoded", - "Authorization": f"Basic {code}", + "Authorization": f"Basic {backend_basic_auth.decode('utf-8')}", } data = { "grant_type": "authorization_code", @@ -59,3 +66,7 @@ class TestFlaskOpenId(BaseTest): } response = client.post("/openid/token", data=data, headers=headers) assert response + assert response.is_json + assert 'access_token' in response.json + assert 'id_token' in response.json + assert 'refresh_token' in response.json diff --git a/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_authorization_service.py b/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_authorization_service.py index 00622a1f..11108cd6 100644 --- a/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_authorization_service.py +++ b/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_authorization_service.py @@ -134,7 +134,7 @@ class TestAuthorizationService(BaseTest): active_task.task_name, processor.bpmn_process_instance ) finance_user = AuthorizationService.create_user_from_sign_in( - {"username": "testuser2", "sub": "open_id"} + {"username": "testuser2", "sub": "open_id", "iss": "https://test.stuff"} ) ProcessInstanceService.complete_form_task( processor, spiff_task, {}, finance_user, active_task