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.

Sign up for GitHub

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

Jump to bottom

VAULT-6433: Add namespace path to MFA read/list endpoints #16911

Merged
merged 4 commits into from
Aug 29, 2022
Merged

VAULT-6433: Add namespace path to MFA read/list endpoints #16911

merged 4 commits into from
Aug 29, 2022

Conversation

VioletHynes
Copy link
Contributor

namespace_path is now returned:

violet@violet-TX54KFJWXM vault % curl \
     --header "X-Vault-Token: hvs.INkinqiHlLMccLwuOsTB9WTI" \
     --header "X-Vault-Namespace: abc" \
    --request GET \
    'http://127.0.0.1:8200/v1/identity/mfa/method?list=true'
{"request_id":"dac656b6-b86b-9e51-bf47-c78d07acc370","lease_id":"","renewable":false,"lease_duration":0,"data":{"key_info":{"25241e4f-6146-c1fb-9cdd-761e56599e73":{"base_url":"okta.com","id":"25241e4f-6146-c1fb-9cdd-761e56599e73","mount_accessor":"","name":"","namespace_id":"cBQTS","namespace_path":"abc/","org_name":"dev-08217481","type":"okta","username_format":"{{identity.entity.aliases.auth_userpass_3aae7eb0.name}}[email protected]"}},"keys":["25241e4f-6146-c1fb-9cdd-761e56599e73"]},"wrap_info":null,"warnings":null,"auth":null}
violet@violet-TX54KFJWXM vault % vault write -field method_id -namespace=abc identity/mfa/method/okta2 org_name="dev-08217481" api_token="00bQm3U15RXP2Q1tfzBdrndK2aiTjjHqxXkR7cGaqA" base_url="okta.com" username_format="{{identity.entity.aliases.auth_userpass_3aae7eb0.name}}[email protected]" primary_email=true

violet@violet-TX54KFJWXM vault % vault read -namespace=abc identity/mfa/method/25241e4f-6146-c1fb-9cdd-761e56599e73
Key                Value
---                -----
base_url           okta.com
id                 25241e4f-6146-c1fb-9cdd-761e56599e73
mount_accessor     n/a
name               n/a
namespace_id       cBQTS
namespace_path     abc/
org_name           dev-08217481
type               okta
username_format    {{identity.entity.aliases.auth_userpass_3aae7eb0.name}}[email protected]

@VioletHynes VioletHynes assigned mpalmi and hghaf099 and unassigned mpalmi and hghaf099 Aug 26, 2022
@VioletHynes VioletHynes requested review from mpalmi and hghaf099 August 26, 2022 19:31
@VioletHynes VioletHynes marked this pull request as ready for review August 26, 2022 19:31
hghaf099
hghaf099 reviewed Aug 26, 2022
View reviewed changes
vault/login_mfa.go
@@ -1361,6 +1361,10 @@ func (b *LoginMFABackend) mfaLoginEnforcementConfigByNameAndNamespace(name, name
func (b *LoginMFABackend) mfaLoginEnforcementConfigToMap(eConfig *mfa.MFAEnforcementConfig) (map[string]interface{}, error) {
resp := make(map[string]interface{})
resp["name"] = eConfig.Name
ns, err := b.namespacer.NamespaceByID(context.Background(), eConfig.NamespaceID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we handle the error if it is not nil?

Copy link
Contributor Author

@VioletHynes VioletHynes Aug 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not really sure - I don't like failing silently but similarly, the endpoint wouldn't have complained before and would have e.g. returned error, so I thought it best to keep the previous behaviour somewhat consistent (i.e. not introducing ways it could fail now where it wouldn't before).

I'm happy to change to e.g. returning the error if you feel it's appropriate!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this check is done all over the place. My preference would be to do it here as well. But, I don't have a strong feeling about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

Copy link
Contributor

Choose a reason for hiding this comment

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

How do you feel about changing this logic to the following instead?

ns, err := b.namespacer.NamespaceByID(context.Background(), eConfig.NamespaceID)
if ns == nil || err != nil {
 return nil, err
}

resp["namespace_path"] = ns.Path
...

It's a bit more concise, but the same logic, so feel free to ignore :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, changed!

vault/login_mfa.go Outdated
@@ -1417,6 +1421,10 @@ func (b *MFABackend) mfaConfigToMap(mConfig *mfa.Config) (map[string]interface{}
respData["id"] = mConfig.ID
respData["name"] = mConfig.Name
respData["namespace_id"] = mConfig.NamespaceID
ns, err := b.namespacer.NamespaceByID(context.Background(), mConfig.NamespaceID)
if ns != nil && err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

The same comment about error handling here

@hghaf099
Copy link
Contributor

Looks good. Just a minor comment about error handling.

mpalmi
mpalmi approved these changes Aug 26, 2022
View reviewed changes
Copy link
Contributor

@mpalmi mpalmi left a comment

Choose a reason for hiding this comment

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

Looks good! One minor nit, feel free to ignore.

vault/login_mfa.go
@@ -1361,6 +1361,10 @@ func (b *LoginMFABackend) mfaLoginEnforcementConfigByNameAndNamespace(name, name
func (b *LoginMFABackend) mfaLoginEnforcementConfigToMap(eConfig *mfa.MFAEnforcementConfig) (map[string]interface{}, error) {
resp := make(map[string]interface{})
resp["name"] = eConfig.Name
ns, err := b.namespacer.NamespaceByID(context.Background(), eConfig.NamespaceID)
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you feel about changing this logic to the following instead?

ns, err := b.namespacer.NamespaceByID(context.Background(), eConfig.NamespaceID)
if ns == nil || err != nil {
 return nil, err
}

resp["namespace_path"] = ns.Path
...

It's a bit more concise, but the same logic, so feel free to ignore :).

@VioletHynes VioletHynes merged commit 7718995 into main Aug 29, 2022
@VioletHynes VioletHynes deleted the violethynes/VAULT-6433 branch August 29, 2022 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hghaf099 hghaf099 hghaf099 approved these changes

@mpalmi mpalmi mpalmi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@VioletHynes @hghaf099 @mpalmi