Keystone V3: Introduce OAuth1 auth support#1935
Keystone V3: Introduce OAuth1 auth support#1935jtopjian merged 14 commits intogophercloud:masterfrom
Conversation
|
Build succeeded.
|
|
Build failed.
|
|
Build succeeded.
|
|
@jtopjian done cleanup, ready for review. |
jtopjian
left a comment
There was a problem hiding this comment.
@kayrus Thank you for working on this.
This is a lot to take in and unfortunately I didn't have enough time to do a full review today. I've left two comments for now.
In addition, it looks like a lot of the opts structs are missing builder interfaces - we should probably include those.
I hope to have more time tomorrow to continue looking at this - I'm sorry about that.
| if nonce == "" { | ||
| // when nonce is not set, generate a random one | ||
| query.Set("oauth_nonce", strconv.FormatInt(rand.Int63(), 10)+query.Get("oauth_timestamp")) | ||
| } |
There was a problem hiding this comment.
Should there be an else here?
There was a problem hiding this comment.
No, but you pointed me to the fact, that there is no need to pass "nonce" as a parameter, since oauth_nonce is already parsed by gophercloud.BuildQueryString.
| func stringToSign(method string, u string, params string) []byte { | ||
| parsedURL, _ := url.Parse(u) | ||
| if strings.HasPrefix(parsedURL.Host, "127.0.0.1") { | ||
| // Workaround for unit tests with dynamic ports |
There was a problem hiding this comment.
I was able to get around this by doing the following:
func TestAuthenticate(t *testing.T) {
l, _ := net.Listen("tcp", "127.0.0.1:33199")
th.Server = httptest.NewUnstartedServer(th.Mux)
th.Server.Listener = l
th.Server.Start()
//th.SetupHTTP()
defer th.TeardownHTTP()
HandleAuthenticate(t)
...However, the test is still failing, but I'm not sure why.
I'd definitely like to see this conditional removed somehow, though.
There was a problem hiding this comment.
I created an extra testhelper function to listen the static port. Additionally I noticed that using "https://identity:443/" URL containing the default port is not supported by oauth, therefore I modified the logic to normalize URLs with default port.
I don't think that build interface is necessary here. I don't expect that oauth method must be extended by additional parameters or logic.
No worries. |
2ad403a to
7561bac
Compare
We should include the builder interfaces even if we can't think of a reason to use them. Any time in the past that I have assumed they wouldn't be used, someone usually reports how they're missing :) |
|
Build succeeded.
|
I need to rewrite existing logic, since |
|
Right, I see. I'm not trying to cause large rewrites, but I think if the The interface methods can be pretty flexible with their return values, so that might be a way to link the Create function to the opts structure. |
|
@jtopjian As you mentioned As for the rest |
|
Build succeeded.
|
|
@jtopjian the only way to properly introduce the build interface for |
|
Could you create a new method to return the required information specific to this package? |
|
@jtopjian Not sure if I understood you correctly, but see my latest commit. |
|
Yes, that should work. |
|
@jtopjian while I took a look on these interfaces. I noticed that there is no need to pass scope map into the UPD: Ignore my comment. It was done by purpose, since |
|
Build failed.
|
|
recheck |
|
Build succeeded.
|
|
@kayrus Thank you for working on this. The general logic looks good and I appreciate the time you took investigating how to successfully generate oauth1-based requests. I've done some refactoring to this which you can see here (jtopjian@5aad4d7). Let me know what you think. |
|
@jtopjian thanks for you time. I added my comments. Don't hesitate to commit directly to my branch to run the CI/CD pipeline next time. Github repo owners have this ability by default. |
Thanks for the review - I've made changes per your comments and will commit to this branch. I didn't want to do that initially until you had a first look. |
|
Build failed.
|
|
Build succeeded.
|
|
Build succeeded.
|
|
@jtopjian rebased to master |
|
Build succeeded.
|
|
Build failed.
|
|
recheck |
|
Build succeeded.
|
|
@jtopjian PR is ready |
Resolves #1418
@jtopjian let me know if you find something wrong in this code before I start code cleanup.