-
Notifications
You must be signed in to change notification settings - Fork 32
Anonymous credentials draft endpoints #1016
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (90.92%) is below the target coverage (95.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #1016 +/- ##
==========================================
- Coverage 92.81% 90.92% -1.90%
==========================================
Files 17 28 +11
Lines 1281 2932 +1651
Branches 65 247 +182
==========================================
+ Hits 1189 2666 +1477
- Misses 78 206 +128
- Partials 14 60 +46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hellais
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments on small improvements and some questions.
We can discuss on slack anything which is not clear.
| measurement = ujson.loads(msm_jstr) | ||
| if sorted(measurement.keys()) == ["content", "format"]: | ||
|
|
||
| is_verified = g(measurement, 'is_verified', False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if at some point we renamed this function g as something a bit more understandable. It's basically getting the key and if not it's setting to to False. Correct?
I believe modern versions of python have native support for this.
I guess we can take note of it as a future refactoring work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just do that I think, the fastpath now runs in python 3.12 in AWS, this can be replaced with measurement.get('is_verified', False)
We can open another issue for that
fastpath/fastpath/db.py
Outdated
| def tf(v: bool) -> str: | ||
| return "t" if v else "f" | ||
|
|
||
| def s_or_n(x : Optional[Any]) -> Optional[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call this function a bit more explicitly, instead of these shorthands?
Even better would be if we just use an explicit lambda function inside of the age_range field, eg.
age_range=lambda x: ujson.dumps(x) if x is not None else None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow the lambda part. Where would you call the lambda? is there a place where you can use it as argument to automatically parse the thing?
But in the meanwhile I renamed s_or_n to serialize_optional to have a more descriptive name
| compressed_len = len(data) | ||
| data = zstd.decompress(data) | ||
| ratio = len(data) / len(data) | ||
| ratio = compressed_len / len(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just leave this inlined? I think there is no need to have an extra line and variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
| log.debug(f"Zstd compression ratio {ratio}") | ||
| except Exception as e: | ||
| except Exception: | ||
| log.info("Failed zstd decompression") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we log e inside of log.info. Make sure you use {e} inside of log.info(f"") which should perform sanitisation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, also did the same thing with other exceptions
| # time window | ||
| now = datetime.now(timezone.utc) | ||
| h = sha512(data_bin).hexdigest()[:16] | ||
| ts = now.strftime("%Y%m%d%H%M%S.%f") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use ISO standard? Make sure it has in it T and Z to indicate timezone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if that's a good idea because ts is used to compute the measurement id:
| msmt_uid = f"{ts}_{cc}_{test_name}_{h}" |
So we would be changing the measurement id format. I'm not too confident that this change won't break anything else. What do you think?
| f"[Try {t+1}/{N_RETRIES}] Error trying to send measurement to the fastpath ({settings.fastpath_url}). Error: {exc}" | ||
| ) | ||
| sleep_time = random.uniform(0, min(3, 0.3 * 2 ** t)) | ||
| await asyncio.sleep(sleep_time) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? cc @mmaker
| assert resp.status_code == status.HTTP_200_OK, f"Unexpected status code: {resp.status_code} - {url}. {resp.content}" | ||
| return resp.json() | ||
|
|
||
| def post(client, url, data, headers=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move these into some kind of utils.py module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, also refactored other tests to use the utils.py module
| content_encoding: str = Header(default=None), | ||
| ) -> SubmitMeasurementResponse | Dict[str, Any]: | ||
| """ | ||
| Submit measurement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write a better docstring?
Also, it would be good to know where we plug in the values for "valid" probe_age_ranges and probe_msm_range (can you rename these variables to that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed the variables and also added extra documentation to explain how these variables are used, but I'm not sure if this is what was expected here
This PR adds support for the anonymous credentials protocol to the Ooni API:
ooniauth-pyas a dependency/sign_credential,/manifest,/submitNote that the new measurement
/submitendpoint won't replace the old one, and for now it's meant to be used mostly during developmentAlso updates the fastpath so that it's able to work with the new fields
Migrations
This PR will require some database migrations
Postgres
Adds the following tables to postgres:
ooniprobe_manifest: Describes the manifest that is reported to users when they registerooniprobe_server_state: Defines the key pair (secret_key, public_parameters) that is used for authenticationYou can find the new models here:
backend/ooniapi/services/ooniprobe/src/ooniprobe/models.py
Line 58 in 3b3bfb2
And the migrations:
https://github.com/ooni/backend/blob/userauth-dep/ooniapi/common/src/common/alembic/versions/7e28b5d17a7f_add_server_state_table_for_anonymous_.py
Clickhouse
In clickhouse we need to add the fields necessary for:
As an example of the changes, you can look at the
clickhouse_init.sqlscript in fastpath:backend/fastpath/clickhouse_init.sql
Lines 42 to 46 in 3b3bfb2
These are the alter table statements required to run the migration in production:
Feedback
This is still early work to define the API for the anonymous credentials protocol. Some things that could benefit from a bit of feedback are:
/sign_credentialused to be named/registerbut it clashes with the older/registerone, so I chose a different namecloses #1014 #1015