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

[VCDA-3475] fix authentication issue with system admin refresh tokens by implementing fallback #116

Merged
merged 4 commits into from
Apr 28, 2022

Conversation

Anirudh9794
Copy link
Contributor

@Anirudh9794 Anirudh9794 commented Apr 27, 2022

  • when authenticating using refresh token, follow the following steps

  • authenticate as a system org user

  • if the above step fails, authenticate as a tenant user

  • this PR also fixes authentication based tests which were broken


This change is Reviewable

Signed-off-by: Aniruddha Shamasundar <[email protected]>
Copy link
Collaborator

@arunmk arunmk left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 9 files at r1.
Reviewable status: 8 of 9 files reviewed, 2 unresolved discussions (waiting on @Anirudh9794, @arunmk, and @sahithi)


pkg/config/cloudconfig.go line 124 at r1 (raw file):

		return nil, fmt.Errorf("Unable to decode yaml file: [%v]", err)
	}
	//config.VCD.Host = strings.TrimRight(config.VCD.Host, "/")

why is this commented out?


pkg/vcdcapiclient/defined_entity.go line 1 at r1 (raw file):

package vcdcapiclient

I am guessing this is extra content?

Signed-off-by: Aniruddha Shamasundar <[email protected]>
Copy link
Contributor Author

@Anirudh9794 Anirudh9794 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 9 files reviewed, 2 unresolved discussions (waiting on @Anirudh9794, @arunmk, and @sahithi)


pkg/config/cloudconfig.go line 124 at r1 (raw file):

Previously, arunmk (Arun M. Krishnakumar) wrote…

why is this commented out?

I missed removing this line before sending out the PR. Will remove the line

@Anirudh9794
Copy link
Contributor Author

pkg/vcdcapiclient/defined_entity.go line 1 at r1 (raw file):

Previously, arunmk (Arun M. Krishnakumar) wrote…

I am guessing this is extra content?

Yes, I deleted this file

Copy link
Collaborator

@sahithi sahithi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 6 of 9 files reviewed, 3 unresolved discussions (waiting on @Anirudh9794 and @arunmk)


pkg/vcdclient/auth.go line 51 at r3 (raw file):

		// NOTE: for a system admin user using refresh token, the userOrg will still be tenant org.
		// try setting authentication as a system org user
		err = vcdClient.SetToken("system",

Shouldn't the order be another way around? First, attempt with the tenant org and then use the system.

regardless, it shouldn't matter.

Copy link
Collaborator

@sahithi sahithi left a comment

Choose a reason for hiding this comment

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

This code will go to common CPI, right?

Reviewable status: 6 of 9 files reviewed, 3 unresolved discussions (waiting on @Anirudh9794 and @arunmk)

@Anirudh9794
Copy link
Contributor Author

This is already in CPI common library.

Copy link
Contributor Author

@Anirudh9794 Anirudh9794 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 9 files reviewed, 3 unresolved discussions (waiting on @arunmk and @sahithi)


pkg/vcdclient/auth.go line 51 at r3 (raw file):

Previously, sahithi (Sahithi Ayloo) wrote…

Shouldn't the order be another way around? First, attempt with the tenant org and then use the system.

regardless, it shouldn't matter.

Done

@Anirudh9794 Anirudh9794 merged commit d08db02 into vmware:main Apr 28, 2022
Anirudh9794 added a commit to Anirudh9794/cluster-api-provider-cloud-director-1 that referenced this pull request Apr 28, 2022
… by implementing fallback (vmware#116)

* Implement fallback approach to authenticate with refresh token as system org user

Signed-off-by: Aniruddha Shamasundar <[email protected]>

* Fix authentication test cases in CAPVCD

Signed-off-by: Aniruddha Shamasundar <[email protected]>

* deleted accidentally committed file

Signed-off-by: Aniruddha Shamasundar <[email protected]>

* Address review comment

Signed-off-by: Aniruddha Shamasundar <[email protected]>
(cherry picked from commit d08db02)
Anirudh9794 added a commit that referenced this pull request May 2, 2022
…s not provided (#119)

* [VCDA-3475] fix authentication issue with system admin refresh tokens by implementing fallback (#116)

* Implement fallback approach to authenticate with refresh token as system org user

Signed-off-by: Aniruddha Shamasundar <[email protected]>

* Fix authentication test cases in CAPVCD

Signed-off-by: Aniruddha Shamasundar <[email protected]>

* deleted accidentally committed file

Signed-off-by: Aniruddha Shamasundar <[email protected]>

* Address review comment

Signed-off-by: Aniruddha Shamasundar <[email protected]>
(cherry picked from commit d08db02)

* formatting changes

Signed-off-by: Aniruddha Shamasundar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants