-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat: Implement rudimentary tokenserver route in syncstorage-rs #871
Conversation
25e95aa
to
61908d0
Compare
…storage-rs codebase. Fix #864 Requires an existing tokenserver mysql db.
19bfd6c
to
72cf6c3
Compare
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 looks ok, but still seems a bit WIP.
I'd feel better with less println!
and hard coded values, particularly if we're going to land this in master.
There's obviously a tension between stuff being wip and landing on master. As the new tokenserver route is not used yet, I feel it is prudent to land rougher code since it breaks the larger task up into smaller chunks, which we have been having a difficult time doing. |
@@ -128,7 +128,7 @@ macro_rules! build_app { | |||
) | |||
// Tokenserver | |||
.service( | |||
web::resource(&cfg_path("/1.0/sync/1.5")).route(web::get().to(tokenserver::get)), | |||
web::resource("/1.0/sync/1.5".to_string()).route(web::get().to(tokenserver::get)), |
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.
Now that this route does something other than nothing, we should probably disable this during development somehow on the live service. A setting or maybe something as simple as detecting a debug build (but note the docker-compose tests in ci run against release mode)
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.
The docker-compose tests don't hit this tokenserver yet, so that should be fine.
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.
You have to go far out of your way to configure your firefox to use this new tokenserver route and it's not documented anywhere, so does this really buy anything? [edit] I'm not sure if ops has a whitelist of urls on the nginx frontend. If they do, presumably nobody would even be able to hit this route on production?
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'll admit I'm being pedantic but I think it makes sense at some point to disable the route when it's not needed anyway (thinking about it more, presence of TOKENSERVER_DATABASE_URL
would be a good toggle).
Definitely a long shot anyone hits this endpoint but I was more concerned about it being untested/load tested, particularly w/ this handler potentially loading the Python runtime. So I'd rather just disable it sooner rather than later so there's one less thing on prod to worry about while it's still under development.
let email = format!("{:}@api.accounts.firefox.com", token_data.claims.sub); | ||
|
||
// TODO pull out of settings instead | ||
let shared_secret = env::var("SYNC_MASTER_SECRET").expect("SYNC_MASTER_SECRET must be set"); |
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.
nit: Tempted to move these up a bit in a future release. They'd be the trigger that @pjenvey is looking for and would save the cycles of building the token_data
above.
Description
Add the hacky initial tokenserver route. Uses an existing tokenserver mysql database.
Testing
First, you need python3 and to run
pip3 install tokenlib
.Need a tokenserver mysql database:
Run syncstorage-rs.
In firefox, create a new profile. In about:config, set identity.sync.tokenserver.uri to http://localhost:8000/1.0/sync/1.5
Log in to sync with your normal prod fxa credentials. It should be able to put your stuff in your localhost syncstorage-rs.
Issue(s)
Fix #864