Skip to content
This repository has been archived by the owner on Apr 11, 2023. It is now read-only.

feat: #49 /login endpoint #54

Merged
merged 1 commit into from
Sep 18, 2020
Merged

feat: #49 /login endpoint #54

merged 1 commit into from
Sep 18, 2020

Conversation

llorllale
Copy link
Contributor

@llorllale llorllale commented Sep 18, 2020 •
edited
Loading

closes #50

  • Added /hydra/login
  • Added hydra url startup param
  • Pulled in latest fixes from edge-core
  • Moved some parts of the code around a bit

Signed-off-by: George Aristy george.aristy@securekey.com

@llorllale llorllale added the type: enhancement New feature or request label Sep 18, 2020
@llorllale llorllale added this to the 0.1.5 milestone Sep 18, 2020
@cla-bot cla-bot bot added the cla-signed label Sep 18, 2020
@codecov
Copy link

codecov bot commented Sep 18, 2020 •
edited
Loading

Codecov Report

Merging #54 into master will increase coverage by 0.60%.
The diff coverage is 83.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
+ Coverage   87.08%   87.69%   +0.60%     
==========================================
  Files           6        8       +2     
  Lines         457      455       -2     
==========================================
+ Hits          398      399       +1     
+ Misses         38       34       -4     
- Partials       21       22       +1
Impacted Files Coverage Δ
pkg/restapi/operation/dependencies.go 20.00% <20.00%> (ø)
pkg/restapi/operation/transient.go 60.00% <60.00%> (ø)
pkg/restapi/operation/operations.go 87.43% <89.47%> (+4.27%) ⬆️
cmd/auth-rest/startcmd/start.go 90.18% <100.00%> (+0.23%) ⬆️
pkg/restapi/controller.go 100.00% <100.00%> (ø)
pkg/bootstrap/user/userprofile.go 84.61% <0.00%> (-2.89%) ⬇️
cmd/auth-rest/main.go 75.00% <0.00%> (-2.78%) ⬇️
pkg/internal/common/support/httphandler.go 100.00% <0.00%> (ø)
... and 2 more

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 cf9371d...698fad2. Read the comment docs.

@llorllale
Copy link
Contributor Author

Can we disregard the red flag on coverage?

  • of all new lines of code, only 1 is not tested because the code is unreachable (transient.go#L27)
  • all other untested lines are existing code that I moved around

DRK3
DRK3 reviewed Sep 18, 2020
View reviewed changes
pkg/restapi/operation/operations.go
login, err := c.hydra.GetLoginRequest(req)
if err != nil {
common.WriteErrorResponsef(w, logger,
http.StatusBadGateway, "failed to fetch login request from hydra: %s", err.Error())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is http.StatusBadGateway the appropriate header for all errors returned from GetLoginRequest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I don't know. There is a login.Error() string method... I guess we can figure that out later.

DRK3
DRK3 reviewed Sep 18, 2020
View reviewed changes
pkg/restapi/operation/operations_test.go Outdated
@@ -17,6 +17,9 @@ import (
"strings"
"testing"

"github.com/ory/hydra-client-go/client/admin"
"github.com/ory/hydra-client-go/models"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Extra line break

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

DRK3
DRK3 reviewed Sep 18, 2020
View reviewed changes
test/bdd/fixtures/auth-rest/docker-compose.yml Outdated
@@ -23,6 +23,7 @@ services:
- AUTH_REST_GOOGLE_CLIENTSECRET=TODO
- AUTH_REST_SDS_URL=TODO # onboard user: https://github.com/trustbloc/hub-auth/issues/38
- AUTH_REST_KEYSERVER_URL=TODO # onboard user: https://github.com/trustbloc/hub-auth/issues/38
- AUTH_REST_HYDRA_URL=http://TODO.com # setup Hydra in BDD tests: https://github.com/trustbloc/hub-auth/issues/52
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add the TODO word here to be consistent with the other TODOs
(and I guess that would apply to the TODOs above these two lines as well...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@DRK3
Copy link
Collaborator

DRK3 commented Sep 18, 2020

@llorllale Only a few minor things to take a quick look at - nothing major. Can be merged

feat: trustbloc#49 /login endpoint
698fad2 
Signed-off-by: George Aristy <george.aristy@securekey.com>
@fqutishat fqutishat merged commit 18adaf0 into trustbloc:master Sep 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@talwinder50 talwinder50 talwinder50 approved these changes

@DRK3 DRK3 DRK3 approved these changes

@aholovko aholovko Awaiting requested review from aholovko

@baha-ai baha-ai Awaiting requested review from baha-ai

Assignees
No one assigned
Labels
cla-signed type: enhancement New feature or request
Projects
None yet
Milestone
0.1.5
Development

Successfully merging this pull request may close these issues.

hydra login handler
4 participants
@llorllale @DRK3 @talwinder50 @fqutishat