t1msh / node-oauth20-provider Goto Github PK

I think you forgot to add procedure for removing authorization code, while used once, beacuse if you use your auth_code you shoulnt be able to use it again.

I visited http://localhost:3000/authorization

{"error":"invalid_request","error_description":"RedirectUri is mandatory for authorization endpoint"}

In lib/model/client.js you coment the typical client schema with id of _id but in /lib/controller/token/refreshToken:30 you call client.id, with gives us undefined value. On the same line you compare obj.clientId with client.id with !== bit you arent shure that they are both same type, so I recomend you to to at least use toString() method on both objects!

    /**
     * Typical client schema:
     * _id:    { type: "object", required: true, unique: true },
     * name:   { type: "string", required: true },
     * secret: { type: "string", required: true },

But yout want to call it from /lib/controller/token/refreshToken.js:57

Hi,
this is more a question on revocation.
How can the Authorization Server revoke a valid Access Token ?
In my use case I have a third party app using Client Credentials Flow, what if at some point I find the app is malicious and I want to revoke its access?
Thanks

Hi,

I'm not sure that an issue should be opened for this but it's the only place I found that I can describe a problem that I have with this library, which is really nice, by the way.

I feel that the way tokens can be saved is limited because of the way the controller interacts with the (access|refresh)Token models.

Because token generation (generateToken) is separated from token serialization (save), a serialization method which affects the token's representation (like a JWT) can't be used.

Each call to generateToken is followed by a save, I feel that changing this interaction with a single call to a create function just like save without the first parameter, which returns an error or a token which would be then used would support both current serialization method and the one I would like to use.

Something along the lines of:
Original

        // Generate new accessToken and save it
        function(cb) {
            accessTokenValue = oauth2.model.accessToken.generateToken();
            oauth2.model.accessToken.save(accessTokenValue, oauth2.model.code.getUserId(code), oauth2.model.code.getClientId(code), oauth2.model.code.getScope(code), oauth2.model.accessToken.ttl, function(err) {
                if (err)
                    cb(new error.serverError('Failed to call accessToken::save method'));
                else {
                    oauth2.logger.debug('Access token saved: ', accessTokenValue);
                    cb();
                }
            });
        },

Improved version

        // Generate new accessToken
        function(cb) {
            oauth2.model.accessToken.create(accessTokenValue, oauth2.model.code.getUserId(code), oauth2.model.code.getClientId(code), oauth2.model.code.getScope(code), oauth2.model.accessToken.ttl, function(err, token) {
                if (err)
                    cb(new error.serverError('Failed to call accessToken::save method'));
                else {
                    accessTokenValue = token;
                    oauth2.logger.debug('Access token saved: ', accessTokenValue);
                    cb();
                }
            });
        },

What do you think?

The ttl for access tokens is always returned as the default value, rather than being recalculated from the absolute expiration time. This means, for example, that if the refresh_token grant returns an existing bearer token, then the ttl is not accurate.

I have a ruby client hitting my server, and it's using the ttl to schedule it's refresh_token requests (rather than waiting for a request to come back with an expired response for the bearer token, and refreshing then). Because the ttl is not accurate, it's upsetting that client.

I can pull request a simple resolution for this if you consider it valid?

I don't see why a call to toLowercase() should be in the bearer middleware while the token is retrieved, it alters the representation of the token, case matters.

I notice that the latest version of https://github.com/t1msh/node-oauth20-provider/blob/master/lib/controller/token.js allows the clientId, secret etc. to be sent in the body of the request. ( added in commit - 2129b57 ).

The client implementation I am using is sending the these credentials in the body.

I was wondering when this change would be in the NPM package.

Thanks
Paul

Hi,
inside the decision.js file you have hardcoded the grant page. Because I need a custom grant page, I just render an external ejs file using this way:

module.exports = function(req, res, client, scope, user) {

    res.render('grant', {layout: false, user: user, client: client});
};

It would be great to have something like this so that we can make custom grant page without modifying node-oauth20-provider code.

We encountered an issue with how the accessToken structure is stored in the request, which results in details from one request overwriting details of another request.

This comes up specifically when:

1. You're handling more than one request at a time
2. You're referencing the req.oauth2.accessToken structure after a call to an external service.

In our case, we have a rate limiter middleware which references the accessToken, and then we make a HTTP request after that to some external resource. The issue is caused by the fact that req.oauth2 actually is always the same one instance.

req.oauth2 = _self

Here's how it plays out:

1. Two requests come into our app, say they're for bearer tokens A and B.
2. Node handles the token A request, and hits the rate limiter (which calls to redis for example).
3. While waiting for a response from redis, node begins handling token B.
4. At this point, the accessToken structure on the shared req.oauth2 object is changed.
5. The token B request hits the rate limiter.
6. While waiting for a response for the token B request, the token A rate limiter request has returned so node starts working on that again.
7. The token A request now actually contains authentication information from the token B request.
8. We call out to the external resource with the token B credentials for the token A request.

If your API is providing information specific to a user, then potentially sensitive information can be provided to the wrong person.

We found that by making the change in this commit the problem no longer occurs.

We will have a proper PR coming, but in the meantime wanted to ensure this issue is surfaced.

Change 8b3db5b introduced a change that broke existing code. By changing the signature of checkPassword to callback, rather than return values, ALL existing implementations using this library would have broken (including mine!)

The library should have been updated to 0.4.0, not 0.3.5!

To maintain backwards compatibility, the implementation should have returned the checked password response if the callback was not provided.

much better provide database mongodb example than redis because mongodb is known than redis or mysql or postgresql if relational database I hope you can add more example for that for better understanding..

I'm loving this library, but there's only one thing that's stopping me from implementing it. I have user passwords hashed using the bcrypt library. It hashes passwords for me, but it's asynchronous, so I can't use the checkPassword method.

Hi,
When generating a new access token, this module will remove old refresh tokens(https://github.com/t1msh/node-oauth20-provider/blob/master/lib/controller/token/authorizationCode.js#L35). Is it required by the spec? The different refresh tokens may have different scopes, so it makes no sense to revoke the old refresh tokens.

The doc states that you can use a callback in server.get to render the decision page.

server.get('/authorization', isAuthorized, oauth2.controller.authorization, function(req, res) {
    // Render our decision page
    // Look into ./test/server for further information
    res.render('authorization', {layout: false});
});
server.post('/authorization', isAuthorized, oauth2.controller.authorization);

But the decision page always rendered is the one specified in oauth20.decision.
The file lib/controller/authorization states req.ouath.decision(req, res, client, scope, user, redirectUri)

This is an error in the documentation right?

function(err) {
        if (err) response.error(req, res, err, redirectUri);
        else {
            if (req.method == 'GET')
                req.oauth2.decision(req, res, client, scope, user, redirectUri);
            else if (grantType == 'authorization_code')
                authorization.code(req, res, client, scope, user, redirectUri);
            else if (grantType == 'implicit')
                authorization.implicit(req, res, client, scope, user, redirectUri);
            else
                response.error(req, res, new error.invalidRequest('Wrong request method'), redirectUri);
        }
    });

This may or may not be an issue, but I would like to know how can a resource server validate the access token with the Authorization Server? Is there any example/endpoint that I could look at. I am looking for implementation of /introspection or equivalent as per - https://tools.ietf.org/html/rfc7662

Hello, I've noticed that there's a little bug in response.js source at line 30:

redirectUri += query.stringify({

it lacks a question mark, it should be:

redirectUri += "?" + query.stringify({

otherwise when the user press "Cancel" in the grant page, the server returns an error.
Can you please correct it?
Thanks a lot.