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

feat: modify "Manage Google Accounts" button to rely on gcloud ADC #3715

Merged

Conversation

diegomarquezp
Copy link
Contributor

@diegomarquezp diegomarquezp commented Feb 24, 2023

This modifies the behavior of this button in the following way:

  • If ADC are not set, it will show a clickable entry explaining the user is not logged in
    image
  • Clicking this will show more information
    image
  • If set, it will offer to view the current account.
    image
    • Clicking this button will display a singleton list of users (only user associated with ADC)
    • This used to have two buttons to sign in with a different user or log out. It will now show an avatar and email only. No action on click.

NOTE

(fixed - see below) The ADC credentials are cached in google-auth-library-java (see). Running gcloud auth application-default login|revoke will not update this component properly:

  1. plugin starts with ADC set
  2. run gcloud auth ... revoke
  3. menu button will still show as if ADC are set

this is fixed in #3716

@diegomarquezp diegomarquezp marked this pull request as ready for review March 2, 2023 15:56
LOGIN_MENU_LOGGED_IN=Manage Google Accounts...
LOGIN_MENU_LOGGED_OUT=Sign in to Google...
LOGIN_MENU_LOGGED_IN=View Application Default Credentials account...
LOGIN_MENU_LOGGED_OUT=Please log in via gcloud CLI...
Copy link
Member

Choose a reason for hiding this comment

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

If a new user see this, it seems reasonable they'd have one of two thoughts:

  • I don't know how to do that.
  • I should go execute gcloud auth login

But neither of these is the action we're looking to have completed.

Rather than fitting the request in the disabled menu item, do we need to open the dialog and display more information about specifically what we're asking the user to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense - I will use a separate dialog

