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

OAuth integration #1

Closed
wants to merge 748 commits into from
Closed

OAuth integration #1

wants to merge 748 commits into from

Conversation

peterserfozo
Copy link
Contributor

No description provided.

mxr576 and others added 30 commits March 26, 2018 09:15
Push logs about failed builds to a repository and leverage caching on Travis
- Expose our configurations that contains translatable strings for the
Configuration translation module.
- Use label instead of string in schema.yml where it is needed.
- Use mail in appreciate places in schema.yml as type.
- Deprecate entity_labels.yml, because that way it becomes easier to
translate one entity related configuration objects in one place.
- Do not redirect user to the Connection error page if unable to
retrieve API product list on the app settings form.
available or incorrect default value is set for a field.
In testing profile only the plain text filter is available and since we
got rid of the restriced_html as default input format for out for
elements some Format selector form elements are not visible anymore.
@cnovak
Copy link
Collaborator

cnovak commented May 11, 2018

Do we need to put this in a beta branch in case issues need to be fixed in alpha?

@mxr576 mxr576 requested review from mxr576 and tamasd and removed request for tamasd and mxr576 May 14, 2018 07:23
@peterserfozo peterserfozo requested review from mxr576 and tamasd May 14, 2018 07:24
@mxr576
Copy link
Contributor

mxr576 commented May 14, 2018

@cnovak No, we should not wait with this until the first beta. We should not just add fixes to the next alphas. We should introduce new features (that should be available in the first stable) in the next alpa releases to be able to get feedbacks about them. We should not introduce new features in betas because we have to guarantee backward compatibility after the first beta has released.

The APIs are frozen enough so that contributed module and theme authors can start upgrading their projects.

The OAuth integration is a typical example of a feature that may introduce some changes in the module's API, but this is fine because we do not need to worry about BC in alphas.

https://www.drupal.org/docs/8/understanding-drupal-version-numbers/what-are-alpha-and-beta-releases-and-release-candidates

We should release new alphas regularly, probably after each sprint until we have not covered all user stories that should be available in the first stable release.


// Display warning message whether there is no available key.
if (empty($basic_auth_keys) && (empty($oauth_keys) || empty($oauth_token_keys))) {
$this->messenger->addWarning(t('There is no available key for connecting to Apigee Edge server. <a href=":link">Create a new key.</a>', [
Copy link
Contributor

Choose a reason for hiding this comment

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

Use $this->t() instead.

$form['authentication']['key_oauth'] = [
'#type' => 'item',
'#title' => $title,
'#description' => $this->t('Select a basic authentication key or <a href=":link">' . $description_link . '</a>.', [
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add the $description_link by using a placeholder too.

}
else {
$title = $this->t('There is no available OAuth (SAML) token key for connecting to Apigee Edge.');
$description_link = $this->t('create a new OAuth (SAML) token key');
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd call this variables as $description_link_text.


$this->key = $key;
$this->keyType = $key->getKeyType();
$this->leeway = $leeway;
Copy link
Contributor

Choose a reason for hiding this comment

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

Because the value of the leeway could not come elsewhere it should come from the OAuth key storage I think.
(It is not necessary to be fixed now just leave a TODO comment here, remove it from the constructor and set its default value to 30s.)

$this->key->save();
}
catch (EntityStorageException $exception) {
throw new OauthAuthenticationException('Could not save the OAuth response.');
Copy link
Contributor

Choose a reason for hiding this comment

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

s/OAuth response/OAuth token data/g

}

if (!empty($missing_env_variables)) {
$form_state->setError($form, t('The following environment variables are not set: @missing_env_variables.', [
Copy link
Contributor

Choose a reason for hiding this comment

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

$this->t()

use Http\Message\Authentication;

/**
* Key type for Apigee Edge basic authentication credentials.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/basic/Oauth/g

/**
* Key type for Apigee Edge basic authentication credentials.
*
* @KeyType(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you subclass this KeyType annotation class as EdgeKeyType, where could set some default settings for our keys like

  • group => "apigee_edge"
  • multivalue=>TRUE
  • origanization, endpoint, username are required", etc

and use @EdgeKeyType in the annotation block? That way you would not need to repeat yourself here.

@@ -86,7 +102,7 @@ public function validateKeyValue(array $form, FormStateInterface $form_state, $k
/**
* {@inheritdoc}
*/
public function getAuthenticationMethod(KeyInterface $key) : Authentication {
public function getAuthenticationMethod(KeyInterface $key, KeyInterface $key_token = NULL) : Authentication {
Copy link
Contributor

Choose a reason for hiding this comment

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

Key token interface is not needed here, can't you introduce a new optional parameter just in OauthKeyType class's getAuthenticationMethod() in the end of the original parameter list (function getAuthenticationMethod(KeyInterface $key) : Authentication {}).

* @param \Drupal\key\KeyInterface|null $key_token
* The OAuth token key entity.
*
* @return CredentialsInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

use FQCN.

@cnovak cnovak closed this May 14, 2018
cnovak pushed a commit that referenced this pull request May 15, 2018
Created module skeleton and the authentication form class.
mxr576 added a commit that referenced this pull request Dec 18, 2018
Kernel tests for entity controller caches.
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.

6 participants