auth service added with check scope operation #62

nurali-techie · 2018-11-14T09:40:01Z

Related to fabric8-services/fabric8-env#10

codecov · 2018-11-14T09:46:46Z

Codecov Report

Merging #62 into master will increase coverage by 0.9%.
The diff coverage is 81.48%.

@@            Coverage Diff            @@
##           master      #62     +/-   ##
=========================================
+ Coverage   63.56%   64.46%   +0.9%     
=========================================
  Files          31       32      +1     
  Lines        1386     1410     +24     
=========================================
+ Hits          881      909     +28     
+ Misses        434      427      -7     
- Partials       71       74      +3

Impacted Files	Coverage Δ
auth/token_context.go	`20% <ø> (ø)`
auth/jwk/fetch_keys.go	`56.14% <ø> (ø)`
auth/token.go	`90.5% <100%> (ø)`
goamiddleware/jwt_token_context.go	`80.95% <50%> (ø)`	⬆️
auth/service.go	`83.33% <80.95%> (ø)`
goasupport/forward_signer.go	`75% <0%> (+75%)`	⬆️

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 1fa3279...a4bed3e. Read the comment docs.

service/auth.go

xcoulon · 2018-11-14T09:49:36Z

@nurali-techie @alexeykazakov I see that this repo declares a dependency on fabric8-auth-client using the latest changes from master. Should we keep it as-is or fix a version ?

service/auth.go

dipak-pawar · 2018-11-14T10:11:06Z

I see that this repo declares a dependency on fabric8-auth-client using the latest changes from master. Should we keep it as-is or fix a version ?

Currently, we don't have any way to automatically submit PR to it's consumer if we update any client(in this case auth). Also this is autogenerated code, we can easily and quickly fix it if something break due to change in client. If we see too much pain and broken code due to using master from client, then maybe we can develop automation to submit PR to it's consumer then lock this version. Otherwise we'll have to put extra efforts to update all consumers for single change in client repo.

xcoulon · 2018-11-14T10:43:59Z

@dipak-pawar yes, I agree with you. I was just concerned that we may have unexpected changes brought my some client libs, but at the same time, we should "embrace" those changes and we should rarely have breaking changes in the API anyways (most of the time: new endpoints or new params for existing endpoints)

nurali-techie · 2018-11-14T14:54:16Z

@xcoulon @dipak-pawar one more option as below.

The generate_client_setup() script should take version as param.

(note - here T1, T2 is timeline ..)

T1 - f8-auth call generate_client_setup(1.0.0)
T2 - f8-auth ci.centos job runs and generate_client_setup() generate client in f8-auth-client with release 1.0.0
T3 - f8-common want to use f8-auth-client and it see release 1.0.0 so it add dependency in dep for f8-auth-client:1.0.0
T4 - f8-auth make changes in goa_design but they are compatible (so we will not change version and keep 1.0.0 only when calling generate_client_setup())
T5 - f8-auth ci.centos job runs and generate_client_setup() generate client in f8-auth-client with release 1.0.0 (each time delete existing release branch and create new release with 1.0.0)
T6 - f8-common ci.centos build run and it gets latest changes from f8-auth-client:1.0.0
T7 - f8-auth make changes in goa_design but they are incompatible so we will change version and call generate_client_setup(1.0.1).
T8 - f8-auth ci.centos job runs and generate_client_setup() generate client in f8-auth-client with release 1.0.1

Now, there will be two version for f8-auth-client 1.0.0 and 1.0.1 and f8-common using 1.0.0 will keep running/compiling without any issue. In case f8-common want latest changes it should change version to 1.0.1.

Benefit, f8-common build will not_failed due to incompatible changes in f8-auth goa_design. Also, if there are no incompatible changes in f8-auth goa_design then f8-common will always keep getting latest changes.

alexeykazakov · 2018-11-14T14:56:57Z

service/auth.go

@@ -0,0 +1,67 @@
+package service


I was wondering if we should rename token package to auth package and put this service into that auth package. Would like to keep all auth related stuff under the same package/subpackages.

@nurali-techie @xcoulon et al, any thoughts?

yes, that would be easier if all AuthN/AuthZ stuff belongs to the auth pkg, indeed