if (account.getName() != null) {
name.setText(account.getName());
if (account.getName().isPresent()) {
name.setText(account.getName().get());
Copy link
Member

Choose a reason for hiding this comment

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

Since we're using Optional, consider replacing isPresent()+get() with:

account.getName().ifPresent(name::setText);

accounts = Collections.singletonList(apiFactory.getAccount());
} catch (IOException ex) {
logger.log(Level.SEVERE, "Could not obtain ADC account", ex);
accounts = Collections.emptyList();
Copy link
Member

Choose a reason for hiding this comment

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

Syntax sugar (meaning: your choice, but consider for readability) -- a private getAccounts() method would prevent needing to have this relatively clunky "declare then initialize then use" order of events.

private List<Account> getAccounts() {
  try {
    return Collections.singletonList(apiFactory.getAccount());
  } catch (IOException ex) {
    ...
    return Collections.emptyList();
  }
}
void createAccountsPane(...) {
  for (Account account : getAccounts()) {
    ...
  }
}


@Override
public void run() {
checkIsLoggedIn();
Copy link
Member

Choose a reason for hiding this comment

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

Are we certain this is efficient enough to be running every two seconds?

If we were to change some assumptions (like, displaying the login status in the menu) could we turn this from a polling implementation to a on-demand check?

Copy link
Contributor Author

@diegomarquezp diegomarquezp Mar 2, 2023

Choose a reason for hiding this comment

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

This polling approach was what I tried when noticing that updating the ADC did not reflect in the oauth library due to caching (fixed in #3716). I will move checkLoggedIn to isEnabled

@@ -37,6 +37,8 @@ public interface IGoogleApiFactory {
*/
public Account getAccount() throws IOException;

public boolean isLoggedIn();
Copy link
Contributor Author

@diegomarquezp diegomarquezp Mar 2, 2023

Choose a reason for hiding this comment

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

Temporary, as better approach is used in #3713

try {
return apiFactory.getAccount();
} catch (IOException ex) {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

If this returns null, then the above account.getName() and account.getEmail() will throw a NullPointerException.

@diegomarquezp diegomarquezp merged commit 1ed9bcb into gcloud-cli-creds Mar 14, 2023
@diegomarquezp diegomarquezp deleted the gcloud-cli-creds-manage-google-accounts branch March 14, 2023 19:57
diegomarquezp added a commit that referenced this pull request Jun 6, 2023
* Adjustments to use application default credentials

* feat: modify "Manage Google Accounts" button to rely on gcloud ADC (#3715)

* feat: modify "Manage Google Accounts" button to rely on gcloud ADC

* fix: use `setBaseEnabled()` to correctly propagate login state

* chore: better UI, on-demand cred check

* chore: adapt single account to deploy dialog (#3717)

* feat: modify "Manage Google Accounts" button to rely on gcloud ADC

* fix: use `setBaseEnabled()` to correctly propagate login state

* chore: better UI, on-demand cred check

* chore: [wip] adapt single account to deploy dialog

* chore: display gcloud instruction messages

* fix: layout to wrap no ADC message

* chore: cleanup of `gcloud-cli-creds` (#3714)

* chore: cleanup of deploy classes

* chore: cleanup of deploy UI classes

* chore: cleanup of cloudsdk staging helper

* chore: better missing-credentials handling

* chore: removal of Credential argument in `newApi()` methods (#3713)

* feat: initial removal of Credential argument in newApi() methods

* chore: move account resolution to new class

* chore: correct precond check, correct exception logging

* chore: do not throw exception when checking for credentials

* fix: compilation errors

* fix: compilation errors in tests

* chore: enable or disable deploy button depending on ADC being found. (#3718)

* feat: modify "Manage Google Accounts" button to rely on gcloud ADC

* fix: use `setBaseEnabled()` to correctly propagate login state

* chore: better UI, on-demand cred check

* chore: [wip] adapt single account to deploy dialog

* chore: display gcloud instruction messages

* fix: layout to wrap no ADC message

* chore: disable deploy button when not logged in

* chore: use new `hasCredentialsSet` method

* chore: fix unit tests using gcloud ADC (#3722)

* feat: initial removal of Credential argument in newApi() methods

* chore: move account resolution to new class

* chore: cleanup of deploy classes

* chore: cleanup of deploy UI classes

* chore: cleanup of cloudsdk staging helper

* chore: correct precond check, correct exception logging

* feat: modify "Manage Google Accounts" button to rely on gcloud ADC

* fix: use `setBaseEnabled()` to correctly propagate login state

* chore: better UI, on-demand cred check

* chore: [wip] adapt single account to deploy dialog

* chore: display gcloud instruction messages

* fix: layout to wrap no ADC message

* chore: disable deploy button when not logged in

* chore: do not throw exception when checking for credentials

* fix: compilation errors

* fix: compilation errors in tests

* chore: initial compilation-enabling adjustments

* chore: use `hasCredentialsSet()`

* chore: add test account provider

* chore: fix usagetracker and projectselector test

* chore: expose `getCredential()`

* chore: fix projectselector tests

* chore: null check for `setPRoviderState()`

* chore: add avatar URL to test accounts

* chore: fix login tests

* chore: fix googleapis tests

* chore: add convenience state+defaultprovider method

* chore: fix dataflow tests

* chore: fix dataflow core tests

* chore: fix error-prone state check in localserver

* chore: correct static member initialization order in `TestAccountProvider`

* chore: initialize static account provider once

* chore: use mock methods instead of test acct provider in `MiniSelectorTest`

* chore: fixes after merging `gcloud-cli-creds`

* chore: use avatar urls in test accounts

* chore: fix accounts panel tests

* chore: fix dataflow preferences test

* chore: fix login `Account` package

* chore: fix usagetracker test ii

* fix: project repository test

* chore: fix googleapis proxy tests

* fix: dataflow preferences test

* chore: fix login tests, remove unused elements (pending cleanup)

* chore: fix app engine deploy tests

* chore: remove unnecessary test in `GcpProjectQueryJobTest`

* chore: fix localserver tests iii

* chore: more dataflow ui test fixes

* chore: fix project repository test

* chore: accounts panel test to assert correct number of children

* chore: run tests on any pr

* chore: fix update accounts logic in `GoogleLoginService`

* chore: remove initialization of static members in constructors

* chore: fire listeners in `AccountSelector`

* chore: fix `GoogleApiFactory` initalization in `GcpLocalRunTabTest`

* chore: fix cache check in `AccountSelector`

* chore: fix `CloudSdkProcessWrapperTest`

* chore: correct stubbing in `DefaultedPipelineOptionsComponentTest`

* chore: remove unused imports in `CloudSdkProcessWrapper`

* chore: remove unused import in `AccountPanelsTest`

* chore: trigger update on `GcpLocalRunTabTest`

* chore: fix `fireSelectionListeners` after setting change tracker variable

* chore: fix `GcpLocalRUnTabTest`

* chore: refactor force `AccountSelector` check to a better named method

* chore: return validation states in `RunOptionsDefaultsComponent`

* chore: add status checks to other test in `RODCTest`

* chore: make `forceCheck()` public in `AccountSelector`

* chore: remove unused thrown exception from delcaration in `GcpLocalRunTabTest`

* chore: set flag to enable `forceAccountCheck`

* chore: have dataflow Command RunOptionsDefaultScomponent not found to return gcloud CLI email

* chore: fix login service test

* chore: fix assertions, verify projects loaded in `RunOptionsDefaultCOmponentsStest`

* chore: update project lists on account change in `RunOptionsDefaultsComponent`

* chore: lock `login` `com.google.api` versions to `1.25.0`

* chore: remove unthrown exception declaration in `GcpLocalRunTabTest`

* chore: fix tests in `RunOptionsDefaultsCOmponentTest`

* chore: correct versions in login MANIFEST.MF

* chore: fix Command setUpServiceKeyCreation not found in Command GcpLocalRunTabTest not found'

* chore: make account selector to return update result

* chore: fix non-existent version of `com.google.api.services.oauth2`

* chore: fix logic check in `forceAccountCHeck` for `AccountSelector`

* chore: exclude `oauth2` package in manifest check

* chore: fix comments in default pipeline options test

* chore: fixes in dataflow tests

* chore: remove TODO comment

* chore: using `EnumMap` in `TestAccountProvider`

* chore: handle `GoogleApiFactory` credential-related methods internally (#3726)

* chore: change credential and account to `Optional`

* chore: adapt localserver tests

* chore: adapt dataflow plugin

* chore: adapt login plugin

* chore: adapt projectselector tests

* chore: adapt projectselector (credential)

* chore: adapt appengine deploy (hascredentials)

* chore: move ITestAccountProvider

* chore: adapt appengine.deploy (hascredentials)

* chore: adapt appengine.localserver (hascredentials)

* chore: adapt dataflow.ui (hascredentials)

* chore: adapt googleapis test (hascredential)

* chore: adapt login (hascredentials)

* chore: adapt projectselector (hascredentials)

* chore: fix setup in gcplocalruntabtest

* chore: adapt test account provider to new interface

* chore: remove `hasCredentialsSet` from `TestAccountProvider`

* chore: test fixes

* chore: correct import (`java.util.Optional`)

* chore: workaround for undetected mock in `FlexDeployPreferencesDialogTest`

* chore: remove unused import

* chore: `Optional` convenience methods

* chore: adapt tests of `AccountSelector` (#3728)

* chore: remove unnecessary login module classes (#3727)

* chore: remove login service OAuth related classes

* chore: adapt appengine deploy

* chore: adapt appengine deploy tests

* chore: adapt dataflow core

* chore: adapt dataflow core tests

* chore: adapt dataflow ui

* chore:adapt dataflow ui tests

* chore: adapt tests in login ui

* chore: fix test case in dataflow core

* chore: improve Optional usage

* chore: update appengine-plugins-core to 0.9.11 (#3732)

* chore: fix project list initialization in local server launch config (#3729)

* chore: fix project list initialization in local server launch config

* chore: adapt tests to initially set credentials

* chore: initialize account selector at right time

* chore: fix initialization

* chore: fix initialization ii

* chore: initialize account selector with empty creds

* chore: initialize api factory before instantiating the localserver tabl

* chore: (cleanup) convert `GoogleApiFactory` to static usage (#3733)

* chore: update snakeyaml to 2.0

* chore: adapt appengine.deploy.ui

* chore: adapt appengine.deploy.ui.test

* chore: remove usages in `appengine.deploy.ui`

* chore: remove reference from appengine.deploy.ui.test

* chore: hide constructor in googleapis

* chore: adapt localserver

* chore: adapt localserver tests

* chore: adapt dataflow core

* chore: adapt dataflow ui

* chore: adapt dataflow ui test

* chore: adapt googleapis

* chore: adapt login

* chore: adapt projectselector

* chore: adapt projectselector tests

* chore: delete `GoogleLoginService` (due at last merge)

* chore: compilation errors

* chore: compilation errors ii

* chore: adapt localserver test ii

* chore: adapt dataflow core test ii

* chore: adapt dataflow ui ii

* chore: adapt datafow ui test ii

* chore: use GoogleApiFactory mock

* chore: use static instance in defaulted pipeline options

* chore: more compilation errors

* feat: compute credentials from well-known ADC file path (apiary credentials) (#3731)

* chore: compute adc path with auth java library

* chore: add polling and subscription to adc

* chore: fix listener logic for credential changes

* chore: compute cred identifier (refresh token) manually

* chore: adapt test account provider

* chore: listen to cred changes in credentials menu item

* chore: non optional-wrapped ADC file

* chore: add dispose listener to account selector

* chore: use `WatchService`

* chore: tests for `DefaultAccountProvider`

* chore: update snakeyaml to 2.0

* chore temp folder to be public

* chore: add more propagation checks

* chore: reorder change counters

* chore: debug info for test

* chore: more logging

* chore: info level for adc watcher

* chore: use custom constructor

* chore: fix constructor

* chore: wait for adc file change in test

* chore: use class level listener

* chore: mock non-final method to identify credential

* chore: make `CredentialWithId` public

* chore: various test fixes

* test: fix listener wait workflow

* chore: adapt to static apifacotry instance

* chore: adapt to static apifactory instance ii

* chore: correct tests

* chore: compute custom refresh token

* chore: improve logging

* chore: more logging

* chore: increase delay between event handling

* chore: more logging ii

* chore: thread safety

* chore: changes polling logging

* chore: fix break on polling

* chore: thread safety in credentials

* chore: more logging for account and credential test logic

* chore: final test fixes

* chore: fix logging levels for production class

* chore: better path resolution

* chore: init transport cache manually

* chore: exclide OSGI-INF from build

* chore: adapt transport cache tests

* chore: correct reactivity in appengine deploy dialog (#3735)

* chore: remove `@Component` annotation from api factory

* chore: react on credential change via gcloud CLI

* chore: remove comments

* chore: handle correct exception type

* chore: grace period for json file to be written

* chore: use separate thread logic to check adc file

* chore: accomodate (deprecated) dataflow tests

* chore: accomodate (deprecate) dataflow tests ii

* chore: accomodate (deprecate) dataflow tests iii

* chore: revert default pipeline adjustments

* chore: reset mock singleton in `GoogleApiFactory`

* chore: remove unused import

---------

Co-authored-by: Burke Davison <burkedavison@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants