Add additional options for service auth #25
Conversation
There was a problem hiding this comment.
In general, baseplate.go v0 is not really getting new features, which are preferentially being built into the baseplate.go v2 ecosystem.
Do you know what services need this? Are they using baseplate.go v0? Can they adopt v2?
Either way, I think an implementation in v2 will be required, regardless of whether one is integrated into v0.
lib/go/edgecontext/service.go
Outdated
| if s.isService() { | ||
| token := AuthenticationToken(s) | ||
| if token.OnBehalfOf == nil { | ||
| return |
There was a problem hiding this comment.
the original go code was written long time ago that we have slightly different style guidance now, we prefer not to use naked returns and always return explicitly:
| return | |
| return "", false |
same for the rest
There was a problem hiding this comment.
makes sense! i'll update the new code to follow that style
|
For context we did add this implementation in v2, this is mainly backporting this so folks using the python libraries will have access to these new fields |
| self.assertEqual(token.subject, "service/test-service") | ||
| self.assertEqual(token.on_behalf_of_roles, ["admin"]) | ||
| self.assertEqual(token.on_behalf_of_id, "t2_deadbeef") | ||
| self.assertEqual(token.requests_elevated_access, True) |
There was a problem hiding this comment.
🔕 since these tests run with pytest, I would probably recommend using regular assertion statements (e.g. assert token.subject == "service/test_service"). pytest will do its magic assertion rewriting to produce a better error output than the TestCase assert mehtods.
Not necessary to change though, I know you're following the pre-existing pattern in this file.
There was a problem hiding this comment.
makes sense - i can at least update the new changes to follow the new style
There was a problem hiding this comment.
take it back - looks like the linters were complaining so going to keep it as is - if we want to update the linters in drone, will defer that into a separate change
This reverts commit 57c2f33.
💸 TL;DR
Adding additional options for service auth
🧪 Testing Steps / Validation
✅ Checks