Account Registration
Created by: rdgoite
Implementation of registration/sign-up and modification of the the authentication logic.
Merge request reports
Activity
75 75 .antMatchers(HttpMethod.POST, "/submissionEnvelopes").authenticated() 76 76 .antMatchers(HttpMethod.POST, "/projects**").authenticated() Created by: MightyAx
At this point is it possible to determine which users are wranglers? If so we can clear off Ticket #125
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.
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) { 75 75 .antMatchers(HttpMethod.POST, "/submissionEnvelopes").authenticated() 76 76 .antMatchers(HttpMethod.POST, "/projects**").authenticated() 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)); 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) { 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(); 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) { 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 theassertCorrectRegisteredAccount
method.
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?
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.
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.
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(); 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)); 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).