Merge lp:~alecu/unity-scope-click/sso-for-signatures into lp:unity-scope-click

Proposed by Alejandro J. Cura
Status: Rejected
Rejected by: dobey
Proposed branch: lp:~alecu/unity-scope-click/sso-for-signatures
Merge into: lp:unity-scope-click
Diff against target: 118 lines (+33/-4)
7 files modified
libclickscope/click/webclient.cpp (+1/-1)
libclickscope/click/webclient.h (+1/-1)
libclickscope/tests/mock_webclient.h (+2/-2)
libclickscope/tests/test_index.cpp (+12/-0)
libclickscope/tests/test_reviews.cpp (+13/-0)
scope/clickapps/apps-scope.cpp (+2/-0)
scope/clickstore/store-scope.cpp (+2/-0)
To merge this branch: bzr merge lp:~alecu/unity-scope-click/sso-for-signatures
Reviewer Review Type Date Requested Status
dobey (community) Needs Fixing
PS Jenkins bot continuous-integration Approve
Review via email: mp+280762@code.launchpad.net

Commit message

Fix the signing of store webservice urls

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alejandro J. Cura (alecu) wrote :

Tried adding scope harness tests to this branch, but there's no way to have u1-creds provide fake credentials, so I reverted that.

Also tried adding unit tests, but it needed a lot of refactoring just to check the credentials service was set, so I decided against it.

Revision history for this message
dobey (dobey) wrote :

At the very least, we will need to do this same thing for the apps scope as well, so that the requests for previews are properly signed as well.

review: Approve
Revision history for this message
dobey (dobey) :
review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
dobey (dobey) wrote :

I think instead of changing index.cpp here, we should instead change webclient.{cpp,h} to change the default value for the sign argument to true, in the call method which has that argument, and inside the call method with fewer arguments which calls the longer call method. This will change all requests to be signed by default, and will take care of the other cases where we need to sign, and aren't.

Revision history for this message
dobey (dobey) wrote :

Looks ok to me now.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
dobey (dobey) wrote :

After getting a private package shared with me to test this, and trying to test this on my mako with the silo build, I have been unable to get the private package to show up in the query in the click scope, but it does appear in the response when using a command line tool to test. This leads me to believe that somehow the request is not actually being signed. In trying to debug further, I was getting hangs so I think there is still something wrong with the Qt interaction and what is required for this to work correctly. As a result, I've asked this bug to be moved off OTA9 as it's not critical, and we can work further on this after you return from vacation.

