From f839bae1f5b0c4bec7574f44f04252c7dfcc10a9 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 19 Jan 2016 11:38:29 +0000 Subject: [PATCH 1/6] Add rest of user model fields to api. First step to moving user interactions to api. --- app/dao/users_dao.py | 3 +- app/encryption.py | 10 ++ app/models.py | 22 ++++ app/schemas.py | 2 +- app/user/rest.py | 14 +- migrations/versions/0006_add_user_details.py | 40 ++++++ requirements.txt | 1 + tests/app/conftest.py | 9 +- tests/app/dao/test_users_dao.py | 8 +- tests/app/user/test_rest.py | 132 ++++++++++++++----- 10 files changed, 193 insertions(+), 48 deletions(-) create mode 100644 app/encryption.py create mode 100644 migrations/versions/0006_add_user_details.py diff --git a/app/dao/users_dao.py b/app/dao/users_dao.py index 5b389e6b7..db23fbe37 100644 --- a/app/dao/users_dao.py +++ b/app/dao/users_dao.py @@ -8,7 +8,8 @@ from app.models import User def save_model_user(usr, update_dict={}): if update_dict: - del update_dict['id'] + if update_dict.get('id'): + del update_dict['id'] db.session.query(User).filter_by(id=usr.id).update(update_dict) else: db.session.add(usr) diff --git a/app/encryption.py b/app/encryption.py new file mode 100644 index 000000000..51caaab72 --- /dev/null +++ b/app/encryption.py @@ -0,0 +1,10 @@ +from flask.ext.bcrypt import generate_password_hash, check_password_hash + + +def hashpw(password): + return generate_password_hash(password.encode('UTF-8'), 10) + + +def check_hash(password, hashed_password): + # If salt is invalid throws a 500 should add try/catch here + return check_password_hash(hashed_password, password) diff --git a/app/models.py b/app/models.py index 2c6b8be1d..745e4ef0e 100644 --- a/app/models.py +++ b/app/models.py @@ -2,6 +2,10 @@ from . import db import datetime from sqlalchemy.dialects.postgresql import UUID +from app.encryption import ( + hashpw, + check_hash +) def filter_null_value_fields(obj): @@ -14,6 +18,7 @@ class User(db.Model): __tablename__ = 'users' id = db.Column(db.Integer, primary_key=True) + name = db.Column(db.String, nullable=False, index=True, unique=False) email_address = db.Column(db.String(255), nullable=False, index=True, unique=True) created_at = db.Column( db.DateTime, @@ -27,6 +32,23 @@ class User(db.Model): unique=False, nullable=True, onupdate=datetime.datetime.now) + _password = db.Column(db.String, index=False, unique=False, nullable=False) + mobile_number = db.Column(db.String, index=False, unique=False, nullable=False) + password_changed_at = db.Column(db.DateTime, index=False, unique=False, nullable=True) + logged_in_at = db.Column(db.DateTime, nullable=True) + failed_login_count = db.Column(db.Integer, nullable=False, default=0) + state = db.Column(db.String, nullable=False, default='pending') + + @property + def password(self): + raise AttributeError("Password not readable") + + @password.setter + def password(self, password): + self._password = hashpw(password) + + def check_password(self, password): + return check_hash(password, self._password) user_to_service = db.Table( diff --git a/app/schemas.py b/app/schemas.py index f5d9a0349..542144ea7 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -11,7 +11,7 @@ from marshmallow import post_load class UserSchema(ma.ModelSchema): class Meta: model = models.User - exclude = ("updated_at", "created_at", "user_to_service") + exclude = ("updated_at", "created_at", "user_to_service", "_password") # TODO process users list, to return a list of user.id diff --git a/app/user/rest.py b/app/user/rest.py index 2731712a7..c27a12e7c 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -17,8 +17,13 @@ user = Blueprint('user', __name__) @user.route('', methods=['POST']) def create_user(): user, errors = user_schema.load(request.get_json()) + req_json = request.get_json() + if not req_json.get('password'): + return jsonify(result="error", message={'error': 'password missing'}), 400 if errors: return jsonify(result="error", message=errors), 400 + + user.password = req_json.get('password') save_model_user(user) return jsonify(data=user_schema.dump(user).data), 201 @@ -37,15 +42,8 @@ def update_user(user_id): delete_model_user(user) else: status_code = 200 - # TODO there has got to be a better way to do the next three lines - update_user, errors = user_schema.load(request.get_json()) - if errors: - return jsonify(result="error", message=errors), 400 - update_dict, errors = user_schema.dump(update_user) - # TODO FIX ME - # Remove update_service model which is added to db.session db.session.rollback() - save_model_user(user, update_dict=update_dict) + save_model_user(user, update_dict=request.get_json()) return jsonify(data=user_schema.dump(user).data), status_code diff --git a/migrations/versions/0006_add_user_details.py b/migrations/versions/0006_add_user_details.py new file mode 100644 index 000000000..a789a348c --- /dev/null +++ b/migrations/versions/0006_add_user_details.py @@ -0,0 +1,40 @@ +"""empty message + +Revision ID: 0006_add_user_details +Revises: 0005_add_job_details +Create Date: 2016-01-19 11:16:06.518285 + +""" + +# revision identifiers, used by Alembic. +revision = '0006_add_user_details' +down_revision = '0005_add_job_details' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.add_column('users', sa.Column('_password', sa.String(), nullable=False)) + op.add_column('users', sa.Column('failed_login_count', sa.Integer(), nullable=False)) + op.add_column('users', sa.Column('logged_in_at', sa.DateTime(), nullable=True)) + op.add_column('users', sa.Column('mobile_number', sa.String(), nullable=False)) + op.add_column('users', sa.Column('name', sa.String(), nullable=False)) + op.add_column('users', sa.Column('password_changed_at', sa.DateTime(), nullable=True)) + op.add_column('users', sa.Column('state', sa.String(), nullable=False)) + op.create_index(op.f('ix_users_name'), 'users', ['name'], unique=False) + ### end Alembic commands ### + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.drop_index(op.f('ix_users_name'), table_name='users') + op.drop_column('users', 'state') + op.drop_column('users', 'password_changed_at') + op.drop_column('users', 'name') + op.drop_column('users', 'mobile_number') + op.drop_column('users', 'logged_in_at') + op.drop_column('users', 'failed_login_count') + op.drop_column('users', '_password') + ### end Alembic commands ### diff --git a/requirements.txt b/requirements.txt index 5112e5b2f..d76120cfd 100644 --- a/requirements.txt +++ b/requirements.txt @@ -10,6 +10,7 @@ marshmallow==2.4.2 marshmallow-sqlalchemy==0.8.0 flask-marshmallow==0.6.2 itsdangerous==0.24 +Flask-Bcrypt==0.6.2 git+https://github.com/alphagov/notifications-python-client.git@0.1.5#egg=notifications-python-client==0.1.5 diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 79cf576c2..e8854a57a 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -12,7 +12,14 @@ import uuid def sample_user(notify_db, notify_db_session, email="notify@digital.cabinet-office.gov.uk"): - user = User(**{'email_address': email}) + data = { + 'name': 'Test User', + 'email_address': email, + 'password': 'password', + 'mobile_number': '+44 7700 900986', + 'state': 'active' + } + user = User(**data) save_model_user(user) return user diff --git a/tests/app/dao/test_users_dao.py b/tests/app/dao/test_users_dao.py index 6190ab20b..a8b15c603 100644 --- a/tests/app/dao/test_users_dao.py +++ b/tests/app/dao/test_users_dao.py @@ -8,7 +8,13 @@ from app.models import User def test_create_user(notify_api, notify_db, notify_db_session): email = 'notify@digital.cabinet-office.gov.uk' - user = User(**{'email_address': email}) + data = { + 'name': 'Test User', + 'email_address': email, + 'password': 'password', + 'mobile_number': '+44 7700 900986' + } + user = User(**data) save_model_user(user) assert User.query.count() == 1 assert User.query.first().email_address == email diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 7a681af08..19ffd3448 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -19,7 +19,17 @@ def test_get_user_list(notify_api, notify_db, notify_db_session, sample_user, sa assert response.status_code == 200 json_resp = json.loads(response.get_data(as_text=True)) assert len(json_resp['data']) == 2 - assert {"email_address": sample_user.email_address, "id": sample_user.id} in json_resp['data'] + expected = { + "name": "Test User", + "email_address": sample_user.email_address, + "id": sample_user.id, + "mobile_number": "+44 7700 900986", + "password_changed_at": None, + "logged_in_at": None, + "state": "active", + "failed_login_count": 0 + } + assert expected in json_resp['data'] def test_get_user(notify_api, notify_db, notify_db_session, sample_user, sample_admin_service_id): @@ -36,7 +46,17 @@ def test_get_user(notify_api, notify_db, notify_db_session, sample_user, sample_ headers=[header]) assert resp.status_code == 200 json_resp = json.loads(resp.get_data(as_text=True)) - assert json_resp['data'] == {"email_address": sample_user.email_address, "id": sample_user.id} + expected = { + "name": "Test User", + "email_address": sample_user.email_address, + "id": sample_user.id, + "mobile_number": "+44 7700 900986", + "password_changed_at": None, + "logged_in_at": None, + "state": "active", + "failed_login_count": 0 + } + assert json_resp['data'] == expected def test_post_user(notify_api, notify_db, notify_db_session, sample_admin_service_id): @@ -46,8 +66,16 @@ def test_post_user(notify_api, notify_db, notify_db_session, sample_admin_servic with notify_api.test_request_context(): with notify_api.test_client() as client: assert User.query.count() == 1 - data = {'email_address': 'user@digital.cabinet-office.gov.uk'} - + data = { + "name": "Test User", + "email_address": "user@digital.cabinet-office.gov.uk", + "password": "password", + "mobile_number": "+44 7700 900986", + "password_changed_at": None, + "logged_in_at": None, + "state": "active", + "failed_login_count": 0 + } auth_header = create_authorization_header(service_id=sample_admin_service_id, path=url_for('user.create_user'), method='POST', @@ -73,7 +101,14 @@ def test_post_user_missing_attribute_email(notify_api, notify_db, notify_db_sess with notify_api.test_client() as client: assert User.query.count() == 1 data = { - 'blah': 'blah.blah'} + "name": "Test User", + "password": "password", + "mobile_number": "+44 7700 900986", + "password_changed_at": None, + "logged_in_at": None, + "state": "active", + "failed_login_count": 0 + } auth_header = create_authorization_header(service_id=sample_admin_service_id, path=url_for('user.create_user'), method='POST', @@ -89,6 +124,37 @@ def test_post_user_missing_attribute_email(notify_api, notify_db, notify_db_sess assert {'email_address': ['Missing data for required field.']} == json_resp['message'] +def test_post_user_missing_attribute_password(notify_api, notify_db, notify_db_session, sample_admin_service_id): + """ + Tests POST endpoint '/' missing attribute password. + """ + with notify_api.test_request_context(): + with notify_api.test_client() as client: + assert User.query.count() == 1 + data = { + "name": "Test User", + "email_address": "user@digital.cabinet-office.gov.uk", + "mobile_number": "+44 7700 900986", + "password_changed_at": None, + "logged_in_at": None, + "state": "active", + "failed_login_count": 0 + } + auth_header = create_authorization_header(service_id=sample_admin_service_id, + path=url_for('user.create_user'), + method='POST', + request_body=json.dumps(data)) + headers = [('Content-Type', 'application/json'), auth_header] + resp = client.post( + url_for('user.create_user'), + data=json.dumps(data), + headers=headers) + assert resp.status_code == 400 + assert User.query.count() == 1 + json_resp = json.loads(resp.get_data(as_text=True)) + assert {'error': 'password missing'} == json_resp['message'] + + def test_put_user(notify_api, notify_db, notify_db_session, sample_user, sample_admin_service_id): """ Tests PUT endpoint '/' to update a user. @@ -98,7 +164,8 @@ def test_put_user(notify_api, notify_db, notify_db_session, sample_user, sample_ assert User.query.count() == 2 new_email = 'new@digital.cabinet-office.gov.uk' data = { - 'email_address': new_email} + 'email_address': new_email + } auth_header = create_authorization_header(service_id=sample_admin_service_id, path=url_for('user.update_user', user_id=sample_user.id), method='PUT', @@ -112,9 +179,18 @@ def test_put_user(notify_api, notify_db, notify_db_session, sample_user, sample_ assert User.query.count() == 2 user = User.query.filter_by(email_address=new_email).first() json_resp = json.loads(resp.get_data(as_text=True)) - assert json_resp['data'] == {'email_address': new_email, 'id': user.id} + expected = { + "name": "Test User", + "email_address": new_email, + "mobile_number": "+44 7700 900986", + "password_changed_at": None, + "id": user.id, + "logged_in_at": None, + "state": "active", + "failed_login_count": 0 + } + assert json_resp['data'] == expected assert json_resp['data']['email_address'] == new_email - assert json_resp['data']['id'] == user.id def test_put_user_not_exists(notify_api, notify_db, notify_db_session, sample_user, sample_admin_service_id): @@ -144,33 +220,6 @@ def test_put_user_not_exists(notify_api, notify_db, notify_db_session, sample_us assert user.email_address != new_email -def test_put_user_missing_email(notify_api, notify_db, notify_db_session, sample_user, sample_admin_service_id): - """ - Tests PUT endpoint '/' missing attribute email. - """ - with notify_api.test_request_context(): - with notify_api.test_client() as client: - assert User.query.count() == 2 - new_email = 'new@digital.cabinet-office.gov.uk' - data = { - 'blah': new_email} - auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('user.update_user', user_id=sample_user.id), - method='PUT', - request_body=json.dumps(data)) - headers = [('Content-Type', 'application/json'), auth_header] - resp = client.put( - url_for('user.update_user', user_id=sample_user.id), - data=json.dumps(data), - headers=headers) - assert resp.status_code == 400 - assert User.query.count() == 2 - user = User.query.get(sample_user.id) - json_resp = json.loads(resp.get_data(as_text=True)) - assert user.email_address == sample_user.email_address - assert {'email_address': ['Missing data for required field.']} == json_resp['message'] - - def test_get_user_services(notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id): """ Tests GET endpoint "//service/" to retrieve services for a user. @@ -280,7 +329,18 @@ def test_delete_user(notify_api, notify_db, notify_db_session, sample_user, samp assert resp.status_code == 202 json_resp = json.loads(resp.get_data(as_text=True)) assert User.query.count() == 1 - assert json_resp['data'] == {'id': sample_user.id, 'email_address': sample_user.email_address} + expected = { + "name": "Test User", + "email_address": sample_user.email_address, + "mobile_number": "+44 7700 900986", + "password_changed_at": None, + "id": sample_user.id, + "logged_in_at": None, + "state": "active", + "failed_login_count": 0 + + } + assert json_resp['data'] == expected def test_delete_user_not_exists(notify_api, notify_db, notify_db_session, sample_user, sample_admin_service_id): From 44824c9985f67a421150c1a1d5923452127019f6 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 19 Jan 2016 12:07:14 +0000 Subject: [PATCH 2/6] Update error message for password to match marshmallow errors. --- app/user/rest.py | 5 ++++- tests/app/user/test_rest.py | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index c27a12e7c..9e1fbb6d0 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -19,7 +19,8 @@ def create_user(): user, errors = user_schema.load(request.get_json()) req_json = request.get_json() if not req_json.get('password'): - return jsonify(result="error", message={'error': 'password missing'}), 400 + errors = {'password': ['Missing data for required field.']} + return jsonify(result="error", message=errors), 400 if errors: return jsonify(result="error", message=errors), 400 @@ -41,6 +42,8 @@ def update_user(user_id): status_code = 202 delete_model_user(user) else: + # TODO removed some validation checking by using load + # which will need to be done in another way status_code = 200 db.session.rollback() save_model_user(user, update_dict=request.get_json()) diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 19ffd3448..e6cf0ee3b 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -152,7 +152,7 @@ def test_post_user_missing_attribute_password(notify_api, notify_db, notify_db_s assert resp.status_code == 400 assert User.query.count() == 1 json_resp = json.loads(resp.get_data(as_text=True)) - assert {'error': 'password missing'} == json_resp['message'] + assert {'password': ['Missing data for required field.']} == json_resp['message'] def test_put_user(notify_api, notify_db, notify_db_session, sample_user, sample_admin_service_id): From 4fc5c3432046d3b39b9e44209850272de1860c0f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 19 Jan 2016 12:07:00 +0000 Subject: [PATCH 3/6] Change Tokens to ApiKey Added name to ApiKey model --- app/authentication/auth.py | 4 +- app/dao/api_key_dao.py | 51 ++++++++++++ app/dao/tokens_dao.py | 51 ------------ app/models.py | 9 +- app/schemas.py | 11 ++- app/service/rest.py | 44 +++++----- application.py | 12 +-- .../versions/0007_change_to_api_keys.py | 47 +++++++++++ tests/__init__.py | 6 +- .../app/authentication/test_authentication.py | 32 +++---- tests/app/conftest.py | 26 +++--- tests/app/dao/test_api_key_dao.py | 69 +++++++++++++++ tests/app/dao/test_tokens_dao.py | 62 -------------- tests/app/service/test_rest.py | 83 ++++++++++--------- 14 files changed, 281 insertions(+), 226 deletions(-) create mode 100644 app/dao/api_key_dao.py delete mode 100644 app/dao/tokens_dao.py create mode 100644 migrations/versions/0007_change_to_api_keys.py create mode 100644 tests/app/dao/test_api_key_dao.py delete mode 100644 tests/app/dao/test_tokens_dao.py diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 7b9eccb96..62de0ac59 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -2,7 +2,7 @@ from flask import request, jsonify, _request_ctx_stack from client.authentication import decode_jwt_token, get_token_issuer from client.errors import TokenDecodeError, TokenRequestError, TokenExpiredError, TokenPayloadError -from app.dao.tokens_dao import get_unsigned_token +from app.dao.api_key_dao import get_unsigned_secret def authentication_response(message, code): @@ -49,5 +49,5 @@ def requires_auth(): def fetch_client(client): return { "client": client, - "secret": get_unsigned_token(client) + "secret": get_unsigned_secret(client) } diff --git a/app/dao/api_key_dao.py b/app/dao/api_key_dao.py new file mode 100644 index 000000000..3f1951f67 --- /dev/null +++ b/app/dao/api_key_dao.py @@ -0,0 +1,51 @@ +from flask import current_app +from itsdangerous import URLSafeSerializer + +from app import db +from app.models import ApiKey + + +def save_model_api_key(api_key, update_dict={}): + if update_dict: + del update_dict['id'] + db.session.query(ApiKey).filter_by(id=api_key.id).update(update_dict) + else: + api_key.secret = _generate_secret() + db.session.add(api_key) + db.session.commit() + + +def get_model_api_keys(service_id=None, raise_=True): + """ + :param raise_: when True query api_keys using one() which will raise NoResultFound exception + when False query api_keys usong first() which will return None and not raise an exception. + """ + if service_id: + # If expiry date is None the api_key is active + if raise_: + return ApiKey.query.filter_by(service_id=service_id, expiry_date=None).one() + else: + return ApiKey.query.filter_by(service_id=service_id, expiry_date=None).first() + return ApiKey.query.filter_by().all() + + +def get_unsigned_secret(service_id): + """ + There should only be one valid api_keys for each service. + This method can only be exposed to the Authentication of the api calls. + """ + api_key = ApiKey.query.filter_by(service_id=service_id, expiry_date=None).one() + return _get_secret(api_key.secret) + + +def _generate_secret(token=None): + import uuid + if not token: + token = uuid.uuid4() + serializer = URLSafeSerializer(current_app.config.get('SECRET_KEY')) + return serializer.dumps(str(token), current_app.config.get('DANGEROUS_SALT')) + + +def _get_secret(signed_secret): + serializer = URLSafeSerializer(current_app.config.get('SECRET_KEY')) + return serializer.loads(signed_secret, salt=current_app.config.get('DANGEROUS_SALT')) diff --git a/app/dao/tokens_dao.py b/app/dao/tokens_dao.py deleted file mode 100644 index 54a5248d9..000000000 --- a/app/dao/tokens_dao.py +++ /dev/null @@ -1,51 +0,0 @@ -from flask import current_app -from itsdangerous import URLSafeSerializer - -from app import db -from app.models import Token - - -def save_model_token(token, update_dict={}): - if update_dict: - del update_dict['id'] - db.session.query(Token).filter_by(id=token.id).update(update_dict) - else: - token.token = _generate_token() - db.session.add(token) - db.session.commit() - - -def get_model_tokens(service_id=None, raise_=True): - """ - :param raise_: when True query tokens using one() which will raise NoResultFound exception - when False query tokens usong first() which will return None and not raise an exception. - """ - if service_id: - # If expiry date is None the token is active - if raise_: - return Token.query.filter_by(service_id=service_id, expiry_date=None).one() - else: - return Token.query.filter_by(service_id=service_id, expiry_date=None).first() - return Token.query.filter_by().all() - - -def get_unsigned_token(service_id): - """ - There should only be one valid token for each service. - This method can only be exposed to the Authentication of the api calls. - """ - token = Token.query.filter_by(service_id=service_id, expiry_date=None).one() - return _get_token(token.token) - - -def _generate_token(token=None): - import uuid - if not token: - token = uuid.uuid4() - serializer = URLSafeSerializer(current_app.config.get('SECRET_KEY')) - return serializer.dumps(str(token), current_app.config.get('DANGEROUS_SALT')) - - -def _get_token(token): - serializer = URLSafeSerializer(current_app.config.get('SECRET_KEY')) - return serializer.loads(token, salt=current_app.config.get('DANGEROUS_SALT')) diff --git a/app/models.py b/app/models.py index 745e4ef0e..4fb0ee9d7 100644 --- a/app/models.py +++ b/app/models.py @@ -85,13 +85,14 @@ class Service(db.Model): restricted = db.Column(db.Boolean, index=False, unique=False, nullable=False) -class Token(db.Model): - __tablename__ = 'tokens' +class ApiKey(db.Model): + __tablename__ = 'api_key' id = db.Column(db.Integer, primary_key=True) - token = db.Column(db.String(255), unique=True, nullable=False) + name = db.Column(db.String(255), nullable=False) + secret = db.Column(db.String(255), unique=True, nullable=False) service_id = db.Column(db.Integer, db.ForeignKey('services.id'), index=True, nullable=False) - service = db.relationship('Service', backref=db.backref('tokens', lazy='dynamic')) + service = db.relationship('Service', backref=db.backref('api_key', lazy='dynamic')) expiry_date = db.Column(db.DateTime) diff --git a/app/schemas.py b/app/schemas.py index 542144ea7..edc87f4a3 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -1,6 +1,5 @@ from . import ma from . import models -from marshmallow import post_load # TODO I think marshmallow provides a better integration and error handling. # Would be better to replace functionality in dao with the marshmallow supported @@ -19,7 +18,7 @@ class UserSchema(ma.ModelSchema): class ServiceSchema(ma.ModelSchema): class Meta: model = models.Service - exclude = ("updated_at", "created_at", "tokens", "templates", "jobs") + exclude = ("updated_at", "created_at", "api_key", "templates", "jobs") class TemplateSchema(ma.ModelSchema): @@ -28,9 +27,9 @@ class TemplateSchema(ma.ModelSchema): exclude = ("updated_at", "created_at", "service_id", "jobs") -class TokenSchema(ma.ModelSchema): +class ApiKeySchema(ma.ModelSchema): class Meta: - model = models.Token + model = models.ApiKey exclude = ["service"] @@ -45,7 +44,7 @@ service_schema = ServiceSchema() services_schema = ServiceSchema(many=True) template_schema = TemplateSchema() templates_schema = TemplateSchema(many=True) -token_schema = TokenSchema() -tokens_schema = TokenSchema(many=True) +api_key_schema = ApiKeySchema() +api_keys_schema = ApiKeySchema(many=True) job_schema = JobSchema() jobs_schema = JobSchema(many=True) diff --git a/app/service/rest.py b/app/service/rest.py index 50863d148..1f9c77f99 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -1,6 +1,6 @@ from datetime import datetime -from flask import (jsonify, request, current_app) +from flask import (jsonify, request) from sqlalchemy.exc import DataError from sqlalchemy.orm.exc import NoResultFound @@ -10,8 +10,8 @@ from app.dao.services_dao import ( save_model_service, get_model_services, delete_model_service) from app.dao.templates_dao import ( save_model_template, get_model_templates, delete_model_template) -from app.dao.tokens_dao import (save_model_token, get_model_tokens, get_unsigned_token) -from app.models import Token +from app.dao.api_key_dao import (save_model_api_key, get_model_api_keys, get_unsigned_secret) +from app.models import ApiKey from app.schemas import ( services_schema, service_schema, template_schema) @@ -29,13 +29,11 @@ def create_service(): # db.session.commit try: save_model_service(service) - save_model_token(Token(service_id=service.id)) except DAOException as e: return jsonify(result="error", message=str(e)), 400 - return jsonify(data=service_schema.dump(service).data, token=get_unsigned_token(service.id)), 201 + return jsonify(data=service_schema.dump(service).data), 201 -# TODO auth to be added @service.route('/', methods=['PUT', 'DELETE']) def update_service(service_id): try: @@ -64,7 +62,6 @@ def update_service(service_id): return jsonify(data=service_schema.dump(service).data), status_code -# TODO auth to be added. @service.route('/', methods=['GET']) @service.route('/', methods=['GET']) def get_service(service_id=None): @@ -78,9 +75,8 @@ def get_service(service_id=None): return jsonify(data=data) -# TODO auth to be added -@service.route('//token/renew', methods=['POST']) -def renew_token(service_id=None): +@service.route('//api-key/renew', methods=['POST']) +def renew_api_key(service_id=None): try: get_model_services(service_id=service_id) except DataError: @@ -89,20 +85,22 @@ def renew_token(service_id=None): return jsonify(result="error", message="Service not found"), 404 try: - service_token = get_model_tokens(service_id=service_id, raise_=False) - if service_token: - # expire existing token - save_model_token(service_token, update_dict={'id': service_token.id, 'expiry_date': datetime.now()}) + service_api_key = get_model_api_keys(service_id=service_id, raise_=False) + if service_api_key: + # expire existing api_key + save_model_api_key(service_api_key, update_dict={'id': service_api_key.id, 'expiry_date': datetime.now()}) # create a new one - save_model_token(Token(service_id=service_id)) + # TODO: what validation should be done here? + secret_name = request.get_json()['name'] + save_model_api_key(ApiKey(service_id=service_id, name=secret_name)) except DAOException as e: return jsonify(result='error', message=str(e)), 400 - unsigned_token = get_unsigned_token(service_id) - return jsonify(data=unsigned_token), 201 + unsigned_api_key = get_unsigned_secret(service_id) + return jsonify(data=unsigned_api_key), 201 -@service.route('//token/revoke', methods=['POST']) -def revoke_token(service_id): +@service.route('//api-key/revoke', methods=['POST']) +def revoke_api_key(service_id): try: get_model_services(service_id=service_id) except DataError: @@ -110,13 +108,12 @@ def revoke_token(service_id): except NoResultFound: return jsonify(result="error", message="Service not found"), 404 - service_token = get_model_tokens(service_id=service_id, raise_=False) - if service_token: - save_model_token(service_token, update_dict={'id': service_token.id, 'expiry_date': datetime.now()}) + service_api_key = get_model_api_keys(service_id=service_id, raise_=False) + if service_api_key: + save_model_api_key(service_api_key, update_dict={'id': service_api_key.id, 'expiry_date': datetime.now()}) return jsonify(), 202 -# TODO auth to be added. @service.route('//template/', methods=['POST']) def create_template(service_id): try: @@ -135,7 +132,6 @@ def create_template(service_id): return jsonify(data=template_schema.dump(template).data), 201 -# TODO auth to be added @service.route('//template/', methods=['PUT', 'DELETE']) def update_template(service_id, template_id): try: diff --git a/application.py b/application.py index 6648ee2a0..3a050fe50 100644 --- a/application.py +++ b/application.py @@ -26,10 +26,10 @@ def list_routes(): def create_admin_user_service(): """ Convience method to create a admin user and service - :return: API token for admin service + :return: API secret for admin service """ - from app.models import User, Service, Token - from app.dao import tokens_dao, users_dao, services_dao + from app.models import User, Service, ApiKey + from app.dao import api_key_dao, users_dao, services_dao from flask import current_app user = User(**{'email_address': current_app.config['ADMIN_USER_EMAIL_ADDRESS']}) @@ -41,9 +41,9 @@ def create_admin_user_service(): 'active': True, 'restricted': True}) services_dao.save_model_service(service) - token = Token(**{'service_id': service.id}) - tokens_dao.save_model_token(token) - print('Token: {}'.format(tokens_dao.get_unsigned_token(service.id))) + api_key = ApiKey(**{'service_id': service.id}) + api_key_dao.save_model_api_key(api_key) + print('ApiKey: {}'.format(api_key_dao.get_unsigned_secret(service.id))) if __name__ == '__main__': diff --git a/migrations/versions/0007_change_to_api_keys.py b/migrations/versions/0007_change_to_api_keys.py new file mode 100644 index 000000000..27718976f --- /dev/null +++ b/migrations/versions/0007_change_to_api_keys.py @@ -0,0 +1,47 @@ +"""empty message + +Revision ID: 0007_change_to_api_keys +Revises: 0005_add_job_details +Create Date: 2016-01-19 10:50:46.269618 + +""" + +# revision identifiers, used by Alembic. +revision = '0007_change_to_api_keys' +down_revision = '0005_add_job_details' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.create_table('api_key', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('name', sa.String(length=255), nullable=False), + sa.Column('secret', sa.String(length=255), nullable=False), + sa.Column('service_id', sa.Integer(), nullable=False), + sa.Column('expiry_date', sa.DateTime(), nullable=True), + sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), + sa.PrimaryKeyConstraint('id'), + sa.UniqueConstraint('secret') + ) + op.create_index(op.f('ix_api_key_service_id'), 'api_key', ['service_id'], unique=False) + op.drop_table('tokens') + ### end Alembic commands ### + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.create_table('tokens', + sa.Column('id', sa.INTEGER(), nullable=False), + sa.Column('token', sa.VARCHAR(length=255), autoincrement=False, nullable=False), + sa.Column('service_id', sa.INTEGER(), autoincrement=False, nullable=False), + sa.Column('expiry_date', postgresql.TIMESTAMP(), autoincrement=False, nullable=True), + sa.ForeignKeyConstraint(['service_id'], ['services.id'], name='tokens_service_id_fkey'), + sa.PrimaryKeyConstraint('id', name='tokens_pkey'), + sa.UniqueConstraint('token', name='tokens_token_key') + ) + op.drop_index(op.f('ix_api_key_service_id'), table_name='api_key') + op.drop_table('api_key') + ### end Alembic commands ### diff --git a/tests/__init__.py b/tests/__init__.py index e7796406e..cb11a2f6a 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -1,6 +1,6 @@ from client.authentication import create_jwt_token -from app.dao.tokens_dao import get_unsigned_token +from app.dao.api_key_dao import get_unsigned_secret def create_authorization_header(service_id, path, method, request_body=None): @@ -8,14 +8,14 @@ def create_authorization_header(service_id, path, method, request_body=None): token = create_jwt_token( request_method=method, request_path=path, - secret=get_unsigned_token(service_id), + secret=get_unsigned_secret(service_id), client_id=service_id, request_body=request_body) else: token = create_jwt_token(request_method=method, request_path=path, - secret=get_unsigned_token(service_id), + secret=get_unsigned_secret(service_id), client_id=service_id) return 'Authorization', 'Bearer {}'.format(token) diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 3b1c54652..58baf9bfe 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -1,7 +1,7 @@ from client.authentication import create_jwt_token from flask import json, url_for -from app.dao.tokens_dao import get_unsigned_token +from app.dao.api_key_dao import get_unsigned_secret def test_should_not_allow_request_with_no_token(notify_api): @@ -33,13 +33,13 @@ def test_should_not_allow_request_with_incorrect_token(notify_api): assert data['error'] == 'Invalid token: signature' -def test_should_not_allow_incorrect_path(notify_api, notify_db, notify_db_session, sample_token): +def test_should_not_allow_incorrect_path(notify_api, notify_db, notify_db_session, sample_api_key): with notify_api.test_request_context(): with notify_api.test_client() as client: token = create_jwt_token(request_method="GET", request_path="/bad", - secret=get_unsigned_token(sample_token.service_id), - client_id=sample_token.service_id) + secret=get_unsigned_secret(sample_api_key.service_id), + client_id=sample_api_key.service_id) response = client.get(url_for('status.show_status'), headers={'Authorization': "Bearer {}".format(token)}) assert response.status_code == 403 @@ -47,10 +47,10 @@ def test_should_not_allow_incorrect_path(notify_api, notify_db, notify_db_sessio assert data['error'] == 'Invalid token: request' -def test_should_not_allow_incorrect_method(notify_api, notify_db, notify_db_session, sample_token): +def test_should_not_allow_incorrect_method(notify_api, notify_db, notify_db_session, sample_api_key): with notify_api.test_request_context(): with notify_api.test_client() as client: - token = __create_post_token(sample_token.service_id, {}) + token = __create_post_token(sample_api_key.service_id, {}) response = client.get(url_for('status.show_status'), headers={'Authorization': "Bearer {}".format(token)}) assert response.status_code == 403 @@ -58,11 +58,11 @@ def test_should_not_allow_incorrect_method(notify_api, notify_db, notify_db_sess assert data['error'] == 'Invalid token: request' -def test_should_not_allow_invalid_secret(notify_api, notify_db, notify_db_session, sample_token): +def test_should_not_allow_invalid_secret(notify_api, notify_db, notify_db_session, sample_api_key): with notify_api.test_request_context(): with notify_api.test_client() as client: token = create_jwt_token(request_method="POST", request_path="/", secret="not-so-secret", - client_id=sample_token.service_id) + client_id=sample_api_key.service_id) response = client.get(url_for('status.show_status'), headers={'Authorization': "Bearer {}".format(token)}) assert response.status_code == 403 @@ -70,10 +70,10 @@ def test_should_not_allow_invalid_secret(notify_api, notify_db, notify_db_sessio assert data['error'] == 'Invalid token: signature' -def test_should_allow_valid_token(notify_api, notify_db, notify_db_session, sample_token): +def test_should_allow_valid_token(notify_api, notify_db, notify_db_session, sample_api_key): with notify_api.test_request_context(): with notify_api.test_client() as client: - token = __create_get_token(sample_token.service_id) + token = __create_get_token(sample_api_key.service_id) response = client.get(url_for('status.show_status'), headers={'Authorization': 'Bearer {}'.format(token)}) assert response.status_code == 200 @@ -86,20 +86,20 @@ JSON_BODY = json.dumps({ }) -def test_should_allow_valid_token_with_post_body(notify_api, notify_db, notify_db_session, sample_token): +def test_should_allow_valid_token_with_post_body(notify_api, notify_db, notify_db_session, sample_api_key): with notify_api.test_request_context(): with notify_api.test_client() as client: - token = __create_post_token(sample_token.service_id, JSON_BODY) + token = __create_post_token(sample_api_key.service_id, JSON_BODY) response = client.post(url_for('status.show_status'), data=JSON_BODY, headers={'Authorization': 'Bearer {}'.format(token)}) assert response.status_code == 200 -def test_should_not_allow_valid_token_with_invalid_post_body(notify_api, notify_db, notify_db_session, sample_token): +def test_should_not_allow_valid_token_with_invalid_post_body(notify_api, notify_db, notify_db_session, sample_api_key): with notify_api.test_request_context(): with notify_api.test_client() as client: - token = __create_post_token(sample_token.service_id, JSON_BODY) + token = __create_post_token(sample_api_key.service_id, JSON_BODY) response = client.post(url_for('status.show_status'), data="spurious", headers={'Authorization': 'Bearer {}'.format(token)}) @@ -111,7 +111,7 @@ def test_should_not_allow_valid_token_with_invalid_post_body(notify_api, notify_ def __create_get_token(service_id): return create_jwt_token(request_method="GET", request_path=url_for('status.show_status'), - secret=get_unsigned_token(service_id), + secret=get_unsigned_secret(service_id), client_id=service_id) @@ -119,7 +119,7 @@ def __create_post_token(service_id, request_body): return create_jwt_token( request_method="POST", request_path=url_for('status.show_status'), - secret=get_unsigned_token(service_id), + secret=get_unsigned_secret(service_id), client_id=service_id, request_body=request_body ) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index e8854a57a..b8c931343 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -1,9 +1,9 @@ import pytest -from app.models import (User, Service, Template, Token, Job) +from app.models import (User, Service, Template, ApiKey, Job) from app.dao.users_dao import (save_model_user) from app.dao.services_dao import save_model_service from app.dao.templates_dao import save_model_template -from app.dao.tokens_dao import save_model_token +from app.dao.api_key_dao import save_model_api_key from app.dao.jobs_dao import save_job import uuid @@ -51,7 +51,7 @@ def sample_template(notify_db, service=None): if service is None: service = sample_service(notify_db, notify_db_session) - sample_token(notify_db, notify_db_session, service=service) + sample_api_key(notify_db, notify_db_session, service=service) data = { 'name': template_name, 'template_type': template_type, @@ -64,15 +64,15 @@ def sample_template(notify_db, @pytest.fixture(scope='function') -def sample_token(notify_db, - notify_db_session, - service=None): +def sample_api_key(notify_db, + notify_db_session, + service=None): if service is None: service = sample_service(notify_db, notify_db_session) - data = {'service_id': service.id} - token = Token(**data) - save_model_token(token) - return token + data = {'service_id': service.id, 'name': service.name} + api_key = ApiKey(**data) + save_model_api_key(api_key) + return api_key @pytest.fixture(scope='function') @@ -105,7 +105,7 @@ def sample_job(notify_db, def sample_admin_service_id(notify_db, notify_db_session): admin_user = sample_user(notify_db, notify_db_session, email="notify_admin@digital.cabinet-office.gov.uk") admin_service = sample_service(notify_db, notify_db_session, service_name="Sample Admin Service", user=admin_user) - data = {'service_id': admin_service.id} - token = Token(**data) - save_model_token(token) + data = {'service_id': admin_service.id, 'name': 'sample admin key'} + api_key = ApiKey(**data) + save_model_api_key(api_key) return admin_service.id diff --git a/tests/app/dao/test_api_key_dao.py b/tests/app/dao/test_api_key_dao.py new file mode 100644 index 000000000..0f4e374a5 --- /dev/null +++ b/tests/app/dao/test_api_key_dao.py @@ -0,0 +1,69 @@ +from datetime import datetime + +from pytest import fail +from sqlalchemy.orm.exc import NoResultFound + +from app.dao.api_key_dao import (save_model_api_key, + get_model_api_keys, + get_unsigned_secret, + _generate_secret, + _get_secret) +from app.models import ApiKey + + +def test_secret_is_signed_and_can_be_read_again(notify_api): + import uuid + with notify_api.test_request_context(): + token = str(uuid.uuid4()) + signed_secret = _generate_secret(token=token) + assert token == _get_secret(signed_secret) + assert signed_secret != token + + +def test_save_api_key_should_create_new_api_key(notify_api, notify_db, notify_db_session, sample_service): + api_key = ApiKey(**{'service_id': sample_service.id, 'name': sample_service.name}) + save_model_api_key(api_key) + + all_api_keys = get_model_api_keys() + assert len(all_api_keys) == 1 + assert all_api_keys[0] == api_key + + +def test_save_api_key_should_update_the_api_key(notify_api, notify_db, notify_db_session, sample_api_key): + now = datetime.utcnow() + saved_api_key = get_model_api_keys(sample_api_key.service_id) + save_model_api_key(saved_api_key, update_dict={'id': saved_api_key.id, 'expiry_date': now}) + all_api_keys = get_model_api_keys() + assert len(all_api_keys) == 1 + assert all_api_keys[0].expiry_date == now + assert all_api_keys[0].secret == saved_api_key.secret + assert all_api_keys[0].id == saved_api_key.id + assert all_api_keys[0].service_id == saved_api_key.service_id + + +def test_get_api_key_should_raise_exception_when_service_does_not_exist(notify_api, notify_db, notify_db_session, + sample_service): + try: + get_model_api_keys(sample_service.id) + fail("Should have thrown a NoResultFound exception") + except NoResultFound: + pass + + +def test_get_api_key_should_return_none_when_service_does_not_exist(notify_api, notify_db, notify_db_session, + sample_service): + assert get_model_api_keys(service_id=sample_service.id, raise_=False) is None + + +def test_should_return_api_key_for_service(notify_api, notify_db, notify_db_session, sample_api_key): + api_key = get_model_api_keys(sample_api_key.service_id) + assert api_key == sample_api_key + + +def test_should_return_unsigned_api_key_for_service_id(notify_api, + notify_db, + notify_db_session, + sample_api_key): + unsigned_api_key = get_unsigned_secret(sample_api_key.service_id) + assert sample_api_key.secret != unsigned_api_key + assert unsigned_api_key == _get_secret(sample_api_key.secret) diff --git a/tests/app/dao/test_tokens_dao.py b/tests/app/dao/test_tokens_dao.py deleted file mode 100644 index f7747367e..000000000 --- a/tests/app/dao/test_tokens_dao.py +++ /dev/null @@ -1,62 +0,0 @@ -import uuid -from app.dao.tokens_dao import (save_model_token, get_model_tokens, get_unsigned_token, _generate_token, _get_token) -from datetime import datetime -from app.models import Token -from pytest import fail -from sqlalchemy.orm.exc import NoResultFound - - -def test_token_is_signed_and_can_be_read_again(notify_api): - import uuid - with notify_api.test_request_context(): - token = str(uuid.uuid4()) - signed_token = _generate_token(token=token) - assert token == _get_token(signed_token) - assert signed_token != token - - -def test_save_token_should_create_new_token(notify_api, notify_db, notify_db_session, sample_service): - api_token = Token(**{'service_id': sample_service.id}) - save_model_token(api_token) - - all_tokens = get_model_tokens() - assert len(all_tokens) == 1 - assert all_tokens[0] == api_token - - -def test_save_token_should_update_the_token(notify_api, notify_db, notify_db_session, sample_token): - now = datetime.utcnow() - saved_token = get_model_tokens(sample_token.service_id) - save_model_token(saved_token, update_dict={'id': saved_token.id, 'expiry_date': now}) - all_tokens = get_model_tokens() - assert len(all_tokens) == 1 - assert all_tokens[0].expiry_date == now - assert all_tokens[0].token == saved_token.token - assert all_tokens[0].id == saved_token.id - assert all_tokens[0].service_id == saved_token.service_id - - -def test_get_token_should_raise_exception_when_service_does_not_exist(notify_api, notify_db, notify_db_session, - sample_service): - try: - get_model_tokens(sample_service.id) - fail("Should have thrown a NoResultFound exception") - except NoResultFound: - pass - - -def test_get_token_should_return_none_when_service_does_not_exist(notify_api, notify_db, notify_db_session, - sample_service): - assert get_model_tokens(service_id=sample_service.id, raise_=False) is None - - -def test_should_return_token_for_service(notify_api, notify_db, notify_db_session, sample_token): - token = get_model_tokens(sample_token.service_id) - assert token == sample_token - - -def test_should_return_unsigned_token_for_service_id(notify_api, notify_db, notify_db_session, - sample_token): - unsigned_token = get_unsigned_token(sample_token.service_id) - assert sample_token.token != unsigned_token - assert unsigned_token == _get_token(sample_token.token) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index d5f5f7a58..350e03283 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1,7 +1,7 @@ import json from flask import url_for from app.dao.services_dao import save_model_service -from app.models import (Service, Token, Template) +from app.models import (Service, ApiKey, Template) from tests import create_authorization_header from tests.app.conftest import sample_user as create_sample_user @@ -70,7 +70,6 @@ def test_post_service(notify_api, notify_db, notify_db_session, sample_user, sam json_resp = json.loads(resp.get_data(as_text=True)) assert json_resp['data']['name'] == service.name assert json_resp['data']['limit'] == service.limit - assert json_resp['token'] is not None def test_post_service_multiple_users(notify_api, notify_db, notify_db_session, sample_user, sample_admin_service_id): @@ -311,76 +310,84 @@ def test_delete_service_not_exists(notify_api, notify_db, notify_db_session, sam assert Service.query.count() == 2 -def test_renew_token_should_return_token_when_service_does_not_have_a_valid_token(notify_api, notify_db, - notify_db_session, - sample_service, - sample_admin_service_id): +def test_renew_api_key_should_create_new_api_key_for_service(notify_api, notify_db, + notify_db_session, + sample_service, + sample_admin_service_id): with notify_api.test_request_context(): with notify_api.test_client() as client: + data = {'name': 'some secret name'} auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.renew_token', service_id=sample_service.id), - method='POST') - response = client.post(url_for('service.renew_token', service_id=sample_service.id), + path=url_for('service.renew_api_key', + service_id=sample_service.id), + method='POST', + request_body=json.dumps(data)) + response = client.post(url_for('service.renew_api_key', service_id=sample_service.id), + data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 201 assert response.get_data is not None - saved_token = Token.query.filter_by(service_id=sample_service.id).first() - assert saved_token.service_id == sample_service.id + saved_api_key = ApiKey.query.filter_by(service_id=sample_service.id).first() + assert saved_api_key.service_id == sample_service.id + assert saved_api_key.name == 'some secret name' -def test_renew_token_should_expire_the_old_token_and_create_a_new_token(notify_api, notify_db, notify_db_session, - sample_token, sample_admin_service_id): +def test_renew_api_key_should_expire_the_old_api_key_and_create_a_new_api_key(notify_api, notify_db, notify_db_session, + sample_api_key, sample_admin_service_id): with notify_api.test_request_context(): with notify_api.test_client() as client: - assert Token.query.count() == 2 + assert ApiKey.query.count() == 2 + data = {'name': 'some secret name'} auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.renew_token', - service_id=sample_token.service_id), - method='POST') + path=url_for('service.renew_api_key', + service_id=sample_api_key.service_id), + method='POST', + request_body=json.dumps(data)) - response = client.post(url_for('service.renew_token', service_id=sample_token.service_id), + response = client.post(url_for('service.renew_api_key', service_id=sample_api_key.service_id), + data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 201 - assert Token.query.count() == 3 - all_tokens = Token.query.filter_by(service_id=sample_token.service_id).all() - for x in all_tokens: - if x.id == sample_token.id: + assert ApiKey.query.count() == 3 + all_api_keys = ApiKey.query.filter_by(service_id=sample_api_key.service_id).all() + for x in all_api_keys: + if x.id == sample_api_key.id: assert x.expiry_date is not None else: assert x.expiry_date is None - assert x.token is not sample_token.token + assert x.secret is not sample_api_key.secret -def test_create_token_should_return_error_when_service_does_not_exist(notify_api, notify_db, notify_db_session, - sample_service, sample_admin_service_id): +def test_renew_api_key_should_return_error_when_service_does_not_exist(notify_api, notify_db, notify_db_session, + sample_service, sample_admin_service_id): with notify_api.test_request_context(): with notify_api.test_client() as client: auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.renew_token', service_id="123"), + path=url_for('service.renew_api_key', service_id="123"), method='POST') - response = client.post(url_for('service.renew_token', service_id=123), + response = client.post(url_for('service.renew_api_key', service_id=123), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 404 -def test_revoke_token_should_expire_token_for_service(notify_api, notify_db, notify_db_session, - sample_token, sample_admin_service_id): +def test_revoke_api_key_should_expire_api_key_for_service(notify_api, notify_db, notify_db_session, + sample_api_key, sample_admin_service_id): with notify_api.test_request_context(): with notify_api.test_client() as client: - assert Token.query.count() == 2 + assert ApiKey.query.count() == 2 auth_header = create_authorization_header(service_id=sample_admin_service_id, - path=url_for('service.revoke_token', - service_id=sample_token.service_id), + path=url_for('service.revoke_api_key', + service_id=sample_api_key.service_id), method='POST') - response = client.post(url_for('service.revoke_token', service_id=sample_token.service_id), + response = client.post(url_for('service.revoke_api_key', service_id=sample_api_key.service_id), headers=[auth_header]) assert response.status_code == 202 - tokens_for_service = Token.query.filter_by(service_id=sample_token.service_id).first() - assert tokens_for_service.expiry_date is not None + api_keys_for_service = ApiKey.query.filter_by(service_id=sample_api_key.service_id).first() + assert api_keys_for_service.expiry_date is not None -def test_create_service_should_create_new_token_for_service(notify_api, notify_db, notify_db_session, sample_user, - sample_admin_service_id): +def test_create_service_should_create_new_service_for_user(notify_api, notify_db, notify_db_session, sample_user, + sample_admin_service_id): with notify_api.test_request_context(): with notify_api.test_client() as client: data = { @@ -394,12 +401,10 @@ def test_create_service_should_create_new_token_for_service(notify_api, notify_d method='POST', request_body=json.dumps(data)) headers = [('Content-Type', 'application/json'), auth_header] - assert Token.query.count() == 1 resp = client.post(url_for('service.create_service'), data=json.dumps(data), headers=headers) assert resp.status_code == 201 - assert Token.query.count() == 2 def test_create_template(notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id): From 56b33496ffdaf75c71bf3e5c97a78d636cdf7bbe Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 19 Jan 2016 12:12:46 +0000 Subject: [PATCH 4/6] Added name to api key when creating key for admin user. NOTE: this will change in the next PR --- application.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/application.py b/application.py index 3a050fe50..a9395c573 100644 --- a/application.py +++ b/application.py @@ -41,7 +41,7 @@ def create_admin_user_service(): 'active': True, 'restricted': True}) services_dao.save_model_service(service) - api_key = ApiKey(**{'service_id': service.id}) + api_key = ApiKey(**{'service_id': service.id, 'name': 'Admin API KEY (temporary)'}) api_key_dao.save_model_api_key(api_key) print('ApiKey: {}'.format(api_key_dao.get_unsigned_secret(service.id))) From 43d54b6ad3d094364af3b9f99ce8726dc02e7395 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 19 Jan 2016 12:17:01 +0000 Subject: [PATCH 5/6] Update the down revision to the version 0006 file. --- migrations/versions/0007_change_to_api_keys.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/migrations/versions/0007_change_to_api_keys.py b/migrations/versions/0007_change_to_api_keys.py index 27718976f..d63c111d6 100644 --- a/migrations/versions/0007_change_to_api_keys.py +++ b/migrations/versions/0007_change_to_api_keys.py @@ -8,7 +8,7 @@ Create Date: 2016-01-19 10:50:46.269618 # revision identifiers, used by Alembic. revision = '0007_change_to_api_keys' -down_revision = '0005_add_job_details' +down_revision = '0006_add_user_details' from alembic import op import sqlalchemy as sa From 15b2d414ccd1eb4f171d9f8aa46ae0aacec40a04 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 19 Jan 2016 13:11:22 +0000 Subject: [PATCH 6/6] Updates from review comments: Update api_key relationship. Check that id in dict exists before deleting it --- app/dao/api_key_dao.py | 3 ++- app/models.py | 2 +- app/schemas.py | 4 ++-- app/service/rest.py | 4 ++-- tests/app/conftest.py | 2 +- 5 files changed, 8 insertions(+), 7 deletions(-) diff --git a/app/dao/api_key_dao.py b/app/dao/api_key_dao.py index 3f1951f67..3a1912a70 100644 --- a/app/dao/api_key_dao.py +++ b/app/dao/api_key_dao.py @@ -7,7 +7,8 @@ from app.models import ApiKey def save_model_api_key(api_key, update_dict={}): if update_dict: - del update_dict['id'] + if update_dict['id']: + del update_dict['id'] db.session.query(ApiKey).filter_by(id=api_key.id).update(update_dict) else: api_key.secret = _generate_secret() diff --git a/app/models.py b/app/models.py index 4fb0ee9d7..4721cdb62 100644 --- a/app/models.py +++ b/app/models.py @@ -92,7 +92,7 @@ class ApiKey(db.Model): name = db.Column(db.String(255), nullable=False) secret = db.Column(db.String(255), unique=True, nullable=False) service_id = db.Column(db.Integer, db.ForeignKey('services.id'), index=True, nullable=False) - service = db.relationship('Service', backref=db.backref('api_key', lazy='dynamic')) + service = db.relationship('Service', backref=db.backref('api_keys', lazy='dynamic')) expiry_date = db.Column(db.DateTime) diff --git a/app/schemas.py b/app/schemas.py index edc87f4a3..ec98a69f1 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -18,7 +18,7 @@ class UserSchema(ma.ModelSchema): class ServiceSchema(ma.ModelSchema): class Meta: model = models.Service - exclude = ("updated_at", "created_at", "api_key", "templates", "jobs") + exclude = ("updated_at", "created_at", "api_keys", "templates", "jobs") class TemplateSchema(ma.ModelSchema): @@ -30,7 +30,7 @@ class TemplateSchema(ma.ModelSchema): class ApiKeySchema(ma.ModelSchema): class Meta: model = models.ApiKey - exclude = ["service"] + exclude = ("service", "secret", "expiry_date") class JobSchema(ma.ModelSchema): diff --git a/app/service/rest.py b/app/service/rest.py index 1f9c77f99..aa722a83c 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -78,7 +78,7 @@ def get_service(service_id=None): @service.route('//api-key/renew', methods=['POST']) def renew_api_key(service_id=None): try: - get_model_services(service_id=service_id) + service = get_model_services(service_id=service_id) except DataError: return jsonify(result="error", message="Invalid service id"), 400 except NoResultFound: @@ -92,7 +92,7 @@ def renew_api_key(service_id=None): # create a new one # TODO: what validation should be done here? secret_name = request.get_json()['name'] - save_model_api_key(ApiKey(service_id=service_id, name=secret_name)) + save_model_api_key(ApiKey(service=service, name=secret_name)) except DAOException as e: return jsonify(result='error', message=str(e)), 400 unsigned_api_key = get_unsigned_secret(service_id) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index b8c931343..ced56de67 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -105,7 +105,7 @@ def sample_job(notify_db, def sample_admin_service_id(notify_db, notify_db_session): admin_user = sample_user(notify_db, notify_db_session, email="notify_admin@digital.cabinet-office.gov.uk") admin_service = sample_service(notify_db, notify_db_session, service_name="Sample Admin Service", user=admin_user) - data = {'service_id': admin_service.id, 'name': 'sample admin key'} + data = {'service': admin_service, 'name': 'sample admin key'} api_key = ApiKey(**data) save_model_api_key(api_key) return admin_service.id