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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: refactor auth action #2746

Closed
wants to merge 7 commits into from
Closed

Conversation

jannyHou
Copy link
Contributor

@jannyHou jannyHou commented Apr 12, 2019

Description

connect to #2467

This PR starts to refactor the authentication action to takes in a getter for the newly created AuthenticationStrategy interface in #2688.

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

@@ -1,5 +1,7 @@
{
"editor.rulers": [80],
"editor.rulers": [
80
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will revert it.


export interface AuthenticationStrategy {
name: string;
isPassportStrategy?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

-1

AFAIK, passport strategies use different API contract than LB4 AuthenticationStrategy. IMO, we should tell people to use StrategyAdapter to convert Passport strategies into LB strategies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bajtos yeah that flag is added at the wrong place, see my comment 53cfeeb#r33223459 .

@jannyHou jannyHou changed the title feat: refactor auth action [WIP]feat: refactor auth action Apr 17, 2019
@jannyHou jannyHou force-pushed the auth-action branch 2 times, most recently from 792a108 to 53cfeeb Compare April 17, 2019 19:57
@jannyHou jannyHou changed the base branch from master to dremond_authentication_2312 April 23, 2019 19:09
@emonddr emonddr force-pushed the dremond_authentication_2312 branch 2 times, most recently from cddb4ea to 6a196cf Compare April 24, 2019 00:32
Resolve authentication strategies registered via extension point
@emonddr emonddr force-pushed the dremond_authentication_2312 branch from 53f74bd to 0c022bb Compare April 24, 2019 20:28
@jannyHou jannyHou force-pushed the auth-action branch 2 times, most recently from b727dff to 92b10a2 Compare April 28, 2019 01:07
emonddr and others added 6 commits April 27, 2019 21:13
Resolve authentication strategies registered via extension point
Add tests for registered jwt authentication strategy. And fixup some copyright comments.
removed duplicate interface definitions
moved acceptance artifacts to fixtures folder
Move authentication action and strategy providers into main code
@emonddr emonddr force-pushed the dremond_authentication_2312 branch 3 times, most recently from 45f1fd4 to bcf041e Compare April 28, 2019 06:05
@jannyHou jannyHou changed the title [WIP]feat: refactor auth action feat: refactor auth action Apr 29, 2019
@emonddr emonddr force-pushed the dremond_authentication_2312 branch from 6096b8b to f016ff5 Compare April 29, 2019 19:12
@emonddr emonddr force-pushed the dremond_authentication_2312 branch 2 times, most recently from d484f7f to 519e0d1 Compare April 30, 2019 16:45
@emonddr emonddr force-pushed the dremond_authentication_2312 branch 3 times, most recently from 42eb051 to 5e2ab05 Compare May 1, 2019 01:27
@jannyHou
Copy link
Contributor Author

jannyHou commented May 1, 2019

Close this one in favor of #2822

@jannyHou jannyHou closed this May 1, 2019
@jannyHou jannyHou deleted the auth-action branch May 30, 2019 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants