-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some initial comments. Haven't had a chance to test it yet.
If it's ok, I want to hand over the review to @juhoojala.
} | ||
|
||
export class GitAuthScheme { | ||
private readonly _getSigningKey: (arg1: string) => Promise<jwksRsa.Key>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having these underscore functions, I'd prefer it if the name describes what it exactly is. The underscore gives no indication into why the underscore is there or why it's different from a non-underscore version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This comment applies generally.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if there are clearly two versions of a function or a method, let's say a private and a public one, it's not worth the effort to think of two different names. That would also make it less clear if the two methods are related or not. We could come up with another convention, but this is widely used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I guess it's fine for private/public differentiation.
scope: 'openid email', | ||
}); | ||
} | ||
return this.clientLogin(username, password); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is userLogin used and when is clientLogin used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comments to code. The difference is between signed-up regular users and non-interactive clients.
import { sanitizeSubClaim } from './authentication-hapi-plugin'; | ||
import { AccessToken } from './types'; | ||
|
||
interface Auth0LoginResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be Auth0DecodedHash
from the 'auth0-js' package?
src/shared/gitlab-client.ts
Outdated
public async getUsers() { | ||
const users = await this.fetchJson<User[]>( | ||
`users`, | ||
true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this boolean for? Shouldn't the second parameter be an object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing below.
src/shared/gitlab-client.ts
Outdated
const groups = await this.fetchJson<Group[]>(`groups?${qs.stringify({ search })}`, true); | ||
const groups = await this.fetchJson<Group[]>( | ||
`groups?${qs.stringify({ search })}`, | ||
true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here
src/shared/gitlab-client.ts
Outdated
group = await this.fetchJson<Group>(`groups/${groupIdOrPath}?${qs.stringify(sudo)}`, true); | ||
group = await this.fetchJson<Group>( | ||
`groups/${groupIdOrPath}?${qs.stringify(sudo)}`, | ||
true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here
src/shared/gitlab-client.ts
Outdated
project = await this.fetchJson<Project>(`projects/${projectId}?${qs.stringify(sudo)}`, true); | ||
project = await this.fetchJson<Project>( | ||
`projects/${projectId}?${qs.stringify(sudo)}`, | ||
true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And hgere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually pretty surprising. After looking at what's happening here, I found: microsoft/TypeScript#7485. So TS 2.4 would've caught this. Anyway, fixed.
ea3446a
to
30399fa
Compare
Refactor the auth0 configuration parameters
582e1f4
to
dfe498e
Compare
src/config/config-production.ts
Outdated
@@ -142,6 +148,12 @@ const EXTERNAL_BASEURL = env.EXTERNAL_BASEURL || `http://localhost:${PORT}`; | |||
// External baseUrl for git clone urls | |||
const EXTERNAL_GIT_BASEURL = env.EXTERNAL_GIT_BASEURL || `http://localhost:${GITLAB_PORT}`; | |||
|
|||
// External baseUrl for git clone urls | |||
const GIT_VHOST = env.GIT_VHOST || parseUrl(EXTERNAL_GIT_BASEURL).hostname; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference between EXTERNAL_GIT_BASEURL and GIT_VHOST?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed comments
const invalidSecret = secret + '9'; | ||
|
||
it('should be able to get the accessToken with correct credentials', async () => { | ||
// Arrange |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arrange is empty? Maybe remove the titles altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
@@ -0,0 +1,120 @@ | |||
import { expect } from 'chai'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these integration tests or unit tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment
👍 This can now be merged, but we need to check for infra changes. |
Avoid using `import * as`
Add more methods to CharlesClient Allow defining the config as an S3 url
docs/testing.md
Outdated
copy it to the `gitPassword` field of the corresponding team in the `auth0` block | ||
of the configuration file. | ||
is the id of the corresponding Auth0 client. Additionally, create one more new user, which | ||
you don't need add to any group. This will be admin user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...need to add...
...will be the admin...
docs/testing.md
Outdated
is the id of the corresponding Auth0 client. Additionally, create one more new user, which | ||
you don't need add to any group. This will be admin user. | ||
|
||
After setting up these configuration, update the passwords for all users |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After setting this up, update...
store: 'memory', | ||
ttl: 1000, | ||
}) as Cache; | ||
}) as {} as Cache; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
caching
returns its own Cache
type which is different than the Cache
type returned here. Is that by accident? The casting hides this incompatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is by design, but admittedly suboptimal. We have just rewritten the Cache interface with promises.
👍 |
Description
Add a proxy and authentication scheme for git http connections. Allow charles to accept git http requests with Auth0 credentials, authenticate against Auth0, rewrite the authorization header and pass forward to GitLab.
The GitLab account passwords are derived from the usernames with HMAC-SHA1, requiring a secret key. The key can be set with the environment variable
GITLAB_PASSWORD_SECRET
.This PR also includes a new admin endpoint
GET /operations/regenerate-gitlab-passwords
. It lets you set the passwords for all the users in GitLab to the derived ones. This is needed for instance if the password secret changes.Testing locally
First you need to reset the passwords. I suggest doing this by temporarily changing line https://github.com/lucified/minard-backend/blob/git-http-authentication/src/operations/operations-hapi-plugin.ts#L58 to
restarting minard-backend and requesting
The integration test configuration files will also need to be updated by removing all the
gitPassword
definitions. After these steps the integration tests should pass.TODO