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

MG-2048 - Authorize clients and users with PATs #2499

Open
wants to merge 75 commits into
base: main
Choose a base branch
from

Conversation

nyagamunene
Copy link
Contributor

What type of PR is this?

This is a feature because it adds the following functionality: It adds authorization to things and users using PATs.

What does this do?

It adds PATs authorization to things and users middleware

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

Have you included tests for your changes?

Yes

Did you document any new/modified feature?

No

Notes

@nyagamunene nyagamunene self-assigned this Oct 31, 2024
@nyagamunene nyagamunene changed the title MG-2048 - Authorize things with PATs MG-2048 - Authorize things and users with PATs Nov 3, 2024
@nyagamunene nyagamunene force-pushed the AuthorizeUsersThings branch 2 times, most recently from 56daa42 to 923af86 Compare November 5, 2024 12:52
@nyagamunene nyagamunene changed the base branch from main to auth-refactor November 7, 2024 10:57
@nyagamunene nyagamunene marked this pull request as ready for review November 11, 2024 14:50
@@ -229,7 +384,7 @@ func (am *authorizationMiddleware) RemoveParentGroup(ctx context.Context, sessio
}

if th.ParentGroup != "" {
if err := am.extAuthorize(ctx, clients.GroupOpSetChildClient, authz.PolicyReq{
if err := am.extAuthorize(ctx, clients.GroupOpSetChildThing, authz.PolicyReq{
Domain: session.DomainID,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Domain: session.DomainID,
if err := am.extAuthorize(ctx, clients.GroupOpSetChildClient, authz.PolicyReq{

@@ -200,7 +341,7 @@ func (am *authorizationMiddleware) SetParentGroup(ctx context.Context, session a
return errors.Wrap(err, errSetParentGroup)
}

if err := am.extAuthorize(ctx, clients.GroupOpSetChildClient, authz.PolicyReq{
if err := am.extAuthorize(ctx, clients.GroupOpSetChildThing, authz.PolicyReq{
Domain: session.DomainID,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Domain: session.DomainID,
if err := am.extAuthorize(ctx, clients.GroupOpSetChildClient, authz.PolicyReq{
Domain: session.DomainID,

Comment on lines 38 to 37
resp.Type = mgauthn.AccessToken
if strings.HasPrefix(token, "pat"+seperator) {
resp.Type = mgauthn.PersonalAccessToken
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be done in auth service, During authentication process.
Is there any reason behind this ?

@@ -10,6 +10,7 @@ option go_package = "github.com/absmach/magistrala/internal/grpc/auth/v1";
// functionalities for magistrala services.
service AuthService {
rpc Authorize(AuthZReq) returns (AuthZRes) {}
rpc AuthorizePAT(AuthZpatReq) returns (AuthZRes) {}
rpc Authenticate(AuthNReq) returns (AuthNRes) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets have seperate RPC for PAT Authentication like PAT,

Comment on lines 109 to 114
r.Post("/authorize", kithttp.NewServer(
(authorizePATEndpoint(svc)),
decodeAuthorizePATRequest,
api.EncodeResponse,
opts...,
).ServeHTTP)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this endpoint, For now we can provide authoirzePAT via gRPC only.

auth/service.go Outdated
Comment on lines 176 to 183
if strings.HasPrefix(token, patPrefix+patSecretSeparator) {
pat, err := svc.IdentifyPAT(ctx, token)
if err != nil {
return Key{}, err
}
return Key{
ID: pat.ID,
Type: PersonalAccessToken,
Subject: pat.User,
User: pat.User,
IssuedAt: pat.IssuedAt,
ExpiresAt: pat.ExpiresAt,
}, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this logic to Authenticate function in file pkg/authn/authsvc/authn.go

Lets have seperate RPC for IdentifyPAT.

The Authenticate function in file pkg/authn/authsvc/authn.go will call IdentifyPAT base on token prefix

Comment on lines 44 to 56
}
return authn.Session{DomainUserID: res.GetId(), UserID: res.GetUserId(), DomainID: res.GetDomainId()}, nil
return authn.Session{ID: res.GetId(), UserID: res.GetUserId(), DomainID: res.GetDomainId()}, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets have seperate RPC call IdentifyPAT, move the logic to here

Something like below code

Suggested change
}
return authn.Session{DomainUserID: res.GetId(), UserID: res.GetUserId(), DomainID: res.GetDomainId()}, nil
return authn.Session{ID: res.GetId(), UserID: res.GetUserId(), DomainID: res.GetDomainId()}, nil
}
switch {
case strings.HasPrefix(token, patPrefix+patSecretSeparator):
res, err := a.authSvcClient.AuthenticatePAT(ctx, token)
if err != nil {
return authn.Session{}, errors.Wrap(errors.ErrAuthentication, err)
}
return authn.Session{ID: res.GetId(), UserID: res.GetUserId(), DomainID: res.GetDomainId()}, nil
default:
res, err := a.authSvcClient.Authenticate(ctx, &grpcAuthV1.AuthNReq{Token: token})
if err != nil {
return authn.Session{}, errors.Wrap(errors.ErrAuthentication, err)
}
return authn.Session{ID: res.GetId(), UserID: res.GetUserId(), DomainID: res.GetDomainId()}, nil
}

@@ -179,6 +390,19 @@ func (am *authorizationMiddleware) Delete(ctx context.Context, session authn.Ses
session.SuperAdmin = true
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Do PAT Authorization before checkAdmin or UserAuthz

@arvindh123 arvindh123 force-pushed the auth-refactor branch 2 times, most recently from 3f43476 to 4323d34 Compare November 12, 2024 18:21
@nyagamunene nyagamunene force-pushed the AuthorizeUsersThings branch 2 times, most recently from 2e489f4 to 511e918 Compare November 13, 2024 14:06
@dborovcanin dborovcanin force-pushed the auth-refactor branch 2 times, most recently from 9bf293f to 33a88f8 Compare November 25, 2024 10:28
@dborovcanin dborovcanin force-pushed the auth-refactor branch 4 times, most recently from 9d4dce1 to 4daa02b Compare December 2, 2024 10:27
@nyagamunene nyagamunene changed the base branch from auth-refactor to main December 2, 2024 14:17
@dborovcanin dborovcanin changed the title MG-2048 - Authorize things and users with PATs MG-2048 - Authorize Clients and users with PATs Dec 4, 2024
@nyagamunene nyagamunene changed the title MG-2048 - Authorize Clients and users with PATs MG-2048 - Authorize clients and users with PATs Dec 5, 2024
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

Attention: Patch coverage is 0.29499% with 338 lines in your changes missing coverage. Please review.

Project coverage is 41.94%. Comparing base (e95d1bf) to head (2c719f6).

Files with missing lines Patch % Lines
users/middleware/authorization.go 0.00% 185 Missing ⚠️
clients/middleware/authorization.go 0.00% 152 Missing ⚠️
auth/tracing/tracing.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2499      +/-   ##
==========================================
- Coverage   43.38%   41.94%   -1.45%     
==========================================
  Files         398      326      -72     
  Lines       51476    44094    -7382     
==========================================
- Hits        22335    18496    -3839     
+ Misses      26951    24219    -2732     
+ Partials     2190     1379     -811     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nyagamunene nyagamunene force-pushed the AuthorizeUsersThings branch 3 times, most recently from 07a8787 to 328600e Compare December 6, 2024 16:38
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🚀 Ready for merge
Development

Successfully merging this pull request may close these issues.

4 participants