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

Fixes saml login flow to work with anonymous auth #1839

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
199683c
Fixes anonymous auth flow to work with SAML
DarshitChanpura Mar 6, 2024
cbdf7a6
Adds hardcoded credentials for anonymous user
DarshitChanpura Mar 11, 2024
a0f2db4
Updates basic auth header to be a config constant
DarshitChanpura Mar 11, 2024
3c0c2f9
Removes unneeded usage of anonymous auth header constant
DarshitChanpura Mar 11, 2024
8322a23
Updates logic to display anonymous auth login button
DarshitChanpura Mar 11, 2024
f331377
Adds test to check whether anonymous auth login button is displayed c…
DarshitChanpura Mar 11, 2024
63abb41
Fixes integrationtests
DarshitChanpura Mar 12, 2024
5492d2a
Adds integration tests for anonymous auth login with basic authorizat…
DarshitChanpura Mar 12, 2024
9d47261
Generates random password for anonymous user
DarshitChanpura Mar 12, 2024
d682701
Fixes lint errors
DarshitChanpura Mar 12, 2024
ca8b4ef
Adds saml auth header to differentiate saml requests
DarshitChanpura Mar 20, 2024
f63bbf2
Fixes linter errors
DarshitChanpura Mar 21, 2024
43c8499
Merge remote-tracking branch 'upstream/main' into fixes-saml-login-flow
DarshitChanpura Mar 21, 2024
573c715
Fixes basic auth tests
DarshitChanpura Mar 21, 2024
b9b9911
Removes console loggers
DarshitChanpura Mar 21, 2024
87ac6e0
Merge remote-tracking branch 'upstream/main' into fixes-saml-login-flow
DarshitChanpura Mar 26, 2024
e0585b9
Merge remote-tracking branch 'upstream/main' into fixes-saml-login-flow
DarshitChanpura Mar 27, 2024
366f413
Fixes lint error
DarshitChanpura Mar 28, 2024
4317901
Addresses feedback
DarshitChanpura Apr 1, 2024
2724e03
Resolves #1840
DarshitChanpura Apr 2, 2024
edcf65a
Merge remote-tracking branch 'upstream/main' into fixes-saml-login-flow
DarshitChanpura Apr 2, 2024
932fd5b
Replace magic value with constant
DarshitChanpura Apr 2, 2024
1f1ecfa
Renames query param and removes unused variables
DarshitChanpura Apr 8, 2024
43c0c9d
Merge remote-tracking branch 'upstream/main' into fixes-saml-login-flow
DarshitChanpura Apr 8, 2024
7b68f1c
Merge remote-tracking branch 'upstream/main' into fixes-saml-login-flow
DarshitChanpura Apr 9, 2024
2a1289d
Uses enum instead of magic constant
DarshitChanpura Apr 9, 2024
35a7f7e
Extracts template function to a separate util file
DarshitChanpura Apr 9, 2024
77608a0
Renames test
DarshitChanpura Apr 9, 2024
d756b6c
Removes unnecessary modifications required to solve this bug
DarshitChanpura Apr 11, 2024
f03d3ab
Fixes import
DarshitChanpura Apr 11, 2024
d58301f
Removes unused param
DarshitChanpura Apr 11, 2024
2819b10
Removes unused method param
DarshitChanpura Apr 11, 2024
1ad7343
Removes incorrect method param
DarshitChanpura Apr 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Removes unnecessary modifications required to solve this bug
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
  • Loading branch information
DarshitChanpura committed Apr 11, 2024
commit d756b6ccdb137a039107b4ecf9a43c31248163a9
10 changes: 4 additions & 6 deletions server/auth/types/authentication_type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import { SecuritySessionCookie } from '../../session/security_cookie';
import { SecurityClient } from '../../backend/opensearch_security_client';
import { resolveTenant, isValidTenant } from '../../multitenancy/tenant_resolver';
import { UnauthenticatedError } from '../../errors';
import { AuthType, GLOBAL_TENANT_SYMBOL } from '../../../common';
import { GLOBAL_TENANT_SYMBOL } from '../../../common';

