WIP: Set creator while WI creation #7

pranavgore09 · 2016-10-12T10:11:33Z

Fixes iss-244
Why:
We need to record who created what (in WI for now). Need to fill up the field 'Fields['system.creator']' by current logged in user's email.

What:
As of now there is no special type which can identify a user in fields. Make use of tokenManager to read/extract tokens.

How:
Add a new UserType, Unmarshall FieldDefinition for UserType.
Move Equaler, Lifecycle to new package to avaoid cyclic import.
Remove some tests on temperory purpose.

ToDo:

add email to the system.creator
add middleware to consume Locate method.
write/update tests which will use login context.

aslakknutsen · 2016-10-12T10:28:35Z

models/workitem_repository.go

@@ -132,6 +134,22 @@ func (r *GormWorkItemRepository) Create(ctx context.Context, typeID string, fiel
 		Type:   typeID,
 		Fields: Fields{},
 	}
+	publicKey, err := token.ParsePublicKey(token.RSAPublicKey)


This can't go here. Most of this logic needs to move to the /workitem.go REST endpoint.

@aslakknutsen what is your objection here, concretely? (i.e why does it need to be moved?)

The Repository should deal with storage of the WI. Not creating a new TokenManager per Create and dealing with Public/Private keys. 'Someone' above this needs to locate the User and set the system.creator field. The Repository shouldn't be querying for the account.User either.

I tried to move it inside controller's create method but facing problem where to put UUID (inside context maybe) so that repositories can access or pass by param.

tsmaeder · 2016-10-12T12:26:32Z

infra/equaler.go

@@ -1,4 +1,4 @@
-package models
+package infra


Golang common lore discourages use of generic words like "utils" or in this case "infra" for packages. What is this package about? If it's about gorm support, let's call it "gormsupport". Go packages are usually smaller than Java packages. Don't hesitate to make many of them instead of lumping too many things in the same package

Yeah, Agreed. "infra" is very generic name. As of now it contains Equaler, DummyEqualer and Lifecycle interfaces. So it is not gorm only specific. Will try to rename more specifics (maybe need to split into 2).

tsmaeder · 2016-10-12T12:33:35Z

models/user_type.go

+	if valueType.Kind() != reflect.String {
+		return nil, fmt.Errorf("value %v should be %s, but is %s", value, "string", valueType.Name())
+	}
+	// check with DB


We should check that it's a valid user id, right? Are we waiting for a user storage to be coded?

Yes waiting for that part (some validation will be done there), also I will add related comments around the code. Thanks.

We don't need to check the user id here. The user id is coming from a signed and verified token produced by us. If we can't trust ourselves, then...

done in 6f7e

tsmaeder · 2016-10-12T12:35:41Z

models/workitem_repository.go

@@ -132,6 +134,22 @@ func (r *GormWorkItemRepository) Create(ctx context.Context, typeID string, fiel
 		Type:   typeID,
 		Fields: Fields{},
 	}
+	publicKey, err := token.ParsePublicKey(token.RSAPublicKey)


@aslakknutsen what is your objection here, concretely? (i.e why does it need to be moved?)

Login entrypoint mounted on /api/login/authorize Contributed-by Konrad Kleine <kwk@users.noreply.github.com> Contributed-by Shoubhik Bose <sbose78@gmail.com> Contributed-by Pranav Gore <pranavgore09@gmail.com> Fixes fabric8-services#20, fabric8-services#219, fabric8-services#321, fabric8-services#325, fabric8-services#327, fabric8-services#328, fabric8-services#337 Related fabric8-services#304

Contributed-by Konrad Kleine <kwk@users.noreply.github.com> Fixes fabric8-services#20, fabric8-services#321, fabric8-services#327, fabric8-services#328 Related fabric8-services#304

Why: We need to record who created what (in WI for now). Need to fill up the field 'Fields['system.creator']' by current logged in user's email. What: As of now there is no special type which can identify a user in fields. Make use of tokenManager to read/extract tokens. How: Add a new UserType, Unmarshall FieldDefinition for UserType. Remove some tests on temperory purpose. Moved Lifecycle to new package gormsupport Moved Equaler and DummyEqualer to groundwork package ToDo: add middleware to consume Locate method. write/update tests which will use login context. Decide about test modifications. Need a way for "login" in tests.

…just convert as the method name say.

…o user_auth0

…en using tokenManager. Few TODOs because of not having logged in user tests yet.

tsmaeder · 2016-10-19T09:00:48Z

extractuser.go

+		return func(ctx context.Context, rw http.ResponseWriter, req *http.Request) error {
+			// for now ignore the error becasue still test for logged in user is not done.
+			uuid, _ := manager.Locate(ctx)
+			ctxWithUser := context.WithValue(ctx, "uuid", uuid.String())


Won't this crash when there is not user?

Why convert uuid to string?

yes, you are right, error is ignored temporarily. And when there is no token found, "Locate" returns uuid.UUID{} which is "000-00...all zeros"

As of now, it is simple to keep string because for remote-work-items creator/user is not created before actual importing. So anything can go in it for now :)

