Skip to content
Snippets Groups Projects

Account Registration

Merged Rodrey Mark Goite requested to merge feature/account-registration into master

Created by: rdgoite

Implementation of registration/sign-up and modification of the the authentication logic.

Merge request reports

Merged by (Jan 8, 2025 8:15pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
75 75 .antMatchers(HttpMethod.POST, "/submissionEnvelopes").authenticated()
76 76 .antMatchers(HttpMethod.POST, "/projects**").authenticated()
  • Rodrey Mark Goite
  • Rodrey Mark Goite
  • Rodrey Mark Goite
  • Rodrey Mark Goite
    Rodrey Mark Goite @rodrey started a thread on commit a7b8e084
  • 24 @Autowired
    25 private AccountService accountService;
    26
    27 @MockBean
    28 private AccountRepository accountRepository;
    29
    30 @Nested
    31 @DisplayName("Registration")
    32 class Registration {
    33
    34 @Test
    35 void success() {
    36 //given:
    37 String providerReference = "67fe90";
    38 Account account = new Account(providerReference);
    39 assumeThat(account.getRoles()).isEmpty();
    • Created by: rolando-ebi

      assumeThat() will skip this test if the condition is false, but I don't see how the presence of roles invalidates the necessity for a test for successful registration.

      If we later decided to initialise accounts with basic permissions (via granting/adding Roles) then this test would be skipped, which I don't think is the intention.

  • Rodrey Mark Goite
    Rodrey Mark Goite @rodrey started a thread on commit a7b8e084
  • 23 private static final Logger LOGGER = LoggerFactory.getLogger(AuthenticationController.class);
    24
    25 private final AccountService accountService;
    26
    27 public AuthenticationController(AccountService accountService) {
    28 this.accountService = accountService;
    29 }
    30
    31 @PostMapping(path="/registration", produces=APPLICATION_JSON_UTF8_VALUE)
    32 public ResponseEntity<?> register(Authentication authentication) {
    33 var openIdAuthentication = (OpenIdAuthentication) authentication;
    34 var userInfo = (UserInfo) openIdAuthentication.getCredentials();
    35 Account persistentAccount = null;
    36 try {
    37 persistentAccount = accountService.register(new Account(userInfo.getSubjectId()));
    38 } catch (DuplicateAccount duplicateAccount) {
  • Rodrey Mark Goite
    Rodrey Mark Goite @rodrey started a thread on commit a7b8e084
  • 75 75 .antMatchers(HttpMethod.POST, "/submissionEnvelopes").authenticated()
    76 76 .antMatchers(HttpMethod.POST, "/projects**").authenticated()
    • Created by: rdgoite

      The changes in this PR provides the basis for us to update the system to address #125. However, it does require modifications in the code, that's separate from the scope of the authorisation tasks.

  • Rodrey Mark Goite
    Rodrey Mark Goite @rodrey started a thread on commit a7b8e084
  • 120 /*
    121 Similar scenario to byRegisteredUser but somehow the Account was either,
    122 1) unrecognised and so was treated as an authenticated Guest, or
    123 2) Account was erroneously assigned the Guest role.
    124 Essentially, we want to handle duplicated subject id in our system.
    125 */
    126 @Test
    127 void byUnrecognisedRegisteredUser() throws Exception {
    128 //given:
    129 UserInfo userInfo = new UserInfo("cc9a9a1", "https://secure.tld/auth");
    130 Authentication authentication = new OpenIdAuthentication(userInfo);
    131
    132 //and:
    133 doThrow(new DuplicateAccount())
    134 .when(accountService)
    135 .register(any(Account.class));
    • Created by: rolando-ebi

      We should utilise the matcher here to simulate the two scenarios mentioned in the comment above this test rather than for any account, otherwise this doesn't really test anything

  • Rodrey Mark Goite
    Rodrey Mark Goite @rodrey started a thread on commit a7b8e084
  • 23 private static final Logger LOGGER = LoggerFactory.getLogger(AuthenticationController.class);
    24
    25 private final AccountService accountService;
    26
    27 public AuthenticationController(AccountService accountService) {
    28 this.accountService = accountService;
    29 }
    30
    31 @PostMapping(path="/registration", produces=APPLICATION_JSON_UTF8_VALUE)
    32 public ResponseEntity<?> register(Authentication authentication) {
    33 var openIdAuthentication = (OpenIdAuthentication) authentication;
    34 var userInfo = (UserInfo) openIdAuthentication.getCredentials();
    35 Account persistentAccount = null;
    36 try {
    37 persistentAccount = accountService.register(new Account(userInfo.getSubjectId()));
    38 } catch (DuplicateAccount duplicateAccount) {
  • Rodrey Mark Goite
    Rodrey Mark Goite @rodrey started a thread on commit a7b8e084
  • 24 @Autowired
    25 private AccountService accountService;
    26
    27 @MockBean
    28 private AccountRepository accountRepository;
    29
    30 @Nested
    31 @DisplayName("Registration")
    32 class Registration {
    33
    34 @Test
    35 void success() {
    36 //given:
    37 String providerReference = "67fe90";
    38 Account account = new Account(providerReference);
    39 assumeThat(account.getRoles()).isEmpty();
    • Created by: rdgoite

      The assumption makes it explicit that that current requirement is that Accounts are construtcted with no roles. When we change the rules, this test will fail, prompting us to make appropriate changes in the code (if we need to).

  • Rodrey Mark Goite
    Rodrey Mark Goite @rodrey started a thread on commit a7b8e084
  • 23 private static final Logger LOGGER = LoggerFactory.getLogger(AuthenticationController.class);
    24
    25 private final AccountService accountService;
    26
    27 public AuthenticationController(AccountService accountService) {
    28 this.accountService = accountService;
    29 }
    30
    31 @PostMapping(path="/registration", produces=APPLICATION_JSON_UTF8_VALUE)
    32 public ResponseEntity<?> register(Authentication authentication) {
    33 var openIdAuthentication = (OpenIdAuthentication) authentication;
    34 var userInfo = (UserInfo) openIdAuthentication.getCredentials();
    35 Account persistentAccount = null;
    36 try {
    37 persistentAccount = accountService.register(new Account(userInfo.getSubjectId()));
    38 } catch (DuplicateAccount duplicateAccount) {
  • Rodrey Mark Goite
    Rodrey Mark Goite @rodrey started a thread on commit a7b8e084
  • 120 /*
    121 Similar scenario to byRegisteredUser but somehow the Account was either,
    122 1) unrecognised and so was treated as an authenticated Guest, or
    123 2) Account was erroneously assigned the Guest role.
    124 Essentially, we want to handle duplicated subject id in our system.
    125 */
    126 @Test
    127 void byUnrecognisedRegisteredUser() throws Exception {
    128 //given:
    129 UserInfo userInfo = new UserInfo("cc9a9a1", "https://secure.tld/auth");
    130 Authentication authentication = new OpenIdAuthentication(userInfo);
    131
    132 //and:
    133 doThrow(new DuplicateAccount())
    134 .when(accountService)
    135 .register(any(Account.class));
    • Created by: rdgoite

      In this scenario, as far as the controller is concerned, it's only interested in the external behaviour of its collaborator, which is throwing the exception. The check for if the controller passes the correct information to the service is already defined in another test named byAuthenticatedGuest in the assertCorrectRegisteredAccount method.

  • Rodrey Mark Goite
    Rodrey Mark Goite @rodrey started a thread on commit a7b8e084
  • 24 @Autowired
    25 private AccountService accountService;
    26
    27 @MockBean
    28 private AccountRepository accountRepository;
    29
    30 @Nested
    31 @DisplayName("Registration")
    32 class Registration {
    33
    34 @Test
    35 void success() {
    36 //given:
    37 String providerReference = "67fe90";
    38 Account account = new Account(providerReference);
    39 assumeThat(account.getRoles()).isEmpty();
    • Created by: rolando-ebi

      The assumption makes it explicit that that current requirement is that Accounts are constructed with no roles.

      But GuestAccounts are constructed with non-empty roles (they contain GUEST as their role), and aren't GuestAccounts always passed into the register() method since they are the only account that has access to the /register endpoint?

  • Rodrey Mark Goite
    Rodrey Mark Goite @rodrey started a thread on commit a7b8e084
  • 2
    3 import org.humancellatlas.ingest.security.exception.DuplicateAccount;
    4 import org.springframework.stereotype.Component;
    5
    6 @Component
    7 public class DefaultAccountService implements AccountService {
    8
    9 private final AccountRepository accountRepository;
    10
    11 public DefaultAccountService(AccountRepository accountRepository) {
    12 this.accountRepository = accountRepository;
    13 }
    14
    15 @Override
    16 public Account register(Account account) {
    17 account.addRole(Role.CONTRIBUTOR);
    • Created by: rolando-ebi

      If the account is a GuestAccount instance, won't it have the Guest role? We should probably remove that role here too.

      Alternatively, we should change the model a bit here to reduce confusion. register() should be solely responsible for creating Accounts, and so it shouldn't take an Account as input but rather information for creating an Account.

  • Rodrey Mark Goite
    Rodrey Mark Goite @rodrey started a thread on commit a7b8e084
  • 120 /*
    121 Similar scenario to byRegisteredUser but somehow the Account was either,
    122 1) unrecognised and so was treated as an authenticated Guest, or
    123 2) Account was erroneously assigned the Guest role.
    124 Essentially, we want to handle duplicated subject id in our system.
    125 */
    126 @Test
    127 void byUnrecognisedRegisteredUser() throws Exception {
    128 //given:
    129 UserInfo userInfo = new UserInfo("cc9a9a1", "https://secure.tld/auth");
    130 Authentication authentication = new OpenIdAuthentication(userInfo);
    131
    132 //and:
    133 doThrow(new DuplicateAccount())
    134 .when(accountService)
    135 .register(any(Account.class));
    • Created by: rolando-ebi

      Ok, I'm a bit confused by the comment. Is an "authenticated Guest" the same as the GuestAccount? If a GuestAccount is supplied to accountService.register(), I don't think a DuplicateAccount() exception would be thrown. Perhaps this test (i.e asserting the error path when attempting to register UnrecognisedRegisteredUsers ) should foremost exist in the AccountServiceTest.

  • Rodrey Mark Goite
    Rodrey Mark Goite @rodrey started a thread on commit a7b8e084
  • 24 @Autowired
    25 private AccountService accountService;
    26
    27 @MockBean
    28 private AccountRepository accountRepository;
    29
    30 @Nested
    31 @DisplayName("Registration")
    32 class Registration {
    33
    34 @Test
    35 void success() {
    36 //given:
    37 String providerReference = "67fe90";
    38 Account account = new Account(providerReference);
    39 assumeThat(account.getRoles()).isEmpty();
    • Created by: rdgoite

      That's for the GuestAccount. This is an assumption for the generic Account that was created in the test. I guess we need another test that removes the GUEST role in the service.

  • Rodrey Mark Goite
    Rodrey Mark Goite @rodrey started a thread on commit a7b8e084
  • 120 /*
    121 Similar scenario to byRegisteredUser but somehow the Account was either,
    122 1) unrecognised and so was treated as an authenticated Guest, or
    123 2) Account was erroneously assigned the Guest role.
    124 Essentially, we want to handle duplicated subject id in our system.
    125 */
    126 @Test
    127 void byUnrecognisedRegisteredUser() throws Exception {
    128 //given:
    129 UserInfo userInfo = new UserInfo("cc9a9a1", "https://secure.tld/auth");
    130 Authentication authentication = new OpenIdAuthentication(userInfo);
    131
    132 //and:
    133 doThrow(new DuplicateAccount())
    134 .when(accountService)
    135 .register(any(Account.class));
    • Created by: rdgoite

      The GuestAccount is practically an anonymous User that came to us with a valid access token; that token could refer to previously registered User. The setup upon which this can occur is already discussed in the comment in the code.

  • Rodrey Mark Goite
    Rodrey Mark Goite @rodrey started a thread on commit a7b8e084
  • 24 @Autowired
    25 private AccountService accountService;
    26
    27 @MockBean
    28 private AccountRepository accountRepository;
    29
    30 @Nested
    31 @DisplayName("Registration")
    32 class Registration {
    33
    34 @Test
    35 void success() {
    36 //given:
    37 String providerReference = "67fe90";
    38 Account account = new Account(providerReference);
    39 assumeThat(account.getRoles()).isEmpty();
    • Created by: rolando-ebi

      A Guest account is-an Account, constructed with non-empty roles. What's the required behaviour when a GuestAccount is supplied to register()?

      Since Accounts and GuestAccounts can be mutated with Roles, the requirement "current requirement is that Accounts are construtcted with no roles" isn't guaranteed. Indeed, AccountService.register() can be called with a GuestAccount and mutated providerRef/subjectId and then register() will further mutate the Account and persist/return it with the Guest and Contributor role.

      It sounds more like register() should take something resembling UserInfo as its parameter. Then the requirement that accounts aren't assigned roles prior to registration is already encoded in the model (this scenario becomes impossible).

    Please register or sign in to reply