export interface IAuthenticationType {
type: string;
Expand Down Expand Up @@ -119,7 +119,7 @@ export abstract class AuthenticationType implements IAuthenticationType {
try {
const additionalAuthHeader = await this.getAdditionalAuthHeader(request);
Object.assign(authHeaders, additionalAuthHeader);
authInfo = await this.securityClient.authinfo(request, '', additionalAuthHeader);
authInfo = await this.securityClient.authinfo(request, additionalAuthHeader);
cookie = this.getCookie(request, authInfo);

// set tenant from cookie if exist
Expand Down Expand Up @@ -210,8 +210,7 @@ export abstract class AuthenticationType implements IAuthenticationType {
}
}
if (!authInfo) {
const authRequestType = cookie.isAnonymousAuth ? AuthType.ANONYMOUS : cookie.authType;
authInfo = await this.securityClient.authinfo(request, authRequestType, authHeaders);
authInfo = await this.securityClient.authinfo(request, authHeaders);
}
authState.authInfo = authInfo;

Expand Down Expand Up @@ -244,10 +243,9 @@ export abstract class AuthenticationType implements IAuthenticationType {
authHeader: any,
authInfo: any
): Promise<string | undefined> {
const authType = cookie.isAnonymousAuth ? AuthType.ANONYMOUS : cookie.authType;
if (!authInfo) {
try {
authInfo = await this.securityClient.authinfo(request, authType, authHeader);
authInfo = await this.securityClient.authinfo(request, authHeader);
} catch (error: any) {
throw new UnauthenticatedError(error);
}
Expand Down
29 changes: 21 additions & 8 deletions server/auth/types/basic/basic_auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,12 @@ import { BasicAuthRoutes } from './routes';
import { AuthenticationType } from '../authentication_type';
import { LOGIN_PAGE_URI } from '../../../../common';
import { composeNextUrlQueryParam } from '../../../utils/next_url';
import { AUTH_HEADER_NAME, AuthType, OPENDISTRO_SECURITY_ANONYMOUS } from '../../../../common';
import {
ANONYMOUS_AUTH_LOGIN,
AUTH_HEADER_NAME,
AuthType,
OPENDISTRO_SECURITY_ANONYMOUS,
} from '../../../../common';

export class BasicAuthentication extends AuthenticationType {
public readonly type: string = AuthType.BASIC;
Expand Down Expand Up @@ -111,13 +116,21 @@ export class BasicAuthentication extends AuthenticationType {
request,
this.coreSetup.http.basePath.serverBasePath
);

const redirectLocation = `${this.coreSetup.http.basePath.serverBasePath}${LOGIN_PAGE_URI}?${nextUrlParam}`;
return response.redirected({
headers: {
location: `${redirectLocation}`,
},
});
if (this.config.auth.anonymous_auth_enabled) {
const redirectLocation = `${this.coreSetup.http.basePath.serverBasePath}${ANONYMOUS_AUTH_LOGIN}?${nextUrlParam}`;
return response.redirected({
headers: {
location: `${redirectLocation}`,
},
});
} else {
const redirectLocation = `${this.coreSetup.http.basePath.serverBasePath}${LOGIN_PAGE_URI}?${nextUrlParam}`;
return response.redirected({
headers: {
location: `${redirectLocation}`,
},
});
}
} else {
return response.unauthorized({
body: `Authentication required`,
Expand Down
6 changes: 1 addition & 5 deletions server/auth/types/basic/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,11 +186,7 @@ export class BasicAuthRoutes {
}
context.security_plugin.logger.info('The Redirect Path is ' + redirectUrl);
try {
user = await this.securityClient.authenticateWithHeaders(
request,
AuthType.ANONYMOUS,
{}
);
user = await this.securityClient.authenticateWithHeaders(request, {});
} catch (error) {
context.security_plugin.logger.error(
`Failed authentication: ${error}. Redirecting to Login Page`
Expand Down
3 changes: 1 addition & 2 deletions server/auth/types/openid/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,7 @@ export class OpenIdAuthRoutes {
const user = await this.securityClient.authenticateWithHeader(
request,
this.openIdAuthConfig.authHeaderName as string,
`Bearer ${tokenResponse.idToken}`,
AuthType.OPEN_ID
`Bearer ${tokenResponse.idToken}`
);

// set to cookie
Expand Down
3 changes: 1 addition & 2 deletions server/auth/types/saml/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,7 @@ export class SamlAuthRoutes {
const user = await this.securityClient.authenticateWithHeader(
request,
'authorization',
credentials.authorization,
AuthType.SAML
credentials.authorization
);

let expiryTime = Date.now() + this.config.session.ttl;
Expand Down
11 changes: 1 addition & 10 deletions server/backend/opensearch_security_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ export class SecurityClient {
request: OpenSearchDashboardsRequest,
headerName: string,
headerValue: string,
authRequestType: string,
whitelistedHeadersAndValues: any = {},
additionalAuthHeaders: any = {}
): Promise<User> {
Expand All @@ -72,7 +71,6 @@ export class SecurityClient {
const esResponse = await this.esClient
.asScoped(request)
.callAsCurrentUser('opensearch_security.authinfo', {
[AUTH_TYPE_PARAM]: authRequestType,
headers,
});
return {
Expand All @@ -90,14 +88,12 @@ export class SecurityClient {

public async authenticateWithHeaders(
request: OpenSearchDashboardsRequest,
authRequestType: string,
additionalAuthHeaders: any = {}
): Promise<User> {
try {
const esResponse = await this.esClient
.asScoped(request)
.callAsCurrentUser('opensearch_security.authinfo', {
[AUTH_TYPE_PARAM]: authRequestType,
headers: additionalAuthHeaders,
});
return {
Expand All @@ -112,16 +108,11 @@ export class SecurityClient {
}
}

public async authinfo(
request: OpenSearchDashboardsRequest,
authRequestType: string = '',
headers: any = {}
) {
public async authinfo(request: OpenSearchDashboardsRequest, headers: any = {}) {
try {
return await this.esClient
.asScoped(request)
.callAsCurrentUser('opensearch_security.authinfo', {
[AUTH_TYPE_PARAM]: authRequestType,
headers,
});
} catch (error: any) {
Expand Down
12 changes: 1 addition & 11 deletions server/backend/opensearch_security_plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@
* permissions and limitations under the License.
*/

import { AUTH_TYPE_PARAM } from '../../common';
import { addQueryParamsToURLIfAny } from './utils';

// eslint-disable-next-line import/no-default-export
export default function (Client: any, config: any, components: any) {
const ca = components.clientAction.factory;
Expand All @@ -29,14 +26,7 @@ export default function (Client: any, config: any, components: any) {
*/
Client.prototype.opensearch_security.prototype.authinfo = ca({
url: {
fmt: `/_plugins/_security/authinfo`,
opt: {
[AUTH_TYPE_PARAM]: {
type: 'string',
required: false,
},
},
template: (params) => addQueryParamsToURLIfAny(params, '/_plugins/_security/authinfo'),
fmt: '/_plugins/_security/authinfo',
},
});

Expand Down
29 changes: 0 additions & 29 deletions server/backend/test/utils.test.ts

This file was deleted.

27 changes: 0 additions & 27 deletions server/backend/utils.ts

This file was deleted.

8 changes: 1 addition & 7 deletions server/readonly/readonly_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import {
isPrivateTenant,
LOGIN_PAGE_URI,
CUSTOM_ERROR_PAGE_URI,
AuthType,
} from '../../common';
import { SecurityClient } from '../backend/opensearch_security_client';
import { IAuthenticationType, OpenSearchAuthInfo } from '../auth/types/authentication_type';
Expand Down Expand Up @@ -105,12 +104,7 @@ export class ReadonlyService extends BaseReadonlyService {
return false;
}

const authType = cookie
? cookie.isAnonymousAuth
? AuthType.ANONYMOUS
: cookie.authType
: '';
const authInfo = await this.securityClient.authinfo(request, authType, headers);
const authInfo = await this.securityClient.authinfo(request, headers);

if (!authInfo.user_requested_tenant && cookie) {
authInfo.user_requested_tenant = cookie.tenant;
Expand Down
17 changes: 13 additions & 4 deletions test/jest_integration/basic_auth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -227,11 +227,16 @@ describe('start OpenSearch Dashboards server', () => {
.unset(AUTHORIZATION_HEADER_NAME);

expect(response.status).toEqual(302);
expect(response.header.location).toEqual('/app/login?nextUrl=%2Fapp%2Fhome');
expect(response.header.location).toEqual('/auth/anonymous?nextUrl=%2Fapp%2Fhome');

const response2 = await osdTestServer.request.get(root, response.header.location);

expect(response2.status).toEqual(200);
expect(response2.status).toEqual(302);
expect(response2.header.location).toEqual('/app/login?nextUrl=%2Fapp%2Fhome');

const response3 = await osdTestServer.request.get(root, response2.header.location);

expect(response3.status).toEqual(200);
});

it('redirect for home follows login for anonymous auth disabled', async () => {
Expand Down Expand Up @@ -260,10 +265,14 @@ describe('start OpenSearch Dashboards server', () => {
.unset(AUTHORIZATION_HEADER_NAME);

expect(response.status).toEqual(302);
expect(response.header.location).toEqual(expectedPath);

const response2 = await osdTestServer.request.get(root, response.header.location);

expect(response2.status).toEqual(200);
expect(response2.status).toEqual(302);
expect(response2.header.location).toEqual(expectedPath);

const response3 = await osdTestServer.request.get(root, response2.header.location);

expect(response3.status).toEqual(200);
});
});
Loading