[BACK-2709] rename a function and a struct for clarity #671
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ewollesen
force-pushed
the
eric/rename-details
branch
2 times, most recently
from
ae5cb31 to
07f0734
Compare
My motivation with this change is to prevent a potential authentication error wherein someone accidentally breaks assumptions about the authentication process due to a misunderstanding about the purpose of the Details struct. So let's get more concrete. The Authenticate function in data/service/api/v1/authenticate_middleware.go was being used to check that authentication was previously performed. The Authenticate function itself did no actual authentication. Intead, it checked for the existence of a Details value in the request's Context. If a Details value was found, then it was assumed that the request was previously authenticated. All of that is OK, as long as the Details object is only added to a request Context after successful authentication. The problem I'm trying to avoid with this renaming, is some future me (or you!) coming along with some piece of data that needs to be passed to a request handler, and I see this handy Details value that gets passed via the Context, and I think "I'll just add it into this Details thing. Easy-peasey. All the while not realizing that the mere existence of the Details value indicates to request handlers that the request has been authenticated. This renaming can't prevent the scenario above from happening, but it should make it obvious that these values, now called AuthDetails, are specifically for authentication information. Hopefully that's enough of a warning to scare future me away from touching them for non-authentication purposes.
ewollesen
force-pushed
the
eric/rename-details
branch
from
07f0734 to
14c054b
Compare
toddkazakov
approved these changes
Oct 18, 2023
ewollesen
changed the title
ewollesen
added a commit
that referenced
this pull request
Oct 18, 2023
When I started working on the alerts configs, I had a discussion with Todd about routing, and how the data service for some unknown reason uses its own authentication method, rather than the various api.Require* methods provided by platform and used in the other services. In the process of working on #671 it became clear to me that the reason was the difference in the function signatures of request handlers in the data service as opposed to those in other services. Specifically, data service request handlers have this signature: func Foo(service.Context) Whereas rest.HandlerFunc has this signature: func Foo(rest.ResponseWriter, *rest.Request) So I modified the data service's MakeRoute function to accept a list of rest.MiddlewareSimple, and added a ToRestRoute method that when provided with the existing adapter function in the data api service, would convert the data service routes to *rest.Route, while also applying middleware. The result is that data service routes can be specified including rest.MiddlewareSimple (like api.Require), and we no longer require a separate Authentication helper for data service routes. There are some TODO comments that indicate that going further down the data service handler rabbit hole might not have been the intended path, but rather moving to a rest.HandlerFunc signature was at one time the plan (in 2018 or so). If so, this should only make it clearer what needs to change to effect that migration.
|
Closed in favor of #672 |
ewollesen
added a commit
that referenced
this pull request
Oct 23, 2023
…Func (#672) * rename a function and a struct for clarity My motivation with this change is to prevent a potential authentication error wherein someone accidentally breaks assumptions about the authentication process due to a misunderstanding about the purpose of the Details struct. So let's get more concrete. The Authenticate function in data/service/api/v1/authenticate_middleware.go was being used to check that authentication was previously performed. The Authenticate function itself did no actual authentication. Intead, it checked for the existence of a Details value in the request's Context. If a Details value was found, then it was assumed that the request was previously authenticated. All of that is OK, as long as the Details object is only added to a request Context after successful authentication. The problem I'm trying to avoid with this renaming, is some future me (or you!) coming along with some piece of data that needs to be passed to a request handler, and I see this handy Details value that gets passed via the Context, and I think "I'll just add it into this Details thing. Easy-peasey. All the while not realizing that the mere existence of the Details value indicates to request handlers that the request has been authenticated. This renaming can't prevent the scenario above from happening, but it should make it obvious that these values, now called AuthDetails, are specifically for authentication information. Hopefully that's enough of a warning to scare future me away from touching them for non-authentication purposes. * adapt the data service's request handlers to rest.HandlerFunc When I started working on the alerts configs, I had a discussion with Todd about routing, and how the data service for some unknown reason uses its own authentication method, rather than the various api.Require* methods provided by platform and used in the other services. In the process of working on #671 it became clear to me that the reason was the difference in the function signatures of request handlers in the data service as opposed to those in other services. Specifically, data service request handlers have this signature: func Foo(service.Context) Whereas rest.HandlerFunc has this signature: func Foo(rest.ResponseWriter, *rest.Request) So I modified the data service's MakeRoute function to accept a list of rest.MiddlewareSimple, and added a ToRestRoute method that when provided with the existing adapter function in the data api service, would convert the data service routes to *rest.Route, while also applying middleware. The result is that data service routes can be specified including rest.MiddlewareSimple (like api.Require), and we no longer require a separate Authentication helper for data service routes. There are some TODO comments that indicate that going further down the data service handler rabbit hole might not have been the intended path, but rather moving to a rest.HandlerFunc signature was at one time the plan (in 2018 or so). If so, this should only make it clearer what needs to change to effect that migration.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The main content of this PR is here: https://github.com/tidepool-org/platform/pull/671/files#diff-cc0a547148df99e305aed886f026d59dfe3eeaf26c4c82b41ee445420b5f506e
My motivation with this change is to prevent a potential authentication error wherein someone accidentally breaks assumptions about the authentication process due to a misunderstanding about the purpose of the Details struct.
So let's get more concrete. The Authenticate function in data/service/api/v1/authenticate_middleware.go was being used to check that authentication was previously performed. The Authenticate function itself did no actual authentication. Intead, it checked for the existence of a Details value in the request's Context. If a Details value was found, then it was assumed that the request was previously authenticated. All of that is OK, as long as the Details object is only added to a request Context after successful authentication.
The problem I'm trying to avoid with this renaming, is some future me (or you!) coming along with some piece of data that needs to be passed to a request handler, and I see this handy Details value that gets passed via the Context, and I think "I'll just add it into this Details thing. Easy-peasey. All the while not realizing that the mere existence of the Details value indicates to request handlers that the request has been authenticated.
This renaming can't prevent the scenario above from happening, but it should make it obvious that these values, now called AuthDetails, are specifically for authentication information. Hopefully that's enough of a warning to scare future me away from touching them for non-authentication purposes.
Update:
I recognize that this is a lot of modified files for what is at the end of the day a personal feeling about a particular name and how it's used. I think it's worth making the change, but if others don't feel like this improves the code's readability and would help prevent future errors, I can drop it down to just a nice comment on the Authenticate function or something. :)