JENKINS-41348# BlueOcean specific CredentialsProvider #768

vivek · 2017-01-31T09:29:09Z

Provides User store/scoped credentials by using appropriate properties
on OrganizationFolder.

Description

BlueOcean credentials provider is a proxy to credentials stored in user store.

This is how it works:

When an OrganizationFolder is created, we attach authenticated user's user Id and credential Id of the credential in authenticated user's store with USER scope.
Later, when job runs, it asks BlueOceanCredentialsProvider to give credentialId for the OrganizationFolder created in previous step, it looks for these properties and returns right Credentials object by looking in to user's store.
A github OrganizationFolder configuration page on classic lists the BlueOcean Provider credential

Changes to credential creation for git pipeline flow (for @cliffmeyers)

curl -v -XPOST -u admin:admin -H ‘content-type: application/json’ http://localhost:8080/jenkins/blue/rest/organizations/jenkins/credentials/user/

{
  "credentials" : {
    "privateKeySource" : {
      "privateKey" : "abcabc1212",
      "stapler-class" : "com.cloudbees.jenkins.plugins.sshcredentials.impl.BasicSSHUserPrivateKey$DirectEntryPrivateKeySource"
    },
    "passphrase" : "ssh2",
    "scope" : "USER",
    "domain" : "blueocean-git-domain",
    "description" : "ssh2 desc",
    "$class" : "com.cloudbees.jenkins.plugins.sshcredentials.impl.BasicSSHUserPrivateKey",
    "username" : "ssh2"
  }
}

There are three changes:

POST URL is changed to /jenkins/blue/rest/organizations/jenkins/credentials/user/
domain element in request body: for git use case its recommended UI passes domain name, for example 'blueocean-git-domain'. If its not supplied this credential will be created in default domain 'blueocean-domain'.
See JENKINS-41348.
scope element in request body should be set to USER.

Submitter checklist

Link to JIRA ticket in description, if appropriate.
Change is code complete and matches issue description
Appropriate unit or acceptance tests or explanation to why this change has no tests
Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.
Ran Acceptance Test Harness against PR changes.

Reviewer checklist

Run the changes and verified the change matches the issue description
Reviewed the code
Verified that the appropriate tests have been written or valid explanation given

@reviewbybees

Provides User store/scoped credentials by using appropriate properties on OrganizationFolder.

i386 · 2017-01-31T09:31:50Z

Gosh you're fast :)

vivek · 2017-01-31T09:34:35Z

@stephenc PTAL, specially at BlueOceanCredentialsProvider.java

vivek · 2017-01-31T09:37:30Z

@i386 its been bothering me, better to get it out of the way sooner than later:) We also need similar approach for git ssh keys - these are stored in system store. Will chat with @cliffmeyers tomorrow to get it wrapped with this PR.

stephenc

Mostly OK as is, but i'd prefer if there were some changes.

stephenc · 2017-01-31T10:26:15Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanCredentialsProvider.java

+ * @author Stephen Connoly
+ * @author Vivek Pandey
+ */
+@Extension(optional = true)


Not an optional extension.

You may need to specify an ordinal to ensure your priority is ahead of the standard stores

stephenc · 2017-01-31T10:27:42Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanCredentialsProvider.java

