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

MF-1516 - Fix API key issuing #1530

Merged
merged 7 commits into from
Dec 24, 2021
Merged

MF-1516 - Fix API key issuing #1530

merged 7 commits into from
Dec 24, 2021

Conversation

fbugarski
Copy link
Contributor

@fbugarski fbugarski commented Dec 17, 2021

Signed-off-by: Filip Bugarski filipbugarski@gmail.com

What does this do?

This pull request fixes issue with storing Mainflux API keys.

Which issue(s) does this PR fix/relate to?

Fixes #1516.

List any changes that modify/break current functionality

The only API key can be created by the user HTTP API.

Have you included tests for your changes?

Yes. Tests are fixed to match new changes.

Did you document any new/modified functionality?

Yes.

Notes

N/A

Signed-off-by: Filip Bugarski <filipbugarski@gmail.com>
@fbugarski fbugarski requested a review from a team as a code owner December 17, 2021 13:41
Signed-off-by: Filip Bugarski <filipbugarski@gmail.com>
Signed-off-by: Filip Bugarski <filipbugarski@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2021

Codecov Report

Merging #1530 (af21e46) into master (ad8b7dd) will increase coverage by 0.00%.
The diff coverage is 83.33%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1530   +/-   ##
=======================================
  Coverage   67.60%   67.61%           
=======================================
  Files         133      133           
  Lines       10930    10933    +3     
=======================================
+ Hits         7389     7392    +3     
+ Misses       2937     2936    -1     
- Partials      604      605    +1     
Impacted Files Coverage Δ
auth/keys.go 100.00% <ø> (ø)
auth/service.go 74.27% <71.42%> (-0.36%) ⬇️
auth/api/grpc/requests.go 60.86% <100.00%> (ø)
auth/api/http/keys/requests.go 75.00% <100.00%> (-5.00%) ⬇️
users/service.go 63.97% <100.00%> (ø)
things/service.go 66.07% <0.00%> (+0.71%) ⬆️

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 ad8b7dd...af21e46. Read the comment docs.

auth/service.go Outdated
@@ -167,7 +167,15 @@ func (svc service) Identify(ctx context.Context, token string) (Identity, error)
}

switch key.Type {
case APIKey, RecoveryKey, UserKey:
case RecoveryKey, LoginKey:

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the empty line.

auth/service.go Outdated

return Identity{ID: key.IssuerID, Email: key.Subject}, nil
case APIKey:

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

auth/api/logging.go Show resolved Hide resolved
Signed-off-by: Filip Bugarski <filipbugarski@gmail.com>
Signed-off-by: Filip Bugarski <filipbugarski@gmail.com>
dborovcanin
dborovcanin previously approved these changes Dec 21, 2021

ts := newServer(svc)
defer ts.Close()
client := ts.Client()

uk := issueRequest{Type: auth.UserKey}
uk := issueRequest{Type: auth.LoginKey}
Copy link
Contributor

Choose a reason for hiding this comment

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

uk can be changed to lk

@@ -104,11 +104,11 @@ func TestIssue(t *testing.T) {
status int
}{
{
Copy link
Contributor

@arvindh123 arvindh123 Dec 22, 2021

Choose a reason for hiding this comment

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

Status Created case is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Line 118

Copy link
Contributor

Choose a reason for hiding this comment

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

Line 118 have test for ak - api key
But in master the there is test case for user key aka login key
https://github.com/mainflux/mainflux/blob/ad8b7ddf5afac43699f7080d006dd40de0559222/auth/api/http/keys/endpoint_test.go#L107

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You cant create login key over HTTP API anymore.

@@ -58,7 +58,7 @@ func TestIssue(t *testing.T) {
{
desc: "issue user key",
Copy link
Contributor

Choose a reason for hiding this comment

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

Description can be changed to issue login key

@@ -67,7 +67,7 @@ func TestIssue(t *testing.T) {
{
desc: "issue user key with no time",
Copy link
Contributor

Choose a reason for hiding this comment

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

Description shall be changed to login key

@@ -180,7 +180,7 @@ func TestRetrieve(t *testing.T) {
IssuedAt: time.Now(),
}

_, userToken, err := svc.Issue(context.Background(), "", auth.Key{Type: auth.UserKey, IssuedAt: time.Now(), IssuerID: id, Subject: email})
_, userToken, err := svc.Issue(context.Background(), "", auth.Key{Type: auth.LoginKey, IssuedAt: time.Now(), IssuerID: id, Subject: email})
assert.Nil(t, err, fmt.Sprintf("Issuing user key expected to succeed: %s", err))
Copy link
Contributor

@arvindh123 arvindh123 Dec 22, 2021

Choose a reason for hiding this comment

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

user key shall be changed to login key
and also in line number 187

Signed-off-by: Filip Bugarski <filipbugarski@gmail.com>
Signed-off-by: Filip Bugarski <filipbugarski@gmail.com>
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

@drasko drasko merged commit dd7d52e into absmach:master Dec 24, 2021
bioinformatx pushed a commit to bioinformatx/mainflux that referenced this pull request Jan 28, 2022
* Fix API keys saving

Signed-off-by: Filip Bugarski <filipbugarski@gmail.com>

* Fix API key creation

Signed-off-by: Filip Bugarski <filipbugarski@gmail.com>

* Fix tests

Signed-off-by: Filip Bugarski <filipbugarski@gmail.com>

* Delete empty lines

Signed-off-by: Filip Bugarski <filipbugarski@gmail.com>

* Remove empty lines

Signed-off-by: Filip Bugarski <filipbugarski@gmail.com>

* Fix typo

Signed-off-by: Filip Bugarski <filipbugarski@gmail.com>

* Change user key to login key

Signed-off-by: Filip Bugarski <filipbugarski@gmail.com>
Signed-off-by: skovacevic <stefan.kovacevic@mainflux.io>
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.

API key not saved in DB
5 participants