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

extract password auth into a dedicated method #486

Merged
merged 1 commit into from
Oct 17, 2024
Merged

Conversation

cfbao
Copy link
Member

@cfbao cfbao commented Oct 17, 2024

One nit I have about the current OktaAuthenticator class is that it puts DSSO and password auth at different abstraction levels.

This PR extracts password auth into its own dedicated private method, similar to DSSO auth.
The main public method now just shows the simple flow of "try cache -> try DSSO -> try password".

@github-actions github-actions bot added language/csharp size/M A medium-sized PR. labels Oct 17, 2024
@@ -55,67 +55,27 @@ bool bypassBrowserSecurity
}

var orgUrl = GetOrgBaseAddress( org );
var oktaAnonymous = oktaClientFactory.CreateAnonymousClient( orgUrl );
Copy link
Member Author

@cfbao cfbao Oct 17, 2024

Choose a reason for hiding this comment

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

a side benefit of wrapping password auth in a dedicated private method is that it exposes and prevents when we declare/define variables too early, like here

@cfbao cfbao force-pushed the okta-auth-refactor branch from 943a753 to 6e91870 Compare October 17, 2024 20:44
@cfbao cfbao force-pushed the okta-auth-refactor branch from 6e91870 to 3b7df3f Compare October 17, 2024 20:50
@cfbao cfbao marked this pull request as ready for review October 17, 2024 20:51
@cfbao cfbao requested a review from a team as a code owner October 17, 2024 20:51
@cfbao cfbao merged commit f279984 into main Oct 17, 2024
9 checks passed
@cfbao cfbao deleted the okta-auth-refactor branch October 17, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
language/csharp size/M A medium-sized PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants