Restrict access of authenticated users by smacker · Pull Request #60 · src-d/code-annotation

smacker · 2018-02-06T13:41:02Z

Fixes: #48

bzz

LGTM

\cc @carlosms for review

dpordomingo

I like your implementation!!!

I'd only require some things:

checkAccess does not need to be an OAuth member, #60 (comment)
checkAccess has a too long body that could be extracted in separate functions, #60 (comment)
coding style #60 (comment)

questions:

do we really need to exclude pending users? #60 (comment)

dpordomingo · 2018-02-07T10:57:54Z

+const orgPrefix = "org:"
+const teamPrefix = "team:"
+
+func (o *OAuth) checkAccess(client *http.Client, restriction, login string) error {


does not need access to (o *OAuth), does it?

doesn't, but I prefer to group functions as struct methods.

wow... I think that's an ugly thing (and also forces extra steps in unit tests...) :(

dpordomingo · 2018-02-07T11:08:12Z

+		}
+		return nil
+	}
+	if strings.HasPrefix(restriction, teamPrefix) {


we (at src-d) are separating code blocks with a new line in between.
(examples in gogit)

if we do that, we need linter for it. And please describe which blocks should be separated. From reading the source code of go-git I don't always see the pattern.

Most of our code is always written that way (with NewLine after every code block).
(random files, also in borges... https://github.com/src-d/borges/blob/master/archiver.go https://github.com/src-d/borges/blob/master/common.go)

(we would also need a linter to ensure the opposite; the reason is it's an internal convention we decided to follow...)

The rule could be:

a NewLine after } if it's not followed by another }

dpordomingo · 2018-02-07T11:15:12Z

+		if resp.StatusCode != http.StatusOK {
+			return NoAccessError
+		}
+		// Don't allow access to pending members


Why do we need to forbid "pending" members? If we don't care about that, all code below this comment can be replaced by

return nil

because pending member isn't member

but they was added by the org's owner, what suits our purpose...

dpordomingo · 2018-02-07T11:17:43Z

+
+func (o *OAuth) checkAccess(client *http.Client, restriction, login string) error {
+	if strings.HasPrefix(restriction, orgPrefix) {
+		org := strings.TrimPrefix(restriction, orgPrefix)


too long.
we could extract the body of both ifs to separated functions.

func (o *OAuth) checkAccess(client *http.Client, restriction, login string) error { if strings.HasPrefix(restriction, orgPrefix) { org := strings.TrimPrefix(restriction, orgPrefix) return checkUserInOrganization(client, org, login) } if strings.HasPrefix(restriction, teamPrefix) { team := strings.TrimPrefix(restriction, teamPrefix) return checkUserInTeam(client, team, login) } return return errors.New("invalid restriction") } func checkUserInOrganization(client *http.Client, org string, login string) error {} func checkUserInTeam(client *http.Client, ream string, login string) error {}

doesn't feel too long for me, but ok.

dpordomingo · 2018-02-07T11:20:12Z


+### Restrict access
+
+It's possible to restrict access to the tool or `reviewer` role by setting environmental variables:


Why not add them to the .env.tpl file?

carlosms

Besides the inline comments, there is a big obstacle in my opinion:

Right now we ask the administrator to set a team-id, but it seems that you can't get that ID from the GitHub web interface.

We should do something about it, ideally the code should be able to work with a team name instead of the ID.
I think that using something like team:<org-name>/<team-name> we should be able to use this api call to the the team ID:
https://developer.github.com/v3/orgs/teams/#list-teams

carlosms · 2018-02-07T10:46:47Z

 	"golang.org/x/oauth2/github"
 )

+var NoAccessError = errors.New("access deniend")


Exported without comment

carlosms · 2018-02-07T11:05:17Z

+		if err == NoAccessError {
+			role = model.Worker
+		}
+	}


With the code above, the reviewers are a subgroup of the workers, right?

This is not wrong, but it does not match with what we discussed IRL. We said that for greater flexibility we would allow independent orgs/teams for each role.

What about something like this?

role := model.Requester if o.restrictReviewerAccess != "" { err := o.checkAccess(client, o.restrictReviewerAccess, user.Login) if err != nil && err != NoAccessError { return nil, err } if err == NoAccessError { role = model.Worker } } if role == model.Worker && o.restrictAccess != "" { if err := o.checkAccess(client, o.restrictAccess, user.Login); err != nil { return nil, err } }

carlosms · 2018-02-07T11:07:41Z

+		}
 	}
+
+	role := model.Requester


We are using the reviewer and requester words for the same concept.
Maybe this PR is a good place to fix this.

carlosms · 2018-02-07T11:17:31Z

+		}
+		return nil
+	}
+	return errors.New("invalid restriction")


Since this is an error intended for the admin responsible for the settings, and not a developer, I'd add more details to help:
return errors.New("invalid restriction %s, it must be one of 'org:<organization-name>' or '<team:team-id>'")

carlosms · 2018-02-07T11:39:49Z

+OAUTH_RESTRICT_ACCESS=org:my-organization
+OAUTH_RESTRICT_REVIEWER_ACCESS=team:123456
+```
+


May I suggest something more extended? The following text assumes that Reviewers are not a subgroup of the Workers. It is also worth noting that the current readme does not explain the two different roles.

Access Control

It is possible to restrict access and choose each user's role by adding their GitHub accounts to a specific organization or team.

This is optional, if you don't set any restrictions all users with a valid GitHub account will be able to login as a Reviewer. You may also set a restriction only for Reviewer users, and leave open access to anyone as Workers.

To do so, set the following variables in you .env file:

OAUTH_RESTRICT_ACCESS

OAUTH_RESTRICT_REVIEWER_ACCESS

Both variables accept a string with either org:<organization-name> or team:<team-id>. For example:

OAUTH_RESTRICT_ACCESS=org:my-organization OAUTH_RESTRICT_REVIEWER_ACCESS=team:123456

smacker · 2018-02-07T13:09:41Z

Comments are addressed. About team-id:

Current API for teams isn't stable
Get id is as simple as http://bfy.tw/GTDG

bzz · 2018-02-07T14:41:57Z

Probably a minor thing, but CI seems to be failing, @smacker could you briefly summarize the reason and how is it taken care of?

All reviews seems to be addressed, so @carlosms @dpordomingo could you please make another round?

smacker · 2018-02-07T14:47:08Z

@bzz restart of CI helped. It didn't clean up after itself first. (not sure why)

carlosms · 2018-02-07T15:07:35Z

@smacker:

Comments are addressed. About team-id:

Current API for teams isn't stable

Get id is as simple as http://bfy.tw/GTDG

As amusing your answer may be, the point still stands. We ask our users to provide the team-id without explaining what it is and how to obtain it.

Could you please try to come up with a more constructive proposal?
Or, if there isn't a clean way to manage teams, maybe we should consider it a limitation and remove it, allowing to filter only by organization membership.

smacker · 2018-02-07T15:16:00Z

@carlosms haha. Let me rephrase. I propose to keep it as an internal feature. From requirements, I understood we need teams, not only organizations. And for internal usage, we can grab ID easily. Maybe we need to just remove documentation about it.

The main reason why I don't want to write more code around it and expose it to external users:

Warning: The API may change without advance notice during the preview period. Preview features are not supported for production use. If you experience any issues, contact GitHub support.
https://developer.github.com/v3/orgs/teams/

carlosms · 2018-02-07T15:34:11Z

@smacker

I propose to keep it as an internal feature. From requirements, I understood we need teams, not only organizations. And for internal usage, we can grab ID easily. Maybe we need to just remove documentation about it.

I personally don't like the idea of having undocumented functionality, we either support teams or not. And if it's needed for our internal needs, that's a sign that it may be needed by other users.
But I think this decision should be probably made by @bzz

The main reason why I don't want to write more code around it and expose it to external users:

Warning: The API may change without advance notice during the preview period. Preview features are not supported for production use. If you experience any issues, contact GitHub support.
https://developer.github.com/v3/orgs/teams/

Then maybe we should be using the latest v4 API:
https://developer.github.com/v4/object/team/

smacker · 2018-02-07T15:36:44Z

v4 is GraphQL API, not a REST one.

bzz · 2018-02-07T18:20:12Z

@carlosms thank you for keeping the discussion focused and appreciate the constructive feedback on why teams are not exposed yet. I think it should be OK for now to keep it as is, merge PR and then add nice-to-have integration that uses the name/more docs around it.

@carlosms @dpordomingo could you please make another pass over it as the rest of the feedback seems to be addressed?

carlosms

Approved as requested by @bzz, but please open a new issue (or add a TODO comment in #48) to continue the conversation about team IDs.

smacker · 2018-02-08T14:58:30Z

@carlosms the issue #72
I didn't change README because it's not a priority now.

dpordomingo

This comments could be addressed by #72, so would not block the merge

dpordomingo · 2018-02-08T19:53:29Z

+const orgPrefix = "org:"
+const teamPrefix = "team:"
+
+func (o *OAuth) checkAccess(client *http.Client, restriction, login string) error {


wow... I think that's an ugly thing (and also forces extra steps in unit tests...) :(

dpordomingo · 2018-02-08T20:28:42Z

+		return nil, fmt.Errorf("can't parse github user response: %s", err)
 	}

+	role := model.Requester


I think this code would be easier to read if it was:

if o.restrictRequesterAccess != "" { if err := o.checkAccess(client, o.restrictRequesterAccess, user.Login); err == nil { user.Role = model.Requester return &user, nil } else if err != ErrNoAccess { return nil, err } } if o.restrictAccess != "" { if err := o.checkAccess(client, o.restrictAccess, user.Login); err == nil { user.Role = model.Worker return &user, nil } else { return nil, err } } user.Role = model.Requester return &user, nil

If I'm not wrong, that way it's easier to understand the logic behind the role assignment depending on GitHub groups and the default value if nothing happens.

dpordomingo

Everything is fine but one thing easy to change.
Once it is addressed, feel free to merge 😄

dpordomingo · 2018-02-08T20:38:40Z

 JWT_SIGNING_KEY=testing
 DB_CONNECTION=sqlite:///path/to/db.db
+OAUTH_RESTRICT_ACCESS=
+OAUTH_RESTRICT_REVIEWER_ACCESS=


Reviewer instead of Requesters?
I don't see the reason because this PR introduces a new concept colliding with an already existing one. We decide to call that role requester, so it would be a good idea to stick to our convention,
I'd suggest doing s/reviewer/requester/g before merging.

👍 for consistency.

Let's rename and merge, with subsequent improvements planned in #72

let's try to use correct names everywhere. In the issue description, it's called reviewers.

It might be a misunderstanding when the latest issue was created.
In the schema definition issue and in our code, we're already using requester.

Signed-off-by: Maxim Sukharev <maxim@sourced.tech>

smacker · 2018-02-09T09:57:05Z

pushed rename reviewer -> requester.

dpordomingo

LGTM, many thanks!

smacker requested a review from bzz February 6, 2018 13:41

bzz requested a review from carlosms February 7, 2018 10:20

bzz approved these changes Feb 7, 2018

View reviewed changes

dpordomingo suggested changes Feb 7, 2018

View reviewed changes

carlosms suggested changes Feb 7, 2018

View reviewed changes

smacker force-pushed the access_restritions branch from b413329 to 83a0cf3 Compare February 7, 2018 13:10

smacker force-pushed the access_restritions branch from 83a0cf3 to e0bdc4d Compare February 7, 2018 15:37

dpordomingo mentioned this pull request Feb 7, 2018

Prepare first release of code annotation #70

Closed

16 tasks

carlosms approved these changes Feb 7, 2018

View reviewed changes

smacker force-pushed the access_restritions branch from e0bdc4d to d19af6f Compare February 8, 2018 14:57

dpordomingo reviewed Feb 8, 2018

View reviewed changes

dpordomingo mentioned this pull request Feb 8, 2018

Teams API and ID #72

Open

dpordomingo suggested changes Feb 8, 2018

View reviewed changes

bzz mentioned this pull request Feb 9, 2018

Reviewer: new page on the frontend, showing ML-extracted features #47

Closed

4 tasks

Restrict access of authenticated users

8b3174e

Signed-off-by: Maxim Sukharev <maxim@sourced.tech>

smacker force-pushed the access_restritions branch from d19af6f to 8b3174e Compare February 9, 2018 09:56

carlosms mentioned this pull request Feb 9, 2018

Implement features endpoint #76

Merged

dpordomingo approved these changes Feb 9, 2018

View reviewed changes

smacker merged commit 14d9a1a into src-d:master Feb 9, 2018


		### Restrict access

		It's possible to restrict access to the tool or `reviewer` role by setting environmental variables:

Conversation

smacker commented Feb 6, 2018

Uh oh!

bzz left a comment

Choose a reason for hiding this comment

Uh oh!

dpordomingo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpordomingo Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosms left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Access Control

Uh oh!

smacker commented Feb 7, 2018

Uh oh!

bzz commented Feb 7, 2018

Uh oh!

smacker commented Feb 7, 2018

Uh oh!

carlosms commented Feb 7, 2018

Uh oh!

smacker commented Feb 7, 2018

Uh oh!

carlosms commented Feb 7, 2018

Uh oh!

smacker commented Feb 7, 2018

Uh oh!

bzz commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlosms left a comment

Choose a reason for hiding this comment

Uh oh!

smacker commented Feb 8, 2018

Uh oh!

dpordomingo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpordomingo left a comment

dpordomingo left a comment •

edited

Loading

dpordomingo Feb 8, 2018 •

edited

Loading

bzz commented Feb 7, 2018 •

edited

Loading