diff --git a/api/api_specs.py b/api/api_specs.py index f780a626a55e..69d20164e51d 100644 --- a/api/api_specs.py +++ b/api/api_specs.py @@ -33,6 +33,7 @@ ('enterprise_impact', 'int'), ('shipping_year', 'int'), ('breaking_change', 'bool'), + ('confidential', 'bool'), ('bug_url', 'link'), ('category', 'int'), ('cc_emails', 'emails'), diff --git a/api/converters.py b/api/converters.py index 57cb03d1435b..0b547581628c 100644 --- a/api/converters.py +++ b/api/converters.py @@ -383,6 +383,7 @@ def feature_entry_to_json_verbose( 'first_enterprise_notification_milestone': fe.first_enterprise_notification_milestone, 'enterprise_impact': fe.enterprise_impact, 'breaking_change': fe.breaking_change, + 'confidential': fe.confidential, 'shipping_year': fe.shipping_year, 'flag_name': fe.flag_name, 'finch_name': fe.finch_name, @@ -536,12 +537,16 @@ def feature_entry_to_json_basic(fe: FeatureEntry, 'enterprise_impact': fe.enterprise_impact, 'enterprise_product_category': fe.enterprise_product_category or ENTERPRISE_PRODUCT_CATEGORY_CHROME_BROWSER_UPDATE, 'breaking_change': fe.breaking_change, + 'confidential': fe.confidential, 'first_enterprise_notification_milestone': fe.first_enterprise_notification_milestone, 'blink_components': fe.blink_components or [], 'resources': { 'samples': fe.sample_links or [], 'docs': fe.doc_links or [], }, + 'creator': fe.creator_email, + 'editors': fe.editor_emails or [], + 'owners': fe.owner_emails or [], 'created': {'by': fe.creator_email, 'when': _date_to_str(fe.created)}, 'updated': {'by': fe.updater_email, 'when': _date_to_str(fe.updated)}, 'accurate_as_of': _date_to_str(fe.accurate_as_of), diff --git a/api/converters_test.py b/api/converters_test.py index b02a2e77fa07..1c255adeb7e5 100644 --- a/api/converters_test.py +++ b/api/converters_test.py @@ -114,6 +114,10 @@ def test_feature_entry_to_json_basic__normal(self): 'samples': ['https://example.com/samples'], 'docs': ['https://example.com/docs'], }, + 'confidential': False, + 'creator': 'creator@example.com', + 'editors': ['feature_editor@example.com', 'owner_1@example.com'], + 'owners': ['feature_owner@example.com'], 'created': { 'by': 'creator@example.com', 'when': expected_date @@ -201,6 +205,10 @@ def test_feature_entry_to_json_basic__feature_release(self): 'samples': ['https://example.com/samples'], 'docs': ['https://example.com/docs'], }, + 'confidential': False, + 'creator': 'creator@example.com', + 'editors': ['feature_editor@example.com', 'owner_1@example.com'], + 'owners': ['feature_owner@example.com'], 'created': { 'by': 'creator@example.com', 'when': expected_date @@ -398,6 +406,7 @@ def test_feature_entry_to_json_verbose__normal(self): 'editors': ['feature_editor@example.com', 'owner_1@example.com'], 'creator': 'creator@example.com', 'comments': 'notes', + 'confidential': False, 'browsers': { 'chrome': { 'bug': 'https://example.com/bug', diff --git a/api/features_api.py b/api/features_api.py index bca42add8b3b..1535b9fc1581 100644 --- a/api/features_api.py +++ b/api/features_api.py @@ -59,6 +59,9 @@ def get_one_feature(self, feature_id: int) -> VerboseFeatureDict: user = users.get_current_user() if feature.deleted and not permissions.can_edit_feature(user, feature_id): self.abort(404, msg='Feature %r not found' % feature_id) + if not permissions.can_view_feature(user, feature): + self.abort(404, msg='Feature %r not found' % feature_id) + return converters.feature_entry_to_json_verbose(feature) def do_search(self): diff --git a/api/features_api_test.py b/api/features_api_test.py index b8525ea19645..a07e4345513e 100644 --- a/api/features_api_test.py +++ b/api/features_api_test.py @@ -141,6 +141,19 @@ def setUp(self): stage_type=360, milestones=MilestoneSet(desktop_first=2)) self.ship_stage_3.put() + self.feature_confidential = FeatureEntry( + name='feature confidential', summary='sum A', feature_type=2, + owner_emails=['confidential_owner@example.com'], category=1, + editor_emails=['confidential_editor@example.com'], + creator_email='confidential_creator@examplecom', + intent_stage=core_enums.INTENT_IMPLEMENT, + unlisted=True, confidential=True) + self.feature_confidential.put() + self.feature_confidential_id = self.feature_3.key.integer_id() + self.ship_stage_3 = Stage(feature_id=self.feature_3_id, + stage_type=360, milestones=MilestoneSet(desktop_first=1)) + self.ship_stage_3.put() + self.request_path = '/api/v0/features' self.handler = features_api.FeaturesAPI() @@ -180,9 +193,9 @@ def test_get__all_listed_feature_names(self): # as it only returns feature names. self.assertEqual(2, len(actual['features'])) self.assertEqual(2, actual['total_count']) - self.assertEqual(2, len(actual['features'][0])) + self.assertEqual(3, len(actual['features'][0])) self.assertEqual('feature two', actual['features'][0]['name']) - self.assertEqual(2, len(actual['features'][1])) + self.assertEqual(3, len(actual['features'][1])) self.assertEqual('feature one', actual['features'][1]['name']) def test_get__all_listed__pagination(self): @@ -320,11 +333,12 @@ def test_get__all_unlisted_can_edit(self): testing_config.sign_in('admin@example.com', 123567890) with test_app.test_request_context(self.request_path): actual = self.handler.do_get() - self.assertEqual(3, len(actual['features'])) - self.assertEqual(3, actual['total_count']) - self.assertEqual('feature three', actual['features'][0]['name']) - self.assertEqual('feature two', actual['features'][1]['name']) - self.assertEqual('feature one', actual['features'][2]['name']) + self.assertEqual(4, len(actual['features'])) + self.assertEqual(4, actual['total_count']) + self.assertEqual('feature confidential', actual['features'][0]['name']) + self.assertEqual('feature three', actual['features'][1]['name']) + self.assertEqual('feature two', actual['features'][2]['name']) + self.assertEqual('feature one', actual['features'][3]['name']) def test_get__user_query_no_sort__signed_out(self): """Get all features with a specified owner, unlisted not shown.""" @@ -361,8 +375,8 @@ def test_get__user_query_with_sort__with_perms(self): url = self.request_path + '?sort=-summary' with test_app.test_request_context(url): actual = self.handler.do_get() - self.assertEqual(3, len(actual['features'])) - self.assertEqual(3, actual['total_count']) + self.assertEqual(4, len(actual['features'])) + self.assertEqual(4, actual['total_count']) self.assertEqual('sum Z', actual['features'][0]['summary']) self.assertEqual('sum K', actual['features'][1]['summary']) self.assertEqual('sum A', actual['features'][2]['summary']) diff --git a/api/legacy_converters.py b/api/legacy_converters.py index 9cca93d907a3..bca33d920f25 100644 --- a/api/legacy_converters.py +++ b/api/legacy_converters.py @@ -32,6 +32,7 @@ def feature_to_legacy_json(f: Feature) -> dict[str, Any]: d['category'] = FEATURE_CATEGORIES[f.category] d['enterprise_feature_categories'] = f.enterprise_feature_categories d['enterprise_product_category'] = f.enterprise_product_category + d['confidential'] = f.confidential d['category_int'] = f.category if f.feature_type is not None: d['feature_type'] = FEATURE_TYPES[f.feature_type] diff --git a/client-src/elements/form-definition.ts b/client-src/elements/form-definition.ts index 96829972005a..28c33ce14588 100644 --- a/client-src/elements/form-definition.ts +++ b/client-src/elements/form-definition.ts @@ -182,6 +182,7 @@ export const ENTERPRISE_NEW_FEATURE_FORM_FIELDS = [ 'enterprise_product_category', 'first_enterprise_notification_milestone', 'enterprise_impact', + 'confidential', ]; // The fields that are available to every feature. @@ -237,6 +238,7 @@ export const FLAT_ENTERPRISE_METADATA_FIELDS: MetadataFields = { 'enterprise_feature_categories', 'enterprise_product_category', 'enterprise_impact', + 'confidential', 'first_enterprise_notification_milestone', 'screenshot_links', ], diff --git a/client-src/elements/form-field-specs.ts b/client-src/elements/form-field-specs.ts index 1184d2a7fdc9..1ac3797068cd 100644 --- a/client-src/elements/form-field-specs.ts +++ b/client-src/elements/form-field-specs.ts @@ -2135,6 +2135,15 @@ export const ALL_FIELDS: Record = { take action to continue using some existing functionaity.`, }, + confidential: { + type: 'checkbox', + label: 'Confidential', + initial: false, + help_text: html`This is a confidential feature that should only be visible + to admins, chromium contributors and the feature's owners, contributors and + creator.`, + }, + intent_cc_emails: { type: 'input', attrs: MULTI_EMAIL_FIELD_ATTRS, diff --git a/framework/permissions.py b/framework/permissions.py index 8d07688b64b9..285b7f48d756 100644 --- a/framework/permissions.py +++ b/framework/permissions.py @@ -44,12 +44,61 @@ def is_google_or_chromium_account(user: User) -> bool: return user.email().endswith(('@chromium.org', '@google.com')) return False -def can_view_feature(unused_user, unused_feature) -> bool: +def can_view_feature_formatted(user: User, feature: dict) -> bool: """Return True if the user is allowed to view the given feature.""" - # Note, for now there are no private features, only unlisted ones. - return True + if not feature: + return False + + return not feature['confidential'] or _can_view_confidential_feature_formatted(user, feature) + +def _can_view_confidential_feature_formatted(user: User, feature: dict): + """ Check if the user is an owner, editor, spec mentor, or creator + for this feature or has a google.com or chromium.org account. + If yes, they feature can be viewed, otherwise they cannot view + confidential features.""" + if not user: + return False + + if is_google_or_chromium_account(user) or can_admin_site(user): + return True + email = user.email() + if ( + email in feature['owners'] or + email in feature['editors'] or + email == feature['creator']): + return True + return False + + +def can_view_feature(user: User, feature: FeatureEntry) -> bool: + """Return True if the user is allowed to view the given feature.""" + if not feature: + return False + + return not feature.confidential or _can_view_confidential_feature(user, feature) + +def _can_view_confidential_feature(user: User, feature: FeatureEntry): + """ Check if the user is an owner, editor, spec mentor, or creator + for this feature or has a google.com or chromium.org account. + If yes, they feature can be viewed, otherwise they cannot view + confidential features.""" + if not user or not feature: + return False + + if is_google_or_chromium_account(user) or can_admin_site(user): + return True + + email = user.email() + if ( + email in feature.owner_emails or + email in feature.editor_emails or + email == feature.creator_email): + return True + + return False + def can_create_feature(user: User) -> bool: """Return True if the user is allowed to create features.""" if not user: diff --git a/framework/permissions_test.py b/framework/permissions_test.py index dcc9426f03e0..e0cc2318ee17 100644 --- a/framework/permissions_test.py +++ b/framework/permissions_test.py @@ -94,10 +94,22 @@ def setUp(self): self.feature_1.put() self.feature_id = self.feature_1.key.integer_id() + # Feature for checking permissions against + self.feature_confidential = core_models.FeatureEntry( + name='feature one', summary='sum', + creator_email="feature_creator@example.com", + owner_emails=['feature_owner@example.com'], + editor_emails=['feature_editor@example.com'], + spec_mentor_emails=['mentor@example.com'], category=1, + confidential=True) + self.feature_confidential.put() + self.feature_confidential_id = self.feature_confidential.key.integer_id() + def tearDown(self): for user in self.users: user.delete() self.feature_1.key.delete() + self.feature_confidential.key.delete() def check_function_results( self, func, additional_args, @@ -194,16 +206,23 @@ def test_can_admin_site(self): def test_can_view_feature(self): self.check_function_results( permissions.can_view_feature, (None,), - unregistered=True, registered=True, - special=True, site_editor=True, admin=True, anon=True) + unregistered=False, registered=False, + special=False, site_editor=False, admin=False, anon=False) self.check_function_results_with_feature( - permissions.can_view_feature, (self.feature_id,), + permissions.can_view_feature, (self.feature_1,), unregistered=True, registered=True, feature_owner=True, feature_editor=True, creator=True, site_editor=True, admin=True, spec_mentor=True, ) + self.check_function_results_with_feature( + permissions.can_view_feature, (self.feature_confidential,), + unregistered=False, registered=False, + feature_owner=True, feature_editor=True, + creator=True, site_editor=False, admin=True, spec_mentor=False, + ) + def test_can_create_feature(self): self.check_function_results( permissions.can_create_feature, tuple(), @@ -233,13 +252,13 @@ def test_can_edit_feature(self): def test_can_review_gate(self): approvers = [] self.check_function_results( - permissions.can_review_gate, (None, None, approvers), + permissions.can_review_gate, (self.feature_1, None, approvers), unregistered=False, registered=False, special=False, site_editor=False, admin=True, anon=False) approvers = ['registered@example.com'] self.check_function_results( - permissions.can_review_gate, (None, None, approvers), + permissions.can_review_gate, (self.feature_1, None, approvers), unregistered=False, registered=True, special=False, site_editor=False, admin=True, anon=False) diff --git a/internals/core_models.py b/internals/core_models.py index 48eb72fda77f..2df9bd1e88ea 100644 --- a/internals/core_models.py +++ b/internals/core_models.py @@ -105,6 +105,7 @@ class FeatureEntry(ndb.Model): # Copy from Feature first_enterprise_notification_milestone = ndb.IntegerProperty() enterprise_impact = ndb.IntegerProperty(default=ENTERPRISE_IMPACT_NONE) breaking_change = ndb.BooleanProperty(default=False) + confidential = ndb.BooleanProperty(default=False) shipping_year = ndb.IntegerProperty() # Implementation in Chrome diff --git a/internals/data_types.py b/internals/data_types.py index c4c24f9833af..78ca55962dd1 100644 --- a/internals/data_types.py +++ b/internals/data_types.py @@ -217,6 +217,7 @@ class VerboseFeatureDict(TypedDict): screenshot_links: list[str] first_enterprise_notification_milestone: int | None breaking_change: bool + confidential: bool enterprise_impact: int shipping_year: int | None diff --git a/internals/feature_helpers.py b/internals/feature_helpers.py index 3840f08a93e6..47baa237eff0 100644 --- a/internals/feature_helpers.py +++ b/internals/feature_helpers.py @@ -21,6 +21,7 @@ from api import converters from framework import rediscache from framework import users +from framework import permissions from internals import stage_helpers from internals.core_enums import * from internals.core_models import FeatureEntry, Stage @@ -45,6 +46,17 @@ def filter_unlisted(feature_list: list[dict]) -> list[dict]: return listed_features +def filter_confidential(feature_list: list[dict]) -> list[dict]: + """Filters a feature list to display only features the user should see.""" + user = users.get_current_user() + visible_features = [] + for f in feature_list: + if permissions.can_view_feature_formatted(user, f): + visible_features.append(f) + + return visible_features + + def get_entries_by_id_async(ids) -> Future | None: if ids: q = FeatureEntry.query(FeatureEntry.key.IN( @@ -90,9 +102,10 @@ def get_features_in_release_notes(milestone: int): f['feature_type_int'] == FEATURE_TYPE_ENTERPRISE_ID) and (f['first_enterprise_notification_milestone'] == None or f['first_enterprise_notification_milestone'] <= milestone)] + features = [f for f in features] rediscache.set(cache_key, features) - return features + return filter_confidential(features) def get_in_milestone(milestone: int, @@ -279,6 +292,8 @@ def get_in_milestone(milestone: int, if not show_unlisted: features_by_type[shipping_type] = filter_unlisted( features_by_type[shipping_type]) + features_by_type[shipping_type] = filter_confidential( + features_by_type[shipping_type]) return features_by_type @@ -361,6 +376,7 @@ def get_feature_names_by_ids(feature_ids: list[int], feature_name_dict = { 'id': feature_id, 'name': fe.name, + 'confidential': fe.confidential } result_dict[feature_id] = feature_name_dict @@ -376,7 +392,7 @@ def get_feature_names_by_ids(feature_ids: list[int], result_list = [ result_dict[feature_id] for feature_id in feature_ids if feature_id in result_dict] - return result_list + return filter_confidential(result_list) def get_by_ids(feature_ids: list[int], update_cache: bool=True) -> list[dict[str, Any]]: @@ -436,7 +452,7 @@ def get_by_ids(feature_ids: list[int], result_list = [ result_dict[feature_id] for feature_id in feature_ids if feature_id in result_dict] - return result_list + return filter_confidential(result_list) def get_features_by_impl_status(limit: int | None=None, update_cache: bool=False, @@ -487,4 +503,4 @@ def get_features_by_impl_status(limit: int | None=None, update_cache: bool=False rediscache.set(cache_key, feature_list) - return feature_list + return filter_confidential(feature_list) diff --git a/internals/feature_helpers_test.py b/internals/feature_helpers_test.py index 34328e371f78..60cfac3d24e0 100644 --- a/internals/feature_helpers_test.py +++ b/internals/feature_helpers_test.py @@ -192,7 +192,8 @@ def test_get_by_ids__cache_hit(self): cached_feature = { 'name': 'fake cached_feature', 'id': self.feature_1.key.integer_id(), - 'unlisted': False + 'unlisted': False, + 'confidential': False } rediscache.set(cache_key, cached_feature) @@ -255,9 +256,9 @@ def test_get_feature_names_by_ids__cache_miss(self): self.feature_2.key.integer_id()]) self.assertEqual(2, len(actual)) - self.assertEqual(2, len(actual[0])) + self.assertEqual(3, len(actual[0])) self.assertEqual('feature a', actual[0]['name']) - self.assertEqual(2, len(actual[1])) + self.assertEqual(3, len(actual[1])) self.assertEqual('feature b', actual[1]['name']) lookup_key_1 = '%s|%s' % (FeatureEntry.FEATURE_NAME_CACHE_KEY, @@ -273,14 +274,15 @@ def test_get_feature_names_by_ids__cache_hit(self): FeatureEntry.FEATURE_NAME_CACHE_KEY, self.feature_1.key.integer_id()) cached_feature = { 'name': 'fake cached_feature', - 'id': self.feature_1.key.integer_id() + 'id': self.feature_1.key.integer_id(), + 'confidential': False } rediscache.set(cache_key, cached_feature) actual = feature_helpers.get_feature_names_by_ids([self.feature_1.key.integer_id()]) self.assertEqual(1, len(actual)) - self.assertEqual(2, len(actual[0])) + self.assertEqual(3, len(actual[0])) self.assertEqual(cached_feature, actual[0]) def test_get_feature_names_by_ids__batch_order(self): @@ -293,7 +295,7 @@ def test_get_feature_names_by_ids__batch_order(self): ]) self.assertEqual(4, len(actual)) - self.assertEqual(2, len(actual[0])) + self.assertEqual(3, len(actual[0])) self.assertEqual('feature d', actual[0]['name']) self.assertEqual('feature a', actual[1]['name']) self.assertEqual('feature c', actual[2]['name']) @@ -321,7 +323,7 @@ def test_get_feature_names_by_ids__cached_correctly(self): ]) self.assertEqual(4, len(actual)) - self.assertEqual(2, len(actual[0])) + self.assertEqual(3, len(actual[0])) self.assertEqual('feature d', actual[0]['name']) self.assertEqual('feature a', actual[1]['name']) self.assertEqual('feature c', actual[2]['name']) @@ -459,7 +461,7 @@ def test_get_in_milestone__cached(self): """If there is something in the cache, we use it.""" cache_key = '%s|%s|%s' % ( FeatureEntry.DEFAULT_CACHE_KEY, 'milestone', 1) - cached_test_feature = {'test': [{'name': 'test_feature', 'unlisted': False}]} + cached_test_feature = {'test': [{'name': 'test_feature', 'unlisted': False, 'confidential': False}]} rediscache.set(cache_key, cached_test_feature) actual = feature_helpers.get_in_milestone(milestone=1) diff --git a/internals/search_queries.py b/internals/search_queries.py index d45d00f2ed2c..f86d7a70523d 100644 --- a/internals/search_queries.py +++ b/internals/search_queries.py @@ -273,6 +273,7 @@ def sorted_by_review_date(descending: bool) -> Future: 'browsers.chrome.bug': FeatureEntry.bug_url, 'launch_bug_url': FeatureEntry.launch_bug_url, 'breaking_change': FeatureEntry.breaking_change, + 'confidential': FeatureEntry.confidential, 'enterprise_impact': FeatureEntry.enterprise_impact, 'shipping_year': FeatureEntry.shipping_year, diff --git a/scripts/seed_datastore.py b/scripts/seed_datastore.py index d97877f11acd..f2190ba2cd83 100755 --- a/scripts/seed_datastore.py +++ b/scripts/seed_datastore.py @@ -70,6 +70,7 @@ def add_features(server: str, after: datetime, detailsAfter: datetime): fe.summary = f['summary'] fe.category = MISC fe.breaking_change = f['breaking_change'] + fe.confidential = f['confidential'] fe.first_enterprise_notification_milestone = f[ 'first_enterprise_notification_milestone'] fe.blink_components = f['blink_components']