I see pkg name token is meaningful for the content it has atm. I am not sure is in future token pkg get something which is unrelated to auth at all .. in that case mixing auth and token would not be right.

In case we want to do this refactoring, I still want to see token as sub-package in itself.

auth -> token (all existing token pkg file) -> service (this new auth.go file but named service.go)

With this input .. I will leave final call for @alexeykazakov

Well.. token is kinda sub area of auth anyway. Can't imagine anything which we would like to keep in token which is unrelated to auth at all right now. But if we have it in the future we can always introduce a new token package outside of auth.
For now I would have auth only.
While I would be fine with having:

auth -> token (all existing token pkg files and jwk subpackage) -> service.go (no need for service subpackage)

But IMO

auth -> (all existing token pkg files and jwk subpackage) service.go (no need for service subpackage)

is even simpler. But I'm fine with both options.

Done in fb124aa

alexeykazakov · 2018-11-14T15:06:29Z

service/auth.go

+	CheckSpaceScope(ctx context.Context, spaceID, requiredScope string) (bool, error)
+}
+
+func NewAuthService(hostURL string) (Auth, error) {


hostURL is confusing. Should be authURL or something like this. Also add a comment to the function with explaining what param is expected.

Done in fb124aa

service/auth.go

alexeykazakov · 2018-11-14T15:27:26Z

service/auth.go

+}
+
+func (d *doer) Do(ctx context.Context, req *http.Request) (*http.Response, error) {
+	token := jwt.ContextJWT(ctx)


Authorization header is not enough. We should also forward Requiest ID if it's present in the context.
Take a look at https://github.com/fabric8-services/fabric8-auth/blob/master/notification/service/notification.go#L82 for example.

Done in ebc712d

service/auth.go

alexeykazakov · 2018-11-14T17:33:19Z

service/auth_test.go

+}
+
+func (s *AuthServiceTestSuite) TestCheckSpaceScope() {
+	s.T().Run("scope_found_true", func(t *testing.T) {


We have to check the request as well. Is the request to the Auth service has all expected headers? Authorization header has expected token? Request ID header is present? URL is expected?
This should be checked for all requests in the test.

Also check all potential responses from Auth: https://github.com/fabric8-services/fabric8-auth/blob/48c0a1b90a0ff1b7cd8621c67acd8c5c13a4cb3b/design/resource.go#L68-L71
Which are: 200, 401, 500, 404 (when the resource ID is unknown)

@alexeykazakov plz check logic code_link for anything other than 200, it throws exception. We already have 401 handle in test test_code_link so I guess no more test needed.

How do you know if throws an error if it's other than 200 if you don't have tests for that? It's not my eyes which you should relay on to check the logic. It should be automated tests.
Please add the tests I'm asking for.
Also please add checks for the requests:

We have to check the request as well. Is the request to the Auth service has all expected headers? Authorization header has expected token? Request ID header is present? URL is expected?
This should be checked for all requests in the test.

Done in ebc712d

alexeykazakov · 2018-11-14T17:34:39Z

service/auth_test.go

+			Reply(401)
+
+		authZ, err := s.authService.CheckSpaceScope(context.Background(), spaceID.String(), "manage")
+		assert.Error(t, err)


Check the error type and message.

Done in fb124aa

alexeykazakov · 2018-11-14T17:50:56Z

The generate_client_setup() script should take version as param.

I don't think it's a good idea to edit releases. If there some changes then we should do another release (or not do it at all) and keep the released versions intact.

If we want to track compatible and incompatible changes in our releases then we should use Semantic Versioning: MAJOR.MINOR.PATCH format. PATCH means compatible changes. Fro example 1.0.1, 1.0.2, etc. The problem here is that in that case we will need to update the version param manually every time we change design package which can be error prone. I wander if goa supports something for API versioning here which we could use to automatically generated versions based on changes...

And btw, let's move this discussion to a better place (a new issue in common)? :)

nurali-techie · 2018-11-15T05:07:11Z

@alexeykazakov @xcoulon @dipak-pawar issue created to track dep entry issue for goa_client_repo #64
Can someone plz give example of incompatible change in goa_design. I have marked same as TODO in issue desc.

xcoulon · 2018-11-15T08:09:40Z

@nurali-techie AFAIK, we only have incremental changes in the design, so we should have 1.0.0, 1.1.0, 1.2.0, etc. I'm not even sure why we would want to use the PATCH part of the semantic version

nurali-techie · 2018-11-15T08:37:19Z

@xcoulon 1.0.1 and 1.0.1 is just place holder for conveying the solution. So plz focus on overall solution and this version/branch style is just a small part to achieve solution. So 1.0.0 and 1.0.1 used to exhibits two different things rather purposing versioning_system ;-)
Plz comment on issue rather this PR - #64

xcoulon · 2018-11-15T09:26:57Z

@xcoulon 1.0.1 and 1.0.1 is just place holder for conveying the solution. So plz focus on overall solution and this version/branch style is just a small part to achieve solution. So 1.0.0 and 1.0.1 used to exhibits two different things rather purposing versioning_system ;-)
Plz comment on issue rather this PR - #64

ok, so taking at step back at your proposal: having tags on the -client and making a release each time something changes would make our lives easier, indeed, but that's only a small part of the whole solution: on the "consumer" (i.e., client service) side, we'd still need to update the dependency. BUT at the same time, we should rarely break the API, so a client service, we may not need to upgrade the client lib dependency so often, unless we need to use a new API or a new param in an existing API for example.

Back to your initial proposal, I would prefer a simple tag the repo with an incremental version (eg: 1.0.0, 1.1.0, .. 1.123.0, etc.) rather than pushing on a branch named 1.0.0, because the actual client version would not be 1.0.0 after the second change. If we really want to follow semantic versioning, then we'd need some tooling (ideally) to detect a breaking change in the API design to switch to 2.0.0, etc. (but I'm not sure if goa provides such tooling out-of-the-box)

@nurali-techie does this answer your question?

nurali-techie · 2018-11-15T09:50:42Z

@xcoulon I really don't want new version every time. It looks we are going away from problem domain. We want to solve the incompatible change should not impact the consumer rather finding what is right versioning_system.
@xcoulon plz add your comment in issue rather here in PR. Here is issue link - #64

alexeykazakov · 2018-11-19T21:06:42Z

service/auth_test.go

+		err := s.authService.RequireScope(context.Background(), resID.String(), "manage")
+		assert.Error(t, err)
+		_, ok := err.(errors.ForbiddenError)
+		assert.True(t, ok, "error is not forbidden error")


You can also copy this https://github.com/fabric8-services/fabric8-auth/blob/master/test/error.go file to common and reuse it like this: https://github.com/fabric8-services/fabric8-auth/blob/d546608eb57ef7a6dfb27ad6e500f8dd212eaef6/authorization/role/service/role_management_service_blackbox_test.go#L55
We will benefit from that Assert helper in other tests too.

alexeykazakov · 2018-11-19T21:09:26Z

service/auth_test.go

 			Reply(401)

-		authZ, err := s.authService.CheckSpaceScope(context.Background(), spaceID.String(), "manage")
+		err := s.authService.RequireScope(context.Background(), resID.String(), "manage")
+		assert.Error(t, err)


Check error type & message. You could explicitly return InternalError in RequireScope instead of error btw for that case.

Done in fb124aa

alexeykazakov · 2018-11-19T21:10:00Z

service/auth_test.go

+			Reply(500)
+
+		err := s.authService.RequireScope(context.Background(), resID.String(), "manage")
+		assert.Error(t, err)


Check error type & message.

Done in fb124aa

sbryzak

Looks quite straight forward, I would just add some more comments in the code describing what steps are happening.

auth/service.go

nurali-techie added 2 commits November 14, 2018 14:33

auth service added with check scope operation

1a8e927

remove fmt print stat

160f543

nurali-techie changed the title ~~auth service added with scope check operation~~ auth service added with check scope operation Nov 14, 2018

nurali-techie requested a review from alexeykazakov November 14, 2018 09:40

xcoulon reviewed Nov 14, 2018

View reviewed changes

service/auth.go Outdated Show resolved Hide resolved

Merge branch 'master' into env_i10

3fa01ff

golangcibot reviewed Nov 14, 2018

View reviewed changes

service/auth.go Outdated Show resolved Hide resolved

test added

fbcf3ed

nurali-techie mentioned this pull request Nov 14, 2018

check user permission in env api fabric8-services/fabric8-env#14

Merged

1 task

alexeykazakov suggested changes Nov 14, 2018

View reviewed changes

nurali-techie mentioned this pull request Nov 15, 2018

dep entry for goa client repo #64

Open

incorporated review comments on method signature and new tests

e1b3d2a

alexeykazakov reviewed Nov 19, 2018

View reviewed changes

alexeykazakov added 2 commits November 19, 2018 19:59

Address PR review comments

fb124aa

Add request ID header

ebc712d

alexeykazakov approved these changes Nov 20, 2018

View reviewed changes

xcoulon requested a review from sbryzak November 20, 2018 06:02

Merge branch 'master' into env_i10

185b18d

sbryzak approved these changes Nov 22, 2018

View reviewed changes

auth/service.go Outdated Show resolved Hide resolved

auth/service.go Outdated Show resolved Hide resolved

auth/service.go Outdated Show resolved Hide resolved

alexeykazakov added 2 commits November 22, 2018 07:38

Address PR comments

1076d91

Cleanup

a4bed3e

alexeykazakov merged commit b10e057 into fabric8-services:master Nov 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auth service added with check scope operation #62

auth service added with check scope operation #62

nurali-techie commented Nov 14, 2018

codecov bot commented Nov 14, 2018 •

edited

Loading

xcoulon commented Nov 14, 2018

dipak-pawar commented Nov 14, 2018

xcoulon commented Nov 14, 2018

nurali-techie commented Nov 14, 2018 •

edited

Loading

alexeykazakov Nov 14, 2018 •

edited by xcoulon

Loading

xcoulon Nov 15, 2018

nurali-techie Nov 19, 2018 •

edited

Loading

alexeykazakov Nov 19, 2018

alexeykazakov Nov 20, 2018

alexeykazakov Nov 14, 2018

alexeykazakov Nov 20, 2018

alexeykazakov Nov 14, 2018

alexeykazakov Nov 20, 2018

alexeykazakov Nov 14, 2018

nurali-techie Nov 16, 2018

alexeykazakov Nov 16, 2018 •

edited

Loading

alexeykazakov Nov 20, 2018

alexeykazakov Nov 14, 2018

alexeykazakov Nov 20, 2018

alexeykazakov commented Nov 14, 2018 •

edited

Loading

nurali-techie commented Nov 15, 2018

xcoulon commented Nov 15, 2018

nurali-techie commented Nov 15, 2018

xcoulon commented Nov 15, 2018

nurali-techie commented Nov 15, 2018

alexeykazakov Nov 19, 2018

alexeykazakov Nov 19, 2018

alexeykazakov Nov 20, 2018

alexeykazakov Nov 19, 2018

alexeykazakov Nov 20, 2018

sbryzak left a comment

auth service added with check scope operation #62

auth service added with check scope operation #62

Conversation

nurali-techie commented Nov 14, 2018

codecov bot commented Nov 14, 2018 • edited Loading

Codecov Report

xcoulon commented Nov 14, 2018

dipak-pawar commented Nov 14, 2018

xcoulon commented Nov 14, 2018

nurali-techie commented Nov 14, 2018 • edited Loading

alexeykazakov Nov 14, 2018 • edited by xcoulon Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nurali-techie Nov 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeykazakov Nov 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeykazakov commented Nov 14, 2018 • edited Loading

nurali-techie commented Nov 15, 2018

xcoulon commented Nov 15, 2018

nurali-techie commented Nov 15, 2018

xcoulon commented Nov 15, 2018

nurali-techie commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbryzak left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 14, 2018 •

edited

Loading

nurali-techie commented Nov 14, 2018 •

edited

Loading

alexeykazakov Nov 14, 2018 •

edited by xcoulon

Loading

nurali-techie Nov 19, 2018 •

edited

Loading

alexeykazakov Nov 16, 2018 •

edited

Loading

alexeykazakov commented Nov 14, 2018 •

edited

Loading