feat: obfuscated precomputed assignments #164

sameerank · 2024-12-09T09:11:05Z

labels: mergeable

Fixes: #issue

Motivation and Context

Description

Updates de-obfuscation code to use a salt in hashing the flag key for finding precomputed flags. We expect this to work for both the regular init-ed client and offline init, since both will be obfuscated.

How has this been tested?

…o-getobfuscatedprecomputedflag

src/decoding.ts

Makefile

src/client/eppo-precomputed-client.ts

src/precomputed-requestor.ts

src/obfuscation.ts

leoromanovsky

Will take a thorough read tomorrow 👍

src/client/eppo-precomputed-client.spec.ts

… mainly be used by clients

src/index.ts

src/client/eppo-precomputed-client.ts

src/client/eppo-precomputed-client.spec.ts

src/client/eppo-precomputed-client.ts

rasendubi · 2024-12-17T17:32:32Z

src/client/eppo-precomputed-client.ts

+  // Convenience method that combines setters we need to make assignments work
+  public setSubjectAndPrecomputedFlagsRequestParameters(
+    parameters: PrecomputedFlagsRequestParameters,
+  ) {
+    this.setPrecomputedFlagsRequestParameters(parameters);
+    this.setSubjectData(parameters.precompute.subjectKey, parameters.precompute.subjectAttributes);


This should not be public.

It also seems to be doing a bad thing — allows reinterpreting precomputed configuration from another subject. It also does not reset the poller, so it would continue polling with the previous subject key/attributes

made not public.

What is an alternative approach to setting these instance variables in the client if this method is no longer public?

https://github.com/Eppo-exp/js-client-sdk/blob/ee3409f5ce9af803687769b56052cc468977c353/src/index.ts#L602

Re: the concern of a mismatch arising between a previously running poller and newly provided request parameters + subject data: I'm thinking that adding a this.requestPoller?.stop(); method at the top of this method would prevent that from happening

Re: the concern of a mismatch arising between a previously running poller and newly provided request parameters + subject data: I'm thinking that adding a this.requestPoller?.stop(); method at the top of this method would prevent that from happening

It will not, because stop() does not interrupt an in-progress request, so what could happen is:

Client is initialized with one subject.

Poller sends a request.

Client is re-initialized with another subject.

Previous request completes, and assigns data from previous subject to the store.

What is an alternative approach to setting these instance variables in the client if this method is no longer public?

Constructor. We shall prefer creating a new instance of the client in initialization instead of resetting the existing one. Resetting is problematic as it makes it impossible to use multiple clients simultaneously, and some data may still linger and intermix as in poller example above. (e.g., this is backfiring in our e2e testing effort, and afaik it is problematic for evaluation reasons on eppo portal)

Alternatively, JS/TS don't have much protection against calling typescript's private methods. If we really need to call a method, it's easy to disable typescript protection (although not recommended).

rasendubi · 2024-12-17T17:33:29Z

src/client/eppo-precomputed-client.ts

+  }
+
+  // Convenience method that combines setters we need to make assignments work
+  public setSubjectSaltAndPrecomputedFlagStore(


Shall not be public

If setSubjectAndPrecomputedFlagStore is to be made private and setPrecomputedFlagStore and setSubjectData are also private -- can you provide a suggestion for how the client should set and persist this information in the offlinePrecomputedInit method in js-client-sdk?

https://github.com/Eppo-exp/js-client-sdk/blob/ee3409f5ce9af803687769b56052cc468977c353/src/index.ts#L669-L675

Would it be preferable to delete setSubjectAndPrecomputedFlagStore completely and make both setPrecomputedFlagStore and setSubjectData public? In my mind, I thought of these private individual setters as implementation details and the public setter as a convenient grouping of everything you need to get offline init to work

I would rather have less public methods as all public methods are exposed to our users and become part of our API.

src/precomputed-requestor.ts

src/client/eppo-precomputed-client.ts

rasendubi · 2024-12-17T17:47:16Z

src/client/eppo-precomputed-client.ts

+  precomputedFlagStore: IConfigurationStore<PrecomputedFlag>;
+  isObfuscated?: boolean;


I would also expect EppoPrecomputedClient to receive sdk key, subject key, attributes, assignment logger, optional initial configuration, etc in constructor

I've added the request params object to the init function as I don't want to work the PrecomputedFlagsRequestParameters and EppoPrecomputedClientOptions types in this PR. We will have a chance to iterate on the API before releasing to the general public and have a need to punt some of this work in the very short term.

The request parameters could be included in EppoPrecomputedClientOptions. This would be consistent with how the regular client's constructor uses configurationRequestParameters

js-sdk-common/src/client/eppo-client.ts

Lines 129 to 138 in f036edc

configurationRequestParameters?: FlagConfigurationRequestParameters;

isObfuscated?: boolean;

}) {

this.eventDispatcher = eventDispatcher;

this.flagConfigurationStore = flagConfigurationStore;

this.banditVariationConfigurationStore = banditVariationConfigurationStore;

this.banditModelConfigurationStore = banditModelConfigurationStore;

this.configurationRequestParameters = configurationRequestParameters;

this.isObfuscated = isObfuscated;

}

