Skip to content

Commit

Permalink
rename a function and a struct for clarity
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ewollesen committed Oct 18, 2023
1 parent 9d11f21 commit 07f0734
Show file tree
Hide file tree
Showing 52 changed files with 256 additions and 370 deletions.
2 changes: 1 addition & 1 deletion auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ type Client interface {

type ExternalAccessor interface {
ServerSessionToken() (string, error)
ValidateSessionToken(ctx context.Context, token string) (request.Details, error)
ValidateSessionToken(ctx context.Context, token string) (request.AuthDetails, error)
EnsureAuthorized(ctx context.Context) error
EnsureAuthorizedService(ctx context.Context) error
EnsureAuthorizedUser(ctx context.Context, targetUserID string, permission string) (string, error)
Expand Down
10 changes: 5 additions & 5 deletions auth/client/external.go
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ func (e *External) ServerSessionToken() (string, error) {
return serverSessionToken, nil
}

func (e *External) ValidateSessionToken(ctx context.Context, token string) (request.Details, error) {
func (e *External) ValidateSessionToken(ctx context.Context, token string) (request.AuthDetails, error) {
if ctx == nil {
return nil, errors.New("context is missing")
}
Expand All @@ -214,15 +214,15 @@ func (e *External) ValidateSessionToken(ctx context.Context, token string) (requ
return nil, errors.New("user id is missing")
}

return request.NewDetails(request.MethodSessionToken, result.UserID, token), nil
return request.NewAuthDetails(request.MethodSessionToken, result.UserID, token), nil
}

func (e *External) EnsureAuthorized(ctx context.Context) error {
if ctx == nil {
return errors.New("context is missing")
}

if details := request.DetailsFromContext(ctx); details != nil {
if details := request.GetAuthDetails(ctx); details != nil {
return nil
}

Expand All @@ -234,7 +234,7 @@ func (e *External) EnsureAuthorizedService(ctx context.Context) error {
return errors.New("context is missing")
}

if details := request.DetailsFromContext(ctx); details != nil {
if details := request.GetAuthDetails(ctx); details != nil {
if details.IsService() {
return nil
}
Expand All @@ -254,7 +254,7 @@ func (e *External) EnsureAuthorizedUser(ctx context.Context, targetUserID string
return "", errors.New("authorized permission is missing")
}

if details := request.DetailsFromContext(ctx); details != nil {
if details := request.GetAuthDetails(ctx); details != nil {
if details.IsService() {
return "", nil
}
Expand Down
34 changes: 17 additions & 17 deletions auth/client/external_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,15 +81,15 @@ var _ = Describe("External", func() {
var responseHeaders http.Header
var client *authClient.External
var sessionToken string
var details request.Details
var details request.AuthDetails
var ctx context.Context

BeforeEach(func() {
server = NewServer()
requestHandlers = nil
responseHeaders = http.Header{"Content-Type": []string{"application/json; charset=utf-8"}}
sessionToken = authTest.NewSessionToken()
details = request.NewDetails(request.MethodSessionToken, "", sessionToken)
details = request.NewAuthDetails(request.MethodSessionToken, "", sessionToken)
ctx = context.Background()
ctx = log.NewContextWithLogger(ctx, logger)
ctx = auth.NewContextWithServerSessionToken(ctx, sessionToken)
Expand All @@ -102,7 +102,7 @@ var _ = Describe("External", func() {
client, err = authClient.NewExternal(config, authorizeAs, name, logger)
Expect(err).ToNot(HaveOccurred())
Expect(client).ToNot(BeNil())
ctx = request.NewContextWithDetails(ctx, details)
ctx = request.NewContextWithAuthDetails(ctx, details)
})

AfterEach(func() {
Expand All @@ -123,12 +123,12 @@ var _ = Describe("External", func() {
})

It("returns an error when the details are missing", func() {
ctx = request.NewContextWithDetails(ctx, nil)
ctx = request.NewContextWithAuthDetails(ctx, nil)
errorsTest.ExpectEqual(client.EnsureAuthorized(ctx), request.ErrorUnauthorized())
})

It("returns successfully when the details are for a user", func() {
ctx = request.NewContextWithDetails(ctx, request.NewDetails(request.MethodSessionToken, authTest.RandomUserID(), sessionToken))
ctx = request.NewContextWithAuthDetails(ctx, request.NewAuthDetails(request.MethodSessionToken, authTest.RandomUserID(), sessionToken))
Expect(client.EnsureAuthorized(ctx)).To(Succeed())
})

Expand All @@ -150,12 +150,12 @@ var _ = Describe("External", func() {
})

It("returns an error when the details are missing", func() {
ctx = request.NewContextWithDetails(ctx, nil)
ctx = request.NewContextWithAuthDetails(ctx, nil)
errorsTest.ExpectEqual(client.EnsureAuthorizedService(ctx), request.ErrorUnauthorized())
})

It("returns an error when the details are for not a service", func() {
ctx = request.NewContextWithDetails(ctx, request.NewDetails(request.MethodSessionToken, authTest.RandomUserID(), sessionToken))
ctx = request.NewContextWithAuthDetails(ctx, request.NewAuthDetails(request.MethodSessionToken, authTest.RandomUserID(), sessionToken))
errorsTest.ExpectEqual(client.EnsureAuthorizedService(ctx), request.ErrorUnauthorized())
})

Expand All @@ -173,7 +173,7 @@ var _ = Describe("External", func() {
BeforeEach(func() {
requestUserID = authTest.RandomUserID()
targetUserID = authTest.RandomUserID()
details = request.NewDetails(request.MethodSessionToken, requestUserID, sessionToken)
details = request.NewAuthDetails(request.MethodSessionToken, requestUserID, sessionToken)
authorizedPermission = test.RandomStringFromArray([]string{permission.Write, permission.Read})
})

Expand Down Expand Up @@ -204,58 +204,58 @@ var _ = Describe("External", func() {
})

It("returns an error when the details are missing", func() {
ctx = request.NewContextWithDetails(ctx, nil)
ctx = request.NewContextWithAuthDetails(ctx, nil)
userID, err := client.EnsureAuthorizedUser(ctx, targetUserID, authorizedPermission)
errorsTest.ExpectEqual(err, request.ErrorUnauthorized())
Expect(userID).To(BeEmpty())
})

It("returns successfully when the details are for a service and authorized permission is custodian", func() {
ctx = request.NewContextWithDetails(ctx, request.NewDetails(request.MethodSessionToken, "", sessionToken))
ctx = request.NewContextWithAuthDetails(ctx, request.NewAuthDetails(request.MethodSessionToken, "", sessionToken))
authorizedPermission = permission.Custodian
Expect(client.EnsureAuthorizedUser(ctx, targetUserID, authorizedPermission)).To(Equal(""))
})

It("returns successfully when the details are for a service and authorized permission is owner", func() {
ctx = request.NewContextWithDetails(ctx, request.NewDetails(request.MethodSessionToken, "", sessionToken))
ctx = request.NewContextWithAuthDetails(ctx, request.NewAuthDetails(request.MethodSessionToken, "", sessionToken))
authorizedPermission = permission.Owner
Expect(client.EnsureAuthorizedUser(ctx, targetUserID, authorizedPermission)).To(Equal(""))
})

It("returns successfully when the details are for a service and authorized permission is upload", func() {
ctx = request.NewContextWithDetails(ctx, request.NewDetails(request.MethodSessionToken, "", sessionToken))
ctx = request.NewContextWithAuthDetails(ctx, request.NewAuthDetails(request.MethodSessionToken, "", sessionToken))
authorizedPermission = permission.Write
Expect(client.EnsureAuthorizedUser(ctx, targetUserID, authorizedPermission)).To(Equal(""))
})

It("returns successfully when the details are for a service and authorized permission is view", func() {
ctx = request.NewContextWithDetails(ctx, request.NewDetails(request.MethodSessionToken, "", sessionToken))
ctx = request.NewContextWithAuthDetails(ctx, request.NewAuthDetails(request.MethodSessionToken, "", sessionToken))
authorizedPermission = permission.Read
Expect(client.EnsureAuthorizedUser(ctx, targetUserID, authorizedPermission)).To(Equal(""))
})

It("returns an error when the details are for the target user and authorized permission is custodian", func() {
ctx = request.NewContextWithDetails(ctx, request.NewDetails(request.MethodSessionToken, targetUserID, sessionToken))
ctx = request.NewContextWithAuthDetails(ctx, request.NewAuthDetails(request.MethodSessionToken, targetUserID, sessionToken))
authorizedPermission = permission.Custodian
userID, err := client.EnsureAuthorizedUser(ctx, targetUserID, authorizedPermission)
errorsTest.ExpectEqual(err, request.ErrorUnauthorized())
Expect(userID).To(BeEmpty())
})

It("returns successfully when the details are for the target user and authorized permission is owner", func() {
ctx = request.NewContextWithDetails(ctx, request.NewDetails(request.MethodSessionToken, targetUserID, sessionToken))
ctx = request.NewContextWithAuthDetails(ctx, request.NewAuthDetails(request.MethodSessionToken, targetUserID, sessionToken))
authorizedPermission = permission.Owner
Expect(client.EnsureAuthorizedUser(ctx, targetUserID, authorizedPermission)).To(Equal(targetUserID))
})

It("returns successfully when the details are for the target user and authorized permission is upload", func() {
ctx = request.NewContextWithDetails(ctx, request.NewDetails(request.MethodSessionToken, targetUserID, sessionToken))
ctx = request.NewContextWithAuthDetails(ctx, request.NewAuthDetails(request.MethodSessionToken, targetUserID, sessionToken))
authorizedPermission = permission.Write
Expect(client.EnsureAuthorizedUser(ctx, targetUserID, authorizedPermission)).To(Equal(targetUserID))
})

It("returns successfully when the details are for the target user and authorized permission is view", func() {
ctx = request.NewContextWithDetails(ctx, request.NewDetails(request.MethodSessionToken, targetUserID, sessionToken))
ctx = request.NewContextWithAuthDetails(ctx, request.NewAuthDetails(request.MethodSessionToken, targetUserID, sessionToken))
authorizedPermission = permission.Read
Expect(client.EnsureAuthorizedUser(ctx, targetUserID, authorizedPermission)).To(Equal(targetUserID))
})
Expand Down
4 changes: 2 additions & 2 deletions auth/provider_session.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ func (p *ProviderSession) Validate(validator structure.Validator) {
validator.Time("modifiedTime", p.ModifiedTime).After(p.CreatedTime).BeforeNow(time.Second)
}

func (p *ProviderSession) Sanitize(details request.Details) error {
func (p *ProviderSession) Sanitize(details request.AuthDetails) error {
if details != nil && details.IsService() {
return nil
}
Expand All @@ -254,7 +254,7 @@ func (p *ProviderSession) Sanitize(details request.Details) error {

type ProviderSessions []*ProviderSession

func (p ProviderSessions) Sanitize(details request.Details) error {
func (p ProviderSessions) Sanitize(details request.AuthDetails) error {
for _, providerSession := range p {
if err := providerSession.Sanitize(details); err != nil {
return err
Expand Down
4 changes: 2 additions & 2 deletions auth/restricted_token.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ func (r *RestrictedToken) Authenticates(req *http.Request) bool {
return true
}

func (r *RestrictedToken) Sanitize(details request.Details) error {
func (r *RestrictedToken) Sanitize(details request.AuthDetails) error {
if details != nil && (details.IsService() || details.UserID() == r.UserID) {
return nil
}
Expand All @@ -206,7 +206,7 @@ func (r *RestrictedToken) Sanitize(details request.Details) error {

type RestrictedTokens []*RestrictedToken

func (r RestrictedTokens) Sanitize(details request.Details) error {
func (r RestrictedTokens) Sanitize(details request.AuthDetails) error {
for _, restrictedToken := range r {
if err := restrictedToken.Sanitize(details); err != nil {
return err
Expand Down
4 changes: 2 additions & 2 deletions auth/service/api/v1/oauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func (r *Router) OAuthRoutes() []*rest.Route {
func (r *Router) OAuthProviderAuthorizeGet(res rest.ResponseWriter, req *rest.Request) {
responder := request.MustNewResponder(res, req)
ctx := req.Context()
details := request.DetailsFromContext(ctx)
details := request.GetAuthDetails(ctx)

if details == nil || details.Method() != request.MethodRestrictedToken {
r.htmlOnError(res, req, request.ErrorUnauthenticated())
Expand Down Expand Up @@ -66,7 +66,7 @@ func (r *Router) OAuthProviderAuthorizeGet(res rest.ResponseWriter, req *rest.Re
func (r *Router) OAuthProviderAuthorizeDelete(res rest.ResponseWriter, req *rest.Request) {
responder := request.MustNewResponder(res, req)
ctx := req.Context()
details := request.DetailsFromContext(ctx)
details := request.GetAuthDetails(ctx)

prvdr, err := r.oauthProvider(req)
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions auth/service/api/v1/restricted_token.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func (r *Router) ListUserRestrictedTokens(res rest.ResponseWriter, req *rest.Req

func (r *Router) CreateUserRestrictedToken(res rest.ResponseWriter, req *rest.Request) {
responder := request.MustNewResponder(res, req)
details := request.DetailsFromContext(req.Context())
details := request.GetAuthDetails(req.Context())

userID := req.PathParam("userId")
if userID == "" {
Expand Down Expand Up @@ -144,7 +144,7 @@ func (r *Router) UpdateRestrictedToken(res rest.ResponseWriter, req *rest.Reques

func (r *Router) DeleteRestrictedToken(res rest.ResponseWriter, req *rest.Request) {
responder := request.MustNewResponder(res, req)
details := request.DetailsFromContext(req.Context())
details := request.GetAuthDetails(req.Context())

id := req.PathParam("id")
if id == "" {
Expand Down
12 changes: 6 additions & 6 deletions auth/test/external_accessor.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ type ServerSessionTokenOutput struct {
}

type ValidateSessionTokenOutput struct {
Details request.Details
Error error
AuthDetails request.AuthDetails
Error error
}

type EnsureAuthorizedUserInput struct {
Expand All @@ -33,7 +33,7 @@ type ExternalAccessor struct {
ServerSessionTokenOutput *ServerSessionTokenOutput
ValidateSessionTokenInvocations int
ValidateSessionTokenInputs []string
ValidateSessionTokenStub func(ctx context.Context, token string) (request.Details, error)
ValidateSessionTokenStub func(ctx context.Context, token string) (request.AuthDetails, error)
ValidateSessionTokenOutputs []ValidateSessionTokenOutput
ValidateSessionTokenOutput *ValidateSessionTokenOutput
EnsureAuthorizedInvocations int
Expand Down Expand Up @@ -71,7 +71,7 @@ func (e *ExternalAccessor) ServerSessionToken() (string, error) {
panic("ServerSessionToken has no output")
}

func (e *ExternalAccessor) ValidateSessionToken(ctx context.Context, token string) (request.Details, error) {
func (e *ExternalAccessor) ValidateSessionToken(ctx context.Context, token string) (request.AuthDetails, error) {
e.ValidateSessionTokenInvocations++
e.ValidateSessionTokenInputs = append(e.ValidateSessionTokenInputs, token)
if e.ValidateSessionTokenStub != nil {
Expand All @@ -80,10 +80,10 @@ func (e *ExternalAccessor) ValidateSessionToken(ctx context.Context, token strin
if len(e.ValidateSessionTokenOutputs) > 0 {
output := e.ValidateSessionTokenOutputs[0]
e.ValidateSessionTokenOutputs = e.ValidateSessionTokenOutputs[1:]
return output.Details, output.Error
return output.AuthDetails, output.Error
}
if e.ValidateSessionTokenOutput != nil {
return e.ValidateSessionTokenOutput.Details, e.ValidateSessionTokenOutput.Error
return e.ValidateSessionTokenOutput.AuthDetails, e.ValidateSessionTokenOutput.Error
}
panic("ValidateSessionToken has no output")
}
Expand Down
8 changes: 4 additions & 4 deletions auth/test/mock.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion blob/client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -853,7 +853,7 @@ var _ = Describe("Client", func() {
sessionToken := authTest.NewSessionToken()
authorizeAs = platform.AuthorizeAsUser
requestHandlers = append(requestHandlers, VerifyHeaderKV("X-Tidepool-Session-Token", sessionToken))
ctx = request.NewContextWithDetails(ctx, request.NewDetails(request.MethodAccessToken, userTest.RandomID(), sessionToken))
ctx = request.NewContextWithAuthDetails(ctx, request.NewAuthDetails(request.MethodAccessToken, userTest.RandomID(), sessionToken))
})

authorizeAssertions()
Expand Down
Loading

0 comments on commit 07f0734

Please sign in to comment.