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

Add optional Queryparameter to getAllRoles Service #37

Merged
merged 4 commits into from May 16, 2018

Conversation

Projects
None yet
2 participants
@marco2704
Copy link
Contributor

marco2704 commented Feb 27, 2018

According to improve this method without change its previous behavior, a new Queryparameter was added to getAllRoles method in RoleBasedAuthorizationStrategy. It may be useful in some case.

Thanks!

@oleg-nenashev oleg-nenashev self-requested a review Feb 27, 2018

* @since 2.6.0
* @param type (globalRoles by default, projectRoles, slaveRoles)
*
* @since 2.8.0

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Feb 27, 2018

Member

No need to modify that. The signature changed, but from Stapler PoV the methods are compatible

This comment has been minimized.

Copy link
@marco2704

marco2704 Feb 27, 2018

Author Contributor

Right, changed.

@oleg-nenashev oleg-nenashev self-requested a review Feb 27, 2018

@marco2704

This comment has been minimized.

Copy link
Contributor Author

marco2704 commented Feb 28, 2018

Hi! Another option with less changes:

/**
     * API method to get all groups/users with their role in globalRoles
     * Example: curl -X localhost:8080/role-strategy/strategy/getAllRoles?type=projectRoles
     *
     * @throws IOException in case data fails
     * @since 2.6.0
     */
    @Restricted(NoExternalUse.class)
    public void doGetAllRoles(StaplerRequest req, StaplerResponse rsp) throws IOException {
        checkAdminPerm();
        req.setCharacterEncoding("UTF-8");
        OutputStream os = rsp.getOutputStream();
        JSONObject json = new JSONObject();
        String type = req.hasParameter("type") ? req.getParameter("type") : GLOBAL;
        RoleMap roleMap = this.grantedRoles.get(type);
        for (Map.Entry<Role, Set<String>> grantedRole : roleMap.getGrantedRoles().entrySet()) {
          json.put(grantedRole.getKey().getName(), grantedRole.getValue());
        }
        os.write(json.toString().getBytes("UTF-8"));
    }

Both of them were tested in a staging Jenkins instance. Let me know any advice or improvement.

Thanks in advance!

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Mar 4, 2018

I am fine with the current implementation. Generally it would be better to use https://github.com/stapler/stapler/blob/311dcfd92d229569e7aaaaae364f6e702188fddc/core/src/main/java/org/kohsuke/stapler/json/JsonHttpResponse.java so that the content type is set correctly.

@marco2704 Would it be possible to do so?

@marco2704

This comment has been minimized.

Copy link
Contributor Author

marco2704 commented Mar 4, 2018

I'm not sure but i think it's not possible yet, because of the stapler version which seems to be 1.237 where json package does not exist.

@marco2704

This comment has been minimized.

Copy link
Contributor Author

marco2704 commented Mar 14, 2018

Hi @oleg-nenashev! I've added a similar behavior, just until we can use JsonHttpResponse.java , Regards!

@marco2704

This comment has been minimized.

Copy link
Contributor Author

marco2704 commented Apr 21, 2018

Hi @oleg-nenashev any update about it? Hope we can merge it 😁

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented May 16, 2018

My bad, I missed the notifications. Merging/releasing

@oleg-nenashev oleg-nenashev merged commit a6d15f0 into jenkinsci:master May 16, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.