Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Marketplace login #443

Merged
merged 3 commits into from
Feb 1, 2019
Merged

Marketplace login #443

merged 3 commits into from
Feb 1, 2019

Conversation

IljaN
Copy link
Member

@IljaN IljaN commented Jan 23, 2019

Login directly from market-app to auto-install your api-key.

  • Market app send its callback-url and PKCE-Challenge to marketplace.
  • A marketplace login and approval prompt is presented to the user.
  • If user approves prompt, a one-time token is generated and forwarded to the market app.
  • The Token is used together with PKCE-Verify-code by market app to retrieve the api-key.

This PR can be tested against staging marketplace by setting appstoreurl.

This is NOT OAuth because OAuth requires a static callback-url

Closes #425

@IljaN IljaN self-assigned this Jan 23, 2019
@codecov
Copy link

codecov bot commented Jan 23, 2019

Codecov Report

Merging #443 into master will decrease coverage by 3.36%.
The diff coverage is 1.56%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #443      +/-   ##
============================================
- Coverage     51.57%   48.21%   -3.37%     
- Complexity      255      266      +11     
============================================
  Files            17       17              
  Lines           888      952      +64     
============================================
+ Hits            458      459       +1     
- Misses          430      493      +63
Impacted Files Coverage Δ Complexity Δ
lib/Controller/PageController.php 80% <0%> (-20%) 2 <1> (+1)
lib/Controller/MarketController.php 0% <0%> (ø) 41 <4> (+5) ⬆️
lib/HttpService.php 40.54% <0%> (-6.83%) 32 <3> (+3)
appinfo/routes.php 0% <0%> (ø) 0 <0> (ø) ⬇️
lib/MarketService.php 47.65% <5.26%> (-3.73%) 83 <2> (+2)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 958725c...73a8629. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 23, 2019

Codecov Report

Merging #443 into master will increase coverage by 1.39%.
The diff coverage is 57.74%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #443      +/-   ##
============================================
+ Coverage     51.57%   52.97%   +1.39%     
- Complexity      255      267      +12     
============================================
  Files            17       17              
  Lines           888      959      +71     
============================================
+ Hits            458      508      +50     
- Misses          430      451      +21
Impacted Files Coverage Δ Complexity Δ
lib/Controller/MarketController.php 0% <0%> (ø) 41 <4> (+5) ⬆️
appinfo/routes.php 0% <0%> (ø) 0 <0> (ø) ⬇️
lib/Controller/PageController.php 100% <100%> (ø) 2 <1> (+1) ⬆️
lib/MarketService.php 59.14% <100%> (+7.76%) 83 <2> (+2) ⬆️
lib/HttpService.php 55.08% <86.95%> (+7.71%) 33 <4> (+4) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 958725c...6e800a7. Read the comment docs.

@IljaN IljaN force-pushed the marketplace_login branch 2 times, most recently from 74795d5 to ff20088 Compare January 25, 2019 17:07
@IljaN IljaN changed the title [WIP] Marketplace login Marketplace login Jan 25, 2019
Copy link
Member

@VicDeo VicDeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IljaN Looks good in general. See a few comments.
Vue templates are already indented with spaces so the formatting looks weird for them.

lib/Controller/MarketController.php Outdated Show resolved Hide resolved
lib/Controller/MarketController.php Show resolved Hide resolved
lib/HttpService.php Show resolved Hide resolved
lib/HttpService.php Show resolved Hide resolved
@IljaN
Copy link
Member Author

IljaN commented Jan 30, 2019

@VicDeo Adressed your changes. Tough not sure about:

Vue templates are already indented with spaces so the formatting looks weird for them.

Care to explain?

@IljaN
Copy link
Member Author

IljaN commented Jan 30, 2019

Please don't merge yet

src/App.vue Show resolved Hide resolved
@VicDeo
Copy link
Member

VicDeo commented Jan 30, 2019

@IljaN open the diff for src/App.vue here and you see that your changes are not aligned with the rest in this file. I spotted out one line, the rest are same (tabs instead of spaces)

@IljaN
Copy link
Member Author

IljaN commented Jan 30, 2019

Hmhh App.vue seems to be mixed, sometimes tabs, sometimes spaces 🙈

@IljaN
Copy link
Member Author

IljaN commented Jan 31, 2019

@VicDeo Please re-review

Copy link
Member

@VicDeo VicDeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to merge after adding type hints into PHP doc blocks.

It would be good to squash at least commits 1-5 to compact the history. But it's up to you.

*
* Exchange login token for api key
*
* @param $loginToken
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type is missing

* Exchange login token for api key
*
* @param $loginToken
* @param $codeVerifier
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type is missing

@@ -273,6 +295,40 @@ private function httpGet($path, $options, $apiKey) {
return $response;
}

/**
* @param $path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type is missing

@@ -273,6 +295,40 @@ private function httpGet($path, $options, $apiKey) {
return $response;
}

/**
* @param $path
* @param $options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type is missing

@IljaN IljaN merged commit 9effe15 into master Feb 1, 2019
@delete-merged-branch delete-merged-branch bot deleted the marketplace_login branch February 1, 2019 08:58
@PVince81 PVince81 mentioned this pull request Feb 8, 2019
26 tasks
@PVince81 PVince81 added this to the QA milestone Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants