From 97eae3c211dd7bc39517c055a8a31aa51a4995fa Mon Sep 17 00:00:00 2001 From: Porin Custic Date: Wed, 8 Feb 2023 15:43:50 +0100 Subject: [PATCH 1/7] Remove Google oauth admin authentication provider --- api/admin/controller/__init__.py | 31 +-- api/admin/controller/admin_auth_services.py | 5 +- ...gle_oauth_admin_authentication_provider.py | 225 ---------------- api/admin/problem_details.py | 7 - api/admin/routes.py | 6 - core/model/configuration.py | 8 +- poetry.lock | 82 +----- pyproject.toml | 1 - .../controller/test_admin_auth_services.py | 209 +-------------- tests/api/admin/controller/test_controller.py | 243 ++---------------- ...gle_oauth_admin_authentication_provider.py | 148 ----------- tests/api/admin/test_routes.py | 4 - .../es/LC_MESSAGES/circulation-admin.po | 6 - 13 files changed, 28 insertions(+), 947 deletions(-) delete mode 100644 api/admin/google_oauth_admin_authentication_provider.py delete mode 100644 tests/api/admin/test_google_oauth_admin_authentication_provider.py diff --git a/api/admin/controller/__init__.py b/api/admin/controller/__init__.py index 0cf96800f6..884c5f053c 100644 --- a/api/admin/controller/__init__.py +++ b/api/admin/controller/__init__.py @@ -18,9 +18,6 @@ from api.admin.config import Configuration as AdminClientConfig from api.admin.exceptions import * -from api.admin.google_oauth_admin_authentication_provider import ( - GoogleOAuthAdminAuthenticationProvider, -) from api.admin.opds import AdminAnnotator, AdminFeed from api.admin.password_admin_authentication_provider import ( PasswordAdminAuthenticationProvider, @@ -224,14 +221,7 @@ def __init__(self, manager): def admin_auth_providers(self): auth_providers = [] auth_service = ExternalIntegration.admin_authentication(self._db) - if auth_service and auth_service.protocol == ExternalIntegration.GOOGLE_OAUTH: - auth_providers.append( - GoogleOAuthAdminAuthenticationProvider( - auth_service, - self.url_for("google_auth_callback"), - test_mode=self.manager.testing, - ) - ) + if Admin.with_password(self._db).count() != 0: auth_providers.append( PasswordAdminAuthenticationProvider( @@ -281,7 +271,7 @@ def authenticated_admin(self, admin_details): library = Library.lookup(self._db, role.get("library")) if role.get("library") and not library: self.log.warn( - "%s authentication provider specifiec an unknown library for a new admin: %s" + "%s authentication provider specified an unknown library for a new admin: %s" % (admin_details.get("type"), role.get("library")) ) else: @@ -597,23 +587,6 @@ def sign_in(self): elif admin: return redirect(flask.request.args.get("redirect"), Response=Response) - def redirect_after_google_sign_in(self): - """Uses the Google OAuth client to determine admin details upon - callback. Barring error, redirects to the provided redirect url..""" - if not self.admin_auth_providers: - return ADMIN_AUTH_NOT_CONFIGURED - - auth = self.admin_auth_provider(GoogleOAuthAdminAuthenticationProvider.NAME) - if not auth: - return ADMIN_AUTH_MECHANISM_NOT_CONFIGURED - - admin_details, redirect_url = auth.callback(self._db, flask.request.args) - if isinstance(admin_details, ProblemDetail): - return self.error_response(admin_details) - - admin = self.authenticated_admin(admin_details) - return redirect(redirect_url, Response=Response) - def password_sign_in(self): if not self.admin_auth_providers: return ADMIN_AUTH_NOT_CONFIGURED diff --git a/api/admin/controller/admin_auth_services.py b/api/admin/controller/admin_auth_services.py index c18d972a01..a34b6d4692 100644 --- a/api/admin/controller/admin_auth_services.py +++ b/api/admin/controller/admin_auth_services.py @@ -2,9 +2,6 @@ from flask import Response from flask_babel import lazy_gettext as _ -from api.admin.google_oauth_admin_authentication_provider import ( - GoogleOAuthAdminAuthenticationProvider, -) from api.admin.problem_details import * from core.model import ExternalIntegration, get_one, get_one_or_create from core.util.problem_detail import ProblemDetail @@ -15,7 +12,7 @@ class AdminAuthServicesController(SettingsController): def __init__(self, manager): super().__init__(manager) - provider_apis = [GoogleOAuthAdminAuthenticationProvider] + provider_apis = [] self.protocols = self._get_integration_protocols( provider_apis, protocol_name_attr="NAME" ) diff --git a/api/admin/google_oauth_admin_authentication_provider.py b/api/admin/google_oauth_admin_authentication_provider.py deleted file mode 100644 index 97bec56487..0000000000 --- a/api/admin/google_oauth_admin_authentication_provider.py +++ /dev/null @@ -1,225 +0,0 @@ -import json -from collections import defaultdict - -from flask_babel import lazy_gettext as _ -from oauth2client import client as GoogleClient - -from api.admin.template_styles import * -from core.model import ( - Admin, - AdminRole, - ConfigurationSetting, - ExternalIntegration, - Session, - get_one, -) - -from .admin_authentication_provider import AdminAuthenticationProvider -from .problem_details import GOOGLE_OAUTH_FAILURE, INVALID_ADMIN_CREDENTIALS - - -class GoogleOAuthAdminAuthenticationProvider(AdminAuthenticationProvider): - - NAME = ExternalIntegration.GOOGLE_OAUTH - DESCRIPTION = _("How to Configure a Google OAuth Integration") - DOMAINS = "domains" - - INSTRUCTIONS = _( - "

Configuring a Google OAuth integration in the Circulation Manager " - + "will allow admins to sign into the Admin interface with their Google/GMail credentials.

" - + "

Configure the Google OAuth Service:

" - + "
  1. To use this integration, visit the " - + "Google developer console. " - + "Create a project, click 'Create Credentials' in the left sidebar, and select 'OAuth client ID'. " - + "If you get a warning about the consent screen, click 'Configure consent screen' and enter your " - + "library name as the product name. Save the consent screen information.
    2. " - + "
  2. Choose 'Web Application' as the application type.
    3. " - + "
  3. Leave 'Authorized JavaScript origins' blank, but under 'Authorized redirect URIs', add the url " - + "of your circulation manager followed by '/admin/GoogleAuth/callback', e.g. " - + "'http://mycircmanager.org/admin/GoogleAuth/callback'.
    4. " - "
  4. Click create, and you'll get a popup with your new client ID and secret. " - + "Copy these values and enter them in the form below.
" - ) - - SETTINGS = [ - { - "key": ExternalIntegration.URL, - "label": _("Authentication URI"), - "default": "https://accounts.google.com/o/oauth2/auth", - "required": True, - "format": "url", - }, - { - "key": ExternalIntegration.USERNAME, - "label": _("Client ID"), - "required": True, - }, - { - "key": ExternalIntegration.PASSWORD, - "label": _("Client Secret"), - "required": True, - }, - ] - - LIBRARY_SETTINGS = [ - { - "key": DOMAINS, - "label": _("Allowed Domains"), - "description": _( - "Anyone who logs in with an email address from one of these domains will automatically have librarian-level access to this library. Library manager roles must still be granted individually by other admins. If you want to set up admins individually but still allow them to log in with Google, you can create the admin authentication service without adding any libraries." - ), - "type": "list", - }, - ] - SITEWIDE = True - - TEMPLATE = """ - Sign in with Google - """.format( - link_style - ) - - def __init__(self, integration, redirect_uri, test_mode=False): - super().__init__(integration) - self.redirect_uri = redirect_uri - self.test_mode = test_mode - if self.test_mode: - self.dummy_client = DummyGoogleClient() - - @property - def client(self): - if self.test_mode: - return self.dummy_client - - config = dict() - config["auth_uri"] = self.integration.url - config["client_id"] = self.integration.username - config["client_secret"] = self.integration.password - config["redirect_uri"] = self.redirect_uri - config["scope"] = "https://www.googleapis.com/auth/userinfo.email" - return GoogleClient.OAuth2WebServerFlow(**config) - - @property - def domains(self): - domains = defaultdict(list) - if self.integration: - _db = Session.object_session(self.integration) - for library in self.integration.libraries: - setting = ConfigurationSetting.for_library_and_externalintegration( - _db, self.DOMAINS, library, self.integration - ) - if setting.json_value: - for domain in setting.json_value: - domains[domain.lower()].append(library) - return domains - - def sign_in_template(self, redirect_url): - return self.TEMPLATE % dict(auth_uri=self.auth_uri(redirect_url)) - - def auth_uri(self, redirect_url): - return self.client.step1_get_authorize_url(state=redirect_url) - - def callback(self, _db, request={}): - """Google OAuth sign-in flow""" - - # The Google OAuth client sometimes hits the callback with an error. - # These will be returned as a problem detail. - error = request.get("error") - if error: - return self.google_error_problem_detail(error), None - auth_code = request.get("code") - if auth_code: - redirect_url = request.get("state") - try: - credentials = self.client.step2_exchange(auth_code) - except GoogleClient.FlowExchangeError as e: - return self.google_error_problem_detail(str(e)), None - email = credentials.id_token.get("email") - if not self.staff_email(_db, email): - return INVALID_ADMIN_CREDENTIALS, None - domain = email[email.index("@") + 1 :].lower() - roles = [] - for library in self.domains[domain]: - roles.append( - {"role": AdminRole.LIBRARIAN, "library": library.short_name} - ) - return ( - dict( - email=email, - credentials=credentials.to_json(), - type=self.NAME, - roles=roles, - ), - redirect_url, - ) - - def google_error_problem_detail(self, error): - error_detail = _("Error: %(error)s", error=error) - - # ProblemDetail.detailed requires the detail to be an internationalized - # string, so pass the combined string through _ as well even though the - # components were translated already. Space is a variable so it doesn't - # end up in the translation template. - space = " " - error_detail = _(str(GOOGLE_OAUTH_FAILURE.detail) + space + str(error_detail)) - - return GOOGLE_OAUTH_FAILURE.detailed(error_detail) - - def active_credentials(self, admin): - """Check that existing credentials aren't expired""" - - if admin.credential: - oauth_credentials = GoogleClient.OAuth2Credentials.from_json( - admin.credential - ) - return not oauth_credentials.access_token_expired - return False - - def staff_email(self, _db, email): - # If the admin already exists in the database, they can log in regardless of - # whether their domain has been whitelisted for a library. - admin = get_one(_db, Admin, email=email) - if admin: - return True - - # Otherwise, their email must match one of the configured domains. - staff_domains = list(self.domains.keys()) - domain = email[email.index("@") + 1 :] - return domain.lower() in [ - staff_domain.lower() for staff_domain in staff_domains - ] - - -class DummyGoogleClient: - """Mock Google OAuth client for testing""" - - expired = False - - class Credentials: - """Mock OAuth2Credentials object for testing""" - - access_token_expired = False - - def __init__(self, email): - domain = email[email.index("@") + 1 :] - self.id_token = {"hd": domain, "email": email} - - def to_json(self): - return json.dumps(dict(id_token=self.id_token)) - - def from_json(self, credentials): - return self - - def __init__(self, email="example@nypl.org"): - self.credentials = self.Credentials(email=email) - self.OAuth2Credentials = self.credentials - - def flow_from_client_secrets(self, config, scope=None, redirect_uri=None): - return self - - def step2_exchange(self, auth_code): - return self.credentials - - def step1_get_authorize_url(self, state): - return "GOOGLE REDIRECT" diff --git a/api/admin/problem_details.py b/api/admin/problem_details.py index 6b2a527fd9..33d90d25e5 100644 --- a/api/admin/problem_details.py +++ b/api/admin/problem_details.py @@ -33,13 +33,6 @@ _("Your admin account is not authorized to make this request."), ) -GOOGLE_OAUTH_FAILURE = pd( - "http://librarysimplified.org/terms/problem/google-oauth-failure", - 400, - _("Google OAuth Error"), - _("There was an error connecting with Google OAuth."), -) - INVALID_CSRF_TOKEN = pd( "http://librarysimplified.org/terms/problem/invalid-csrf-token", 400, diff --git a/api/admin/routes.py b/api/admin/routes.py index c845a7ab33..64187b6d93 100644 --- a/api/admin/routes.py +++ b/api/admin/routes.py @@ -106,12 +106,6 @@ def decorated(*args, **kwargs): return decorated -@app.route("/admin/GoogleAuth/callback") -@returns_problem_detail -def google_auth_callback(): - return app.manager.admin_sign_in_controller.redirect_after_google_sign_in() - - @app.route("/admin/sign_in_with_password", methods=["POST"]) @returns_problem_detail def password_auth(): diff --git a/core/model/configuration.py b/core/model/configuration.py index 880c996b34..335fcf6acd 100644 --- a/core/model/configuration.py +++ b/core/model/configuration.py @@ -130,8 +130,7 @@ class ExternalIntegration(Base): # Possible goals of ExternalIntegrations. # - # These integrations are associated with external services such as - # Google Enterprise which authenticate library administrators. + # These integrations are associated with external services which authenticate library administrators ADMIN_AUTH_GOAL = "admin_auth" # These integrations are associated with external services such as @@ -255,11 +254,8 @@ class ExternalIntegration(Base): # Integrations with ANALYTICS_GOAL GOOGLE_ANALYTICS = "Google Analytics" - # Integrations with ADMIN_AUTH_GOAL - GOOGLE_OAUTH = "Google OAuth" - # List of such ADMIN_AUTH_GOAL integrations - ADMIN_AUTH_PROTOCOLS = [GOOGLE_OAUTH] + ADMIN_AUTH_PROTOCOLS = [] # Integrations with LOGGING_GOAL INTERNAL_LOGGING = "Internal logging" diff --git a/poetry.lock b/poetry.lock index a8451f361e..6935be8589 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1029,21 +1029,6 @@ files = [ beautifulsoup4 = "*" lxml = ">=4.6.5" -[[package]] -name = "httplib2" -version = "0.20.4" -description = "A comprehensive HTTP client library." -category = "main" -optional = false -python-versions = ">=2.7, !=3.0.*, !=3.1.*, !=3.2.*, !=3.3.*" -files = [ - {file = "httplib2-0.20.4-py3-none-any.whl", hash = "sha256:8b6a905cb1c79eefd03f8669fd993c36dc341f7c558f056cb5a33b5c2f458543"}, - {file = "httplib2-0.20.4.tar.gz", hash = "sha256:58a98e45b4b1a48273073f905d2961666ecf0fbac4250ea5b47aef259eb5c585"}, -] - -[package.dependencies] -pyparsing = {version = ">=2.4.2,<3.0.0 || >3.0.0,<3.0.1 || >3.0.1,<3.0.2 || >3.0.2,<3.0.3 || >3.0.3,<4", markers = "python_version > \"3.0\""} - [[package]] name = "identify" version = "2.5.1" @@ -1492,7 +1477,6 @@ files = [ {file = "lxml-4.9.2-cp35-cp35m-manylinux_2_5_i686.manylinux1_i686.whl", hash = "sha256:ca989b91cf3a3ba28930a9fc1e9aeafc2a395448641df1f387a2d394638943b0"}, {file = "lxml-4.9.2-cp35-cp35m-manylinux_2_5_x86_64.manylinux1_x86_64.whl", hash = "sha256:822068f85e12a6e292803e112ab876bc03ed1f03dddb80154c395f891ca6b31e"}, {file = "lxml-4.9.2-cp35-cp35m-win32.whl", hash = "sha256:be7292c55101e22f2a3d4d8913944cbea71eea90792bf914add27454a13905df"}, - {file = "lxml-4.9.2-cp35-cp35m-win_amd64.whl", hash = "sha256:998c7c41910666d2976928c38ea96a70d1aa43be6fe502f21a651e17483a43c5"}, {file = "lxml-4.9.2-cp36-cp36m-macosx_10_15_x86_64.whl", hash = "sha256:b26a29f0b7fc6f0897f043ca366142d2b609dc60756ee6e4e90b5f762c6adc53"}, {file = "lxml-4.9.2-cp36-cp36m-manylinux_2_12_i686.manylinux2010_i686.manylinux_2_24_i686.whl", hash = "sha256:ab323679b8b3030000f2be63e22cdeea5b47ee0abd2d6a1dc0c8103ddaa56cd7"}, {file = "lxml-4.9.2-cp36-cp36m-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:689bb688a1db722485e4610a503e3e9210dcc20c520b45ac8f7533c837be76fe"}, @@ -1502,7 +1486,6 @@ files = [ {file = "lxml-4.9.2-cp36-cp36m-musllinux_1_1_aarch64.whl", hash = "sha256:58bfa3aa19ca4c0f28c5dde0ff56c520fbac6f0daf4fac66ed4c8d2fb7f22e74"}, {file = "lxml-4.9.2-cp36-cp36m-musllinux_1_1_x86_64.whl", hash = "sha256:bc718cd47b765e790eecb74d044cc8d37d58562f6c314ee9484df26276d36a38"}, {file = "lxml-4.9.2-cp36-cp36m-win32.whl", hash = "sha256:d5bf6545cd27aaa8a13033ce56354ed9e25ab0e4ac3b5392b763d8d04b08e0c5"}, - {file = "lxml-4.9.2-cp36-cp36m-win_amd64.whl", hash = "sha256:3ab9fa9d6dc2a7f29d7affdf3edebf6ece6fb28a6d80b14c3b2fb9d39b9322c3"}, {file = "lxml-4.9.2-cp37-cp37m-macosx_10_15_x86_64.whl", hash = "sha256:05ca3f6abf5cf78fe053da9b1166e062ade3fa5d4f92b4ed688127ea7d7b1d03"}, {file = "lxml-4.9.2-cp37-cp37m-manylinux_2_12_i686.manylinux2010_i686.manylinux_2_24_i686.whl", hash = "sha256:a5da296eb617d18e497bcf0a5c528f5d3b18dadb3619fbdadf4ed2356ef8d941"}, {file = "lxml-4.9.2-cp37-cp37m-manylinux_2_17_aarch64.manylinux2014_aarch64.manylinux_2_24_aarch64.whl", hash = "sha256:04876580c050a8c5341d706dd464ff04fd597095cc8c023252566a8826505726"}, @@ -1785,25 +1768,6 @@ files = [ {file = "nodeenv-1.6.0.tar.gz", hash = "sha256:3ef13ff90291ba2a4a7a4ff9a979b63ffdd00a464dbe04acf0ea6471517a4c2b"}, ] -[[package]] -name = "oauth2client" -version = "4.1.3" -description = "OAuth 2.0 client library" -category = "main" -optional = false -python-versions = "*" -files = [ - {file = "oauth2client-4.1.3-py2.py3-none-any.whl", hash = "sha256:b8a81cc5d60e2d364f0b1b98f958dbd472887acaf1a5b05e21c28c31a2d6d3ac"}, - {file = "oauth2client-4.1.3.tar.gz", hash = "sha256:d486741e451287f69568a4d26d70d9acd73a2bbfa275746c535b4209891cccc6"}, -] - -[package.dependencies] -httplib2 = ">=0.9.1" -pyasn1 = ">=0.1.7" -pyasn1-modules = ">=0.0.5" -rsa = ">=3.1.4" -six = ">=1.6.1" - [[package]] name = "packaging" version = "21.3" @@ -2146,33 +2110,6 @@ files = [ {file = "py-bcrypt-0.4.tar.gz", hash = "sha256:5fa13bce551468350d66c4883694850570f3da28d6866bb638ba44fe5eabda78"}, ] -[[package]] -name = "pyasn1" -version = "0.4.8" -description = "ASN.1 types and codecs" -category = "main" -optional = false -python-versions = "*" -files = [ - {file = "pyasn1-0.4.8-py2.py3-none-any.whl", hash = "sha256:39c7e2ec30515947ff4e87fb6f456dfc6e84857d34be479c9d4a4ba4bf46aa5d"}, - {file = "pyasn1-0.4.8.tar.gz", hash = "sha256:aef77c9fb94a3ac588e87841208bdec464471d9871bd5050a287cc9a475cd0ba"}, -] - -[[package]] -name = "pyasn1-modules" -version = "0.2.8" -description = "A collection of ASN.1-based protocols modules." -category = "main" -optional = false -python-versions = "*" -files = [ - {file = "pyasn1-modules-0.2.8.tar.gz", hash = "sha256:905f84c712230b2c592c19470d3ca8d552de726050d1d1716282a1f6146be65e"}, - {file = "pyasn1_modules-0.2.8-py2.py3-none-any.whl", hash = "sha256:a50b808ffeb97cb3601dd25981f6b016cbb3d31fbf57a8b8a87428e6158d0c74"}, -] - -[package.dependencies] -pyasn1 = ">=0.4.6,<0.5.0" - [[package]] name = "pycodestyle" version = "2.8.0" @@ -2953,21 +2890,6 @@ files = [ {file = "rfc3987-1.3.8.tar.gz", hash = "sha256:d3c4d257a560d544e9826b38bc81db676890c79ab9d7ac92b39c7a253d5ca733"}, ] -[[package]] -name = "rsa" -version = "4.8" -description = "Pure-Python RSA implementation" -category = "main" -optional = false -python-versions = ">=3.6,<4" -files = [ - {file = "rsa-4.8-py3-none-any.whl", hash = "sha256:95c5d300c4e879ee69708c428ba566c59478fd653cc3a22243eeb8ed846950bb"}, - {file = "rsa-4.8.tar.gz", hash = "sha256:5c6bd9dc7a543b7fe4304a631f8a8a3b674e2bbfc49c2ae96200cdbe55df6b17"}, -] - -[package.dependencies] -pyasn1 = ">=0.1.3" - [[package]] name = "s3transfer" version = "0.5.2" @@ -3087,7 +3009,7 @@ mssql = ["pyodbc"] mssql-pymssql = ["pymssql"] mssql-pyodbc = ["pyodbc"] mysql = ["mysqlclient"] -oracle = ["cx-oracle"] +oracle = ["cx_oracle"] postgresql = ["psycopg2"] postgresql-pg8000 = ["pg8000 (<1.16.6)"] postgresql-psycopg2binary = ["psycopg2-binary"] @@ -3708,4 +3630,4 @@ testing = ["flake8 (<5)", "func-timeout", "jaraco.functools", "jaraco.itertools" [metadata] lock-version = "2.0" python-versions = ">=3.8,<4" -content-hash = "746f9a4cfe9ceb52883fcce64170157a829ab1cdcfe63ec69d01a3fe90d13d2d" +content-hash = "0a79150d2cffbeafcaba2b13e14cf25e81ede49b2d356822ef64cbfa9b4c658b" diff --git a/pyproject.toml b/pyproject.toml index a0b0399aff..e02151b36d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -144,7 +144,6 @@ money = "1.3.0" multipledispatch = "0.6.0" nameparser = "1.1.2" # nameparser is for author name manipulations nltk = "3.8.1" # nltk is a textblob dependency. -oauth2client = "4.1.3" # Deprecated and should be replaced. palace-webpub-manifest-parser = "~3.0.0" Pillow = "9.4.0" py-bcrypt = "0.4" diff --git a/tests/api/admin/controller/test_admin_auth_services.py b/tests/api/admin/controller/test_admin_auth_services.py index 9265371a09..6103758260 100644 --- a/tests/api/admin/controller/test_admin_auth_services.py +++ b/tests/api/admin/controller/test_admin_auth_services.py @@ -1,18 +1,10 @@ -import json - import flask import pytest from werkzeug.datastructures import MultiDict from api.admin.exceptions import * from api.app import initialize_database -from core.model import ( - AdminRole, - ConfigurationSetting, - ExternalIntegration, - create, - get_one, -) +from core.model import AdminRole, ExternalIntegration from .test_controller import SettingsControllerTest @@ -44,37 +36,6 @@ def test_admin_auth_services_get_with_no_services(self): self.manager.admin_auth_services_controller.process_admin_auth_services, ) - def test_admin_auth_services_get_with_google_oauth_service(self): - auth_service, ignore = create( - self._db, - ExternalIntegration, - protocol=ExternalIntegration.GOOGLE_OAUTH, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) - auth_service.url = "http://oauth.test" - auth_service.username = "user" - auth_service.password = "pass" - auth_service.libraries += [self._default_library] - ConfigurationSetting.for_library_and_externalintegration( - self._db, "domains", self._default_library, auth_service - ).value = json.dumps(["nypl.org"]) - - with self.request_context_with_admin("/"): - response = ( - self.manager.admin_auth_services_controller.process_admin_auth_services() - ) - [service] = response.get("admin_auth_services") - - assert auth_service.id == service.get("id") - assert auth_service.name == service.get("name") - assert auth_service.protocol == service.get("protocol") - assert auth_service.url == service.get("settings").get("url") - assert auth_service.username == service.get("settings").get("username") - assert auth_service.password == service.get("settings").get("password") - [library_info] = service.get("libraries") - assert self._default_library.short_name == library_info.get("short_name") - assert ["nypl.org"] == library_info.get("domains") - def test_admin_auth_services_post_errors(self): with self.request_context_with_admin("/", method="POST"): flask.request.form = MultiDict( @@ -105,170 +66,10 @@ def test_admin_auth_services_post_errors(self): ) assert response == MISSING_SERVICE - auth_service, ignore = create( - self._db, - ExternalIntegration, - protocol=ExternalIntegration.GOOGLE_OAUTH, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) - - with self.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict( - [ - ("id", str(auth_service.id)), - ] - ) - response = ( - self.manager.admin_auth_services_controller.process_admin_auth_services() - ) - assert response == CANNOT_CHANGE_PROTOCOL - - with self.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict( - [ - ("protocol", "Google OAuth"), - ] - ) - response = ( - self.manager.admin_auth_services_controller.process_admin_auth_services() - ) - assert response.uri == INCOMPLETE_CONFIGURATION.uri - - self.admin.remove_role(AdminRole.SYSTEM_ADMIN) - self._db.flush() - with self.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict( - [ - ("name", "oauth"), - ("protocol", "Google OAuth"), - ("url", "url"), - ("username", "username"), - ("password", "password"), - ("domains", "nypl.org"), - ] - ) - pytest.raises( - AdminNotAuthorized, - self.manager.admin_auth_services_controller.process_admin_auth_services, - ) - def test_admin_auth_services_post_create(self): - with self.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict( - [ - ("name", "oauth"), - ("protocol", "Google OAuth"), - ("url", "http://url2"), - ("username", "username"), - ("password", "password"), - ( - "libraries", - json.dumps( - [ - { - "short_name": self._default_library.short_name, - "domains": ["nypl.org", "gmail.com"], - } - ] - ), - ), - ] - ) - response = ( - self.manager.admin_auth_services_controller.process_admin_auth_services() - ) - assert response.status_code == 201 - - # The auth service was created and configured properly. - auth_service = ExternalIntegration.admin_authentication(self._db) - assert auth_service.protocol == response.get_data(as_text=True) - assert "oauth" == auth_service.name - assert "http://url2" == auth_service.url - assert "username" == auth_service.username - assert "password" == auth_service.password - - assert [self._default_library] == auth_service.libraries - setting = ConfigurationSetting.for_library_and_externalintegration( - self._db, "domains", self._default_library, auth_service - ) - assert "domains" == setting.key - assert ["nypl.org", "gmail.com"] == json.loads(setting.value) - - def test_admin_auth_services_post_google_oauth_edit(self): - # The auth service exists. - auth_service, ignore = create( - self._db, - ExternalIntegration, - protocol=ExternalIntegration.GOOGLE_OAUTH, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) - auth_service.url = "url" - auth_service.username = "user" - auth_service.password = "pass" - auth_service.libraries += [self._default_library] - setting = ConfigurationSetting.for_library_and_externalintegration( - self._db, "domains", self._default_library, auth_service - ) - setting.value = json.dumps(["library1.org"]) - - with self.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict( - [ - ("name", "oauth"), - ("protocol", "Google OAuth"), - ("url", "http://url2"), - ("username", "user2"), - ("password", "pass2"), - ( - "libraries", - json.dumps( - [ - { - "short_name": self._default_library.short_name, - "domains": ["library2.org"], - } - ] - ), - ), - ] - ) - response = ( - self.manager.admin_auth_services_controller.process_admin_auth_services() - ) - assert response.status_code == 200 - - assert auth_service.protocol == response.get_data(as_text=True) - assert "oauth" == auth_service.name - assert "http://url2" == auth_service.url - assert "user2" == auth_service.username - assert "domains" == setting.key - assert ["library2.org"] == json.loads(setting.value) + # TODO: Should be implemented if new external admin auth service is implemented + return def test_admin_auth_service_delete(self): - auth_service, ignore = create( - self._db, - ExternalIntegration, - protocol=ExternalIntegration.GOOGLE_OAUTH, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) - auth_service.url = "url" - auth_service.username = "user" - auth_service.password = "pass" - auth_service.set_setting("domains", json.dumps(["library1.org"])) - - with self.request_context_with_admin("/", method="DELETE"): - self.admin.remove_role(AdminRole.SYSTEM_ADMIN) - pytest.raises( - AdminNotAuthorized, - self.manager.admin_auth_services_controller.process_delete, - auth_service.protocol, - ) - - self.admin.add_role(AdminRole.SYSTEM_ADMIN) - response = self.manager.admin_auth_services_controller.process_delete( - auth_service.protocol - ) - assert response.status_code == 200 - - service = get_one(self._db, ExternalIntegration, id=auth_service.id) - assert None == service + # TODO: Should be implemented if new external admin auth service is implemented + return diff --git a/tests/api/admin/controller/test_controller.py b/tests/api/admin/controller/test_controller.py index d2b9afa2c7..aaf2ce954a 100644 --- a/tests/api/admin/controller/test_controller.py +++ b/tests/api/admin/controller/test_controller.py @@ -22,9 +22,6 @@ setup_admin_controllers, ) from api.admin.exceptions import * -from api.admin.google_oauth_admin_authentication_provider import ( - GoogleOAuthAdminAuthenticationProvider, -) from api.admin.password_admin_authentication_provider import ( PasswordAdminAuthenticationProvider, ) @@ -350,61 +347,30 @@ def test_admin_auth_providers(self): # no auth service set up. assert [] == ctrl.admin_auth_providers - # The auth service exists. - create( - self._db, - ExternalIntegration, - protocol=ExternalIntegration.GOOGLE_OAUTH, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) - assert 1 == len(ctrl.admin_auth_providers) - assert ( - GoogleOAuthAdminAuthenticationProvider.NAME - == ctrl.admin_auth_providers[0].NAME - ) - - # Here's another admin with a password. + # Here's an admin with a password. pw_admin, ignore = create(self._db, Admin, email="pw@nypl.org") pw_admin.password = "password" - assert 2 == len(ctrl.admin_auth_providers) + assert 1 == len(ctrl.admin_auth_providers) assert { - GoogleOAuthAdminAuthenticationProvider.NAME, PasswordAdminAuthenticationProvider.NAME, } == {provider.NAME for provider in ctrl.admin_auth_providers} - # Only an admin with a password. + # Only an admin with a password is left. self._db.delete(self.admin) - assert 2 == len(ctrl.admin_auth_providers) + assert 1 == len(ctrl.admin_auth_providers) assert { - GoogleOAuthAdminAuthenticationProvider.NAME, PasswordAdminAuthenticationProvider.NAME, } == {provider.NAME for provider in ctrl.admin_auth_providers} - # No admins. Someone new could still log in with google if domains are - # configured. + # No admins. No one can log in anymore self._db.delete(pw_admin) - assert 1 == len(ctrl.admin_auth_providers) - assert ( - GoogleOAuthAdminAuthenticationProvider.NAME - == ctrl.admin_auth_providers[0].NAME - ) + assert 0 == len(ctrl.admin_auth_providers) def test_admin_auth_provider(self): with self.app.test_request_context("/admin"): ctrl = self.manager.admin_sign_in_controller - create( - self._db, - ExternalIntegration, - protocol=ExternalIntegration.GOOGLE_OAUTH, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) - - # We can find a google auth provider. - auth = ctrl.admin_auth_provider(GoogleOAuthAdminAuthenticationProvider.NAME) - assert isinstance(auth, GoogleOAuthAdminAuthenticationProvider) - - # But not a password auth provider, since no admin has a password. + # We can't find a password auth provider, since no admin has a password. auth = ctrl.admin_auth_provider(PasswordAdminAuthenticationProvider.NAME) assert None == auth @@ -412,49 +378,33 @@ def test_admin_auth_provider(self): pw_admin, ignore = create(self._db, Admin, email="pw@nypl.org") pw_admin.password = "password" - # Now we can find both auth providers. - auth = ctrl.admin_auth_provider(GoogleOAuthAdminAuthenticationProvider.NAME) - assert isinstance(auth, GoogleOAuthAdminAuthenticationProvider) + # Now we can find an auth provider. auth = ctrl.admin_auth_provider(PasswordAdminAuthenticationProvider.NAME) assert isinstance(auth, PasswordAdminAuthenticationProvider) def test_authenticated_admin_from_request(self): - # Returns an error if there's no admin auth service. + # Returns an error if there is no admin auth providers. with self.app.test_request_context("/admin"): - flask.session["admin_email"] = self.admin.email - flask.session["auth_type"] = GoogleOAuthAdminAuthenticationProvider.NAME + # You get back a problem detail when you're not authenticated. response = ( self.manager.admin_sign_in_controller.authenticated_admin_from_request() ) - assert ADMIN_AUTH_NOT_CONFIGURED == response + assert 500 == response.status_code + assert ADMIN_AUTH_NOT_CONFIGURED.detail == response.detail # Works once the admin auth service exists. - create( - self._db, - ExternalIntegration, - protocol=ExternalIntegration.GOOGLE_OAUTH, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) + self.admin.password = "password" with self.app.test_request_context("/admin"): flask.session["admin_email"] = self.admin.email - flask.session["auth_type"] = GoogleOAuthAdminAuthenticationProvider.NAME + flask.session["auth_type"] = PasswordAdminAuthenticationProvider.NAME response = ( self.manager.admin_sign_in_controller.authenticated_admin_from_request() ) assert self.admin == response - # Returns an error if you aren't authenticated. - with self.app.test_request_context("/admin"): - # You get back a problem detail when you're not authenticated. - response = ( - self.manager.admin_sign_in_controller.authenticated_admin_from_request() - ) - assert 401 == response.status_code - assert INVALID_ADMIN_CREDENTIALS.detail == response.detail - # Returns an error if the admin email or auth type is missing from the session. with self.app.test_request_context("/admin"): - flask.session["auth_type"] = GoogleOAuthAdminAuthenticationProvider.NAME + flask.session["auth_type"] = PasswordAdminAuthenticationProvider.NAME response = ( self.manager.admin_sign_in_controller.authenticated_admin_from_request() ) @@ -472,116 +422,26 @@ def test_authenticated_admin_from_request(self): # Returns an error if the admin authentication type isn't configured. with self.app.test_request_context("/admin"): flask.session["admin_email"] = self.admin.email - flask.session["auth_type"] = PasswordAdminAuthenticationProvider.NAME + flask.session["auth_type"] = "unknown" response = ( self.manager.admin_sign_in_controller.authenticated_admin_from_request() ) assert 400 == response.status_code assert ADMIN_AUTH_MECHANISM_NOT_CONFIGURED.detail == response.detail - def test_authenticated_admin(self): - - # Unset the base URL -- it will be set automatically when we - # successfully authenticate as an admin. - base_url = ConfigurationSetting.sitewide(self._db, Configuration.BASE_URL_KEY) - base_url.value = None - assert None == base_url.value - - # Creates a new admin with fresh details. - new_admin_details = { - "email": "admin@nypl.org", - "credentials": "gnarly", - "type": GoogleOAuthAdminAuthenticationProvider.NAME, - "roles": [ - { - "role": AdminRole.LIBRARY_MANAGER, - "library": self._default_library.short_name, - } - ], - } - with self.app.test_request_context("/admin/sign_in?redirect=foo"): - flask.request.url = "http://chosen-hostname/admin/sign_in?redirect=foo" - admin = self.manager.admin_sign_in_controller.authenticated_admin( - new_admin_details - ) - assert "admin@nypl.org" == admin.email - assert "gnarly" == admin.credential - [role] = admin.roles - assert AdminRole.LIBRARY_MANAGER == role.role - assert self._default_library == role.library - - # Also sets up the admin's flask session. - assert "admin@nypl.org" == flask.session["admin_email"] - assert ( - GoogleOAuthAdminAuthenticationProvider.NAME - == flask.session["auth_type"] - ) - assert True == flask.session.permanent - - # The first successfully authenticated admin user automatically - # sets the site's base URL. - assert "http://chosen-hostname/" == base_url.value - - # Or overwrites credentials for an existing admin. - existing_admin_details = { - "email": "example@nypl.org", - "credentials": "b-a-n-a-n-a-s", - "type": GoogleOAuthAdminAuthenticationProvider.NAME, - "roles": [ - { - "role": AdminRole.LIBRARY_MANAGER, - "library": self._default_library.short_name, - } - ], - } - with self.app.test_request_context("/admin/sign_in?redirect=foo"): - flask.request.url = "http://a-different-hostname/" - admin = self.manager.admin_sign_in_controller.authenticated_admin( - existing_admin_details - ) - assert self.admin.id == admin.id - assert "b-a-n-a-n-a-s" == self.admin.credential - # No roles were created since the admin already existed. - assert [] == admin.roles - - # We already set the site's base URL, and it doesn't get set - # to a different value just because someone authenticated - # through a different hostname. - assert "http://chosen-hostname/" == base_url.value - def test_admin_signin(self): # Returns an error if there's no admin auth service. with self.app.test_request_context("/admin/sign_in?redirect=foo"): response = self.manager.admin_sign_in_controller.sign_in() assert ADMIN_AUTH_NOT_CONFIGURED == response - create( - self._db, - ExternalIntegration, - protocol=ExternalIntegration.GOOGLE_OAUTH, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) - # Shows the login page if there's an auth service # but no signed in admin. - with self.app.test_request_context("/admin/sign_in?redirect=foo"): - response = self.manager.admin_sign_in_controller.sign_in() - assert 200 == response.status_code - response_data = response.get_data(as_text=True) - assert "GOOGLE REDIRECT" in response_data - assert "Sign in with Google" in response_data - assert "Email" not in response_data - assert "Password" not in response_data - - # If there are multiple auth providers, the login page - # shows them all. self.admin.password = "password" with self.app.test_request_context("/admin/sign_in?redirect=foo"): response = self.manager.admin_sign_in_controller.sign_in() assert 200 == response.status_code response_data = response.get_data(as_text=True) - assert "GOOGLE REDIRECT" in response_data - assert "Sign in with Google" in response_data assert "Email" in response_data assert "Password" in response_data @@ -593,83 +453,12 @@ def test_admin_signin(self): assert 302 == response.status_code assert "foo" == response.headers["Location"] - def test_redirect_after_google_sign_in(self): - self._db.delete(self.admin) - - # Returns an error if there's no admin auth service. - with self.app.test_request_context("/admin/GoogleOAuth/callback"): - response = ( - self.manager.admin_sign_in_controller.redirect_after_google_sign_in() - ) - assert ADMIN_AUTH_NOT_CONFIGURED == response - - # Returns an error if the admin auth service isn't google. - admin, ignore = create(self._db, Admin, email="admin@nypl.org") - admin.password = "password" - with self.app.test_request_context("/admin/GoogleOAuth/callback"): - response = ( - self.manager.admin_sign_in_controller.redirect_after_google_sign_in() - ) - assert ADMIN_AUTH_MECHANISM_NOT_CONFIGURED == response - - self._db.delete(admin) - auth_integration, ignore = create( - self._db, - ExternalIntegration, - protocol=ExternalIntegration.GOOGLE_OAUTH, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) - auth_integration.libraries += [self._default_library] - setting = ConfigurationSetting.for_library_and_externalintegration( - self._db, "domains", self._default_library, auth_integration - ) - - # Returns an error if google oauth fails.. - with self.app.test_request_context("/admin/GoogleOAuth/callback?error=foo"): - response = ( - self.manager.admin_sign_in_controller.redirect_after_google_sign_in() - ) - assert 400 == response.status_code - - # Returns an error if the admin email isn't a staff email. - setting.value = json.dumps(["alibrary.org"]) - with self.app.test_request_context( - "/admin/GoogleOAuth/callback?code=1234&state=foo" - ): - response = ( - self.manager.admin_sign_in_controller.redirect_after_google_sign_in() - ) - assert 401 == response.status_code - - # Redirects to the state parameter if the admin email is valid. - setting.value = json.dumps(["nypl.org"]) - with self.app.test_request_context( - "/admin/GoogleOAuth/callback?code=1234&state=foo" - ): - response = ( - self.manager.admin_sign_in_controller.redirect_after_google_sign_in() - ) - assert 302 == response.status_code - assert "foo" == response.headers["Location"] - def test_password_sign_in(self): # Returns an error if there's no admin auth service and no admins. with self.app.test_request_context("/admin/sign_in_with_password"): response = self.manager.admin_sign_in_controller.password_sign_in() assert ADMIN_AUTH_NOT_CONFIGURED == response - # Returns an error if the admin auth service isn't password auth. - auth_integration, ignore = create( - self._db, - ExternalIntegration, - protocol=ExternalIntegration.GOOGLE_OAUTH, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) - with self.app.test_request_context("/admin/sign_in_with_password"): - response = self.manager.admin_sign_in_controller.password_sign_in() - assert ADMIN_AUTH_MECHANISM_NOT_CONFIGURED == response - - self._db.delete(auth_integration) admin, ignore = create(self._db, Admin, email="admin@nypl.org") admin.password = "password" diff --git a/tests/api/admin/test_google_oauth_admin_authentication_provider.py b/tests/api/admin/test_google_oauth_admin_authentication_provider.py deleted file mode 100644 index e4cdb1c07f..0000000000 --- a/tests/api/admin/test_google_oauth_admin_authentication_provider.py +++ /dev/null @@ -1,148 +0,0 @@ -import json - -from oauth2client import client as GoogleClient - -from api.admin.google_oauth_admin_authentication_provider import ( - DummyGoogleClient, - GoogleOAuthAdminAuthenticationProvider, -) -from api.admin.problem_details import INVALID_ADMIN_CREDENTIALS -from core.model import ( - Admin, - AdminRole, - ConfigurationSetting, - ExternalIntegration, - create, -) -from core.testing import DatabaseTest -from core.util.problem_detail import ProblemDetail - - -class TestGoogleOAuthAdminAuthenticationProvider(DatabaseTest): - def test_callback(self): - super().setup_method() - auth_integration, ignore = create( - self._db, - ExternalIntegration, - protocol=ExternalIntegration.GOOGLE_OAUTH, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) - self.google = GoogleOAuthAdminAuthenticationProvider( - auth_integration, "", test_mode=True - ) - auth_integration.libraries += [self._default_library] - ConfigurationSetting.for_library_and_externalintegration( - self._db, "domains", self._default_library, auth_integration - ).value = json.dumps(["nypl.org"]) - - # Returns a problem detail when Google returns an error. - error_response, redirect = self.google.callback( - self._db, {"error": "access_denied"} - ) - assert True == isinstance(error_response, ProblemDetail) - assert 400 == error_response.status_code - assert True == error_response.detail.endswith("access_denied") - assert None == redirect - - # Successful case creates a dict of admin details - success, redirect = self.google.callback(self._db, {"code": "abc"}) - assert "example@nypl.org" == success["email"] - default_credentials = json.dumps( - {"id_token": {"hd": "nypl.org", "email": "example@nypl.org"}} - ) - assert default_credentials == success["credentials"] - assert GoogleOAuthAdminAuthenticationProvider.NAME == success["type"] - [role] = success.get("roles") - assert AdminRole.LIBRARIAN == role.get("role") - assert self._default_library.short_name == role.get("library") - - # If domains are set, the admin's domain must match one of the domains. - setting = ConfigurationSetting.for_library_and_externalintegration( - self._db, "domains", self._default_library, auth_integration - ) - setting.value = json.dumps(["otherlibrary.org"]) - failure, ignore = self.google.callback(self._db, {"code": "abc"}) - assert INVALID_ADMIN_CREDENTIALS == failure - setting.value = json.dumps(["nypl.org"]) - - # Returns a problem detail when the oauth client library - # raises an exception. - class ExceptionRaisingClient(DummyGoogleClient): - def step2_exchange(self, auth_code): - raise GoogleClient.FlowExchangeError("mock error") - - self.google.dummy_client = ExceptionRaisingClient() - error_response, redirect = self.google.callback(self._db, {"code": "abc"}) - assert True == isinstance(error_response, ProblemDetail) - assert 400 == error_response.status_code - assert True == error_response.detail.endswith("mock error") - assert None == redirect - - def test_domains(self): - super().setup_method() - auth_integration, ignore = create( - self._db, - ExternalIntegration, - protocol=ExternalIntegration.GOOGLE_OAUTH, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) - auth_integration.libraries += [self._default_library] - ConfigurationSetting.for_library_and_externalintegration( - self._db, "domains", self._default_library, auth_integration - ).value = json.dumps(["nypl.org"]) - - google = GoogleOAuthAdminAuthenticationProvider( - auth_integration, "", test_mode=True - ) - - assert ["nypl.org"] == list(google.domains.keys()) - assert [self._default_library] == google.domains["nypl.org"] - - l2 = self._library() - auth_integration.libraries += [l2] - ConfigurationSetting.for_library_and_externalintegration( - self._db, "domains", l2, auth_integration - ).value = json.dumps(["nypl.org", "l2.org"]) - - assert {self._default_library, l2} == set(google.domains["nypl.org"]) - assert [l2] == google.domains["l2.org"] - - def test_staff_email(self): - super().setup_method() - auth_integration, ignore = create( - self._db, - ExternalIntegration, - protocol=ExternalIntegration.GOOGLE_OAUTH, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) - - nypl_admin = create(self._db, Admin, email="admin@nypl.org") - bpl_admin = create(self._db, Admin, email="admin@bklynlibrary.org") - - # If no domains are set, the admin must already exist in the db - # to be considered library staff. - google = GoogleOAuthAdminAuthenticationProvider( - auth_integration, "", test_mode=True - ) - - assert True == google.staff_email(self._db, "admin@nypl.org") - assert True == google.staff_email(self._db, "admin@bklynlibrary.org") - assert False == google.staff_email(self._db, "someone@nypl.org") - - # If domains are set, the admin's domain can match one of the domains - # if the admin doesn't exist yet. - auth_integration.libraries += [self._default_library] - setting = ConfigurationSetting.for_library_and_externalintegration( - self._db, "domains", self._default_library, auth_integration - ) - setting.value = json.dumps(["nypl.org"]) - assert True == google.staff_email(self._db, "admin@nypl.org") - assert True == google.staff_email(self._db, "admin@bklynlibrary.org") - assert True == google.staff_email(self._db, "someone@nypl.org") - assert False == google.staff_email(self._db, "someone@bklynlibrary.org") - - setting.value = json.dumps(["nypl.org", "bklynlibrary.org"]) - assert True == google.staff_email(self._db, "admin@nypl.org") - assert True == google.staff_email(self._db, "admin@bklynlibrary.org") - assert True == google.staff_email(self._db, "someone@nypl.org") - assert True == google.staff_email(self._db, "someone@bklynlibrary.org") diff --git a/tests/api/admin/test_routes.py b/tests/api/admin/test_routes.py index 6e99c78659..0f19cd0b46 100644 --- a/tests/api/admin/test_routes.py +++ b/tests/api/admin/test_routes.py @@ -209,10 +209,6 @@ class TestAdminSignIn(AdminRouteTest): CONTROLLER_NAME = "admin_sign_in_controller" - def test_google_auth_callback(self): - url = "/admin/GoogleAuth/callback" - self.assert_request_calls(url, self.controller.redirect_after_google_sign_in) - def test_sign_in_with_password(self): url = "/admin/sign_in_with_password" self.assert_request_calls( diff --git a/translations/es/LC_MESSAGES/circulation-admin.po b/translations/es/LC_MESSAGES/circulation-admin.po index fb0b507e47..7d8f2bafc5 100644 --- a/translations/es/LC_MESSAGES/circulation-admin.po +++ b/translations/es/LC_MESSAGES/circulation-admin.po @@ -19,12 +19,6 @@ msgid "A valid library staff email is required." msgstr "" "Se requiere un correo electrónico válido del personal de la biblioteca" -msgid "Google OAuth Error" -msgstr "Google OAuth Error" - -msgid "There was an error connecting with Google OAuth" -msgstr "Se ha producido un error al conectar con Google OAuth." - msgid "Invalid CSRF token" msgstr "Ficha CSRF no válida" From 2f7baeb668f4ce86bddd6b9704fa01c9e4dc6829 Mon Sep 17 00:00:00 2001 From: Porin Custic Date: Wed, 8 Feb 2023 16:52:50 +0100 Subject: [PATCH 2/7] Fix typing error --- core/model/configuration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/model/configuration.py b/core/model/configuration.py index 335fcf6acd..7640261a0b 100644 --- a/core/model/configuration.py +++ b/core/model/configuration.py @@ -255,7 +255,7 @@ class ExternalIntegration(Base): GOOGLE_ANALYTICS = "Google Analytics" # List of such ADMIN_AUTH_GOAL integrations - ADMIN_AUTH_PROTOCOLS = [] + ADMIN_AUTH_PROTOCOLS: list[str] = [] # Integrations with LOGGING_GOAL INTERNAL_LOGGING = "Internal logging" From 79e792187170c01d28142701e488e2e5177162a8 Mon Sep 17 00:00:00 2001 From: Porin Custic Date: Mon, 13 Feb 2023 16:28:19 +0100 Subject: [PATCH 3/7] Remove external admin auth --- api/admin/admin_authentication_provider.py | 3 - api/admin/controller/__init__.py | 21 +--- api/admin/controller/admin_auth_services.py | 107 ------------------ api/admin/routes.py | 16 --- core/model/configuration.py | 3 - .../controller/test_admin_auth_services.py | 75 ------------ tests/api/admin/controller/test_controller.py | 12 -- ..._password_admin_authentication_provider.py | 4 +- tests/api/admin/test_routes.py | 19 ---- 9 files changed, 7 insertions(+), 253 deletions(-) delete mode 100644 api/admin/controller/admin_auth_services.py delete mode 100644 tests/api/admin/controller/test_admin_auth_services.py diff --git a/api/admin/admin_authentication_provider.py b/api/admin/admin_authentication_provider.py index 36dbe608bd..02103857b9 100644 --- a/api/admin/admin_authentication_provider.py +++ b/api/admin/admin_authentication_provider.py @@ -1,7 +1,4 @@ class AdminAuthenticationProvider: - def __init__(self, integration): - self.integration = integration - def sign_in_template(self, redirect_url): # Returns HTML to be rendered on the sign in page for # this authentication provider. diff --git a/api/admin/controller/__init__.py b/api/admin/controller/__init__.py index 92ac0b2cb2..9880debc4e 100644 --- a/api/admin/controller/__init__.py +++ b/api/admin/controller/__init__.py @@ -142,9 +142,7 @@ def setup_admin_controllers(manager): manager.admin_patron_auth_service_self_tests_controller = ( PatronAuthServiceSelfTestsController(manager) ) - from api.admin.controller.admin_auth_services import AdminAuthServicesController - manager.admin_auth_services_controller = AdminAuthServicesController(manager) from api.admin.controller.collection_settings import CollectionSettingsController manager.admin_collection_settings_controller = CollectionSettingsController(manager) @@ -214,16 +212,10 @@ def __init__(self, manager): @property def admin_auth_providers(self): - auth_providers = [] - auth_service = ExternalIntegration.admin_authentication(self._db) - if Admin.with_password(self._db).count() != 0: - auth_providers.append( - PasswordAdminAuthenticationProvider( - auth_service, - ) - ) - return auth_providers + return [PasswordAdminAuthenticationProvider()] + + return [] def admin_auth_provider(self, type): # Return an auth provider with the given type. @@ -246,7 +238,7 @@ def authenticated_admin_from_request(self): auth = self.admin_auth_provider(type) if not auth: return ADMIN_AUTH_MECHANISM_NOT_CONFIGURED - if admin and auth.active_credentials(admin): + if admin: flask.request.admin = admin return admin flask.request.admin = None @@ -256,10 +248,7 @@ def authenticated_admin(self, admin_details): """Creates or updates an admin with the given details""" admin, is_new = get_one_or_create(self._db, Admin, email=admin_details["email"]) - admin.update_credentials( - self._db, - credential=admin_details.get("credentials"), - ) + if is_new and admin_details.get("roles"): for role in admin_details.get("roles"): if role.get("role") in AdminRole.ROLES: diff --git a/api/admin/controller/admin_auth_services.py b/api/admin/controller/admin_auth_services.py deleted file mode 100644 index a34b6d4692..0000000000 --- a/api/admin/controller/admin_auth_services.py +++ /dev/null @@ -1,107 +0,0 @@ -import flask -from flask import Response -from flask_babel import lazy_gettext as _ - -from api.admin.problem_details import * -from core.model import ExternalIntegration, get_one, get_one_or_create -from core.util.problem_detail import ProblemDetail - -from . import SettingsController - - -class AdminAuthServicesController(SettingsController): - def __init__(self, manager): - super().__init__(manager) - provider_apis = [] - self.protocols = self._get_integration_protocols( - provider_apis, protocol_name_attr="NAME" - ) - - def process_admin_auth_services(self): - self.require_system_admin() - if flask.request.method == "GET": - return self.process_get() - else: - return self.process_post() - - def process_get(self): - auth_services = self._get_integration_info( - ExternalIntegration.ADMIN_AUTH_GOAL, self.protocols - ) - return dict( - admin_auth_services=auth_services, - protocols=self.protocols, - ) - - def process_post(self): - protocol = flask.request.form.get("protocol") - id = flask.request.form.get("id") - auth_service = ExternalIntegration.admin_authentication(self._db) - fields = {"protocol": protocol, "id": id, "auth_service": auth_service} - error = self.validate_form_fields(**fields) - if error: - return error - - is_new = False - - if not auth_service: - if protocol: - auth_service, is_new = get_one_or_create( - self._db, - ExternalIntegration, - protocol=protocol, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) - else: - return NO_PROTOCOL_FOR_NEW_SERVICE - - name = flask.request.form.get("name") - auth_service.name = name - - [protocol] = [p for p in self.protocols if p.get("name") == protocol] - result = self._set_integration_settings_and_libraries(auth_service, protocol) - if isinstance(result, ProblemDetail): - return result - - if is_new: - return Response(str(auth_service.protocol), 201) - else: - return Response(str(auth_service.protocol), 200) - - def validate_form_fields(self, **fields): - """Check that 1) the user has selected a valid protocol, 2) the user has not - left the required fields blank, and 3) the user is not attempting to - change the protocol of an existing admin auth service.""" - - protocol = fields.get("protocol") - auth_service = fields.get("auth_service") - id = fields.get("id") - - if protocol: - if protocol not in ExternalIntegration.ADMIN_AUTH_PROTOCOLS: - return UNKNOWN_PROTOCOL - else: - wrong_format = self.validate_formats() - if wrong_format: - return wrong_format - if auth_service: - if id and int(id) != auth_service.id: - return MISSING_SERVICE - if protocol != auth_service.protocol: - return CANNOT_CHANGE_PROTOCOL - else: - if id: - return MISSING_SERVICE - - def process_delete(self, protocol): - self.require_system_admin() - service = get_one( - self._db, - ExternalIntegration, - protocol=protocol, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) - if not service: - return MISSING_SERVICE - self._db.delete(service) - return Response(str(_("Deleted")), 200) diff --git a/api/admin/routes.py b/api/admin/routes.py index 3269b425a5..11421c5bae 100644 --- a/api/admin/routes.py +++ b/api/admin/routes.py @@ -387,22 +387,6 @@ def collection_library_registrations(): ) -@app.route("/admin/admin_auth_services", methods=["GET", "POST"]) -@returns_json_or_response_or_problem_detail -@requires_admin -@requires_csrf_token -def admin_auth_services(): - return app.manager.admin_auth_services_controller.process_admin_auth_services() - - -@app.route("/admin/admin_auth_service/", methods=["DELETE"]) -@returns_json_or_response_or_problem_detail -@requires_admin -@requires_csrf_token -def admin_auth_service(protocol): - return app.manager.admin_auth_services_controller.process_delete(protocol) - - @app.route("/admin/individual_admins", methods=["GET", "POST"]) @returns_json_or_response_or_problem_detail @allows_admin_auth_setup diff --git a/core/model/configuration.py b/core/model/configuration.py index 2dd923e6b9..61ab7fd49c 100644 --- a/core/model/configuration.py +++ b/core/model/configuration.py @@ -246,9 +246,6 @@ class ExternalIntegration(Base): # Integrations with ANALYTICS_GOAL GOOGLE_ANALYTICS = "Google Analytics" - # List of such ADMIN_AUTH_GOAL integrations - ADMIN_AUTH_PROTOCOLS: list[str] = [] - # Integrations with LOGGING_GOAL INTERNAL_LOGGING = "Internal logging" LOGGLY = "Loggly" diff --git a/tests/api/admin/controller/test_admin_auth_services.py b/tests/api/admin/controller/test_admin_auth_services.py deleted file mode 100644 index 6103758260..0000000000 --- a/tests/api/admin/controller/test_admin_auth_services.py +++ /dev/null @@ -1,75 +0,0 @@ -import flask -import pytest -from werkzeug.datastructures import MultiDict - -from api.admin.exceptions import * -from api.app import initialize_database -from core.model import AdminRole, ExternalIntegration - -from .test_controller import SettingsControllerTest - - -class TestAdminAuthServices(SettingsControllerTest): - @classmethod - def setup_class(cls): - super().setup_class() - - initialize_database(autoinitialize=False) - - def test_admin_auth_services_get_with_no_services(self): - with self.request_context_with_admin("/"): - response = ( - self.manager.admin_auth_services_controller.process_admin_auth_services() - ) - assert response.get("admin_auth_services") == [] - - # All the protocols in ExternalIntegration.ADMIN_AUTH_PROTOCOLS - # are supported by the admin interface. - assert sorted(p.get("name") for p in response.get("protocols")) == sorted( - ExternalIntegration.ADMIN_AUTH_PROTOCOLS - ) - - self.admin.remove_role(AdminRole.SYSTEM_ADMIN) - self._db.flush() - pytest.raises( - AdminNotAuthorized, - self.manager.admin_auth_services_controller.process_admin_auth_services, - ) - - def test_admin_auth_services_post_errors(self): - with self.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict( - [ - ("protocol", "Unknown"), - ] - ) - response = ( - self.manager.admin_auth_services_controller.process_admin_auth_services() - ) - assert response == UNKNOWN_PROTOCOL - - with self.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict([]) - response = ( - self.manager.admin_auth_services_controller.process_admin_auth_services() - ) - assert response == NO_PROTOCOL_FOR_NEW_SERVICE - - with self.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict( - [ - ("id", "1234"), - ] - ) - response = ( - self.manager.admin_auth_services_controller.process_admin_auth_services() - ) - assert response == MISSING_SERVICE - - def test_admin_auth_services_post_create(self): - # TODO: Should be implemented if new external admin auth service is implemented - return - - def test_admin_auth_service_delete(self): - # TODO: Should be implemented if new external admin auth service is implemented - return diff --git a/tests/api/admin/controller/test_controller.py b/tests/api/admin/controller/test_controller.py index aaf2ce954a..f79272e6d2 100644 --- a/tests/api/admin/controller/test_controller.py +++ b/tests/api/admin/controller/test_controller.py @@ -325,18 +325,6 @@ def test_require_librarian(self): class TestSignInController(AdminControllerTest): def setup_method(self): super().setup_method() - self.admin.credential = json.dumps( - { - "access_token": "abc123", - "client_id": "", - "client_secret": "", - "refresh_token": "", - "token_expiry": "", - "token_uri": "", - "user_agent": "", - "invalid": "", - } - ) self.admin.password_hashed = None def test_admin_auth_providers(self): diff --git a/tests/api/admin/test_password_admin_authentication_provider.py b/tests/api/admin/test_password_admin_authentication_provider.py index e854323f2c..26ba49459e 100644 --- a/tests/api/admin/test_password_admin_authentication_provider.py +++ b/tests/api/admin/test_password_admin_authentication_provider.py @@ -8,7 +8,7 @@ class TestPasswordAdminAuthenticationProvider(DatabaseTest): def test_sign_in(self): - password_auth = PasswordAdminAuthenticationProvider(None) + password_auth = PasswordAdminAuthenticationProvider() # There are two admins with passwords. admin1, ignore = create(self._db, Admin, email="admin1@example.org") @@ -74,7 +74,7 @@ def test_sign_in(self): assert redirect == "/admin/web" def test_sign_in_case_insensitive(self): - password_auth = PasswordAdminAuthenticationProvider(None) + password_auth = PasswordAdminAuthenticationProvider() # There are two admins with passwords. admin1, ignore = create(self._db, Admin, email="admin1@example.org") diff --git a/tests/api/admin/test_routes.py b/tests/api/admin/test_routes.py index 4f04f819b5..28cc67d8bf 100644 --- a/tests/api/admin/test_routes.py +++ b/tests/api/admin/test_routes.py @@ -451,25 +451,6 @@ def test_process_collection_library_registrations(self): self.assert_supported_methods(url, "GET", "POST") -class TestAdminAuthServices(AdminRouteTest): - - CONTROLLER_NAME = "admin_auth_services_controller" - - def test_process_admin_auth_services(self): - url = "/admin/admin_auth_services" - self.assert_authenticated_request_calls( - url, self.controller.process_admin_auth_services - ) - self.assert_supported_methods(url, "GET", "POST") - - def test_process_delete(self): - url = "/admin/admin_auth_service/" - self.assert_authenticated_request_calls( - url, self.controller.process_delete, "", http_method="DELETE" - ) - self.assert_supported_methods(url, "DELETE") - - class TestAdminIndividualAdminSettings(AdminRouteTest): CONTROLLER_NAME = "admin_individual_admin_settings_controller" From 3ad47f1f446d6fb163f52578ee2599ae5604e538 Mon Sep 17 00:00:00 2001 From: Porin Custic Date: Tue, 14 Feb 2023 07:50:36 +0100 Subject: [PATCH 4/7] Remove admin.credental field --- core/model/admin.py | 8 -------- migration/20230214-remove-admin-credential.sql | 6 ++++++ 2 files changed, 6 insertions(+), 8 deletions(-) create mode 100644 migration/20230214-remove-admin-credential.sql diff --git a/core/model/admin.py b/core/model/admin.py index 276fcb7130..e430db5584 100644 --- a/core/model/admin.py +++ b/core/model/admin.py @@ -29,9 +29,6 @@ class Admin(Base, HasSessionCache): id = Column(Integer, primary_key=True) email = Column(Unicode, unique=True, nullable=False) - # Admins who log in with OAuth will have a credential. - credential = Column(Unicode) - # Admins can also log in with a local password. password_hashed = Column(Unicode, index=True) @@ -41,11 +38,6 @@ class Admin(Base, HasSessionCache): def cache_key(self): return self.email - def update_credentials(self, _db, credential=None): - if credential: - self.credential = credential - _db.commit() - @validates("email") def validate_email(self, key, address): # strip any whitespace from email address diff --git a/migration/20230214-remove-admin-credential.sql b/migration/20230214-remove-admin-credential.sql new file mode 100644 index 0000000000..aa4e2052e7 --- /dev/null +++ b/migration/20230214-remove-admin-credential.sql @@ -0,0 +1,6 @@ +-- +-- Remove admin credential field since we are not using any external admin auth service. +-- + +-- Remove the admin.credential +ALTER TABLE admins DROP COLUMN if exists credential; From fbbc6ac1d3700f09d0020270cdc4691bcc6ef336 Mon Sep 17 00:00:00 2001 From: Porin Custic Date: Mon, 20 Feb 2023 13:42:08 +0100 Subject: [PATCH 5/7] Use alembic for migration --- ...e32b5649_remove_admin_credential_column.py | 24 +++++++++++++++++++ .../20230214-remove-admin-credential.sql | 6 ----- 2 files changed, 24 insertions(+), 6 deletions(-) create mode 100644 alembic/versions/20230220_0c2fe32b5649_remove_admin_credential_column.py delete mode 100644 migration/20230214-remove-admin-credential.sql diff --git a/alembic/versions/20230220_0c2fe32b5649_remove_admin_credential_column.py b/alembic/versions/20230220_0c2fe32b5649_remove_admin_credential_column.py new file mode 100644 index 0000000000..bbd5804de5 --- /dev/null +++ b/alembic/versions/20230220_0c2fe32b5649_remove_admin_credential_column.py @@ -0,0 +1,24 @@ +"""Remove admin.credential column + +Revision ID: 0c2fe32b5649 +Revises: 6f96516c7a7b +Create Date: 2023-02-20 12:36:15.204519+00:00 + +""" +import sqlalchemy as sa + +from alembic import op + +# revision identifiers, used by Alembic. +revision = "0c2fe32b5649" +down_revision = "6f96516c7a7b" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.drop_column("admins", "credential") + + +def downgrade() -> None: + op.add_column("admins", sa.Column("credential", sa.Unicode(), nullable=True)) diff --git a/migration/20230214-remove-admin-credential.sql b/migration/20230214-remove-admin-credential.sql deleted file mode 100644 index aa4e2052e7..0000000000 --- a/migration/20230214-remove-admin-credential.sql +++ /dev/null @@ -1,6 +0,0 @@ --- --- Remove admin credential field since we are not using any external admin auth service. --- - --- Remove the admin.credential -ALTER TABLE admins DROP COLUMN if exists credential; From bc994ffb73592956c2002c02883cb57aaa8e1711 Mon Sep 17 00:00:00 2001 From: Porin Custic Date: Mon, 27 Feb 2023 07:59:07 +0100 Subject: [PATCH 6/7] Fixing unknown leftover google_oauth mention --- tests/api/admin/controller/test_controller.py | 30 ------------------- 1 file changed, 30 deletions(-) diff --git a/tests/api/admin/controller/test_controller.py b/tests/api/admin/controller/test_controller.py index 78c2c34e7c..fbcb4c1c19 100644 --- a/tests/api/admin/controller/test_controller.py +++ b/tests/api/admin/controller/test_controller.py @@ -536,19 +536,6 @@ def test_forgot_password_get(self): assert response.status_code == 500 assert response.uri == ADMIN_AUTH_NOT_CONFIGURED.uri - # If there is only Google OAuth we should also get an error - create( - self._db, - ExternalIntegration, - protocol=ExternalIntegration.GOOGLE_OAUTH, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) - with self.app.test_request_context("/admin/forgot_password"): - response = reset_password_ctrl.forgot_password() - - assert response.status_code == 400 - assert response.uri == ADMIN_AUTH_MECHANISM_NOT_CONFIGURED.uri - # If auth providers are set we should get forgot password page - success path self.admin.password = "password" with self.app.test_request_context("/admin/forgot_password"): @@ -640,23 +627,6 @@ def test_reset_password_get(self): as_text=True ) - # If there is only Google OAuth we should also get an error - create( - self._db, - ExternalIntegration, - protocol=ExternalIntegration.GOOGLE_OAUTH, - goal=ExternalIntegration.ADMIN_AUTH_GOAL, - ) - with self.app.test_request_context("/admin/reset_password"): - response = reset_password_ctrl.reset_password(token) - - assert ( - response.status_code == ADMIN_AUTH_MECHANISM_NOT_CONFIGURED.status_code - ) - assert str(ADMIN_AUTH_MECHANISM_NOT_CONFIGURED.detail) in response.get_data( - as_text=True - ) - # If admin is already signed in it gets redirected since it can use regular reset password flow self.admin.password = "password" with self.app.test_request_context("/admin/reset_password"): From 0845185c4fe2dee12a61ecd07d40036543a0867a Mon Sep 17 00:00:00 2001 From: Porin Custic Date: Wed, 1 Mar 2023 18:26:37 +0100 Subject: [PATCH 7/7] Update release version --- api/admin/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/admin/config.py b/api/admin/config.py index 625e4712c8..0229fb18cc 100644 --- a/api/admin/config.py +++ b/api/admin/config.py @@ -13,7 +13,7 @@ class Configuration: APP_NAME = "Palace Collection Manager" PACKAGE_NAME = "@thepalaceproject/circulation-admin" - PACKAGE_VERSION = "0.5.0" + PACKAGE_VERSION = "1.5.1" STATIC_ASSETS = { "admin_js": "circulation-admin.js",