methods for the OIDC auth flow #25
Conversation
message/message.go
Outdated
|
|
||
| const EnduserAuthPoll = "/v1/enduser-auth/poll" | ||
|
|
||
| const EnduserAuthPollStatusNotStarted = "NOT_STARTED" |
There was a problem hiding this comment.
Do we not need a failure state here? Or would the connection simply close in that case?
Co-authored-by: John Maguire <[email protected]>
client.go
Outdated
| Errors message.APIErrors | ||
| } | ||
| if err := json.Unmarshal(respBody, &errors); err != nil { | ||
| return nil, fmt.Errorf("dnclient endpoint returned bad status code '%d', body: %s", resp.StatusCode, respBody) |
There was a problem hiding this comment.
Looking closer, and I think this isn't quite right. This code comes from postDNClient which is used to contact the /v1/dnclient endpoint (hence the "dnclient endpoint" in the error.) This endpoint differs from our primary API:
- It uses a different authentication method, for dnclient
- It doesn't use our standard response format (Data vs. Errors)
- It has a couple envelopes, which are base64-encoded and signed
Take a look at the Enroll call for how we want to handle this - primarily, we attempt to deconstruct the Errors array.
I think that ultimately we want to create a callAPI function, similar to postDNClient, using Enroll as a basis for how to process the message , that is used for enroll, poll, and preauth.
Probably we can remove Errors from the message structs for these calls if we process them in one place like this.
There was a problem hiding this comment.
fair enough. I really would like to land this, so I'm choosing to punt on DRY-ing handling bodies for now if that's cool with you
message/message.go
Outdated
| const EndpointAuthPollStatusNotStarted = "NOT_STARTED" | ||
| const EndpointAuthPollStatusWaiting = "WAITING" | ||
| const EndpointAuthPollStatusStarted = "OIDC_STARTED" | ||
| const EndpointAuthPollStatusSuccess = "SUCCESS" |
There was a problem hiding this comment.
In the final PR, the states that landed were:
WAITING (user has not initiated OIDC), STARTED (waiting on OIDC to complete) and COMPLETED: https://github.com/DefinedNet/api/blob/4f575743842f915ced7dd86c588375d07c641c22/pkg/data/endpoint_auth.go#L51-L53
This doesn't match the doc, and I apologize. Will update now.
There was a problem hiding this comment.
should these get pulled into api from dnapi?
There was a problem hiding this comment.
Do you mean having api import dnapi and use the constants from it?
I would argue no - it doesn't seem to me that a client library should be responsible for how the server behaves. If this repository changes the constants, and api is importing them, it would immediately create a backwards incompatibility in the api that would be non-obvious to whoever updated the dnapi dependency.
dnapi is a public repo so that we can import it into mobile_nebula, and for this reason, I prefer it to be independent from the api.
dnapitest/dnapitest.go
Outdated
| s.expectedRequests = s.expectedRequests[1:] | ||
| res := expected.dncRequestResponse | ||
| w.WriteHeader(res.statusCode) | ||
| _, _ = w.Write(res.response(message.RequestWrapper{})) |
There was a problem hiding this comment.
This does not look correct to me. This is for a /v1/dnclient endpoint message.
Doesn't preauth need to return a pollToken in order for the poll endpoint to behave correctly? I think we should add a test that the handler receives the poll token sent by the client.
There was a problem hiding this comment.
It does! The test injects the "desired response" from the server, and confirms the client gets it. There's no "real handler" because PreAuth doesn't take arguments from the client.
Uh oh!
There was an error while loading. Please reload this page.