-
Notifications
You must be signed in to change notification settings - Fork 27
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add oauth2 authentication method #53
Conversation
lib/ovh.js
Outdated
try { | ||
this.accessToken = await this.oauthClient.getToken({scope: "all"}); | ||
} catch (e) { | ||
return callback('[OVH] failed to retrieveOAuth2 Access Token'); |
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.
return callback('[OVH] failed to retrieveOAuth2 Access Token'); | |
return callback('[OVH] failed to retrieve OAuth2 Access Token'); |
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.
Would it be possible to extract some useful data to output?
It seems to contain something like that:
output: {
statusCode: 401,
payload: {
statusCode: 401,
error: 'Unauthorized',
message: 'Response Error: 401 null'
},
headers: {}
}
tests/06_REST_oauth2.js
Outdated
|
||
var rest = ovh(oauthConfig); | ||
rest.accessToken = { | ||
expired: function() { return 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.
I'm not sure of what this is testing, expired or not, a unique call will always fetch a token.
I would write something like that, in my own classy JS style OFC
'GET /me with expired token': function (done) {
'use strict';
this.timeout(5000);
// First call, expires soon
nock('https://www.ovh.com/auth')
.post('/oauth2/token')
.reply(200, {
'access_token': 'TOKEN1',
'token_type': 'Bearer',
'expires_in': 10,
'scope': 'all'
});
// Refresh call
nock('https://www.ovh.com/auth')
.post('/oauth2/token')
.reply(200, {
'access_token': 'TOKEN2',
'token_type': 'Bearer',
'expires_in': 3599,
'scope': 'all'
});
// Called once with TOKEN1
nock('https://eu.api.ovh.com', {
reqheaders: {
authorization: 'Bearer TOKEN1',
},
})
.get("/1.0/me")
.reply(200, {"status": "ok1"});
// Called again with TOKEN2, after the refresh
nock('https://eu.api.ovh.com', {
reqheaders: {
authorization: 'Bearer TOKEN2',
},
})
.get("/1.0/me")
.reply(200, {"status": "ok2"});
var rest = ovh(oauthConfig);
rest.request('GET', '/me', function (err, msg) {
assert.ok(!err);
assert.equal(msg.status, 'ok1');
const sleep = ms => new Promise(r => setTimeout(r, ms));
sleep(2000);
rest.request('GET', '/me', function (err, msg) {
assert.ok(!err);
assert.equal(msg.status, 'ok2');
done();
});
});
},
tests/06_REST_oauth2.js
Outdated
'expires_in': 3599, | ||
'scope': 'all' | ||
}); | ||
nock('https://eu.api.ovh.com') |
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.
Should test for reqheaders: {authorization: 'Bearer ok'}
lib/ovh.js
Outdated
@@ -367,7 +403,19 @@ class Ovh { | |||
} | |||
} | |||
|
|||
if (path.indexOf('/auth') < 0) { | |||
if (this.oauthConfig && (!this.accessToken || this.accessToken.expired())) { |
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.
if (this.oauthConfig && (!this.accessToken || this.accessToken.expired())) { | |
if (this.oauthConfig && (!this.accessToken || this.accessToken.expired(10))) { |
Since the lib allows it, adding a time buffer might prevents some race condition with expired tokens.
go-ovh has a 10 seconds one, suggesting the same.
510922a
to
3f48a18
Compare
Signed-off-by: Marie JONES <[email protected]>
No description provided.