+ */
+@Extension(optional = true)
+public class BlueOceanCredentialsProvider extends CredentialsProvider {
+    private static final String DISPLAY_NAME = "BlueOcean Folder Credentials";


🐜 I18N this in a Messages resource bundle

yeah APIs have i18N missing, maybe good time to get started on it.

stephenc · 2017-01-31T10:28:25Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanCredentialsProvider.java

+            @Override
+            public String getIconFileName() {
+                return isVisible()
+                        ? "/plugin/credentials/images/48x48/folder-store.png"


🐜 please use a Blue Ocean specific icon

Hmm, I thought this is stock folder credential one and can be used for other CredentialsProvider extension. This is anyway read-only credential provider and is a proxy really. Not sure we really need an icon created for it. @i386?

The stock icon is for the "stock" FolderCredentialsProvider from the folders plugin. You need your own so that a user/admin sees that the credentials are coming from this proxy/gateway provider

Or if there is way to display name of 'store' in that table, strangely it shows name of the folder? In the image below, top item is added by blueocean and the next is by folder credential provider.
cc:@i386

stephenc · 2017-01-31T10:28:58Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanCredentialsProvider.java

+            @Override
+            public List<Domain> getDomains() {
+                //XXX: how to get domain from given Credentials object? Maybe attach domain name to AbstractFolder?
+                return Collections.emptyList();


Collections.singletonList(Domain.global())

@stephenc Why global? The credentials it exposes are in user scope.

actually what I see now is that you want something different, you want

Collections.singletonList(/*domain for "blueocean-github-domain"*/)

The domain is about matching the URL the credentials are requested for against the URLs that the credentials are allowed to be used against.

Scope is a different cross-cutting requirement

I see. So it's more for routing to this proxy. I will give it a try.

stephenc · 2017-01-31T10:29:34Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanCredentialsProvider.java

+
+            @Override
+            public boolean hasPermission(@Nonnull Authentication a, @Nonnull Permission permission) {
+                return abstractFolder.getACL().hasPermission(a,permission);


may need to tweak this, but ok for now

Do need to tweak this, as this is a read only domain we want to always return false for the Credentials/CREATE, Credentials/DELETE, Credentials/MANAGE_DOMAIN and Credentials/UPDATE permissions

stephenc · 2017-01-31T10:32:15Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanCredentialsProvider.java

+    @Override
+    public Set<CredentialsScope> getScopes(ModelObject object) {
+        if(isApplicable(object)){
+            return ImmutableSet.of(CredentialsScope.USER);


Actually you are CredentailsScope.GLOBAL if we are to be strictly correct... this may require changes in the credentials API to allow you to "clone" a credential while changing it's scope

This provider is proxy exposing credentials from USER scope, for this reason I exposed it with USER scope. Exposing them with GLOBAL scope sounds incorrect to me.

Because you are not using the authorise project plugin, the authentication will be ACL.SYSTEM so you want the scope GLOBAL as that allows the credential to be used by jobs running as SYSTEM or jobs running as an authentication with the USE_ITEM permission (limited to jobs in the org folder)

@stephenc It seems to be working how it is, but I will change it to GLOBAL. All I am making sure is that its not less secure and these credentials do not get outside of the scope of the org folder they are attached to. Which I think is not the case.

stephenc · 2017-01-31T10:35:28Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanCredentialsProvider.java

+                                Collections.<DomainRequirement>emptyList()
+                        ),
+                        CredentialsMatchers.allOf(CredentialsMatchers.withId(prop.getId()),
+                                CredentialsMatchers.withScope(CredentialsScope.USER))


I am now wondering if perhaps we shouldn't be more explicit and just target the credential stores scoped to the user and bypass the general lookupCredentials, e.g.

for (CredentialStore store: CredentialsProvider.lookupStores(user)) { // if store has credentials with id, return credential } // return empty list;

as matching based on scope here is ringing some alarm bells

I can give it try, but I saw other places your concern about using USER scope as not a good idea. I guess I need to understand why this is not a good idea - this provider is really a locked down provider very specific to keys generated by blueocean in user store with USER scope.

stephenc · 2017-01-31T10:38:37Z

...peline/src/test/java/io/jenkins/blueocean/blueocean_github_pipeline/GithubOrgFolderTest.java

 import org.junit.Assert;
 import org.junit.Test;

+import java.io.IOException;
 import java.util.Map;

 /**
 * @author Vivek Pandey
 */
 public class GithubOrgFolderTest extends PipelineBaseTest {


Missing a test that the credential gets resolved, see this for inspiration but I do not thing you need to go quite so far as to actually trigger a build. I think you could get away with seeing if the credential can be looked up as SYSTEM using the standard credentials API when you add your property and if you remove the property the credential goes missing again, even though the credential is in "bob"'s user store

Ok, will add a test.

stephenc · 2017-01-31T10:39:37Z

...-github-pipeline/src/main/java/io/jenkins/blueocean/blueocean_github_pipeline/GithubScm.java

+                        Jenkins.getAuthentication(),
+                        URIRequirementBuilder.fromUri(uri).build()),
+                CredentialsMatchers.allOf(CredentialsMatchers.withId(credentialId),
+                        CredentialsMatchers.withScope(CredentialsScope.USER))


This is a bad smell, should not need to match based on scope

Why is that? I want to keep it strict and if there is GitHub access key based credentials it better come from USER scope or we reject it.

Still think this is a bad smell. Given that this credentials provider has very specific needs, I suspect we actually want to be directly checking the credential stores for the User and not bothering with the scope check at all

@stephenc Given I am going to be scanning in context of domain in user store, that is blueocean generated credentials are going to be created in user store in specific domain and searched accordingly, dropping USER scope is fine.

stephenc · 2017-01-31T10:42:46Z

@vivek said:

We also need similar approach for git ssh keys - these are stored in system store. Will chat with @cliffmeyers tomorrow to get it wrapped with this PR.

If you need to handle ssh keys also then actually the BlueOceanCredentialsProvider will have to expose up to two credential ids rather than the single id I initially pseudo-coded

stephenc · 2017-01-31T10:44:01Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanCredentialsProvider.java

+
+    public static class FolderPropertyImpl extends AbstractFolderProperty<AbstractFolder<TopLevelItem>> {
+        private final String user;
+        private final String id;


Sounds like you now need this to be a Collection<String> (probably a Set) rather than a single value

@stephenc jenkins organization folder is either going to be setup by blueocean either using ssh credential(BasicSSHUserPrivateKey) or GitHub accesskey based credential (StandardUsernamePassword), not both. So why a collection?

Well if doing checkout with ssh keys you will need both the api credentials for scan/index and the ssh key for checkout

You don't necessarily need both. I can do MBP job creation only using my ssh key. https://gist.github.com/vivek/7a200b9cd6d645a78402b0ce5b1e56a8.

Our UI flow,

For Git case, we let user create either api key or ssh keys. In case of git, its ssh, or access key (username/password) but not both.

For GitHub, no ssh key, only GitHub accessKey.

stephenc · 2017-01-31T10:45:04Z

It's looking good and definitely a better approach than stuffing the credentials into the global store, just some polish recommended before merging

vivek · 2017-02-01T01:46:10Z

@stephenc applied your suggested changes, tests, i18n etc. except the one related to avoiding USER scope as I think this provider should continue doing it - more strict version of checks. If its a bad idea I want to know why.

The other one was keeping an array of credential id - the thing is it's always going to be one credential attached to the org folder (GitHub/BitBucket) or multi-branch pipeline(git).

…stem) Also refactored helper credential methods at one place.

vivek · 2017-02-01T06:03:22Z

@cliffmeyers git specific credential creation in user store and git pipeline creation with user store keys implementation is completed. PTAL. In general for this ticket, all cases have been covered.

stephenc

Looking a lot better, but I think there are some things you will want to improve before merging

stephenc · 2017-02-01T09:09:28Z

...line/src/main/java/io/jenkins/blueocean/blueocean_git_pipeline/GitPipelineCreateRequest.java

+        User authenticatedUser =  User.current();
+        if(authenticatedUser == null){
+            throw new ServiceException.UnauthorizedException("Must login to create a pipeline");
+        }


You probably will need some additional checks here, such as:

Did the system admin disable the per-user credentials store?

Did the system admin disable this Blue Ocean User Credentials Proxy/Gateway credential store

Did the system admin block storage of the credential type we need to store in the user credentials store

Probably not a blocker for merging this, but rather something you will need to be aware of and decide what you want to do about it and how to behave if the system admin has made configuration choices about credentials storage that prevent the blue ocean pipeline functionality from working

Good point. I will open another ticket to get it addressed properly as it may need some UX change.

stephenc · 2017-02-01T09:11:09Z

...line/src/main/java/io/jenkins/blueocean/blueocean_git_pipeline/GitPipelineCreateRequest.java

        TopLevelItem item = create(Jenkins.getInstance(), getName(), MODE, MultiBranchProjectDescriptor.class);

        if (item instanceof WorkflowMultiBranchProject) {
            WorkflowMultiBranchProject project = (WorkflowMultiBranchProject) item;
+
+            if(scmConfig.getCredentialId() != null) {


Safer to use StringUtils.isNotBlank(scmConfig.getCredentialId())

stephenc · 2017-02-01T09:11:32Z

...line/src/main/java/io/jenkins/blueocean/blueocean_git_pipeline/GitPipelineCreateRequest.java

+            }
+
+            //XXX: set credentialId to empty string if null or we get NPE later on
+            String credentialId = scmConfig.getCredentialId() == null ? "" : scmConfig.getCredentialId();


Or String credentialId = StringUtils.defaultString(scmConfig.getCredentialId())

stephenc · 2017-02-01T09:12:45Z

...rc/main/java/io/jenkins/blueocean/blueocean_github_pipeline/GithubPipelineCreateRequest.java

+        User authenticatedUser =  User.current();
+        if(authenticatedUser == null){
+            throw new ServiceException.UnauthorizedException("Must login to create a pipeline");
+        }


repeat of my comment w.r.t. the GitPipelineCreateRequest authorization check

stephenc · 2017-02-01T09:14:41Z

...-github-pipeline/src/main/java/io/jenkins/blueocean/blueocean_github_pipeline/GithubScm.java

-
+            if(githubCredential == null) {
+                CredentialsUtils.createCredentialsInUserStore(
+                        credential, authenticatedUser, DOMAIN_NAME, new URI(getUri()));


Will the getUri() return the GitHub Enterprise URL if the user is creating a pipeline against GHE?

stephenc · 2017-02-01T09:28:19Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanCredentialsProvider.java

+            @Override
+            public List<Domain> getDomains() {
+                //XXX: how to get domain from given Credentials object? Maybe attach domain name to AbstractFolder?
+                return Collections.emptyList();


actually what I see now is that you want something different, you want

Collections.singletonList(/*domain for "blueocean-github-domain"*/)

The domain is about matching the URL the credentials are requested for against the URLs that the credentials are allowed to be used against.

Scope is a different cross-cutting requirement

stephenc · 2017-02-01T09:29:44Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanCredentialsProvider.java

+
+            @Override
+            public boolean hasPermission(@Nonnull Authentication a, @Nonnull Permission permission) {
+                return abstractFolder.getACL().hasPermission(a,permission);


Do need to tweak this, as this is a read only domain we want to always return false for the Credentials/CREATE, Credentials/DELETE, Credentials/MANAGE_DOMAIN and Credentials/UPDATE permissions

stephenc · 2017-02-01T09:30:38Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanCredentialsProvider.java

+            @Nonnull
+            @Override
+            public List<Credentials> getCredentials(@Nonnull Domain domain) {
+                return BlueOceanCredentialsProvider.getCredentials(Credentials.class, abstractFolder);


if (domain == /*domain for "blueocean-github-domain"*/) { return BlueOceanCredentialsProvider.getCredentials(Credentials.class, abstractFolder); } return Collections.emptyList();

@stephenc blueocean allowes credentials to be stored in different domain. For example GitHub credentials could be stored in different domain than git credentials.

stephenc · 2017-02-01T09:31:36Z

...api-impl/src/main/java/io/jenkins/blueocean/rest/impl/pipeline/credential/CredentialApi.java

@@ -32,6 +33,8 @@

    private  final CredentialsStoreAction credentialStoreAction;
    private  final Reachable parent;
+    public static final String DOMAIN_NAME = "blueocean-domain";


this is a different domain name from that used elsewhere

@stephenc thats right, the way its is currently, GitHub credentials are stored in 'github-domain' got for git keys, UI/client can create credentials in domain of their choice. 'blueocean-domain' is default domain where they get saved. Is this problem?

stephenc · 2017-02-01T09:35:20Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanCredentialsProvider.java

+    }
+
+    public static class FolderPropertyImpl extends AbstractFolderProperty<AbstractFolder<TopLevelItem>> {
+        private final String user;


Need a field to store the domain name(s) that this / these credentials are exposed through

Need a field to store either the DomainSpecification or the URI (and instantiate the domain specification on demand)... my preference is to store the DomainSpecification

You also need to solve for the case of GitHub / BitBucket where there is the possibility of both scan credentials and checkout credentials... where you need the two different credentials, to further complicate matters the domain specifications for these will be different, e.g. the SSH key will need to match both the ssh and git protocols while the API token used for scanning will need to match http and https

@stephenc

(I think you meant DomainRequirement not DomainSpecification)

On further thought, this is not going to really work as the domain created is only once per user and its treated as storage of different credentials. I am going to skip enforcing domain specification checks. As long as domain in the user's store matches, we use it to access SCM.

All changes are as per discussion on PR

…serscope

vivek · 2017-02-06T01:43:53Z

@stephenc I think I have got everything landed here. Last items DomainRequirement/DomainSpecification has been added. It also goes over user credential store with correct impersonation. PTAL so that we can proceed further.

BlueOcean store specific icon is another item but it should not block this PR.

stephenc · 2017-02-06T09:10:03Z

BlueOcean store specific icon is another item but it should not block this PR.

That's why I gave it an 🐜 (which is more a niggle than a bug) :-D

Will take a look later today

stephenc

I really need to write the credentials plugin user / implementer and consumer guides

stephenc · 2017-02-06T11:07:36Z

...rc/main/java/io/jenkins/blueocean/blueocean_github_pipeline/GithubPipelineCreateRequest.java

        }
        return null;
    }

-     static void validateCredentialId(String credentialId, OrganizationFolder item, GitHubSCMNavigator navigator) throws IOException {
+    private String GithubCredentialsDomain(String apiUri) {


ugh... methods start with lowercase character

stephenc · 2017-02-06T11:09:19Z

...rc/main/java/io/jenkins/blueocean/blueocean_github_pipeline/GithubPipelineCreateRequest.java

+                            ErrorMessage.Error.ErrorCodes.INVALID.toString(), "Invalid github API URI: "+e.getMessage())), e);
+        }
+        String path = StringUtils.defaultIfBlank(uri.getPath(), "/");
+        boolean githubEnterprise = false;


unnecessary boolean

stephenc · 2017-02-06T11:12:07Z

...peline/src/test/java/io/jenkins/blueocean/blueocean_github_pipeline/GithubOrgFolderTest.java

+        StandardUsernamePasswordCredentials usernamePasswordCredentials = (StandardUsernamePasswordCredentials) c;
+        assertEquals(credential.getId(), usernamePasswordCredentials.getId());
+        assertEquals(credential.getPassword().getPlainText(),usernamePasswordCredentials.getPassword().getPlainText());
+        assertEquals(credential.getUsername(),usernamePasswordCredentials.getUsername());


Missing a test where there is a credential with the same ID in the system store, verifying that your proxy store is a higher ordinal and hence will get asked first

stephenc · 2017-02-06T11:13:17Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanCredentialsProvider.java

+    public <C extends IdCredentials> ListBoxModel getCredentialIds(@Nonnull Class<C> type,
+                                                                   @Nullable ItemGroup itemGroup,
+                                                                   @Nullable Authentication authentication,
+                                                                   @Nonnull List<DomainRequirement> domainRequirements,


you ACKed but did nothing in this method

stephenc · 2017-02-06T11:19:08Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanCredentialsProvider.java

+    public <C extends IdCredentials> ListBoxModel getCredentialIds(@Nonnull Class<C> type,
+                                                                   @Nullable ItemGroup itemGroup,
+                                                                   @Nullable Authentication authentication,
+                                                                   @Nonnull List<DomainRequirement> domainRequirements,


and in fact you kind of got the idea backwards where you applied it in getCredentials ....

The DomainSpecification should be in the property so that you can only return the credential for usage against the GitHub server.

User enters https://github.com/jenkinsci/blueocean-plugin as SCM URL

doFillScanCredentialsItems creates a domain requirements from that URL and asks for all appropriate credentials

Your provider gets asked for credentials... you need to apply your property's DomainSpecification to see if this matches the domain that you are exposing the user's credentials as

Ok, so that matches, now we go and check the user store for credentials with ID (from property) and in a domain matching the DomainRequirement of the BlueOceanProxyDomainRequirement

You have done 4, you still need 3 or the users credentials will leak further than intended

stephenc · 2017-02-06T11:22:03Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanCredentialsProvider.java

+
+            @Override
+            public boolean hasPermission(@Nonnull Authentication a, @Nonnull Permission permission) {
+                // its read only so for all permissions other than READ, we return false


stephenc · 2017-02-06T11:27:01Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanDomainSpecification.java

+                return Result.POSITIVE;
+            }
+        }
+        return Result.NEGATIVE;


FYI by returning NEGATIVE we prevent the credential from being visible to the user until they are searching with the BlueOceanDomainRequirement. In other words, BlueOceanDomainRequirement is MUST HAVE.

If you want the user to be able to use the credential in other contexts without the context being aware of your requirement then you may want to return UNKNOWN.

stephenc · 2017-02-06T11:28:01Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanDomainSpecification.java

+    @Override
+    public Result test(@Nonnull DomainRequirement scope) {
+        if(scope instanceof BlueOceanDomainRequirement){
+            if(((BlueOceanDomainRequirement)scope).getValue().equals(DOMAIN_SPECIFICATION)){


Redundant test IIUC

stephenc · 2017-02-06T11:28:17Z

...in/java/io/jenkins/blueocean/rest/impl/pipeline/credential/BlueOceanDomainSpecification.java

+    public Result test(@Nonnull DomainRequirement scope) {
+        if(scope instanceof BlueOceanDomainRequirement){
+            if(((BlueOceanDomainRequirement)scope).getValue().equals(DOMAIN_SPECIFICATION)){
+                return Result.POSITIVE;


Probably should be PARTIAL

stephenc · 2017-02-06T11:29:41Z

...-impl/src/main/java/io/jenkins/blueocean/rest/impl/pipeline/credential/CredentialsUtils.java

+    }
+
+    private static List<DomainSpecification> generateDomainSpecifications(@Nullable URI uri){
+        if (uri == null) {


You are aware of https://github.com/jenkinsci/credentials-plugin/blob/master/src/main/java/com/cloudbees/plugins/credentials/domains/URIRequirementBuilder.java

vivek · 2017-02-08T00:32:54Z

@stephenc I think I have all in place. Give a bee if it all looks good, once its in place I can ask Cliff to give it a try before calling it done:)

proxy specific domain (with DomainSepcification creating using uri) attached to folder property.
test case added to pick correct BO specific key and not from system. some other test enhancements.

stephenc · 2017-02-09T16:37:03Z

...peline/src/main/java/io/jenkins/blueocean/blueocean_github_pipeline/GithubEnterpriseScm.java

@@ -28,6 +30,11 @@ public GithubEnterpriseScm(Reachable parent) {
        return super.getUri()+DEFAULT_ENTERPRISE_API_SUFFIX;
    }

+    @Override
+    public String getCredentialDomainName() {
+        return DOMAIN_NAME;


is this needed any more? AFAIK you can just store everything in a domain called blue-ocean-proxy

better to keep these id's in their own domain, for example GitHub enterprise is in it's own domain so is consistent that way.

stephenc · 2017-02-09T16:37:54Z

...peline/src/main/java/io/jenkins/blueocean/blueocean_github_pipeline/GithubEnterpriseScm.java

-    private static final String DEFAULT_ENTERPRISE_API_SUFFIX = "/api/v3";
-    private static final String ID = "github-enterprise";
+    static final String DEFAULT_ENTERPRISE_API_SUFFIX = "/api/v3";
+    static final String ID = "github-enterprise";


I cannot see from the diff, but are you mixing the github enterprise hostname with the id in case the user has more than one GHE server (e.g. a test and prod server)

Missed, that let me add that one.

stephenc · 2017-02-09T16:40:09Z

...rc/main/java/io/jenkins/blueocean/blueocean_github_pipeline/GithubPipelineCreateRequest.java

+                                                ErrorMessage.Error.ErrorCodes.INVALID.toString(),
+                                                "No domain in user credentials found for credentialId: "+ scmConfig.getCredentialId())));
+                    }
+                    if(domain.test(new DomainRequirement())) {


I think this is a bug, you should be testing against your BlueOceanDomainRequirement not DomainRequirement

yes it is, good catch.

stephenc · 2017-02-09T16:40:45Z

...rc/main/java/io/jenkins/blueocean/blueocean_github_pipeline/GithubPipelineCreateRequest.java

+                    }
+                    if(domain.test(new DomainRequirement())) {
+                        ((OrganizationFolder) item)
+                                .addProperty(


does addProperty handle duplicates correctly?

good point, will add validation of this.

No duplicate test needed, this code creates a new org folder and is add property right after creation.

- Also enabled github api tests that gets run if GITHUB_ACCESS_TOKEN jvm property is provided

- github enterprise domain name has hostname suffix - Fixed correct domain requirement

…HUB_ACCESS_TOKEN=$YOUR_GITHUB_AACECSS_TOKEN

…serscope

stephenc

If there are any other issues they can be handled as bugs after merging

vivek · 2017-02-09T21:46:54Z

@stephenc the one issue we should open is handling case where admin turns off access to user credential provider or disables storing credentials in these providers. and another one is BO provider icon.

cliffmeyers · 2017-02-10T15:12:01Z

@vivek testing this on my end looks good. Proper access tokens appear to be used during both org scanning and branch scanning, and I've avoided any rate limit exceptions at least for the modest orgs I've been testing with. :) 🐝

JENKINS-41348# BlueOcean specific CredentialsProvider

954f3ce

Provides User store/scoped credentials by using appropriate properties on OrganizationFolder.

vivek added work-in-progress DO NOT MERGE labels Jan 31, 2017

vivek mentioned this pull request Jan 31, 2017

JENKINS-41348# Create credential store in system store #748

Closed

8 tasks

stephenc suggested changes Jan 31, 2017

View reviewed changes

stephenc reviewed Jan 31, 2017

View reviewed changes

Vivek Pandey added 3 commits January 31, 2017 15:44

BlueOceanCredentialProvider is not optional extension

58036dd

i18n of blueocean credential provider message

fda690a

Added test

cdd7878

Vivek Pandey added 2 commits January 31, 2017 21:09

credentials API to create credential in different stores (user and sy…

26ff252

…stem) Also refactored helper credential methods at one place.

Git pipeline add user id and credential id property to MBP job

ac94b40

vivek added needs-review and removed DO NOT MERGE work-in-progress labels Feb 1, 2017

test fixes

afb960f

stephenc suggested changes Feb 1, 2017

View reviewed changes

Vivek Pandey added 5 commits February 1, 2017 12:04

Use StringUtils for null/balnk check

bd6487b

Store domain in folder property and other changes

5be8e09

All changes are as per discussion on PR

Merge remote-tracking branch 'origin/master' into bug/JENKINS-41348-u…

52995e2

…serscope

Impersonation related fix

e9a6c4c

Added DomainSpecification/DomainRequirement checks

0579ef4

stephenc suggested changes Feb 6, 2017

View reviewed changes

Vivek Pandey and others added 3 commits February 6, 2017 13:36

Removed redundant boolean

fd576c3

Checkpointing work in progress

8530fba

Create domain specification for blueocean credential provider proxy

8aff779

stephenc suggested changes Feb 9, 2017

View reviewed changes

Vivek Pandey added 4 commits February 9, 2017 10:43

Fix related to credentials lookup in validate accesstoken apis

def90b4

- Also enabled github api tests that gets run if GITHUB_ACCESS_TOKEN jvm property is provided

Fixes as per PR comments

de70584

- github enterprise domain name has hostname suffix - Fixed correct domain requirement

github api tests are enabled, to run them use this JVM property -DGIT…

eb0d942

…HUB_ACCESS_TOKEN=$YOUR_GITHUB_AACECSS_TOKEN

Merge remote-tracking branch 'origin/master' into bug/JENKINS-41348-u…

da6251e

…serscope

stephenc approved these changes Feb 9, 2017

View reviewed changes

Vivek Pandey added 2 commits February 9, 2017 15:06

Fix to get BO folder property for child folders of org folder

c02467f

Added test for github org folder

13cba24

vivek merged commit 2f0d848 into master Feb 10, 2017

vivek deleted the bug/JENKINS-41348-userscope branch February 10, 2017 17:00

michaelneale mentioned this pull request Feb 12, 2017

JENKINS-41403# Add pipelineFolderNames and scallAllRepos to org folde… #803

Merged

8 tasks

cliffmeyers mentioned this pull request Feb 13, 2017

[JENKINS-41348] update UI to use user-scoped credentials #821

Merged

8 tasks

JENKINS-41348# BlueOcean specific CredentialsProvider #768

JENKINS-41348# BlueOcean specific CredentialsProvider #768

Conversation

vivek commented Jan 31, 2017 • edited Loading

Description

Changes to credential creation for git pipeline flow (for @cliffmeyers)

Submitter checklist

Reviewer checklist

i386 commented Jan 31, 2017

vivek commented Jan 31, 2017 • edited Loading

vivek commented Jan 31, 2017

stephenc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenc commented Jan 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenc commented Jan 31, 2017

vivek commented Feb 1, 2017 • edited Loading

vivek commented Feb 1, 2017

stephenc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivek Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

vivek commented Feb 6, 2017

stephenc commented Feb 6, 2017

stephenc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivek commented Jan 31, 2017 •

edited

Loading

vivek commented Jan 31, 2017 •

edited

Loading

vivek commented Feb 1, 2017 •

edited

Loading

vivek Feb 1, 2017 •

edited

Loading