Skip to content
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

Fix authorization, module expose and extend profile #3

Merged
merged 4 commits into from
Nov 30, 2022

Conversation

pwirth
Copy link
Contributor

@pwirth pwirth commented Nov 28, 2022

No description provided.

@florianmartens
Copy link
Owner

Thank you for opening a PR 👍🏽. Will look at it later today.

@@ -29,6 +29,10 @@ function Strategy(options, verify) {
"offline.access", // required for refresh tokens to be issued
"users.read",
];
options.skipExtendedProfile = options.skipExtendedProfile || false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to find some Passport strategies that use this approach (having the option to extend the profile) to stay idiomatic with the framework. Can you point to a couple other libraries that implement this? Personally, I think users should just perform a seperate lookup for these values after a user has authenticated themself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exports = module.exports = Strategy;

// Exports.
exports.Strategy = Strategy;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the advantage here? This library already exports the Strategy for the use with e.g. NestJs.

Using NestJs, you can already use the Strategy like this:

import Strategy from 'passport-twitter-oauth2.0';
import { PassportStrategy } from '@nestjs/passport';


export default class TwitterStrategy extends PassportStrategy(
    Strategy,
    'twitter',
) {
 // -- snip --  
}

Copy link
Contributor Author

@pwirth pwirth Nov 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since i changed from passport-twitter to this library, it was not working as i expected, so i changed it to work similar to that other library

import { Strategy, ... } from "passport-twitter-oauth2.0";
@Injectable()
export class TwitterStrategy extends PassportStrategy(Strategy, "twitter") {
    constructor(
        private readonly configService: ConfigService,
        private readonly userService: UserService,
        private readonly socialProfileService: SocialProfileService,
    ) {
        super({
            clientID: configService.getTwitterOAuth2ClientID(),
            clientSecret: configService.getTwitterOAuth2ClientSecret(),
            callbackURL: configService.getTwitterCallbackUrl(),
            clientType: "private",
            pkce: true,
            state: true,
            scope: ["offline.access", "users.read", "tweet.read", "follows.read", "like.read"],
        });
    }
}

@florianmartens florianmartens merged commit 67bc196 into florianmartens:main Nov 30, 2022
@florianmartens
Copy link
Owner

Thank you for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants