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

[#3283] Remove featureflag package #3389

Merged
merged 6 commits into from
Jan 16, 2020
Merged

Conversation

iperdomo
Copy link
Contributor

We want to keep 2 authentication methods for now. ODIC based (Auth0)
in Production, and GAE accounts for local development

We want to keep 2 authentication methods for now. ODIC based (Auth0)
in Production, and GAE accounts for local development
@iperdomo iperdomo requested a review from dlebrero January 14, 2020 04:50
@iperdomo iperdomo self-assigned this Jan 14, 2020
@dlebrero
Copy link
Contributor

Is this a subset of the work to remove the feature flag?

@iperdomo
Copy link
Contributor Author

We're not going to remove the 2 auth methods, so I removed the misleading featureflag package.

@muloem
Copy link
Member

muloem commented Jan 14, 2020

Can you also switch to make the auth0 the default method. Right now it is the alternative entry point?

Copy link
Contributor

@dlebrero dlebrero left a comment

Choose a reason for hiding this comment

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

We should clean up the "showAltAuthButton" and related functionality. There is no need to keep the feature flag that allows both auth methods to work at the same time.

We still want to chose which auth method to use at start up time (for dev vs prod).

Removing this feature will simplify the code as there is no "default" vs "alt" method, but just one and only one auth method.

The org.akvo.flow.rest.security.featureflag.EntryPoint class probably can be deleted.

@iperdomo iperdomo requested a review from dlebrero January 15, 2020 16:44
@iperdomo
Copy link
Contributor Author

I've tested the changes. In development we use GAE accounts, and OIDC in production. - Right now this branch is deployed in akvoflowsandbox

}

@Override
public void commence(HttpServletRequest request, HttpServletResponse response, AuthenticationException authException) throws IOException, ServletException {
if ("auth0".equals(defaultAuthProvider)) {
alternativeEntryPoint.commence(request, response, authException);
if (SystemProperty.environment.value() == SystemProperty.Environment.Value.Production) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Explicit configuration please, so that developers can chose which auth method to use locally.

<custom-filter position="PRE_AUTH_FILTER" ref="gaeFilter"/>
<csrf disabled="true" />
</http>

<http use-expressions="true" entry-point-ref="defaultEntryPoint" authentication-manager-ref="authenticationManager"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can remove the defaultEntryPoint and use Spring's PropertyPlaceholderBlaBlabla... to configure the bean to use here, using as default value the auth0 bean (https://stackoverflow.com/questions/2513484/is-there-a-way-to-specify-a-default-property-value-in-spring-xml).

AFAIK, there is not PropertyPlaceholderBlaBlabla... yet in the Spring config, so it should be added if we opt for this.

* Together with @dlebrero we managed to simplify the code and
  use `System.properties` (defined in `appengine-web.xml`) and
  override the default authentication entrypoint `oidc` with `gae`
@iperdomo iperdomo requested a review from dlebrero January 16, 2020 14:35
@dlebrero dlebrero merged commit ca2e5c5 into develop Jan 16, 2020
@dlebrero dlebrero deleted the issue/3283-feature-flag branch January 16, 2020 14:50
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.

3 participants