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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GitLab support #219

Merged
merged 42 commits into from Aug 29, 2018

Conversation

Projects
None yet
5 participants
@ntsim

ntsim commented Jul 22, 2018

Fixes #22

This PR primarily adds support for GitLab across Staticman's API 馃殌 馃帀. There is a quite a big surface area to this one @eduardoboucas, so there might be things that you might not be certain about. Let me know if there's anything that needs more work or I haven't quite covered!

This includes the following notable changes:

Staticman API config

  • githubBaseUrl added to allow URL of self-hosted GitHub API to be configured.
  • githubAccessTokenUri added to allow the OAuth access token endpoint of self-hosted GitHub instances to be configured.
  • gitlabToken added to allow configuration of Staticman's personal access token in the GitLab instance.
  • `gitlabBaseUrl added to allow URL of self-hosted GitLab API to be configured.
  • gitlabAccessTokenUri added to allow the OAuth access token endpoint of self-hosted GitLab instances to be configured.
  • The new options all default to the publicly available instances of GitHub/GitLab.

Staticman site config (staticman.yml)

  • gitlabAuth.clientId, gitlabAuth.clientSecret and gitlabAuth.redirectUri added to provide OAuth configuration when a user wishes to authenticate via GitLab.
  • githubAuth.redirectUri added for parity with the new gitlabAuth options.
  • auth.required option added to determine if any authentication is required before the user can submit a comment through Staticman.

/entry endpoint

  • New URL pattern added:
     /v3/entry/:service/:username/:repository/:branch/:property
    
  • Accepts gitlab or github as a service parameter. This will be then used as the target for the consequent API calls.
  • Functionality mostly remains the same, but with Staticman now able to submit Merge Requests (PRs) to GitLab. MRs are set to automatically close the source branch, meaning that it is not necessary to have a webhook to close the branch with GitLab.
  • If auth.required site config option is true, then options[auth-token] and options[auth-type] must be passed through as well.
  • options[auth-token] replaces option[github-token] (as this was too specific to GitHub).
  • options[auth-type] has been added to allow the user to choose which authentication provider should be used i.e. gitlab or github. Any future integrations with other authentication providers would be expected to hook into this.
  • A new User model has been implemented which abstracts the user identity models from GitHub and GitLab. This only exposes common properties such as username, email, etc, hopefully simplifying future code.
  • The v2 API functionality is still implemented for backwards compatiblity. This includes the previous authentication behaviour where options[github-token] is used.

/auth endpoint

  • New URL pattern added:
     /v3/auth/:service/:username/:repository/:branch/:property
    
  • Accepts gitlab or github as a service parameter. This will be then used as the target for the consequent API calls.
  • The site owner will have to have set the correct options in their site config e.g. gitlabAuth.clientId, etc, to enable the user to authenticate with all authentication providers.
  • As part of GitLab's OAuth flow, a redirectUri must be provided (see here). Consequently, this has also been added for GitHub to be consistent.
  • The new User model is exposed here in the response's user property to confine the scope of this model for any future consumers.
  • The v2 API functionality is still implemented for backwards compatiblity (will return the GitHub API's user model in the response).

/connect endpoint

  • URL pattern changed to:
     /v3/connect/:service/:username/:repository
    
  • Remains largely unchanged, however with the addition of a service parameter. This has been added to be consistent and future proof against more Git service providers to connect through.
  • GitLab does not have collaborator invites, so this is currently not required for Staticman to collaborate on a project. The user can just add Staticman immediately.

Other changes

  • Minimum supported NodeJS version have been bumped up to 8.11.3 (the current LTS version).
  • New class syntax has been introduced in places such as the Git service classes.
  • Tests run 4x faster by improving how the test site config is setup before tests. Bugs have also been fixed due to config being modified between tests. Config is now mostly just copied to new objects between tests.
  • Various NPM dependencies have been changed/upgraded, such as:
    • github -> @octokit/rest as the previous package was deprecated.
    • request-promise-native -> request-promise as this interops with the GitLab package's request-promise.
    • node-uuid -> uuid as the previous package was deprecated.

sarasate and others added some commits Jun 5, 2018

Update README.md
Added our site gatsbycentral.com which uses staticman.
Remove import and calls to `logger`. #176
This should remove all of the `logger.info()` calls we used to track down #176.
Refactor GitHub class into GitService superclass
Some public APIs have been modified to expose their required parameters
e.g. `authenticateWithCode(code, clientId, clientSecret)` or renamed
in the case of `writeFileAndSendReview()`.
Create initial GitLab service class
This class does not currently include OAuth authentication code as the
authentication API (particularly for OAuth) for the GitHub service class
also needs to be considered first.
Refactor GitHub API to authenticate on instantiation
This commit changes the GitHub class to require authentication credentials
via OAuth or a Personal Access Token upon instantiation. The previous
API required you to call an authentication method first before calling
any of the other methods (which could potentially cause issues).

As we will require authentication credentials at instantiation, there is
no longer any need to have any authentication methods on the class, with
the exception of the `requestOAuthAccessToken` method. This is a static
method which *could* be refactored out of the class (to be determined later).

Some additional polish in the tests has also been performed.
Update GitHub class to use `@octokit/rest` package
This commit replaces the deprecated `github` package with `@octokit/rest`.
One of the major change is that API response bodies are wrapped under a
`data`key (instead of being returned directly). Consequently, a large
amount of refactoring was required to get the tests passing.

Other changes include:
- `user` parameter in API requests has been replaced by `owner`. This is
  probably to make the semantics more flexible for when it is not a user
  e.g. an organisation.
Use `request-promise` to avoid method name collisions
The new `gitlab` package uses `request-promise` which causes method name
collisions when in the same project as `request-promise-native`. This is
due to both of these packages attempting to attach the various Promise
methods such as `then`, `catch`, etc.

Once one of the `request-promise` implementations has configured `request`
it will proceed to error if another implementation tries to attach the same
methods again. To avoid any trouble, the default `request-promise`
implementation using Bluebird will be used (as it is not possible to set
the Promise implementation to be used by the `gitlab` API).
Hook initial GitLab service into Staticman class and fix failing conf鈥
鈥g tests

This change primarily addresses the Staticman tests. It appears that most
Staticman tests had been ignored by accident via an errant `jest.only`
and were not actually running.

Some of the site config tests were also failing and logging unnecessarily
(this is due to a log statement from #176 still remaining in the codebase).

The site config test helper `getConfig` has also been refactored to remove
the special handling of the `recaptcha.secret`. Upon removal there doesn't
seem to have been any notable problems and tests are passing (after some
cleanup in relevant areas).
Abstract additional API calls into Git service classes
This commit abstracts some of the raw GitHub API calls being made -
`getCurrentUser`, `deleteBranch` and `getReview` into the service class
so that they can be generalised for GitLab as well.

Notably, the `handlePR` controller current has some weird coding around
how it handles the received webhook event from GitHub. Specifically, it
seems that it would be easier to just delete the branch immediately upon
receiving the webhook, rather than retrieving the PR and *then* deleting
the branch. This requires more content before a refactor can be performed.
Fix errors from GitLab requests leaking request/response details
This commit stops credentials e.g. personal access tokens being leaked
by the GitLab service class when an error is thrown. This is due to the
underlying request errors exposing too many details, particularly from
`request-promise`.

To solve this, we just re-wrap errors with a new `ApiError` class which
only exposes minimal details.
Rework GitLab pull/commitFile methods to work properly
This commit fixes the `_pullFile` method which previously did not work
correctly and simplifies the `_commitFile` method's API call.
Update `/auth` endpoint to v3 implementation
This commit adds additional handling for the GitLab OAuth flow. As part
of this we require additional site config options:

- `githubAuth.redirectUri`
- `gitlabAuth.clientId`
- `gitlabAuth.clientSecret`
- `gitlabAuth.redirectUri`

Notably, the `githubAuth` controller has also been renamed to `auth` to
be more generic sounding.

Additional changes include:
- Various OAuth has been refactored and moved around (in general)
- Factory method for instantiating GitLab/GitHub. This is mostly just to
  remove the boilerplate of writing `switch` statements in many places.
Add `User` model to encapsulate user identities
This commit adds a `User` model to properly encapsulate what data should
be exposed from the Staticman API and specifically narrow its scope.

With the addition of the GitLab identity provider, we need to be conscious
that the GitLab user data structure will not be the same as the GitHub
user (or any future identity provider's user model). This could be
problematic as:

1. We could expose private user data unintentionally.
2. Staticman consumers will be able too much access to the internal user
   API meaning they could become too reliant on parts that we do not want
   to expose long term.
Fix v2 support for `/auth` endpoint and fix failing tests
This commit re-adds support for the v2 API's `/auth` endpoint by returning
the raw GitHub user in the response `user` field. This has been removed
accidentally when the API was upgraded to v3.

Various other failing tests have also been fixed and OAuth tests have been
separated into their own file.
Add new authentication handling for processing entries
This commit adds new handling for when entries are processed with the
new `auth.required` config option set to true. Staticman will now check
for an `auth-token` field (instead of a `github-token`) and use this to
validate that the user is indeed authenticated already on either GitHub
or GitLab.

Backwards compatibility with the v2 API has been maintained but it should
be analyzed if this is important as the code is somewhat messy as a result.
Speed up tests by using `cloneDeep` for site config
This commit speeds up the tests by up to ~4x (from ~48s to 12s). The
slowness was due to the high cost of instantiating Convict from the site
config every time a test was run. This is mostly due to all the
validations/coercions that it must perform (such as RSA decryption)
every single time. It is much faster to just clone the site config object.

- `lodash` was used for the `cloneDeep` implementation.
@VincentTam

This comment has been minimized.

VincentTam commented Sep 7, 2018

@ntsim Dispite my big thanks to your clear instrustions, my API server is still giving a 401 error. (All npm test pass though.) I don't know why I'm still stuck at the RSA private key. Here's what I've done.

Missing RSA key

  1. I generated a RSA key using the above openssl genrsa cmd and pasted it into config.development.json.
    screenshot from 2018-09-07 13-55-49

  2. I obtained GitLab OAuth application ID and secret throught my personal ac (also called vincenttam).
    oauth

  3. I encrypted them through /encrypt endpoint in Firefox.

  4. I forked your GitLab test repo and replaced the two lines client* with those from /encrypt in staticman.yml, then I pushed it back to GitLab.

  5. Restart the server and input https://gitlab.com/oauth/authorize?client_id=1f5c806921988e45cd0a9ed364e4396a747809ed35038304ad3f6fd07f7c1004&redirect_uri=http://localhost:8000&response_type=code in the address bar of Firefox.

  6. Sign in GitLab with my personal ac (vincenttam), authorise the app, and obtain the code=....
    code

  7. Sent a GET request to /auth.

     GET /v3/auth/gitlab/vincenttam/test-staticman/master/comments?code=the-auth-code
    

    screenshot from 2018-09-07 13-48-07


Missing parameter ['fields']

Besides, POSTing a comment to /entry results in error 500.

screenshot from 2018-09-07 14-31-01

Provided that I don't know Node.js, to gain a deep insight into the request received, I used the most primitive "printf debug method" under functions requireApiVerison and sendResponse.

This enables me to see that nothing is passed from Postman to my Staticman API instance.

emptybody

@ntsim

This comment has been minimized.

ntsim commented Sep 8, 2018

@VincentTam

In terms of the encryption key issue, I don't know what else it could be. I've tripled checked the steps and it's worked for me each time. I would suggest that you absolutely ensure that the key has been pasted in correctly, perhaps use a text editor to manually replace the line breaks in the private key with the \n characters (that's what I've done). A text editor that allows for column selection is useful for this (Sublime has it).

For the fields issue, you should use a x-www-form-urlencoded body in Postman (you can also use a raw JSON body too, but you would need to adjust the data structure), looks like you're currently using form-data. This is how a form POST request would usually be sent from a site's comment section.

@VincentTam

This comment has been minimized.

VincentTam commented Sep 8, 2018

@ntsim Thank you again for your advices. I've switched to Sublime and Postman so as to copy accurately. After recreating another rsaPrivateKey, I can move one step forward.

What would be the source of GITLAB_CREATING_PR?

screenshot from 2018-09-08 22-24-17

  1. I tested my gitlabToken for my bot ("staticmanlab") on GitLab.
    screenshot from 2018-09-08 23-44-38
  2. I double-checked that this token's scope, which contains api.
    screenshot from 2018-09-08 23-45-42
  3. I checked the member list to ensure that "staticmanlab" is invited as a Guest.
@ntsim

This comment has been minimized.

ntsim commented Sep 8, 2018

@VincentTam sorry, my mistake here. You actually need to add the Staticman user to your project as the 'Developer' role. When I was writing up the instructions I forgot that it would need to raise a PR (which requires that roles).

Once you've done that, it should work 馃槃

@VincentTam

This comment has been minimized.

VincentTam commented Sep 8, 2018

@ntsim Thank you so much for your patience and help! 馃榾 It finally worked on my forked project.

screenshot from 2018-09-09 01-03-29

If I start the project from scratch, do I need to manually create comments folder?

@ntsim

This comment has been minimized.

ntsim commented Sep 10, 2018

@VincentTam no problem 馃槃 glad it's working for you now!

Nope you shouldn't need to create the comments directory. It will get created automatically by the comment (you also can't commit bare directories into Git).

@VincentTam

This comment has been minimized.

VincentTam commented Sep 11, 2018

@ntsim Thanks again for this great PR ! I'm sharing my views on some problems you've raised in previous posts: the storage of OAuth tokens in local browsers and cross-platform authentication.

After this success, I tested the setup steps 1--5 again with authentication: false. I immediately got the rsaPrivateKey error. It seems to me that only authenticated users can post a comment.

Since comments from users of whatever Git service provider are welcomed, this brings up the problem of authentication with different Git service providers that you outlined.

Remembering OpenID, I've found this answer about OpenID vs OAuth on Stack Overflow. During the testing, I confused the terms "authentication" and "authorization".

authorization authentication
purpose grant access without further login identity check, requires login
duration long-term short-term
Stack Overflow's answer OAuth OpenID
supposed target Git* Bot commenters
real-world e.g. border control personnel staff card visitors' passport

However, in the current OAuth + Web App flows, the oauth-token that an "authenticated" commenter POSTs along with his/her message never expires.

IMHO, to achieve authentic authentication, the OpenID flow does a better job.

@ntsim

This comment has been minimized.

ntsim commented Sep 12, 2018

@VincentTam have you set up your staticman.yml correctly? It should be auth.required: false:

  auth:
    required: false

I've had no issue just swapping it to false in my test repo then just POSTing to the /entry endpoint without any auth options in the request.

In terms of the actual authentication mechanism:

I'm a little confused about how OpenID would solve anything here. Implementing the original OpenID 2.0 spec would essentially require setting up another server. There's also the issue that it is marked as obsolete in favour of OpenID Connect.

OpenID Connect itself is just a layer on top of OAuth, which provides authentication as well authorization. In fact, GitLab is actually an OpenID Connect identity provider. In the context of Staticman, I believe this would only have the slight benefit of giving us the option to retrieve the user's profile information in the OpenID Connect way.

We would still need OAuth to allow us to do just about anything on GitLab/GitHub. That said, you are correct that currently the authenticated commenter's OAuth access token does not actually expire. This could potentially pose a security issue if we're not careful with how we handle it.

Fortunately, I think we are 'relatively' covered on this as we are using RSA encryption on the access token. The intention of this is to prevent its misuse outside of the Staticman API. Consequently, some malicious user shouldn't be able to just get their hands on the token and start using it to impersonate the user.

That said, as I've mentioned before, we should probably try to offer recommendations to help users store any authentication information correctly on the client side.

Additionally, whilst RSA is currently secure (AFAIK), it does have some concerns over its long-term viability. I believe Libsodium's encryption primitives are usually preferred by security/crypto experts. Perhaps, something worth thinking about @eduardoboucas?

@VincentTam

This comment has been minimized.

VincentTam commented Sep 12, 2018

@ntsim First and foremost, I apologize for my incorrect claim. Swapping auth.required did work.

Thinking back, I should have confused my fork of your test project with my own one.

Big thanks for testing this and explaining the role OpenID.

Currently, only GitLab users can post authenticated comments to GitLab project. The proposed solution is great, but some work is needed to add support for each OAuth provider.

If OpenID is implemented here, provided that a GitHub/Google user has authenticated him/herself through OpenID, it's possible that he/she can post authenticated comments without a GitLab account?

I thought about this while being unaware of the need of setting up another server---that would be too heavy to deal with.

@ntsim

This comment has been minimized.

ntsim commented Sep 12, 2018

No problem 馃槃

As this PR is a bit incomplete without the ability to select the authentication identity provider independently of the site repository provider, I've gone and added the finishing touches (with respect to this discussion) in a new PR #231. Another one for you @eduardoboucas 馃槈

Once that PR is in, it will be possible for a commenter to authenticate with either GitLab or GitHub (provided that the associated OAuth applications are setup correctly). It should be possible to expand this functionality in the future to encapsulate other providers such as Google, Auth0, etc.

I don't think OpenID and its abstractions are really required at any point. Currently Staticman will try to generalise any profile data from GitHub/GitLab into it's own 'User' model. To me, this seems sufficient, but this could be something we could think about further if required.

@VincentTam

This comment has been minimized.

VincentTam commented Sep 13, 2018

@ntsim Thanks to your sample project and support, I'm happy to publish the first Hugo blog powered by Staticman v3. 馃槂

screenshot_2018-09-13-03-37-31

Once cloned, it remains to configure the two config files. With KaTeX and Markdown support, that's much better than existing solutions (e.g. Disqus) for communicating math.

In fact, I find reCaptcha enough to distinguish my prof's and classmates 馃捈 (who don't have a Git* account) from Bots 馃捇.

VincentTam added a commit to VincentTam/staticman that referenced this pull request Sep 14, 2018

Add a new site running Staticman v3
Thanks to eduardoboucas#219 , I have managed to build a Hugo site with Staticman with _native GitLab support_ .
@VincentTam

This comment has been minimized.

VincentTam commented Sep 14, 2018

@eduardoboucas

Not that I'm aware of. Could you create one?

Thanks to @ntsim 's continual help, I've created one running on a free dyno on Heroku at
https://staticman3.herokuapp.com/ for testing, with the associated GitHub/GitLab bot both named @staticmanlab. The source code (of config.production.json) is available on the deploy branch of VincentTam/staticman@dd1969c.

@staticmanapp staticmanapp referenced this pull request Sep 18, 2018

Merged

New comment #59

VincentTam added a commit to VincentTam/staticman that referenced this pull request Oct 5, 2018

Update README.md
In response to @cambragol's HTTPS requests to [my API instance][1], I've built a demo another demo site to test eduardoboucas#219 with GitHub.

[1]: https://staticman3.herokuapp.com

VincentTam added a commit to VincentTam/blog that referenced this pull request Oct 19, 2018

Fixed incorrect API URL
Please read eduardoboucas/staticman#219 for Staticman v3's endpoint scheme.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment