diff --git a/contrib/tempest/tempest/api/share/test_rules.py b/contrib/tempest/tempest/api/share/test_rules.py index 35ff8d168e..deb012e455 100644 --- a/contrib/tempest/tempest/api/share/test_rules.py +++ b/contrib/tempest/tempest/api/share/test_rules.py @@ -45,6 +45,7 @@ class ShareIpRulesForNFSTest(base.BaseSharesTest): access_type, access_to) self.assertIn(int(resp["status"]), test.HTTP_SUCCESS) + self.assertEqual('rw', rule['access_level']) self.shares_client.wait_for_access_rule_status(self.share["id"], rule["id"], "active") @@ -65,6 +66,7 @@ class ShareIpRulesForNFSTest(base.BaseSharesTest): access_type, access_to) self.assertIn(int(resp["status"]), test.HTTP_SUCCESS) + self.assertEqual('rw', rule['access_level']) self.shares_client.wait_for_access_rule_status(self.share["id"], rule["id"], "active") @@ -73,6 +75,21 @@ class ShareIpRulesForNFSTest(base.BaseSharesTest): rule["id"]) self.assertIn(int(resp["status"]), test.HTTP_SUCCESS) + @test.attr(type=["gate", ]) + def test_create_delete_ro_access_rule(self): + resp, rule = self.shares_client.create_access_rule(self.share["id"], + 'ip', + '2.2.2.2', + 'ro') + self.assertIn(int(resp["status"]), test.HTTP_SUCCESS) + self.assertEqual('ro', rule['access_level']) + self.shares_client.wait_for_access_rule_status(self.share["id"], + rule["id"], + "active") + resp, _ = self.shares_client.delete_access_rule(self.share["id"], + rule["id"]) + self.assertIn(int(resp["status"]), test.HTTP_SUCCESS) + class ShareIpRulesForCIFSTest(ShareIpRulesForNFSTest): protocol = "cifs" @@ -90,19 +107,17 @@ class ShareUserRulesForNFSTest(base.BaseSharesTest): msg = "USER rule tests for %s protocol are disabled" % cls.protocol raise cls.skipException(msg) __, cls.share = cls.create_share(cls.protocol) + cls.access_type = "user" + cls.access_to = CONF.share.username_for_user_rules @test.attr(type=["gate", ]) def test_create_delete_user_rule(self): - - # test data - access_type = "user" - access_to = CONF.share.username_for_user_rules - # create rule resp, rule = self.shares_client.create_access_rule(self.share["id"], - access_type, - access_to) + self.access_type, + self.access_to) self.assertIn(int(resp["status"]), test.HTTP_SUCCESS) + self.assertEqual('rw', rule['access_level']) self.shares_client.wait_for_access_rule_status(self.share["id"], rule["id"], "active") @@ -111,6 +126,21 @@ class ShareUserRulesForNFSTest(base.BaseSharesTest): rule["id"]) self.assertIn(int(resp["status"]), test.HTTP_SUCCESS) + @test.attr(type=["gate", ]) + def test_create_delete_ro_access_rule(self): + resp, rule = self.shares_client.create_access_rule(self.share["id"], + self.access_type, + self.access_to, + 'ro') + self.assertIn(int(resp["status"]), test.HTTP_SUCCESS) + self.assertEqual('ro', rule['access_level']) + self.shares_client.wait_for_access_rule_status(self.share["id"], + rule["id"], + "active") + resp, _ = self.shares_client.delete_access_rule(self.share["id"], + rule["id"]) + self.assertIn(int(resp["status"]), test.HTTP_SUCCESS) + class ShareUserRulesForCIFSTest(ShareUserRulesForNFSTest): protocol = "cifs" @@ -167,13 +197,14 @@ class ShareRulesTest(base.BaseSharesTest): self.assertEqual(200, int(resp["status"]), msg) # verify keys - keys = ["state", "id", "access_type", "access_to"] + keys = ["state", "id", "access_type", "access_to", "access_level"] [self.assertIn(key, r.keys()) for r in rules for key in keys] # verify values self.assertEqual("active", rules[0]["state"]) self.assertEqual(self.access_type, rules[0]["access_type"]) self.assertEqual(self.access_to, rules[0]["access_to"]) + self.assertEqual('rw', rules[0]["access_level"]) # our share id in list and have no duplicates gen = [r["id"] for r in rules if r["id"] in rule["id"]] diff --git a/contrib/tempest/tempest/api/share/test_rules_negative.py b/contrib/tempest/tempest/api/share/test_rules_negative.py index 1158bbc88f..f4e3323713 100644 --- a/contrib/tempest/tempest/api/share/test_rules_negative.py +++ b/contrib/tempest/tempest/api/share/test_rules_negative.py @@ -84,6 +84,15 @@ class ShareIpRulesForNFSNegativeTest(base.BaseSharesTest): self.shares_client.create_access_rule, self.share["id"], "ip", "1.2.3.1/") + @test.attr(type=["negative", "gate", ]) + def test_create_access_rule_with_wrong_level(self): + self.assertRaises(exceptions.BadRequest, + self.shares_client.create_access_rule, + self.share["id"], + 'ip', + '2.2.2.2', + 'su') + @test.attr(type=["negative", "gate", ]) def test_create_duplicate_of_ip_rule(self): # test data @@ -172,6 +181,15 @@ class ShareUserRulesForNFSNegativeTest(base.BaseSharesTest): access_type="user", access_to="fakeuser") + @test.attr(type=["negative", "gate", ]) + def test_create_access_rule_with_wrong_level(self): + self.assertRaises(exceptions.BadRequest, + self.shares_client.create_access_rule, + self.share["id"], + 'user', + CONF.share.username_for_user_rules, + 'su') + class ShareUserRulesForCIFSNegativeTest(ShareUserRulesForNFSNegativeTest): protocol = "cifs" diff --git a/contrib/tempest/tempest/services/share/json/shares_client.py b/contrib/tempest/tempest/services/share/json/shares_client.py index eeab49ccd5..5ccef1c1e0 100644 --- a/contrib/tempest/tempest/services/share/json/shares_client.py +++ b/contrib/tempest/tempest/services/share/json/shares_client.py @@ -97,12 +97,13 @@ class SharesClient(rest_client.RestClient): resp, body = self.get("shares/%s" % share_id) return resp, self._parse_resp(body) - def create_access_rule(self, share_id, - access_type="ip", access_to="0.0.0.0"): + def create_access_rule(self, share_id, access_type="ip", + access_to="0.0.0.0", access_level=None): post_body = { "os-allow_access": { "access_type": access_type, "access_to": access_to, + "access_level": access_level, } } body = json.dumps(post_body) diff --git a/manila/api/contrib/share_actions.py b/manila/api/contrib/share_actions.py index 16fe986d4e..d5f16180da 100644 --- a/manila/api/contrib/share_actions.py +++ b/manila/api/contrib/share_actions.py @@ -121,11 +121,11 @@ class ShareActionsController(wsgi.Controller): def _allow_access(self, req, id, body): """Add share access rule.""" context = req.environ['manila.context'] - + access_data = body['os-allow_access'] share = self.share_api.get(context, id) - access_type = body['os-allow_access']['access_type'] - access_to = body['os-allow_access']['access_to'] + access_type = access_data['access_type'] + access_to = access_data['access_to'] if access_type == 'ip': self._validate_ip_range(access_to) elif access_type == 'user': @@ -138,7 +138,8 @@ class ShareActionsController(wsgi.Controller): raise webob.exc.HTTPBadRequest(explanation=exc_str) try: access = self.share_api.allow_access( - context, share, access_type, access_to) + context, share, access_type, access_to, + access_data.get('access_level')) except exception.ShareAccessExists as e: raise webob.exc.HTTPBadRequest(explanation=e.msg) return {'access': access} diff --git a/manila/common/constants.py b/manila/common/constants.py index 0b67610881..2f3a530454 100644 --- a/manila/common/constants.py +++ b/manila/common/constants.py @@ -51,3 +51,11 @@ PING_PORTS = ( SERVICE_INSTANCE_SECGROUP_DATA = ( CIFS_PORTS + NFS_PORTS + SSH_PORTS + PING_PORTS) + +ACCESS_LEVEL_RW = 'rw' +ACCESS_LEVEL_RO = 'ro' + +ACCESS_LEVELS = ( + ACCESS_LEVEL_RW, + ACCESS_LEVEL_RO, +) diff --git a/manila/db/migrations/alembic/versions/211836bf835c_add_access_level.py b/manila/db/migrations/alembic/versions/211836bf835c_add_access_level.py new file mode 100644 index 0000000000..d5015d68dd --- /dev/null +++ b/manila/db/migrations/alembic/versions/211836bf835c_add_access_level.py @@ -0,0 +1,41 @@ +# Copyright 2015 Mirantis Inc. +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +"""add access level + +Revision ID: 211836bf835c +Revises: 162a3e673105 +Create Date: 2014-12-19 05:34:06.790159 + +""" + +# revision identifiers, used by Alembic. +revision = '211836bf835c' +down_revision = '162a3e673105' + +from alembic import op +import sqlalchemy as sa + +from manila.common import constants + + +def upgrade(): + op.add_column('share_access_map', + sa.Column('access_level', sa.String(2), + default=constants.ACCESS_LEVEL_RW)) + + +def downgrade(): + op.drop_column('share_access_map', 'access_level') diff --git a/manila/db/sqlalchemy/models.py b/manila/db/sqlalchemy/models.py index 5f5485b81a..fed670f0d2 100644 --- a/manila/db/sqlalchemy/models.py +++ b/manila/db/sqlalchemy/models.py @@ -264,6 +264,8 @@ class ShareAccessMapping(BASE, ManilaBase): state = Column(Enum(STATE_NEW, STATE_ACTIVE, STATE_DELETING, STATE_DELETED, STATE_ERROR), default=STATE_NEW) + access_level = Column(Enum(*constants.ACCESS_LEVELS), + default=constants.ACCESS_LEVEL_RW) class ShareSnapshot(BASE, ManilaBase): diff --git a/manila/share/api.py b/manila/share/api.py index c205d18a7f..a83b68d45c 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -24,6 +24,7 @@ from oslo.utils import timeutils import six from manila.api import extensions +from manila.common import constants from manila.db import base from manila import exception from manila.i18n import _ @@ -450,7 +451,8 @@ class API(base.Base): snapshots = results return snapshots - def allow_access(self, ctx, share, access_type, access_to): + def allow_access(self, ctx, share, access_type, access_to, + access_level=None): """Allow access to share.""" if not share['host']: msg = _("Share host is None") @@ -459,14 +461,20 @@ class API(base.Base): msg = _("Share status must be available") raise exception.InvalidShare(reason=msg) policy.check_policy(ctx, 'share', 'allow_access') - values = {'share_id': share['id'], - 'access_type': access_type, - 'access_to': access_to} + values = { + 'share_id': share['id'], + 'access_type': access_type, + 'access_to': access_to, + 'access_level': access_level, + } access = [a for a in self.db.share_access_get_all_by_type_and_access( ctx, share['id'], access_type, access_to) if a['state'] != 'error'] if access: raise exception.ShareAccessExists(access_type=access_type, access=access_to) + if access_level not in constants.ACCESS_LEVELS + (None, ): + msg = _("Invalid share access level: %s.") % access_level + raise exception.InvalidShareAccess(reason=msg) access = self.db.share_access_create(ctx, values) self.share_rpcapi.allow_access(ctx, share, access) return access @@ -501,6 +509,7 @@ class API(base.Base): return [{'id': rule.id, 'access_type': rule.access_type, 'access_to': rule.access_to, + 'access_level': rule.access_level, 'state': rule.state} for rule in rules] def access_get(self, context, access_id): diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index 990ca57112..2c57a52a7c 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -17,6 +17,7 @@ import datetime import uuid +import ddt import mock from oslo.config import cfg from oslo.utils import timeutils @@ -89,6 +90,7 @@ def fake_access(id, **kwargs): 'share_id': 'fakeshareid', 'access_type': 'fakeacctype', 'access_to': 'fakeaccto', + 'access_level': 'rw', 'state': 'fakeactive', 'STATE_NEW': 'fakenew', 'STATE_ACTIVE': 'fakeactive', @@ -156,6 +158,7 @@ _FAKE_LIST_OF_ALL_SNAPSHOTS = [ ] +@ddt.ddt class ShareAPITestCase(test.TestCase): def setUp(self): @@ -921,23 +924,33 @@ class ShareAPITestCase(test.TestCase): share_api.policy.check_policy.assert_called_once_with( ctx, 'share', 'get_all_snapshots') - def test_allow_access(self): + @ddt.data(None, 'rw', 'ro') + def test_allow_access(self, level): share = fake_share('fakeid', status='available') values = { 'share_id': share['id'], 'access_type': 'fakeacctype', 'access_to': 'fakeaccto', + 'access_level': level, } - with mock.patch.object(db_driver, 'share_access_create', - mock.Mock(return_value='fakeacc')): - self.share_rpcapi.allow_access(self.context, share, 'fakeacc') - access = self.api.allow_access(self.context, share, 'fakeacctype', - 'fakeaccto') - self.assertEqual(access, 'fakeacc') - db_driver.share_access_create.assert_called_once_with( - self.context, values) - share_api.policy.check_policy.assert_called_once_with( - self.context, 'share', 'allow_access') + self.stubs.Set(db_driver, 'share_access_create', + mock.Mock(return_value='fakeacc')) + access = self.api.allow_access(self.context, share, + 'fakeacctype', 'fakeaccto', + level) + self.assertEqual(access, 'fakeacc') + self.share_rpcapi.allow_access.assert_called_once_with( + self.context, share, 'fakeacc') + db_driver.share_access_create.assert_called_once_with( + self.context, values) + share_api.policy.check_policy.assert_called_once_with( + self.context, 'share', 'allow_access') + + def test_allow_access_invalid_access_level(self): + share = fake_share('fakeid', status='available') + self.assertRaises(exception.InvalidShareAccess, self.api.allow_access, + self.context, share, 'fakeacctype', 'fakeaccto', + 'ab') def test_allow_access_status_not_available(self): share = fake_share('fakeid', status='error') @@ -1014,12 +1027,14 @@ class ShareAPITestCase(test.TestCase): 'id': 'fakeacc0id', 'access_type': 'fakeacctype', 'access_to': 'fakeaccto', + 'access_level': 'rw', 'state': 'fakenew', }, { 'id': 'fakeacc1id', 'access_type': 'fakeacctype', 'access_to': 'fakeaccto', + 'access_level': 'rw', 'state': 'fakeerror', }, ]