Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .env.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@ OAUTH_CLIENT_ID=
OAUTH_CLIENT_SECRET=
JWT_SIGNING_KEY=testing
DB_CONNECTION=sqlite:///path/to/db.db
OAUTH_RESTRICT_ACCESS=
OAUTH_RESTRICT_REQUESTER_ACCESS=
18 changes: 18 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,24 @@ In this case, origin will be the internal database, and destination the new data

The annotations made by the users will be stored in the **`assignments`** table.

## Access control

It is possible to restrict access and choose each user's role by adding their GitHub accounts to a specific [organization](https://help.github.com/articles/collaborating-with-groups-in-organizations/) or [team](https://help.github.com/articles/organizing-members-into-teams/).

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

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

* `OAUTH_RESTRICT_ACCESS`
* `OAUTH_RESTRICT_REQUESTER_ACCESS`

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

```bash
OAUTH_RESTRICT_ACCESS=org:my-organization
OAUTH_RESTRICT_REQUESTER_ACCESS=team:123456
```

## Contributing

[Contributions](https://github.com/src-d/code-annotation/issues) are more than welcome, if you are interested please take a look to
Expand Down
5 changes: 4 additions & 1 deletion cli/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,10 @@ func main() {
// create services
var oauthConfig service.OAuthConfig
envconfig.MustProcess("oauth", &oauthConfig)
oauth := service.NewOAuth(oauthConfig.ClientID, oauthConfig.ClientSecret)
oauth := service.NewOAuth(
oauthConfig.ClientID, oauthConfig.ClientSecret,
oauthConfig.RestrictAccess, oauthConfig.RestrictRequesterAccess,
)

var jwtConfig service.JWTConfig
envconfig.MustProcess("jwt", &jwtConfig)
Expand Down
7 changes: 6 additions & 1 deletion server/handler/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ func OAuthCallback(

code := r.FormValue("code")
ghUser, err := oAuth.GetUser(r.Context(), code)
if err == service.ErrNoAccess {
http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden)
return
}
if err != nil {
logger.Errorf("oauth get user error: %s", err)
// FIXME can it be not server error? for wrong code
Expand All @@ -55,7 +59,8 @@ func OAuthCallback(
Login: ghUser.Login,
Username: ghUser.Username,
AvatarURL: ghUser.AvatarURL,
Role: model.Requester}
Role: ghUser.Role,
}

err = userRepo.Create(user)
if err != nil {
Expand Down
131 changes: 118 additions & 13 deletions server/service/oauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,46 +5,59 @@ import (
"crypto/rand"
"encoding/base64"
"encoding/json"
"errors"
"fmt"
"net/http"
"strings"

"github.com/gorilla/sessions"
"github.com/src-d/code-annotation/server/model"
"golang.org/x/oauth2"
"golang.org/x/oauth2/github"
)

// ErrNoAccess means user doesn't have necessary permissions for a resource
var ErrNoAccess = errors.New("access denied")

// OAuthConfig defines enviroment variables for OAuth
type OAuthConfig struct {
ClientID string `envconfig:"CLIENT_ID" required:"true"`
ClientSecret string `envconfig:"CLIENT_SECRET" required:"true"`
ClientID string `envconfig:"CLIENT_ID" required:"true"`
ClientSecret string `envconfig:"CLIENT_SECRET" required:"true"`
RestrictAccess string `envconfig:"RESTRICT_ACCESS"`
RestrictRequesterAccess string `envconfig:"RESTRICT_REQUESTER_ACCESS"`
}

// OAuth service abstracts OAuth implementation
type OAuth struct {
config *oauth2.Config
store *sessions.CookieStore
config *oauth2.Config
store *sessions.CookieStore
restrictAccess string
restrictRequesterAccess string
}

// NewOAuth return new OAuth service
func NewOAuth(clientID, clientSecret string) *OAuth {
func NewOAuth(clientID, clientSecret, restrictAccess, restrictRequesterAccess string) *OAuth {
config := &oauth2.Config{
ClientID: clientID,
ClientSecret: clientSecret,
Scopes: []string{"read:user"},
Scopes: []string{"read:user", "read:org"},
Endpoint: github.Endpoint,
}
return &OAuth{
config: config,
store: sessions.NewCookieStore([]byte(clientSecret)),
config: config,
store: sessions.NewCookieStore([]byte(clientSecret)),
restrictAccess: restrictAccess,
restrictRequesterAccess: restrictRequesterAccess,
}
}

// GithubUser represents the user response returned by the GitHub auth.
type githubUser struct {
ID int `json:"id"`
Login string `json:"login"`
Username string `json:"name"`
AvatarURL string `json:"avatar_url"`
ID int `json:"id"`
Login string `json:"login"`
Username string `json:"name"`
AvatarURL string `json:"avatar_url"`
Role model.Role `json:"-"`
}

// MakeAuthURL returns string for redirect to provider
Expand All @@ -66,9 +79,11 @@ func (o *OAuth) ValidateState(r *http.Request, state string) error {
if err != nil {
return fmt.Errorf("can't get session: %s", err)
}

if state != session.Values["state"] {
return fmt.Errorf("incorrect state: %s", state)
}

return nil
}

Expand All @@ -78,17 +93,107 @@ func (o *OAuth) GetUser(ctx context.Context, code string) (*githubUser, error) {
if err != nil {
return nil, fmt.Errorf("oauth exchange error: %s", err)
}

client := o.config.Client(ctx, token)
resp, err := client.Get("https://api.github.com/user")
if err != nil {
return nil, fmt.Errorf("can't get user from github: %s", err)
}
defer resp.Body.Close()

var user githubUser
err = json.NewDecoder(resp.Body).Decode(&user)
if err != nil {
return nil, fmt.Errorf("can't parse github response: %s", err)
return nil, fmt.Errorf("can't parse github user response: %s", err)
}

role := model.Requester
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

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

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

user.Role = role

return &user, nil
}

const orgPrefix = "org:"
const teamPrefix = "team:"

func (o *OAuth) checkAccess(client *http.Client, restriction, login string) error {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

if strings.HasPrefix(restriction, orgPrefix) {
org := strings.TrimPrefix(restriction, orgPrefix)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 {}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

return o.checkUserInOrg(client, org, login)
}

if strings.HasPrefix(restriction, teamPrefix) {
team := strings.TrimPrefix(restriction, teamPrefix)
return o.checkUserInTeam(client, team, login)
}

return fmt.Errorf("invalid restriction '%s', it must be one of 'org:<organization-name>' or 'team:<team-id>'", restriction)
}

func (o *OAuth) checkUserInOrg(client *http.Client, org, login string) error {
url := fmt.Sprintf("https://api.github.com/orgs/%s/members/%s", org, login)
resp, err := client.Get(url)
if err != nil {
return fmt.Errorf("can't get user organizations from github: %s", err)
}

// StatusNoContent means user is a member
// https://developer.github.com/v3/orgs/members/#check-membership
if resp.StatusCode != http.StatusNoContent {
return ErrNoAccess
}

return nil
}

func (o *OAuth) checkUserInTeam(client *http.Client, team, login string) error {
url := fmt.Sprintf("https://api.github.com/teams/%s/memberships/%s", team, login)
r, err := http.NewRequest("GET", url, nil)
// The Nested Teams API is currently available for developers to preview only
r.Header.Add("Accept", "application/vnd.github.hellcat-preview+json")
if err != nil {
return fmt.Errorf("can't create team request: %s", err)
}

resp, err := client.Do(r)
if err != nil {
return fmt.Errorf("can't get user organizations from github: %s", err)
}

// Only StatusOK means user is a member
// https://developer.github.com/v3/orgs/teams/#get-team-membership
if resp.StatusCode != http.StatusOK {
return ErrNoAccess
}

// Don't allow access to pending members
defer resp.Body.Close()
var teamResp struct {
State string `json:"state"`
}
if err = json.NewDecoder(resp.Body).Decode(&teamResp); err != nil {
return fmt.Errorf("can't parse github team response: %s", err)
}

if teamResp.State != "active" {
return ErrNoAccess
}

return nil
}