fixed GetToken API in ACL

[AbhilashBalaji]

implementing fix #8428

[hashicorp-cla]

All committers have signed the CLA.

[AbhilashBalaji]

this is my first PR , do let me know if I've made a mistake :)

[mkeeler]

Thanks for the PR. The code looks perfect but it would be good to add a test in agent/acl_endpoint_test.go

[AbhilashBalaji]

I'll get right to it.

[mkeeler]

Taking a second look I realized that fixing this one endpoint is a partial fix. The total necessary changes to fix the ACL APIs to return 404s where appropriate instead of 403s is:

HTTP Paths	HTTPServer Method	Fix
v1/acl/policy/:id, v1/acl/policy/name/:name	ACLPolicyRead	acl.ErrNotFound to NotFoundError
v1/acl/role/:id, v1/acl/role/name/:name	ACLRoleRead	This is manually setting the HTTP status code but should instead return a NotFoundError
v1/acl/rules/translate (GET)	ACLRulesTranslateLegacyToken	You already fixed this one
v1/acl/token/:id	ACLTokenGet	acl.ErrNotFound to NotFoundError
v1/acl/token/self	ACLTokenSelf	acl.ErrNotFound to NotFoundError
v1/acl/logout	ACLLogout	acl.ErrNotFound to BadRequestError{Reason: "Missing token in the request"}

If you are up for fixing the rest of these it would be greatly appreciated.

[AbhilashBalaji]

I'll go ahead and fix the rest of endpoints , would it be cleaner to add the NotFoundError to acl/errors.go ?

[mkeeler]

@AbhilashBalaji I don't think so. Those are for ACL specific errors and this is just a general HTTP resource not found error. One recommendation I would have is when testing, many of these could be done in a loop with the exact same expected output of a NotFoundError (except for the v1/acl/logout endpoint which expects a BadRequestError. For an example look at this test:

consul/agent/acl_endpoint_test.go

Lines 29 to 77 in bb6737d

	func TestACL_Disabled_Response(t *testing.T) {
	t.Parallel()
	a := NewTestAgent(t, "")
	defer a.Shutdown()
	type testCase struct {
	name string
	fn func(resp http.ResponseWriter, req *http.Request) (interface{}, error)
	}
	tests := []testCase{
	{"ACLBootstrap", a.srv.ACLBootstrap},
	{"ACLReplicationStatus", a.srv.ACLReplicationStatus},
	{"AgentToken", a.srv.AgentToken}, // See TestAgent_Token
	{"ACLRulesTranslate", a.srv.ACLRulesTranslate},
	{"ACLRulesTranslateLegacyToken", a.srv.ACLRulesTranslateLegacyToken},
	{"ACLPolicyList", a.srv.ACLPolicyList},
	{"ACLPolicyCRUD", a.srv.ACLPolicyCRUD},
	{"ACLPolicyCreate", a.srv.ACLPolicyCreate},
	{"ACLTokenList", a.srv.ACLTokenList},
	{"ACLTokenCreate", a.srv.ACLTokenCreate},
	{"ACLTokenSelf", a.srv.ACLTokenSelf},
	{"ACLTokenCRUD", a.srv.ACLTokenCRUD},
	{"ACLRoleList", a.srv.ACLRoleList},
	{"ACLRoleCreate", a.srv.ACLRoleCreate},
	{"ACLRoleCRUD", a.srv.ACLRoleCRUD},
	{"ACLBindingRuleList", a.srv.ACLBindingRuleList},
	{"ACLBindingRuleCreate", a.srv.ACLBindingRuleCreate},
	{"ACLBindingRuleCRUD", a.srv.ACLBindingRuleCRUD},
	{"ACLAuthMethodList", a.srv.ACLAuthMethodList},
	{"ACLAuthMethodCreate", a.srv.ACLAuthMethodCreate},
	{"ACLAuthMethodCRUD", a.srv.ACLAuthMethodCRUD},
	{"ACLLogin", a.srv.ACLLogin},
	{"ACLLogout", a.srv.ACLLogout},
	{"ACLAuthorize", a.srv.ACLAuthorize},
	}
	testrpc.WaitForLeader(t, a.RPC, "dc1")
	for _, tt := range tests {
	t.Run(tt.name, func(t *testing.T) {
	req, _ := http.NewRequest("PUT", "/should/not/care", nil)
	resp := httptest.NewRecorder()
	obj, err := tt.fn(resp, req)
	require.NoError(t, err)
	require.Nil(t, obj)
	require.Equal(t, http.StatusUnauthorized, resp.Code)
	require.Contains(t, resp.Body.String(), "ACL support disabled")
	})
	}
	}

[AbhilashBalaji]

@mkeeler I've integrated the changes and modified tests in other modules to reflect these changes.

[freddygv]

Hi @AbhilashBalaji, thanks for the contribution! I'm just picking this back up and I think a more appropriate location for the fix is here:
https://github.com/hashicorp/consul/blob/master/agent/http.go#L404

acl.ErrNotFound should not be considered forbidden, and should instead be checked in the isNotFound function here:
https://github.com/hashicorp/consul/blob/master/agent/http.go#L421

This way we only change one place and have a reduced risk of handling that code incorrectly going forward.

[dhiaayachi]

Hey @AbhilashBalaji, if you're still interested in this, please reopen it and the team will review it again.