review: Needs Fixing
Revision history for this message
dobey (dobey) wrote :

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'libclickscope/click/webclient.cpp'
2--- libclickscope/click/webclient.cpp 2015-11-24 19:14:11 +0000
3+++ libclickscope/click/webclient.cpp 2015-12-18 21:34:57 +0000
4@@ -77,7 +77,7 @@
5 const std::string& iri,
6 const click::web::CallParams& params)
7 {
8- return call(iri, "GET", false,
9+ return call(iri, "GET", true,
10 std::map<std::string, std::string>(), "", params);
11 }
12
13
14=== modified file 'libclickscope/click/webclient.h'
15--- libclickscope/click/webclient.h 2015-11-24 19:14:11 +0000
16+++ libclickscope/click/webclient.h 2015-12-18 21:34:57 +0000
17@@ -119,7 +119,7 @@
18 virtual QSharedPointer<Response> call(
19 const std::string& iri,
20 const std::string& method,
21- bool sign = false,
22+ bool sign = true,
23 const std::map<std::string, std::string>& headers = std::map<std::string, std::string>(),
24 const std::string& data = "",
25 const CallParams& params = CallParams());
26
27=== modified file 'libclickscope/tests/mock_webclient.h'
28--- libclickscope/tests/mock_webclient.h 2014-10-01 15:27:44 +0000
29+++ libclickscope/tests/mock_webclient.h 2015-12-18 21:34:57 +0000
30@@ -87,13 +87,13 @@
31 QSharedPointer<click::web::Response> call(
32 const std::string& iri,
33 const click::web::CallParams& params=click::web::CallParams()) override {
34- return callImpl(iri, "GET", false,
35+ return callImpl(iri, "GET", true,
36 std::map<std::string, std::string>(), "", params);
37 }
38 QSharedPointer<click::web::Response> call(
39 const std::string& iri,
40 const std::string& method,
41- bool sign = false,
42+ bool sign = true,
43 const std::map<std::string, std::string>& headers = std::map<std::string, std::string>(),
44 const std::string& data = "",
45 const click::web::CallParams& params=click::web::CallParams()) override {
46
47=== modified file 'libclickscope/tests/test_index.cpp'
48--- libclickscope/tests/test_index.cpp 2015-11-24 18:23:23 +0000
49+++ libclickscope/tests/test_index.cpp 2015-12-18 21:34:57 +0000
50@@ -140,6 +140,18 @@
51 indexPtr->departments("departments", [](const click::DepartmentList&, const click::HighlightList&, click::Index::Error, int) {});
52 }
53
54+TEST_F(IndexTest, testDetailsSignsCall)
55+{
56+ LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
57+ auto response = responseForReply(reply.asSharedPtr());
58+
59+ EXPECT_CALL(*clientPtr, callImpl(_, _, true, _, _, _))
60+ .Times(1)
61+ .WillOnce(Return(response));
62+
63+ indexPtr->get_details("fake-app", [](const click::PackageDetails, click::Index::Error) {});
64+}
65+
66 TEST_F(IndexTest, testSearchSendsBuiltQueryAsParam)
67 {
68 const std::string FAKE_BUILT_QUERY = "FAKE_QUERY,frameworks:fake-14.04,architecture:fake-arch";
69
70=== modified file 'libclickscope/tests/test_reviews.cpp'
71--- libclickscope/tests/test_reviews.cpp 2015-11-24 18:23:23 +0000
72+++ libclickscope/tests/test_reviews.cpp 2015-12-18 21:34:57 +0000
73@@ -150,6 +150,19 @@
74 click::Reviews::Error) {});
75 }
76
77+TEST_F(ReviewsTest, testFetchReviewsSignsCall)
78+{
79+ LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
80+ auto response = responseForReply(reply.asSharedPtr());
81+
82+ EXPECT_CALL(*clientPtr, callImpl(_, _, true, _, _, _))
83+ .Times(1)
84+ .WillOnce(Return(response));
85+
86+ reviewsPtr->fetch_reviews("", [](click::ReviewList,
87+ click::Reviews::Error) {});
88+}
89+
90 TEST_F(ReviewsTest, testFetchReviewsSendsQueryAsParam)
91 {
92 LifetimeHelper<click::network::Reply, MockNetworkReply> reply;
93
94=== modified file 'scope/clickapps/apps-scope.cpp'
95--- scope/clickapps/apps-scope.cpp 2015-11-24 18:23:23 +0000
96+++ scope/clickapps/apps-scope.cpp 2015-12-18 21:34:57 +0000
97@@ -51,6 +51,8 @@
98 {
99 nam.reset(new click::network::AccessManager());
100 client.reset(new click::web::Client(nam));
101+ QSharedPointer<click::CredentialsService> sso(new click::CredentialsService());
102+ client->setCredentialsService(sso);
103 index.reset(new click::Index(client));
104 pay_package.reset(new pay::Package(client));
105
106
107=== modified file 'scope/clickstore/store-scope.cpp'
108--- scope/clickstore/store-scope.cpp 2015-11-24 18:23:23 +0000
109+++ scope/clickstore/store-scope.cpp 2015-12-18 21:34:57 +0000
110@@ -49,6 +49,8 @@
111 {
112 nam.reset(new click::network::AccessManager());
113 client.reset(new click::web::Client(nam));
114+ QSharedPointer<click::CredentialsService> sso(new click::CredentialsService());
115+ client->setCredentialsService(sso);
116 index.reset(new click::Index(client));
117 depts.reset(new click::DepartmentLookup());
118 highlights.reset(new click::HighlightList());

Subscribers

People subscribed via source and target branches