Add encryption #10
Add encryption #10
Conversation
skynet.go
Outdated
| if err != nil { | ||
| return errors.AddContext(err, "could not execute POST") | ||
| } | ||
| if resp.StatusCode >= 400 { |
There was a problem hiding this comment.
Do we expected to receive non error codes other than successful?
My initial reaction is that we should know the StatusCodes we should receive and handle them accordingly. Something like a switch statement that handles different codes explicitly and gives a developer error if we have missed one?
Otherwise if that is over the top I think we should explicitly check for success and return an error if not so we aren't hiding any weird status codes that might create UX bugs or inconsistencies for users.
Thoughts?
There was a problem hiding this comment.
Yeah, I agree. The potential errors are part of the public API, after all. I'll do this on Monday
There was a problem hiding this comment.
Hmm so I'm actually not sure what checking for explicit codes would give us. I'm not a web dev person but I'm pretty sure that all codes >= 400 are defined to be errors. Any other code should therefore be a success. I don't think we have many success codes at all but I hesitate to enumerate them here because I'm thinking that a portal could define its own (e.g. redirects which should have their own status code). Maybe @kwypchlo can weigh in here but I'm thinking we move this to a followup so as not to block the PR.
There was a problem hiding this comment.
As far as I know, we only use 200 (with response content) and 204 (without content) as OK but any 2xx should be treated as ok while any 4xx and 5xx should be treated as error. We don't have a finite list of what errors does the API return, that would have to be up to the api to document first.
|
This comment seems to have been lost. But we need to add in this MR or in a follow up MR the skykey parameter to the uploads. Then a test that verifies a file uploaded with a skykey can only be downloaded once the skykey is added. Not sure if that level of testing is possible with the current infrastructure, but none the less we need the skykey parameter for the uploads so that the uploads can be encrypted. |
|
Added the skykey parameters to uploads just as you submitted your comment. Will address the rest of the comments on Monday
Not really, because we mock all our HTTP calls and don't actually test the implementation. I feel it should be enough to ensure that the requests contain the necessary parameters and are formed correctly and that responses are parsed correctly. The actual implementation logic can be tested on the Sia side I think. |
ok fair enough. Wasn't sure the limitations of the testing for the SDK. |
| ConnectionOptions | ||
| // PortalCreateSkykeyPath is the relative URL path of the createskykey | ||
| // endpoint. | ||
| PortalCreateSkykeyPath string |
There was a problem hiding this comment.
Looking at this MR more I'm wondering if it makes more sense to rename this to APIRoute and add it to the ConnectionOptions.
That would allow us to generalize the code even more. Unless there is a plan to move the parameters like name, id, skykeytype into their corresponding options struct it seems unnecessary to be defining an Options struct for each endpoint.
Then this line becomes even more clear
url := makeURL(opts.PortalURL, opts.APIRoute)To be able to define the defaults easily we could have just two structs.
ConnectionOptions struct {}
APIRouteOptions struct {}
I'm thinking that there are a number of common API parameters that could be included in the APIRoutes eventually, like async root force etc.
Then the defaults can be defined as
DefaultGetSkyKeyOptions = APIRouteOptions {
ConnectionOptions: defaultConnectionOptions,
APIRoute: "/skynet/skykey",
}
|
Merging, issues were created for the remaining two discussions |
No description provided.