Scope is optional with authorization code grant

[FStefanni]

Summary

This pr makes the scope optional with the authorization code grant.

Please note that this code differs from the original pr.
This is due to my understanding of the specification (which could be wrong, so please validate).
The cited RFC section states that scope is optional, so, for what I understand:

• If scope is missing: OK, no problem
• If scope is given, then it must be valid

Conversely, the original pr did not check the scope at all, so, if a invalid scope where given, it would have passed successfully (which seems to me incorrect).

Linked issue(s)

Related to #89, point 19, original pr oauthjs/node-oauth2-server#647

@jankapunkt: Other related issues: #84 #79 #71 #64

Added tests?

Yes

OAuth2 standard

RFC 6749 section-4.1.1

[jankapunkt]

@FStefanni this is correct, scope is optional but if it's invalid there should be no other codes be generated, see #64 #84 so please add these two into consideration.

[FStefanni]

Hi,

@jankapunkt
@jorenvandeweyer
@Uzlopak

I have updated the pr, so please check whether I have understood the comments, or something is still missing.
Btw, I have also found and fixed a bug in AbstractGrantType.prototype.validateScope(): it did not return a promise in all cases (but now it does).

Regards

[Uzlopak]

To be honest: I dont like it.

I expected a minimal change like this:

AuthorizationCodeGrantType.prototype.saveToken = function(user, client, authorizationCode, requestedScope) {

  const scope = await this.validateScope(user, client, requestedScope);
  const fns = [
      this.generateAccessToken(client, user, scope),
      this.generateRefreshToken(client, user, scope),
      this.getAccessTokenExpiresAt(),
      this.getRefreshTokenExpiresAt()
    ];

  return Promise.all(fns)
    .bind(this)
    .spread(function(accessToken, refreshToken, accessTokenExpiresAt, refreshTokenExpiresAt) {
      const token = {
        accessToken: accessToken,
        authorizationCode: authorizationCode,
        accessTokenExpiresAt: accessTokenExpiresAt,
        refreshToken: refreshToken,
        refreshTokenExpiresAt: refreshTokenExpiresAt,
        scope: scope
      };

      return promisify(this.model.saveToken, 3).call(this.model, token, client, user);
    });
};

[jorenvandeweyer]

I also think you can just use await like @Uzlopak suggests. @jwerre is already rewriting the code base to use await everywhere else.

[jankapunkt]

I would wait until it's done with the rewrite and currently focus on integrity for the current state of code. I wil review later today

[FStefanni]

Hi,

I have re-written by using await, but now 3 tests fail.
I am not able to fix them... can someone help please?

Regards

[Uzlopak]

I like your change. Good work!

I would suggest to delete the failing tests, but this could be an inconsistency throughout the codebase. We should wait till everywhere is async await and then merge your change.

[jankapunkt]

@FStefanni no worries we will check out what's going on here

[jankapunkt]

@Uzlopak @HappyZombies @jorenvandeweyer should this also be added to 4.2.0? I think it's not really "breaking" so I would add it to the release.

[jorenvandeweyer]

not necessary anymore since the function is async now.