Review#1: return 403 instead of 404 if current license does not suppo…

…rt access agreement, minor test fixes.
elastic · Apr 28, 2020 · 3c7cf49 · 3c7cf49
1 parent 99332b2
commit 3c7cf49
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 14 deletions.
diff --git a/x-pack/plugins/security/common/licensing/license_service.test.ts b/x-pack/plugins/security/common/licensing/license_service.test.ts
@@ -98,7 +98,7 @@ describe('license features', function() {
     }
   });
 
-  it('should show login page and other security elements, allow RBAC but forbid role mappings, access agreement, DLS, and sub-feature privileges if license is basic.', () => {
+  it('should show login page and other security elements, allow RBAC but forbid paid features if license is basic.', () => {
     const mockRawLicense = licensingMock.createLicense({
       features: { security: { isEnabled: true, isAvailable: true } },
     });

diff --git a/x-pack/plugins/security/server/routes/authentication/common.test.ts b/x-pack/plugins/security/server/routes/authentication/common.test.ts
@@ -458,16 +458,16 @@ describe('Common authentication routes', () => {
       expect(routeConfig.validate).toBe(false);
     });
 
-    it('returns 404 if current license doesnt allow access agreement acknowledgement.', async () => {
+    it(`returns 403 if current license doesn't allow access agreement acknowledgement.`, async () => {
       license.getFeatures.mockReturnValue({
         allowAccessAgreement: false,
       } as SecurityLicenseFeatures);
 
       const request = httpServerMock.createKibanaRequest();
       await expect(routeHandler(mockContext, request, kibanaResponseFactory)).resolves.toEqual({
-        status: 404,
-        payload: 'Not Found',
-        options: {},
+        status: 403,
+        payload: { message: `Current license doesn't support access agreement.` },
+        options: { body: { message: `Current license doesn't support access agreement.` } },
       });
     });
 

diff --git a/x-pack/plugins/security/server/routes/authentication/common.ts b/x-pack/plugins/security/server/routes/authentication/common.ts
@@ -147,8 +147,10 @@ export function defineCommonRoutes({
     createLicensedRouteHandler(async (context, request, response) => {
       // If license doesn't allow access agreement we shouldn't handle request.
       if (!license.getFeatures().allowAccessAgreement) {
-        logger.warn(`Attempted to acknowledge access agreement when license doesn allow it.`);
-        return response.notFound();
+        logger.warn(`Attempted to acknowledge access agreement when license doesn't allow it.`);
+        return response.forbidden({
+          body: { message: `Current license doesn't support access agreement.` },
+        });
       }
 
       try {

diff --git a/x-pack/plugins/security/server/routes/views/access_agreement.test.ts b/x-pack/plugins/security/server/routes/views/access_agreement.test.ts
@@ -78,7 +78,7 @@ describe('Access agreement view routes', () => {
       await routeHandler(mockContext, request, responseFactory);
 
       expect(responseFactory.renderCoreApp).not.toHaveBeenCalledWith();
-      expect(responseFactory.notFound).toHaveBeenCalledTimes(1);
+      expect(responseFactory.forbidden).toHaveBeenCalledTimes(1);
     });
 
     it('renders view.', async () => {
@@ -108,17 +108,17 @@ describe('Access agreement view routes', () => {
       expect(routeConfig.validate).toBe(false);
     });
 
-    it('returns `Not Found` if current license does not allow access agreement.', async () => {
+    it('returns `403` if current license does not allow access agreement.', async () => {
       const request = httpServerMock.createKibanaRequest();
 
       license.getFeatures.mockReturnValue({
         allowAccessAgreement: false,
       } as SecurityLicenseFeatures);
 
       await expect(routeHandler(mockContext, request, kibanaResponseFactory)).resolves.toEqual({
-        status: 404,
-        payload: 'Not Found',
-        options: {},
+        status: 403,
+        payload: { message: `Current license doesn't support access agreement.` },
+        options: { body: { message: `Current license doesn't support access agreement.` } },
       });
     });
 

diff --git a/x-pack/plugins/security/server/routes/views/access_agreement.ts b/x-pack/plugins/security/server/routes/views/access_agreement.ts
@@ -25,15 +25,21 @@ export function defineAccessAgreementRoutes({
   httpResources.register(
     { path: '/security/access_agreement', validate: false },
     createLicensedRouteHandler(async (context, request, response) =>
-      canHandleRequest() ? response.renderCoreApp() : response.notFound()
+      canHandleRequest()
+        ? response.renderCoreApp()
+        : response.forbidden({
+            body: { message: `Current license doesn't support access agreement.` },
+          })
     )
   );
 
   router.get(
     { path: '/internal/security/access_agreement/state', validate: false },
     createLicensedRouteHandler(async (context, request, response) => {
       if (!canHandleRequest()) {
-        return response.notFound();
+        return response.forbidden({
+          body: { message: `Current license doesn't support access agreement.` },
+        });
       }
 
       // It's not guaranteed that we'll have session for the authenticated user (e.g. when user is