I opted against this pattern because the js-client-sdk doesn't actually use this field in the constructor and explicitly calls a instance.setConfigurationRequestParameters(requestConfiguration);, which is the same approach I'd taken with using setSubjectAndPrecomputedFlagsRequestParameters. If you don't intend to use an argument in a constructor, why even have the argument in the first place?

…o-getobfuscatedprecomputedflag

leoromanovsky

approved but version bump needed before merging

leoromanovsky · 2024-12-18T08:06:12Z

src/client/eppo-precomputed-client.ts

-  public setIsObfuscated(isObfuscated: boolean) {
-    this.isObfuscated = isObfuscated;


before we added the ability to parse whether a configuration is obfuscated from the configuration, when users needed to bootstrap the SDK in offline mode they need to be able to set this. the canonical example is the JS browser client (which by default is in obfuscated mode) getting a configuration from a nodejs server (which is not obfuscated).

that being said, I know there are customers using this method. removing this public method will require a major version bump please.

overall this is a more elegant solution to receive the obfuscation status from the configuration - users will need to upgrade both their nodejs and js browser SDKs to make sure that metadata is included.

that being said, I know there are customers using this method. removing this public method will require a major version bump please.

This is a method on a PrecomputedEppoClient, not the regular one

@rasendubi What is the purpose of providing this obfuscated boolean field in the response (https://github.com/Eppo-exp/eppo-multiplatform/blob/5aadb2ed0572a96fbb999066c0191e949abd20f5/eppo_core/src/precomputed.rs#L83) if we're designing the SDKs to always assume that is it true?

if we're designing the SDKs to always assume that is it true?

That may change in the future. The flag is here to facilitate that change

leoromanovsky · 2024-12-18T08:07:40Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "@eppo/js-client-sdk-common",
-  "version": "4.7.0",
+  "version": "4.7.0-alpha.1",


Suggested change

"version": "4.7.0-alpha.1",

"version": "5.0.0-alpha.1",

removing some public methods that are used by customers (see below)

See my other comment — we are not removing used methods. All changes are for the new precomputed client, not the regular one

src/attributes.ts

src/client/eppo-precomputed-client.ts

src/index.ts

test/testHelpers.ts

…o-getobfuscatedprecomputedflag

Co-authored-by: Oleksii Shmalko <[email protected]>

sameerank · 2025-01-08T21:40:24Z

src/client/eppo-precomputed-client.ts

+      if (!this.precomputedFlagStore.isInitialized()) {
+        logger.error(
+          '[Eppo SDK] EppoPrecomputedClient requires an initialized precomputedFlagStore if requestParameters are not provided',
+        );
+      }
+      if (!this.precomputedFlagStore.salt) {
+        logger.error(
+          '[Eppo SDK] EppoPrecomputedClient requires a precomputedFlagStore with a salt if requestParameters are not provided',
+        );
+      }


Printing instead of throwing errors if the precomputed flag store is misconfigured because the SDK shouldn't disrupt the user's code, but also these scenarios should never happen, and if they do, assignments will always return the user-provided default

sameerank added 10 commits December 9, 2024 01:06

make getMD5HashWithSalt

a1cf3bf

add tests

22538e1

Merge branch 'main' into sameeran/ff-3682-use-salt-in-flag-key-hash-t…

7cd544c

…o-getobfuscatedprecomputedflag

Merge branch 'main' into sameeran/ff-3682-use-salt-in-flag-key-hash-t…

1ff6d8b

…o-getobfuscatedprecomputedflag

Export saltedHasher

b8a2d9d

Fix tests

567e66d

Alter test to use obfuscated file

c8f5171

Merge branch 'main' into sameeran/ff-3682-use-salt-in-flag-key-hash-t…

c0288fe

…o-getobfuscatedprecomputedflag

Change branch name for test data

ed3f0b3

Get all the tests to pass

47333ec

sameerank commented Dec 15, 2024

View reviewed changes

src/decoding.ts Show resolved Hide resolved

sameerank commented Dec 15, 2024

View reviewed changes

Makefile Show resolved Hide resolved

Make more obvious that the salt was decoded

a3ed52a

sameerank commented Dec 15, 2024

View reviewed changes

src/client/eppo-precomputed-client.ts Outdated Show resolved Hide resolved

sameerank commented Dec 15, 2024

View reviewed changes

src/precomputed-requestor.ts Outdated Show resolved Hide resolved

sameerank marked this pull request as ready for review December 15, 2024 21:56

sameerank requested review from typotter and rasendubi December 15, 2024 21:58

Switch to using appendBinary for the salt

25c6f8c

sameerank commented Dec 16, 2024

View reviewed changes

src/obfuscation.ts Outdated Show resolved Hide resolved

sameerank added 3 commits December 15, 2024 20:04

Clean up

a169e29

Include salt in convenience method for setting precomputed flag store

d366925

Add a helper to convert context attributes to subject attributes

8fe80dc

sameerank requested review from leoromanovsky and aarsilv December 16, 2024 05:28

leoromanovsky reviewed Dec 16, 2024

View reviewed changes

src/client/eppo-precomputed-client.spec.ts Show resolved Hide resolved

Change default to isObfuscated since we expect the precomputed api to…

b1048a3

… mainly be used by clients

sameerank commented Dec 16, 2024

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

sameerank commented Dec 16, 2024

View reviewed changes

src/client/eppo-precomputed-client.ts Outdated Show resolved Hide resolved

v4.7.1-alpha.0

b81175f

sameerank added 2 commits December 16, 2024 10:54

Make response data not optional

ed8027d

precomputedFlag variable casing

4e8aa63

sameerank assigned typotter Dec 16, 2024

typotter and others added 2 commits December 17, 2024 00:29

update hashing

6040e96

fix lint

dd6b1e9

rasendubi reviewed Dec 17, 2024

View reviewed changes

handoff and address comments

b178a6c

typotter changed the title ~~feat: use salt in flag key hash~~ feat: obfuscated precomputed assignments Dec 18, 2024

typotter requested a review from leoromanovsky December 18, 2024 07:28

typotter assigned leoromanovsky Dec 18, 2024

typotter added 2 commits December 18, 2024 00:32

Merge branch 'main' into sameeran/ff-3682-use-salt-in-flag-key-hash-t…

749e8c1

…o-getobfuscatedprecomputedflag

bump version

e99306d

leoromanovsky approved these changes Dec 18, 2024

View reviewed changes

rasendubi reviewed Dec 18, 2024

View reviewed changes

sameerank added 2 commits January 6, 2025 11:05

Merge branch 'main' into sameeran/ff-3682-use-salt-in-flag-key-hash-t…

f036edc

…o-getobfuscatedprecomputedflag

Inf is a numeric attribute too

173b3a3

sameerank mentioned this pull request Jan 7, 2025

feat: create offlinePrecomputedInit Eppo-exp/js-client-sdk#117

Merged

sameerank and others added 9 commits January 8, 2025 01:52

Remove unnecessary public methods

d43e9be

Remove more unnecessary functions

bba9863

Add to exported interfaces

0d1f341

Update src/interfaces.ts

bb03341

Co-authored-by: Oleksii Shmalko <[email protected]>

Update src/attributes.ts attributes is ContextAttributes

ad86727

Co-authored-by: Oleksii Shmalko <[email protected]>

Remove redundant 'subjectAttributes as ContextAttributes'

b15f488

Also print error if store is missing salt

3a322a7

Remove buildContextAttributes

928f58d

v4.8.0-alpha.0

5241bf7

sameerank commented Jan 8, 2025

View reviewed changes

sameerank merged commit 65986fe into main Jan 8, 2025
8 checks passed

sameerank deleted the sameeran/ff-3682-use-salt-in-flag-key-hash-to-getobfuscatedprecomputedflag branch January 8, 2025 21:45

sameerank mentioned this pull request Jan 16, 2025

fix: getprecomputedinstance should always return an instance Eppo-exp/js-client-sdk#151

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: obfuscated precomputed assignments #164

feat: obfuscated precomputed assignments #164

sameerank commented Dec 9, 2024 •

edited

Loading

leoromanovsky left a comment

rasendubi Dec 17, 2024

typotter Dec 18, 2024

sameerank Jan 6, 2025

rasendubi Jan 7, 2025

rasendubi Dec 17, 2024

typotter Dec 18, 2024

sameerank Jan 6, 2025

sameerank Jan 6, 2025

rasendubi Jan 7, 2025

rasendubi Dec 17, 2024

typotter Dec 18, 2024

sameerank Jan 6, 2025

leoromanovsky left a comment

leoromanovsky Dec 18, 2024

rasendubi Dec 18, 2024

sameerank Jan 6, 2025

rasendubi Jan 7, 2025

leoromanovsky Dec 18, 2024

rasendubi Jan 7, 2025

sameerank Jan 8, 2025 •

edited

Loading

		precomputedFlagStore: IConfigurationStore<PrecomputedFlag>;
		isObfuscated?: boolean;

	configurationRequestParameters?: FlagConfigurationRequestParameters;
	isObfuscated?: boolean;
	}) {
	this.eventDispatcher = eventDispatcher;
	this.flagConfigurationStore = flagConfigurationStore;
	this.banditVariationConfigurationStore = banditVariationConfigurationStore;
	this.banditModelConfigurationStore = banditModelConfigurationStore;
	this.configurationRequestParameters = configurationRequestParameters;
	this.isObfuscated = isObfuscated;
	}

		public setIsObfuscated(isObfuscated: boolean) {
		this.isObfuscated = isObfuscated;

feat: obfuscated precomputed assignments #164

feat: obfuscated precomputed assignments #164

Conversation

sameerank commented Dec 9, 2024 • edited Loading

labels: mergeable

Motivation and Context

Description

How has this been tested?

leoromanovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoromanovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameerank Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

sameerank commented Dec 9, 2024 •

edited

Loading

sameerank Jan 8, 2025 •

edited

Loading