tsmaeder · 2016-10-19T09:04:57Z

models/workitem_repository.go

+	uuid := ctx.Value("uuid")
+	if uuid == nil {
+		// Doing this just for test cases to feel good. Actually error will be returned which is commented below
+		uuid = ""


Sorry to bring this up so late...but wouldn't it be much simpler to pass the creator in as a parameter to the "Create" repository call?

that is very important and it makes a lot sense !! thanks, let me think a bit.
It will make straight forward to add a creator irrespective of calls to create() method via API or direct calls.

aslakknutsen · 2016-10-19T10:43:37Z

@pranavgore09 Could you rebase this of a almighty-core/master as user_auth is now upstream. Also change the target of the PR to almighty-core/master.

pranavgore09 · 2016-10-19T11:37:17Z

roger that !

pranavgore09 · 2016-10-20T17:29:58Z

I tried to align this branch of mine with upstream/master and failed several times after attempting pulling, rebasing, cherry-picking whatever needed in correct order.
Reasons were remote base branch was deleted, the same was squashed, May be I forgot to rebase yesterday. After all attempts either build and tests were failing or comparison in PR was showing 54 files modified :)

After spending good amount of time I decided to create new branch off upstream/master and redo needed changed but this time in neat and better way.

Lessons learnt -

Avoid working off other branches (other than upstream/master, may create issues if others are working, squashing, force pushing on same !)
Keep PR smaller.
Learn git 😅 (to not to create a mess for self)

So I will be creating 2 PRs, one for architectural package level changes and second for actual task related changes.

Happy coding 😃

pranavgore09 mentioned this pull request Oct 12, 2016

WIP: (Set creator while WI creation) #6

Closed

aslakknutsen requested changes Oct 12, 2016

View reviewed changes

tsmaeder reviewed Oct 12, 2016

View reviewed changes

pranavgore09 force-pushed the user_auth0 branch from d4e0d86 to d9058c7 Compare October 13, 2016 09:33

aslakknutsen force-pushed the user_auth branch 5 times, most recently from b04f035 to c5b67d1 Compare October 16, 2016 22:38

aslakknutsen force-pushed the user_auth branch 4 times, most recently from 0feffbc to 52d7ec6 Compare October 17, 2016 13:40

aslakknutsen and others added 4 commits October 18, 2016 10:08

Add Login Service for user auth via GitHub

ff0b319

Contributed-by Konrad Kleine <kwk@users.noreply.github.com> Fixes fabric8-services#20, fabric8-services#321, fabric8-services#327, fabric8-services#328 Related fabric8-services#304

rename "groundwork" package to "compare"

d5e6698

UserType conversion method should not validate against DB. It should …

3919f25

…just convert as the method name say.

pranavgore09 force-pushed the user_auth0 branch from 6f7e51a to 3919f25 Compare October 18, 2016 04:39

Merge branch 'user_auth' of github.com:aslakknutsen/almighty-core int…

7e88e8a

…o user_auth0

pranavgore09 mentioned this pull request Oct 18, 2016

"System Layering" in core. fabric8-services/fabric8-wit#374

Closed

pranavgore09 added 2 commits October 18, 2016 17:17

remove comments

cba148e

Added a middleware to put UUID in the context by reading-locating tok…

6a87012

…en using tokenManager. Few TODOs because of not having logged in user tests yet.

tsmaeder reviewed Oct 19, 2016

View reviewed changes

aslakknutsen closed this Oct 19, 2016

WIP: Set creator while WI creation #7

WIP: Set creator while WI creation #7

pranavgore09 commented Oct 12, 2016 •

edited

aslakknutsen Oct 12, 2016

tsmaeder Oct 12, 2016

aslakknutsen Oct 12, 2016

pranavgore09 Oct 12, 2016

tsmaeder Oct 12, 2016

pranavgore09 Oct 12, 2016

tsmaeder Oct 12, 2016

pranavgore09 Oct 12, 2016 •

edited

aslakknutsen Oct 16, 2016

pranavgore09 Oct 17, 2016

tsmaeder Oct 12, 2016

tsmaeder Oct 19, 2016

pranavgore09 Oct 19, 2016

tsmaeder Oct 19, 2016

pranavgore09 Oct 19, 2016

aslakknutsen commented Oct 19, 2016

pranavgore09 commented Oct 19, 2016

pranavgore09 commented Oct 20, 2016

WIP: Set creator while WI creation #7

WIP: Set creator while WI creation #7

Conversation

pranavgore09 commented Oct 12, 2016 • edited

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

pranavgore09 Oct 12, 2016 • edited

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

aslakknutsen commented Oct 19, 2016

pranavgore09 commented Oct 19, 2016

pranavgore09 commented Oct 20, 2016

pranavgore09 commented Oct 12, 2016 •

edited

pranavgore09 Oct 12, 2016 •

edited