Skip to content

RegisteredClientRepository and OAuth2AuthorizationService should have a method to query all active entries. #325

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

Closed
bschoenmaeckers opened this issue Jun 25, 2021 · 12 comments
Assignees
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@bschoenmaeckers
Copy link

It is currently not possible to get a list of all registered clients and/or active authorizations (with their corresponding tokens).

RegisteredClientRepository should have a method to get all registered clients. And the OAuth2AuthorizationService should have a method to get all (active) authorizations or all (active) authorizations for a given principal.

@bschoenmaeckers bschoenmaeckers added the type: enhancement A general enhancement label Jun 25, 2021
@jgrandja
Copy link
Collaborator

@bschoenmaeckers A findAll() operation is not needed by the framework at the moment and I'm not anticipating it will be needed.

I see this more of an admin operation, where admins want to view all granted tokens and possibly revoke some. This would be the responsibility of the admin app to extend OAuth2AuthorizationService and provide the extension operations.

I'm going to close this as it's not needed by the existing framework components.

@jgrandja jgrandja self-assigned this Jun 25, 2021
@jgrandja jgrandja added status: declined A suggestion or change that we don't feel we should currently apply and removed type: enhancement A general enhancement labels Jun 25, 2021
@bschoenmaeckers
Copy link
Author

Makes sense! I missed the latest commit which made both JDBC implementations non-final so this should be possible now.

@bschoenmaeckers
Copy link
Author

Makes sense! I missed the latest commit which made both JDBC implementations non-final so this should be possible now.

Nevermind... 473dedb

@jgrandja
Copy link
Collaborator

@bschoenmaeckers We reverted the commit for now but it will be merged in 3 separate commits and the classes will be made non-final.

@bschoenmaeckers
Copy link
Author

@jgrandja Will the findBy methods be accessible to the subclasses as well? This would make it considerable easier to make custom query methods.

@jgrandja
Copy link
Collaborator

Yes, we'll make the methods protected wherever it makes sense. cc/ @sjohnr

@bschoenmaeckers
Copy link
Author

Excellent!

@sjohnr
Copy link
Member

sjohnr commented Jun 25, 2021

Yes, I will fold that in, as that makes a lot of sense for the use case under discussion here. Thanks @bschoenmaeckers!

sjohnr pushed a commit to sjohnr/spring-authorization-server that referenced this issue Jun 29, 2021
@sjohnr sjohnr mentioned this issue Jun 29, 2021
@sjohnr
Copy link
Member

sjohnr commented Jun 29, 2021

@bschoenmaeckers After discussion, and considering previous comments on this issue, I won't be making any changes related to this. Reason being (as @jgrandja mentioned) this is a product feature needed by an admin application, and should not be provided by the framework. I recommend extending the interface and providing the findAll() functionality in your own application. Sorry about the confusion here.

@bschoenmaeckers
Copy link
Author

Hi @sjohnr, I understand your decision. But it will still help to make some helper methods and getters in the JDBC implementation accessible by subclasses. Are you still planning to that or not? Thanks anyway!

@sjohnr
Copy link
Member

sjohnr commented Jun 29, 2021

Yes, I am working on that now. Again, sorry for the confusion 😄, I was just referring to the findAll() discussion.

Update: To be clear, I am working on getter/setter access. If by helper methods you mean findBy, I will not be making that accessible, as it is specifically useful only inside the implementation class. Any querying or logic extension points would be implemented in your application (including any kind of findXyz() methods).

@hezhongfeng
Copy link

I use JdbcTemplate to findAll

@Component
public class CustomRegisteredClientRepo {

  @Autowired
  private JdbcTemplate jdbcTemplate;


  public Page<CustomClient> findAll(Pageable page) {
    Order order = !page.getSort().isEmpty() ? page.getSort().toList().get(0) : Order.by("id");

    SqlRowSet rs = jdbcTemplate.queryForRowSet("SELECT * FROM oauth2_registered_client ORDER BY "
        + order.getProperty() + " " + order.getDirection().name() + " LIMIT " + page.getPageSize()
        + " OFFSET " + page.getOffset());

    long count =
        jdbcTemplate.queryForObject("select count(*) from oauth2_registered_client", long.class);

    List<CustomClient> list = new ArrayList<CustomClient>();

    while (rs.next()) {
      CustomClient client = new CustomClient();
      client.setId(rs.getString("id"));
      client.setClientId(rs.getString("client_id"));
      list.add(client);
    }
    return new PageImpl<CustomClient>(list, page, count);
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

4 participants